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

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

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. 

Thanks,
--Nilay