Re: [PATCH v2 for-6.18/block 05/10] blk-mq: cleanup shared tags case in blk_mq_update_nr_requests()

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/10/15 2:04, Nilay Shroff 写道:
> 
> 
> On 10/14/25 6:35 PM, Chris Mason wrote:
>> Hi everyone,
>>
>> My review automation flagged this one in linux-next, and it looks like a
>> valid bug to me:
>>
>> On Wed, 10 Sep 2025 16:04:40 +0800 Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> For shared tags case, all hctx->sched_tags/tags are the same, it doesn't
>>> make sense to call into blk_mq_tag_update_depth() multiple times for the
>>> same tags.
>>>
>>
>> [ ... ]
>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 9b97f2f3f2c9..80c20700bce8 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -4935,34 +4935,35 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
>>>
>>>   	blk_mq_quiesce_queue(q);
>>>
>>> -	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);
>>> -		}
>>> -		if (ret)
>>> -			goto out;
>>> -	}
>>> -
>>> -	q->nr_requests = nr;
>>> -	if (q->elevator && q->elevator->type->ops.depth_updated)
>>> -		q->elevator->type->ops.depth_updated(q);
>>> -
>>>   	if (blk_mq_is_shared_tags(set->flags)) {
>>>   		if (q->elevator)
>>>   			blk_mq_tag_update_sched_shared_tags(q);
>>                          ^^^^
>>
>> Does blk_mq_tag_update_sched_shared_tags() use the wrong value here?
>>
>> In the old code, q->nr_requests was updated to nr before calling
>> blk_mq_tag_update_sched_shared_tags(). But in the new code, this
>> function is called while q->nr_requests still contains the old value.
>>
>> Looking at blk_mq_tag_update_sched_shared_tags():
>>
>>      void blk_mq_tag_update_sched_shared_tags(struct request_queue *q)
>>      {
>>          sbitmap_queue_resize(&q->sched_shared_tags->bitmap_tags,
>>                               q->nr_requests - q->tag_set->reserved_tags);
>>      }
>>
>> It reads q->nr_requests to calculate the new sbitmap size. With the
>> reordering, this will resize the sbitmap to the old depth instead of
>> the new depth passed in nr.
>>
> Good catch! Yes, I think this needs to be fixed...
> 

Yeah, I'll send a fix ASAP.

Thanks,
Kuai

> Thanks,
> --Nilay
> 
> 
> .
>