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?
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
or
blk_register_queue
->elevator_lock
-> fs_reclaim (GFP_KERNEL)
-> freeze_lock
Other code paths using ->elevator_lock and ->freeze_lock shall be
updated accordingly.
Thanks,
--Nilay
