Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3] blk-mq: fix io hung due to missing commit_rqs
To:     Ming Lei <ming.lei@redhat.com>, Yu Kuai <yukuai1@huaweicloud.com>
Cc:     axboe@kernel.dk, osandov@fb.com, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com
References: <20220726122224.1790882-1-yukuai1@huaweicloud.com>
 <YuCe7e5KapIqzO0S@T590>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <1d970dd9-f39c-bc58-bcf4-782eab5db3aa@huaweicloud.com>
Date:   Wed, 27 Jul 2022 10:26:05 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <YuCe7e5KapIqzO0S@T590>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Ming

?? 2022/07/27 10:11, Ming Lei д??:
> On Tue, Jul 26, 2022 at 08:22:24PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently, in virtio_scsi, if 'bd->last' is not set to true while
>> dispatching request, such io will stay in driver's queue, and driver
>> will wait for block layer to dispatch more rqs. However, if block
>> layer failed to dispatch more rq, it should trigger commit_rqs to
>> inform driver.
>>
>> There is a problem in blk_mq_try_issue_list_directly() that commit_rqs
>> won't be called:
>>
>> // assume that queue_depth is set to 1, list contains two rq
>> blk_mq_try_issue_list_directly
>>   blk_mq_request_issue_directly
>>   // dispatch first rq
>>   // last is false
>>    __blk_mq_try_issue_directly
>>     blk_mq_get_dispatch_budget
>>     // succeed to get first budget
>>     __blk_mq_issue_directly
>>      scsi_queue_rq
>>       cmd->flags |= SCMD_LAST
>>        virtscsi_queuecommand
>>         kick = (sc->flags & SCMD_LAST) != 0
>>         // kick is false, first rq won't issue to disk
>>   queued++
>>
>>   blk_mq_request_issue_directly
>>   // dispatch second rq
>>    __blk_mq_try_issue_directly
>>     blk_mq_get_dispatch_budget
>>     // failed to get second budget
>>   ret == BLK_STS_RESOURCE
>>    blk_mq_request_bypass_insert
>>   // errors is still 0
>>
>>   if (!list_empty(list) || errors && ...)
>>    // won't pass, commit_rqs won't be called
>>
>> In this situation, first rq relied on second rq to dispatch, while
>> second rq relied on first rq to complete, thus they will both hung.
>>
>> Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since
>> it means that request is not queued successfully.
>>
>> Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE'
>> can't be treated as 'errors' here, fix the problem by calling
>> commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'.
>>
>> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>> Changes in v3:
>>   - as poinited out by Ming, v2 will break return value by treating
>>   BLK_STS_*RESOURCE as errors.
>> Changes in v2:
>>   - suggested by Ming, handle blk_mq_dispatch_rq_list() as well.
>>   - change title and modify commit message.
>>
>>   block/blk-mq.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 70177ee74295..7d26b222cbc9 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1931,7 +1931,8 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>>   	/* If we didn't flush the entire list, we could have told the driver
>>   	 * there was more coming, but that turned out to be a lie.
>>   	 */
>> -	if ((!list_empty(list) || errors) && q->mq_ops->commit_rqs && queued)
>> +	if ((!list_empty(list) || errors || needs_resource ||
>> +	     ret == BLK_STS_DEV_RESOURCE) && q->mq_ops->commit_rqs && queued)
> 
> 'ret == BLK_STS_DEV_RESOURCE' may happen before, but 'ret' could be
> overridden later.

 From what I see, if queue_rq return 'BLK_STS_RESOURCE' or
'BLK_STS_DEV_RESOURCE', the loop will break, so that 'ret' could not
be overridden. 'BLK_STS_ZONE_RESOURCE' can be overridden, however,
needs_resource can cover that case.

Thanks,
Kuai
> 
> Thanks,
> Ming
> 
> .
>