Re: [PATCH V4 20/24] block: move elv_register[unregister]_queue out of elevator_lock

[Ming Lei <ming.lei@redhat.com>]

On Thu, May 01, 2025 at 03:56:03PM +0530, Nilay Shroff wrote:
> 
> 
...

> > diff --git a/block/genhd.c b/block/genhd.c
> > index 0e64e7400fb4..59d9febd8c14 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -751,11 +751,17 @@ static void __del_gendisk(struct gendisk *disk)
> >  
> >  static void disable_elv_switch(struct request_queue *q)
> >  {
> > +	struct blk_mq_tag_set *set = q->tag_set;
> > +
> >  	WARN_ON_ONCE(!queue_is_mq(q));
> >  
> >  	mutex_lock(&q->elevator_lock);
> >  	blk_queue_flag_set(QUEUE_FLAG_NO_ELV_SWITCH, q);
> >  	mutex_unlock(&q->elevator_lock);
> > +
> > +	/* wait until in-progress elevator switch is done */
> > +	down_write(&set->update_nr_hwq_lock);
> > +	up_write(&set->update_nr_hwq_lock);
> >  }
> >  
> >  /**
> 
> The disable_elv_switch which is now called just before __del_gendisk 
> disables elevator switch using QUEUE_FLAG_NO_ELV_SWITCH. And I also see
> write-lock (set->update_nr_hwq_lock) in disable_elv_switch which intends
> to wait until in-progress elevator switch is finished but that may not help
> because there's a small window in elv_iosched_store where it evaluates 
> "elevator-switching-disabled" and then when it actually acquires the 
> read-lock (set->update_nr_hwq_lock).
> 
> During the above window, if disable_elv_switch runs then we may enter into 
> the race, where we'd see elv_iosched_store and __del_gendisk running 
> concurrently.

You are right.

> 
> So we may want to update disable_elv_switch such that setting QUEUE_FLAG_
> NO_ELV_SWITCH is protected with write-lock (set->update_nr_hwq_lock) and
> if we do that then we may also not need q->elevator_lock in disable_elv_switch.
> Or another way to fix it might be to move read-lock (set->update_nr_hwq_lock) 
> at the top in elv_iosched_store.  

Looks the approach is good.


Thanks,
Ming