On Sat, Sep 05, 2020 at 07:25:56PM +0800, yangerkun wrote:
> Blk-mq should call commit_rqs once 'bd.last != true' and no more
> request will come(so virtscsi can kick the virtqueue, e.g.). We already
> do that in 'blk_mq_dispatch_rq_list/blk_mq_try_issue_list_directly' while
> list not empty and 'queued > 0'. However, we can seen the same scene
> once the last request in list call queue_rq and return error like
> BLK_STS_IOERR which will not requeue the request, and lead that list
> empty but need call commit_rqs too(Or the request for virtscsi will stay
> timeout until other request kick virtqueue).
It is really one corner case: driver has seen ->last already, so it
should have handled this case in theory.
However, for scsi, .commit_rqs is called by LLD, and request is failed
before calling .queuecommand by scsi middle layer, so causes this trouble.
NVMe has similar issue too.
>
> We found this problem by do fsstress test with offline/online virtscsi
> device repeat quickly.
>
> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
> Reported-by: zhangyi (F) <yi.zhang@xxxxxxxxxx>
> Signed-off-by: yangerkun <yangerkun@xxxxxxxxxx>
> ---
> block/blk-mq.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> v1->v2: delete the comment
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index b3d2785eefe9..cdced4aca2e8 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1412,6 +1412,11 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>
> hctx->dispatched[queued_to_index(queued)]++;
>
> + /* 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)
> + q->mq_ops->commit_rqs(hctx);
> /*
> * Any items that need requeuing? Stuff them into hctx->dispatch,
> * that is where we will continue on next queue run.
> @@ -1425,14 +1430,6 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>
> blk_mq_release_budgets(q, nr_budgets);
>
> - /*
> - * 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 (q->mq_ops->commit_rqs && queued)
> - q->mq_ops->commit_rqs(hctx);
> -
> spin_lock(&hctx->lock);
> list_splice_tail_init(list, &hctx->dispatch);
> spin_unlock(&hctx->lock);
> @@ -2079,6 +2076,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
> struct list_head *list)
> {
> int queued = 0;
> + int errors = 0;
>
> while (!list_empty(list)) {
> blk_status_t ret;
> @@ -2095,6 +2093,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
> break;
> }
> blk_mq_end_request(rq, ret);
> + errors++;
> } else
> queued++;
> }
> @@ -2104,7 +2103,8 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
> * the driver there was more coming, but that turned out to
> * be a lie.
> */
> - if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs && queued)
> + if ((!list_empty(list) || errors) &&
> + hctx->queue->mq_ops->commit_rqs && queued)
> hctx->queue->mq_ops->commit_rqs(hctx);
> }
Looks fine:
Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
Thanks,
Ming
