Re: [PATCH -next v2 9/9] blk-iocost: fix walk_list corruption

[Yu Kuai]

在 2022/12/01 18:29, Tejun Heo 写道:
> On Thu, Dec 01, 2022 at 06:14:32PM +0800, Yu Kuai wrote:
> > Hi,
> >
> > 在 2022/12/01 18:00, Tejun Heo 写道:
> > > On Thu, Dec 01, 2022 at 09:19:54AM +0800, Yu Kuai wrote:
> > > > > > diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> > > > > > index 710cf63a1643..d2b873908f88 100644
> > > > > > --- a/block/blk-iocost.c
> > > > > > +++ b/block/blk-iocost.c
> > > > > > @@ -2813,13 +2813,14 @@ static void ioc_rqos_exit(struct rq_qos *rqos)
> > > > > >     {
> > > > > >     	struct ioc *ioc = rqos_to_ioc(rqos);
> > > > > > +	del_timer_sync(&ioc->timer);
> > > > > > +
> > > > > >     	blkcg_deactivate_policy(rqos->q, &blkcg_policy_iocost);
> > > > > >     	spin_lock_irq(&ioc->lock);
> > > > > >     	ioc->running = IOC_STOP;
> > > > > >     	spin_unlock_irq(&ioc->lock);
> > > > > > -	del_timer_sync(&ioc->timer);
> > > > >
> > > > > I don't about this workaround. Let's fix properly?
> > > >
> > > > Ok, and by the way, is there any reason to delete timer after
> > > > deactivate policy? This seems a litter wreid to me.
> > >
> > > ioc->running is what controls whether the timer gets rescheduled or not. If
> > > we don't shut that down, the timer may as well get rescheduled after being
> > > deleted. Here, the only extra activation point is IO issue which shouldn't
> > > trigger during rq_qos_exit, so the ordering shouldn't matter but this is the
> > > right order for anything which can get restarted.
> >
> > Thanks for the explanation.
> >
> > I'm trying to figure out how to make sure child blkg pins it's parent,
> > btw, do you think following cleanup make sense?
>
> It's on you to explain why any change that you're suggesting is better and
> safe. I know it's not intentional but you're repeatedly suggesting operation
> reorderings in code paths which are really sensitive to ordering at least
> seemingly without putting much effort into thinking through the side
> effects. This costs disproportionate amount of review bandwidth, and
> increases the chance of new subtle bugs. Can you please slow down a bit and
> be more deliberate?

Thanks for the suggestion, I'll pay close attention to explain this "why
the change is better and safe". And sorry for the review pressure. 😔

> Thanks.