[PATCH] blk-mq: don't lose requests if a stopped queue restarts

[PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Shaohua Li]

Normally if driver is busy to dispatch a request the logic is like below:
block layer:					driver:
	__blk_mq_run_hw_queue
a.						blk_mq_stop_hw_queue
b.	rq add to ctx->dispatch

later:
1.						blk_mq_start_hw_queue
2.	__blk_mq_run_hw_queue

But it's possible step 1-2 runs between a and b. And since rq isn't in
ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
there are no subsequent requests kick in.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/blk-mq.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ade8a2d..e6822a2 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -772,6 +772,7 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 
 	WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
 
+again:
 	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
 		return;
 
@@ -853,8 +854,16 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 	 */
 	if (!list_empty(&rq_list)) {
 		spin_lock(&hctx->lock);
-		list_splice(&rq_list, &hctx->dispatch);
+		list_splice_init(&rq_list, &hctx->dispatch);
 		spin_unlock(&hctx->lock);
+		/*
+		 * the queue is expected stopped with BLK_MQ_RQ_QUEUE_BUSY, but
+		 * it's possible the queue is stopped and restarted again
+		 * before this. Queue restart will dispatch requests. And since
+		 * requests in rq_list aren't added into hctx->dispatch yet,
+		 * the requests in rq_list might get lost.
+		 **/
+		goto again;
 	}
 }
 
-- 
1.8.1

Re: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Jens Axboe]

On 05/02/2015 06:31 PM, Shaohua Li wrote:
> Normally if driver is busy to dispatch a request the logic is like below:
> block layer:					driver:
> 	__blk_mq_run_hw_queue
> a.						blk_mq_stop_hw_queue
> b.	rq add to ctx->dispatch
>
> later:
> 1.						blk_mq_start_hw_queue
> 2.	__blk_mq_run_hw_queue
>
> But it's possible step 1-2 runs between a and b. And since rq isn't in
> ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
> there are no subsequent requests kick in.

Good catch! But the patch introduces a potentially never ending loop in 
__blk_mq_run_hw_queue(). Not sure how we can fully close it, but it 
might be better to punt the re-run after adding the requests back to the 
worker. That would turn a potential busy loop (until requests complete) 
into something with nicer behavior, at least. Ala

if (!test_bit(BLK_MQ_S_STOPPED, &hctx->state))
      kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
                                         &hctx->run_work, 0);


-- 
Jens Axboe

Re: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Shaohua Li]

On Mon, May 04, 2015 at 01:17:19PM -0600, Jens Axboe wrote:
> On 05/02/2015 06:31 PM, Shaohua Li wrote:
> >Normally if driver is busy to dispatch a request the logic is like below:
> >block layer:					driver:
> >	__blk_mq_run_hw_queue
> >a.						blk_mq_stop_hw_queue
> >b.	rq add to ctx->dispatch
> >
> >later:
> >1.						blk_mq_start_hw_queue
> >2.	__blk_mq_run_hw_queue
> >
> >But it's possible step 1-2 runs between a and b. And since rq isn't in
> >ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
> >there are no subsequent requests kick in.
> 
> Good catch! But the patch introduces a potentially never ending loop
> in __blk_mq_run_hw_queue(). Not sure how we can fully close it, but
> it might be better to punt the re-run after adding the requests back
> to the worker. That would turn a potential busy loop (until requests
> complete) into something with nicer behavior, at least. Ala
> 
> if (!test_bit(BLK_MQ_S_STOPPED, &hctx->state))
>      kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
>                                         &hctx->run_work, 0);

My first version of the patch is like this, but I changed my mind later.
The assumption is driver will stop queue if it's busy to dispatch
request.  If the driver is buggy, we will have the endless loop here.
Should we assume drivers will not do the right thing?

Thanks,
Shaohua

Re: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Jens Axboe]

On 05/04/2015 01:51 PM, Shaohua Li wrote:
> On Mon, May 04, 2015 at 01:17:19PM -0600, Jens Axboe wrote:
>> On 05/02/2015 06:31 PM, Shaohua Li wrote:
>>> Normally if driver is busy to dispatch a request the logic is like below:
>>> block layer:					driver:
>>> 	__blk_mq_run_hw_queue
>>> a.						blk_mq_stop_hw_queue
>>> b.	rq add to ctx->dispatch
>>>
>>> later:
>>> 1.						blk_mq_start_hw_queue
>>> 2.	__blk_mq_run_hw_queue
>>>
>>> But it's possible step 1-2 runs between a and b. And since rq isn't in
>>> ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
>>> there are no subsequent requests kick in.
>>
>> Good catch! But the patch introduces a potentially never ending loop
>> in __blk_mq_run_hw_queue(). Not sure how we can fully close it, but
>> it might be better to punt the re-run after adding the requests back
>> to the worker. That would turn a potential busy loop (until requests
>> complete) into something with nicer behavior, at least. Ala
>>
>> if (!test_bit(BLK_MQ_S_STOPPED, &hctx->state))
>>       kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
>>                                          &hctx->run_work, 0);
>
> My first version of the patch is like this, but I changed my mind later.
> The assumption is driver will stop queue if it's busy to dispatch
> request.  If the driver is buggy, we will have the endless loop here.
> Should we assume drivers will not do the right thing?

There's really no contract that says the driver MUST stop the queue for 
busy. It could, legitimately, decide to just always run the queue when 
requests complete.

It might be better to simply force this behavior. If we get a BUSY, stop 
the queue from __blk_mq_run_hw_queue(). And if the bit isn't still set 
on re-add, then we know we need to re-run it. I think that would be a 
cleaner API, less fragile, and harder to get wrong. The down side is 
that now this stop happens implicitly by the core, and the driver must 
now have an asymmetric queue start when it frees the limited resource 
that caused the BUSY return. Either that, or we define a 2nd set of 
start/stop bits, one used exclusively by the driver and one used 
exclusively by blk-mq. Then blk-mq could restart the queue on completion 
of a request, since it would then know that blk-mq was the one that 
stopped it.

-- 
Jens Axboe

Re: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Shaohua Li]

On Mon, May 04, 2015 at 01:56:42PM -0600, Jens Axboe wrote:
> On 05/04/2015 01:51 PM, Shaohua Li wrote:
> >On Mon, May 04, 2015 at 01:17:19PM -0600, Jens Axboe wrote:
> >>On 05/02/2015 06:31 PM, Shaohua Li wrote:
> >>>Normally if driver is busy to dispatch a request the logic is like below:
> >>>block layer:					driver:
> >>>	__blk_mq_run_hw_queue
> >>>a.						blk_mq_stop_hw_queue
> >>>b.	rq add to ctx->dispatch
> >>>
> >>>later:
> >>>1.						blk_mq_start_hw_queue
> >>>2.	__blk_mq_run_hw_queue
> >>>
> >>>But it's possible step 1-2 runs between a and b. And since rq isn't in
> >>>ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
> >>>there are no subsequent requests kick in.
> >>
> >>Good catch! But the patch introduces a potentially never ending loop
> >>in __blk_mq_run_hw_queue(). Not sure how we can fully close it, but
> >>it might be better to punt the re-run after adding the requests back
> >>to the worker. That would turn a potential busy loop (until requests
> >>complete) into something with nicer behavior, at least. Ala
> >>
> >>if (!test_bit(BLK_MQ_S_STOPPED, &hctx->state))
> >>      kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
> >>                                         &hctx->run_work, 0);
> >
> >My first version of the patch is like this, but I changed my mind later.
> >The assumption is driver will stop queue if it's busy to dispatch
> >request.  If the driver is buggy, we will have the endless loop here.
> >Should we assume drivers will not do the right thing?
> 
> There's really no contract that says the driver MUST stop the queue
> for busy. It could, legitimately, decide to just always run the
> queue when requests complete.
> 
> It might be better to simply force this behavior. If we get a BUSY,
> stop the queue from __blk_mq_run_hw_queue(). And if the bit isn't
> still set on re-add, then we know we need to re-run it. I think that
> would be a cleaner API, less fragile, and harder to get wrong. The
> down side is that now this stop happens implicitly by the core, and
> the driver must now have an asymmetric queue start when it frees the
> limited resource that caused the BUSY return. Either that, or we
> define a 2nd set of start/stop bits, one used exclusively by the
> driver and one used exclusively by blk-mq. Then blk-mq could restart
> the queue on completion of a request, since it would then know that
> blk-mq was the one that stopped it.

Agree. I'll make the rerun async for now and leave above as a future
improvement.


>From 3e767da0e9f1044659c605120e09726ffd1aeab0 Mon Sep 17 00:00:00 2001
Message-Id: <3e767da0e9f1044659c605120e09726ffd1aeab0.1430770649.git.shli@fb.com>
From: Shaohua Li <shli@fb.com>
Date: Fri, 1 May 2015 16:39:39 -0700
Subject: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

Normally if driver is busy to dispatch a request the logic is like below:
block layer:					driver:
	__blk_mq_run_hw_queue
a.						blk_mq_stop_hw_queue
b.	rq add to ctx->dispatch

later:
1.						blk_mq_start_hw_queue
2.	__blk_mq_run_hw_queue

But it's possible step 1-2 runs between a and b. And since rq isn't in
ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
there are no subsequent requests kick in.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/blk-mq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ade8a2d..e1a5b9e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -855,6 +855,16 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 		spin_lock(&hctx->lock);
 		list_splice(&rq_list, &hctx->dispatch);
 		spin_unlock(&hctx->lock);
+		/*
+		 * the queue is expected stopped with BLK_MQ_RQ_QUEUE_BUSY, but
+		 * it's possible the queue is stopped and restarted again
+		 * before this. Queue restart will dispatch requests. And since
+		 * requests in rq_list aren't added into hctx->dispatch yet,
+		 * the requests in rq_list might get lost.
+		 *
+		 * blk_mq_run_hw_queue() already checks the STOPPED bit
+		 **/
+		blk_mq_run_hw_queue(hctx, true);
 	}
 }
 
-- 
1.8.1

Re: [PATCH] blk-mq: don't lose requests if a stopped queue restarts

[Jens Axboe]

On 05/04/2015 02:20 PM, Shaohua Li wrote:
> On Mon, May 04, 2015 at 01:56:42PM -0600, Jens Axboe wrote:
>> On 05/04/2015 01:51 PM, Shaohua Li wrote:
>>> On Mon, May 04, 2015 at 01:17:19PM -0600, Jens Axboe wrote:
>>>> On 05/02/2015 06:31 PM, Shaohua Li wrote:
>>>>> Normally if driver is busy to dispatch a request the logic is like below:
>>>>> block layer:					driver:
>>>>> 	__blk_mq_run_hw_queue
>>>>> a.						blk_mq_stop_hw_queue
>>>>> b.	rq add to ctx->dispatch
>>>>>
>>>>> later:
>>>>> 1.						blk_mq_start_hw_queue
>>>>> 2.	__blk_mq_run_hw_queue
>>>>>
>>>>> But it's possible step 1-2 runs between a and b. And since rq isn't in
>>>>> ctx->dispatch yet, step 2 will not run rq. The rq might get lost if
>>>>> there are no subsequent requests kick in.
>>>>
>>>> Good catch! But the patch introduces a potentially never ending loop
>>>> in __blk_mq_run_hw_queue(). Not sure how we can fully close it, but
>>>> it might be better to punt the re-run after adding the requests back
>>>> to the worker. That would turn a potential busy loop (until requests
>>>> complete) into something with nicer behavior, at least. Ala
>>>>
>>>> if (!test_bit(BLK_MQ_S_STOPPED, &hctx->state))
>>>>       kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
>>>>                                          &hctx->run_work, 0);
>>>
>>> My first version of the patch is like this, but I changed my mind later.
>>> The assumption is driver will stop queue if it's busy to dispatch
>>> request.  If the driver is buggy, we will have the endless loop here.
>>> Should we assume drivers will not do the right thing?
>>
>> There's really no contract that says the driver MUST stop the queue
>> for busy. It could, legitimately, decide to just always run the
>> queue when requests complete.
>>
>> It might be better to simply force this behavior. If we get a BUSY,
>> stop the queue from __blk_mq_run_hw_queue(). And if the bit isn't
>> still set on re-add, then we know we need to re-run it. I think that
>> would be a cleaner API, less fragile, and harder to get wrong. The
>> down side is that now this stop happens implicitly by the core, and
>> the driver must now have an asymmetric queue start when it frees the
>> limited resource that caused the BUSY return. Either that, or we
>> define a 2nd set of start/stop bits, one used exclusively by the
>> driver and one used exclusively by blk-mq. Then blk-mq could restart
>> the queue on completion of a request, since it would then know that
>> blk-mq was the one that stopped it.
>
> Agree. I'll make the rerun async for now and leave above as a future
> improvement.

Agree, I will apply this one. Thanks!

-- 
Jens Axboe