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
