[PATCH] blk-mq: fix io hung due to missing commit_rqs while scheduler is none

[PATCH] blk-mq: fix io hung due to missing commit_rqs while scheduler is none

[Yu Kuai]

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' and
'BLK_STS_DEV_RESOURCE' as 'errors' in blk_mq_try_issue_list_directly(),
so that 'commit_rqs' will be called when dispatch of last rq failed.

Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 70177ee74295..752b0fe4c128 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2680,6 +2680,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
 		list_del_init(&rq->queuelist);
 		ret = blk_mq_request_issue_directly(rq, list_empty(list));
 		if (ret != BLK_STS_OK) {
+			errors++;
 			if (ret == BLK_STS_RESOURCE ||
 					ret == BLK_STS_DEV_RESOURCE) {
 				blk_mq_request_bypass_insert(rq, false,
@@ -2687,7 +2688,6 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
 				break;
 			}
 			blk_mq_end_request(rq, ret);
-			errors++;
 		} else
 			queued++;
 	}
-- 
2.31.1

Re: [PATCH] blk-mq: fix io hung due to missing commit_rqs while scheduler is none

[Ming Lei]

On Tue, Jul 26, 2022 at 10:38:52AM +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' and
> 'BLK_STS_DEV_RESOURCE' as 'errors' in blk_mq_try_issue_list_directly(),
> so that 'commit_rqs' will be called when dispatch of last rq failed.
> 
> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 70177ee74295..752b0fe4c128 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2680,6 +2680,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
>  		list_del_init(&rq->queuelist);
>  		ret = blk_mq_request_issue_directly(rq, list_empty(list));
>  		if (ret != BLK_STS_OK) {
> +			errors++;

OK, it is because that list becomes empty, but .queue_rq(last_rq_in_list)
returns BLK_STS_*RESOURCE, but scsi can't call ->queuecommand() for this
real last rq. Then blk_mq_try_issue_list_directly() doesn't call
->commit_rqs() too.

Here errors means that request not queued successfully, so this patch
looks fine.

Also I think blk_mq_dispatch_rq_list() needs similar handling too,
right?


thanks,
Ming

Re: [PATCH] blk-mq: fix io hung due to missing commit_rqs while scheduler is none

[Yu Kuai]

Hi, Ming

在 2022/07/26 10:48, Ming Lei 写道:
> On Tue, Jul 26, 2022 at 10:38:52AM +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' and
>> 'BLK_STS_DEV_RESOURCE' as 'errors' in blk_mq_try_issue_list_directly(),
>> so that 'commit_rqs' will be called when dispatch of last rq failed.
>>
>> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   block/blk-mq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 70177ee74295..752b0fe4c128 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2680,6 +2680,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
>>   		list_del_init(&rq->queuelist);
>>   		ret = blk_mq_request_issue_directly(rq, list_empty(list));
>>   		if (ret != BLK_STS_OK) {
>> +			errors++;
> 
> OK, it is because that list becomes empty, but .queue_rq(last_rq_in_list)
> returns BLK_STS_*RESOURCE, but scsi can't call ->queuecommand() for this
> real last rq. Then blk_mq_try_issue_list_directly() doesn't call
> ->commit_rqs() too.
> 
> Here errors means that request not queued successfully, so this patch
> looks fine.
> 
> Also I think blk_mq_dispatch_rq_list() needs similar handling too,
> right?

Yes, you're right, I'll handle blk_mq_dispatch_rq_list() in v2, and also
change the title.

Thanks,
Kuai