Re: [PATCH] blk-mq: fix lockdep warning in __blk_mq_update_nr_hw_queues

[Nilay Shroff <nilay@linux.ibm.com>]

On 8/15/25 3:08 PM, Ming Lei wrote:
> On Fri, Aug 15, 2025 at 05:34:23PM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/08/15 17:15, Yu Kuai 写道:
>>> Will it be simpler if we move blk_mq_freeze_queue_nomemsave() into
>>> blk_mq_elv_switch_none(), after elevator is succeed switching to none
>>> then freeze the queue.
>>>
>>> Later in blk_mq_elv_switch_back we'll know if xa_load() return valid
>>> elevator_type, related queue is already freezed.
>>
>> Like following:
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index e9f037a25fe3..3640fae5707b 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -5010,7 +5010,13 @@ static int blk_mq_elv_switch_none(struct
>> request_queue *q,
>>                 __elevator_get(q->elevator->type);
>>
>>                 elevator_set_none(q);
>> +       } else {
>> +               ret = xa_insert(elv_tbl, q->id, xa_mk_value(1), GFP_KERNEL);
>> +               if (WARN_ON_ONCE(ret))
>> +                       return ret;
>>         }
>> +
>> +       blk_mq_freeze_queue_nomemsave(q);
>>         return ret;
>>  }
>>
>> @@ -5045,9 +5051,6 @@ static void __blk_mq_update_nr_hw_queues(struct
>> blk_mq_tag_set *set,
>>                 blk_mq_sysfs_unregister_hctxs(q);
>>         }
>>
>> -       list_for_each_entry(q, &set->tag_list, tag_set_list)
>> -               blk_mq_freeze_queue_nomemsave(q);
>> -
>>         /*
>>          * Switch IO scheduler to 'none', cleaning up the data associated
>>          * with the previous scheduler. We will switch back once we are done
>> diff --git a/block/elevator.c b/block/elevator.c
>> index e2ebfbf107b3..9400ea9ec024 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -715,16 +715,21 @@ void elv_update_nr_hw_queues(struct request_queue *q,
>> struct elevator_type *e,
>>
>>         WARN_ON_ONCE(q->mq_freeze_depth == 0);
>>
>> -       if (e && !blk_queue_dying(q) && blk_queue_registered(q)) {
>> -               ctx.name = e->elevator_name;
>> -               ctx.et = t;
>> -
>> -               mutex_lock(&q->elevator_lock);
>> -               /* force to reattach elevator after nr_hw_queue is updated
>> */
>> -               ret = elevator_switch(q, &ctx);
>> -               mutex_unlock(&q->elevator_lock);
>> +       if (e) {
>> +               if (!xa_is_value(e) && !blk_queue_dying(q) &&
>> +                   blk_queue_registered(q)) {
>> +                       ctx.name = e->elevator_name;
>> +                       ctx.et = t;
>> +
>> +                       mutex_lock(&q->elevator_lock);
>> +                       /* force to reattach elevator after nr_hw_queue is
>> updated */
>> +                       ret = elevator_switch(q, &ctx);
>> +                       mutex_unlock(&q->elevator_lock);
>> +               }
>> +
>> +               blk_mq_unfreeze_queue_nomemrestore(q);
>>         }
>> -       blk_mq_unfreeze_queue_nomemrestore(q);
>> +
> 
> I feel it doesn't become simpler, :-(
> 
> However we still can avoid the change in elv_update_nr_hw_queues() by moving
> freeze/unfree queue to blk_mq_elv_switch_back(), which looks more readable.
> 
I think yes that seems reasonable but then we also need to move 
elevator_change_done() and blk_mq_free_sched_tags() from 
elv_update_nr_hw_queues() to blk_mq_elv_switch_back(). As you know
both these functions (elevator_change_done and blk_mq_free_sched_tags)
have to be called after we unfreeze the queue.

Thanks,
--Nilay