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 <[email protected]>
---
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
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
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
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
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 <[email protected]>
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 <[email protected]>
---
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
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