Re: [PATCH for-6.18/block 06/10] blk-mq: split bitmap grow and resize case in blk_mq_update_nr_requests()

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/09/10 14:30, Nilay Shroff 写道:
> 
> 
> On 9/9/25 10:09 PM, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/9/9 20:18, Nilay Shroff 写道:
>>>
>>> On 9/8/25 11:45 AM, Yu Kuai wrote:
>>>> From: Yu Kuai <yukuai3@huawei.com>
>>>>
>>>> No functional changes are intended, make code cleaner and prepare to fix
>>>> the grow case in following patches.
>>>>
>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>> ---
>>>>    block/blk-mq.c | 28 ++++++++++++++++------------
>>>>    1 file changed, 16 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 1ff6370f7314..82fa81036115 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -4931,21 +4931,25 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
>>>>                blk_mq_tag_update_sched_shared_tags(q);
>>>>            else
>>>>                blk_mq_tag_resize_shared_tags(set, nr);
>>>> -    } else {
>>>> +    } else if (!q->elevator) {
>>>>            queue_for_each_hw_ctx(q, hctx, i) {
>>>>                if (!hctx->tags)
>>>>                    continue;
>>>> -            /*
>>>> -             * If we're using an MQ scheduler, just update the
>>>> -             * scheduler queue depth. This is similar to what the
>>>> -             * old code would do.
>>>> -             */
>>>> -            if (hctx->sched_tags)
>>>> -                ret = blk_mq_tag_update_depth(hctx,
>>>> -                            &hctx->sched_tags, nr);
>>>> -            else
>>>> -                ret = blk_mq_tag_update_depth(hctx,
>>>> -                            &hctx->tags, nr);
>>>> +            sbitmap_queue_resize(&hctx->tags->bitmap_tags,
>>>> +                nr - hctx->tags->nr_reserved_tags);
>>>> +        }
>>>> +    } else if (nr <= q->elevator->et->nr_requests) {
>>>> +        queue_for_each_hw_ctx(q, hctx, i) {
>>>> +            if (!hctx->sched_tags)
>>>> +                continue;
>>>> +            sbitmap_queue_resize(&hctx->sched_tags->bitmap_tags,
>>>> +                nr - hctx->sched_tags->nr_reserved_tags);
>>>> +        }
>>>> +    } else {
>>>> +        queue_for_each_hw_ctx(q, hctx, i) {
>>>> +            if (!hctx->sched_tags)
>>>> +                continue;
>>>> +            blk_mq_tag_update_depth(hctx, &hctx->sched_tags, nr);
>>>>                if (ret)
>>>>                    goto out;
>>>>            }
>>> The above code is good however can this be bit simplified?
>>> It's a bit difficult to follow through all nesting and so
>>> could it be simplified as below:
>>>
>>> if (shared-tags) {
>>>       if (elevator)
>>>          // resize sched-shared-tags bitmap
>>>       else
>>>          // resize shared-tags bitmap
>>> } else {
>>>       // non-shared tags
>>>       if (elevator) {
>>>           if (new-depth-is-less-than-the-current-depth)
>>>               // resize sched-tags bitmap
>>>           else
>>>               // handle sched tags grow
>>>       } else
>>>           // resize tags bitmap
>>> }
>>
>> AFAIK, if - else if chain should be better than nested if - else, right?
>>
>> If you don't mind, I can add comments to each else if chain to make code cleaner:
>>
>> if () {
>>      /* shared tags */
>>      ...
>> } else if () {
>>      /* non-shared tags and elevator is none */
>>      ...
>> } else if () {
>>      /* non-shared tags and elevator is not none, nr_requests doesn't grow */
>>      ...
>> } else () {
>>      /* non-shared tags and elevator is not none, nr_requests grow */
>>      ...
>> }
>>
> Yeah, I am good with the proper comments as well so that it'd be easy
> for anyone reviewing the code later to understand what those all nested
> if-else conditions meant.
> 

Ok, I'll do that in the next version.

Thanks for the review!
Kuai

> Thanks,
> --Nilay
> 
> .
>