Re: [PATCH] block: don't grab elevator lock during queue initialization

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

On Thu, Apr 03, 2025 at 06:49:05PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/3/25 4:24 PM, Ming Lei wrote:
> > ->elevator_lock depends on queue freeze lock, see block/blk-sysfs.c.
> > 
> > queue freeze lock depends on fs_reclaim.
> > 
> > So don't grab elevator lock during queue initialization which needs to
> > call kmalloc(GFP_KERNEL), and we can cut the dependency between
> > ->elevator_lock and fs_reclaim, then the lockdep warning can be killed.
> > 
> > This way is safe because elevator setting isn't ready to run during
> > queue initialization.
> > 
> > There isn't such issue in __blk_mq_update_nr_hw_queues() because
> > memalloc_noio_save() is called before acquiring elevator lock.
> > 
> > Fixes the following lockdep warning:
> > 
> > https://lore.kernel.org/linux-block/67e6b425.050a0220.2f068f.007b.GAE@google.com/
> > 
> > Reported-by: syzbot+4c7e0f9b94ad65811efb@syzkaller.appspotmail.com
> > Cc: Nilay Shroff <nilay@linux.ibm.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> I think you earlier posted this same patch here:
> https://lore.kernel.org/linux-block/Z-dUCLvf06SfTOHy@fedora/
> 
> I tested that earlier patch and got into another lockdep splat as reported here: 
> https://lore.kernel.org/linux-block/462d4e8a-dd95-48fe-b9fe-a558057f9595@linux.ibm.com/

That is another different one, let's fix this one first.

The ->elevator_lock in blk_register_queue() should be for avoiding race
with updating nr_hw_queues, right?

That is why I think the dependency between elevator lock and freeze lock
is one trouble.

> 
> I don't know why we think your earlier fix which cut dependency between 
> ->elevator_lock and ->freeze_lock doesn't work. But anyways, my view
> is that we've got into these lock chains from two different code paths:

As I explained, blk_mq_enter_no_io() is same with freeze queue, just the
lock in __bio_queue_enter() isn't modeled. If it is done, every lockdep
warning will be re-triggered too.

> 
> path1: elevator_lock 
>          -> fs_reclaim (GFP_KERNEL)
>            -> freeze_lock
> 
> path2: freeze_lock(memalloc_noio) 
>          -> elevator_lock 
>            -> fs_reclaim (this becomes NOP in this case due to memalloc_noio)

No, there isn't fs_reclaim in path2, and memalloc_noio() will avoid it.


Thanks,
Ming