[v2] blk-mq: Fix stall due to recursive flush plug

Commit Message

Ross Lagerwall July 14, 2023, 10:11 a.m. UTC

We have seen rare IO stalls as follows:

* blk_mq_plug_issue_direct() is entered with an mq_list containing two
requests.
* For the first request, it sets last == false and enters the driver's
queue_rq callback.
* The driver queue_rq callback indirectly calls schedule() which calls
blk_flush_plug(). This may happen if the driver has the
BLK_MQ_F_BLOCKING flag set and is allowed to sleep in ->queue_rq.
* blk_flush_plug() handles the remaining request in the mq_list. mq_list
is now empty.
* The original call to queue_rq resumes (with last == false).
* The loop in blk_mq_plug_issue_direct() terminates because there are no
remaining requests in mq_list.

The IO is now stalled because the last request submitted to the driver
had last == false and there was no subsequent call to commit_rqs().

Fix this by returning early in blk_mq_flush_plug_list() if rq_count is 0
which it will be in the recursive case, rather than checking if the
mq_list is empty. At the same time, adjust one of the callers to skip
the mq_list empty check as it is not necessary.

Fixes: dc5fc361d891 ("block: attempt direct issue of plug list")
Signed-off-by: Ross Lagerwall <[e-mail]>
---

In v2:
* Update commit message and add a comment.

 block/blk-core.c | 3 +--
 block/blk-mq.c   | 9 ++++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Bart Van Assche July 14, 2023, 4:04 p.m. UTC | #1

On 7/14/23 03:11, Ross Lagerwall wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 99d8b9812b18..90de50082146 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1144,8 +1144,7 @@ void __blk_flush_plug(struct blk_plug *plug, bool from_schedule)
>   {
>   	if (!list_empty(&plug->cb_list))
>   		flush_plug_callbacks(plug, from_schedule);
> -	if (!rq_list_empty(plug->mq_list))
> -		blk_mq_flush_plug_list(plug, from_schedule);
> +	blk_mq_flush_plug_list(plug, from_schedule);
>   	/*
>   	 * Unconditionally flush out cached requests, even if the unplug
>   	 * event came from schedule. Since we know hold references to the
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 5504719b970d..e6bd9c5f42bb 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2742,7 +2742,14 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>   {
>   	struct request *rq;
>   
> -	if (rq_list_empty(plug->mq_list))
> +	/*
> +	 * We may have been called recursively midway through handling
> +	 * plug->mq_list via a schedule() in the driver's queue_rq() callback.
> +	 * To avoid mq_list changing under our feet, clear rq_count early and
> +	 * bail out specifically if rq_count is 0 rather than checking
> +	 * whether the mq_list is empty.
> +	 */
> +	if (plug->rq_count == 0)
>   		return;
>   	plug->rq_count = 0;

Reviewed-by: Bart Van Assche <[e-mail]>

Jens Axboe July 14, 2023, 7:44 p.m. UTC | #2

On Fri, 14 Jul 2023 11:11:06 +0100, Ross Lagerwall wrote:
> We have seen rare IO stalls as follows:
> 
> * blk_mq_plug_issue_direct() is entered with an mq_list containing two
> requests.
> * For the first request, it sets last == false and enters the driver's
> queue_rq callback.
> * The driver queue_rq callback indirectly calls schedule() which calls
> blk_flush_plug(). This may happen if the driver has the
> BLK_MQ_F_BLOCKING flag set and is allowed to sleep in ->queue_rq.
> * blk_flush_plug() handles the remaining request in the mq_list. mq_list
> is now empty.
> * The original call to queue_rq resumes (with last == false).
> * The loop in blk_mq_plug_issue_direct() terminates because there are no
> remaining requests in mq_list.
> 
> [...]

Applied, thanks!

[1/1] blk-mq: Fix stall due to recursive flush plug
      commit: 70904263512a74a3b8941dd9e6e515ca6fc57821

Best regards,

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index 99d8b9812b18..90de50082146 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1144,8 +1144,7 @@  void __blk_flush_plug(struct blk_plug *plug, bool from_schedule)
 {
 	if (!list_empty(&plug->cb_list))
 		flush_plug_callbacks(plug, from_schedule);
-	if (!rq_list_empty(plug->mq_list))
-		blk_mq_flush_plug_list(plug, from_schedule);
+	blk_mq_flush_plug_list(plug, from_schedule);
 	/*
 	 * Unconditionally flush out cached requests, even if the unplug
 	 * event came from schedule. Since we know hold references to the
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5504719b970d..e6bd9c5f42bb 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2742,7 +2742,14 @@  void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct request *rq;
 
-	if (rq_list_empty(plug->mq_list))
+	/*
+	 * We may have been called recursively midway through handling
+	 * plug->mq_list via a schedule() in the driver's queue_rq() callback.
+	 * To avoid mq_list changing under our feet, clear rq_count early and
+	 * bail out specifically if rq_count is 0 rather than checking
+	 * whether the mq_list is empty.
+	 */
+	if (plug->rq_count == 0)
 		return;
 	plug->rq_count = 0;