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

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

On Sat, Apr 05, 2025 at 07:44:19PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/4/25 2:40 PM, Christoph Hellwig wrote:
> > On Thu, Apr 03, 2025 at 06:54:02PM +0800, Ming Lei wrote:
> >> Fixes the following lockdep warning:
> > 
> > Please spell the actual dependency out here, links are not permanent
> > and also not readable for any offline reading of the commit logs.
> > 
> >> +static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
> >> +				   struct request_queue *q, bool lock)
> >> +{
> >> +	if (lock) {
> > 
> > bool lock(ed) arguments are an anti-pattern, and regularly get Linus
> > screaming at you (in this case even for the right reason :))
> > 
> >> +		/* protect against switching io scheduler  */
> >> +		mutex_lock(&q->elevator_lock);
> >> +		__blk_mq_realloc_hw_ctxs(set, q);
> >> +		mutex_unlock(&q->elevator_lock);
> >> +	} else {
> >> +		__blk_mq_realloc_hw_ctxs(set, q);
> >> +	}
> > 
> > I think the problem here is again that because of all the other
> > dependencies elevator_lock really needs to be per-set instead of
> > per-queue which will allows us to have much saner locking hierarchies.
> > 
> I believe you meant here q->tag_set->elevator_lock? 

I don't know what locks you are planning to invent.

For set->tag_list_lock, it has been very fragile:

blk_mq_update_nr_hw_queues
	set->tag_list_lock
		freeze_queue

If IO failure happens when waiting in above freeze_queue(), the nvme error
handling can't provide forward progress any more, because the error
handling code path requires set->tag_list_lock.

So all queues should be frozen first before calling blk_mq_update_nr_hw_queues,
fortunately that is what nvme is doing.


> If yes then it means that we should be able to grab ->elevator_lock
> before freezing the queue in __blk_mq_update_nr_hw_queues and so locking
> order should be in each code path,
> 
> __blk_mq_update_nr_hw_queues
>     ->elevator_lock 
>       ->freeze_lock

Now tagset->elevator_lock depends on set->tag_list_lock, and this way
just make things worse. Why can't we disable elevator switch during
updating nr_hw_queues?



Thanks,
Ming