[PATCH v2] blk-mq: call commit_rqs while list empty but error happen

[PATCH v2] blk-mq: call commit_rqs while list empty but error happen

[yangerkun]

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).

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@huawei.com>
Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 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);
 }
 
-- 
2.25.4

Re: [PATCH v2] blk-mq: call commit_rqs while list empty but error happen

[Ming Lei]

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@huawei.com>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  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@redhat.com>

Thanks,
Ming

Re: [PATCH v2] blk-mq: call commit_rqs while list empty but error happen

[yangerkun]

Ping...

在 2020/9/5 19:25, yangerkun 写道:
> 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).
> 
> 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@huawei.com>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>   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);
>   }
>   
> 

Re: [PATCH v2] blk-mq: call commit_rqs while list empty but error happen

[Jens Axboe]

On 9/5/20 5:25 AM, 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).
> 
> We found this problem by do fsstress test with offline/online virtscsi
> device repeat quickly.

Applied, thanks.-- 
Jens Axboe