On Tue, Jul 26, 2022 at 08:22:24PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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.
Thanks,
Ming
