Re: [PATCH for-6.18/block 08/10] blk-mq: fix potential deadlock while nr_requests grown

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

On 9/9/25 1:07 PM, Yu Kuai wrote:
> Hi,
> 
> 在 2025/09/09 14:39, Nilay Shroff 写道:
>>
>>
>> On 9/8/25 11:45 AM, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Allocate and free sched_tags while queue is freezed can deadlock[1],
>>> this is a long term problem, hence allocate memory before freezing
>>> queue and free memory after queue is unfreezed.
>>>
>>> [1] https://lore.kernel.org/all/0659ea8d-a463-47c8-9180-43c719e106eb@linux.ibm.com/
>>> Fixes: e3a2b3f931f5 ("blk-mq: allow changing of queue depth through sysfs")
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> [...]
>> [...]
>>
>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>> index 7ea15bf68b4b..a0a7ebad378f 100644
>>> --- a/block/blk-sysfs.c
>>> +++ b/block/blk-sysfs.c
>>> @@ -64,11 +64,12 @@ static ssize_t queue_requests_show(struct gendisk *disk, char *page)
>>>   static ssize_t
>>>   queue_requests_store(struct gendisk *disk, const char *page, size_t count)
>>>   {
>>> -    unsigned long nr;
>>> -    int ret, err;
>>> -    unsigned int memflags;
>>>       struct request_queue *q = disk->queue;
>>>       struct blk_mq_tag_set *set = q->tag_set;
>>> +    struct elevator_tags *et = NULL;
>>> +    unsigned int memflags;
>>> +    unsigned long nr;
>>> +    int ret;
>>>         ret = queue_var_store(&nr, page, count);
>>>       if (ret < 0)
>>> @@ -90,16 +91,24 @@ queue_requests_store(struct gendisk *disk, const char *page, size_t count)
>>>           goto unlock;
>>>       }
>>>   +    if (q->elevator && nr > q->elevator->et->nr_requests) {
>>> +        /* allocate memory before freezing queue to prevent deadlock */
>>> +        et = blk_mq_alloc_sched_tags(set, q->nr_hw_queues, nr);
>>> +        if (!et) {
>>> +            ret = -ENOMEM;
>>> +            goto unlock;
>>> +        }
>>> +    }
>>> +
>> I think we should add a comment above explaining why is it safe
>> to access q->elevator without holding ->elevator_lock.
>>
> 
> I already access q->elevator to check input nr from patch 4, and that's
> why I add comments to explain switching elevator is serialized, is this
> enough?
> 
yes in patch 04/10 you moved the ->elevator_lock after the 
usual sanity checks. However when we run those sanity checks
or the code in this patch where we have to access q->elevator,
it's good to add a comment here in the code (not in commit).
For reference, you may check blk_mq_alloc_sched_tags_batch.
I think similar comment may be added here as well.

Thanks,
--Nilay