[PATCH 0/2] blk-mq: fix race between timeout and queue_rq

[PATCH 0/2] blk-mq: fix race between timeout and queue_rq

[Ming Lei]

Hi,

The first patch trys to fix one race between timeout and queue_rq.

And the second one removes two unnecessary blk_clear_rq_complete().

Thanks,
--
Ming Lei

[PATCH 1/2] blk-mq: fix race between timeout and queue_rq

[Ming Lei]

Either the request is from requeue or just being allocated from
tag pool, its REQ_ATOM_STARTED flag has been cleared already, so
don't test it in blk_mq_start_request().

One memory barrier is needed between writing rq->deadline and
setting REQ_ATOM_STARTED so that timeout can't happen too early
if timeout handler reads obsolete rq->deadline caused by out
of order of the two writes.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/blk-mq.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 47f3938..957815e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -377,13 +377,20 @@ static void blk_mq_start_request(struct request *rq, bool last)
 	blk_add_timer(rq);
 
 	/*
+	 * Order writing rq->deadline and setting the rq's
+	 * REQ_ATOM_STARTED flag so that timeout handler can see
+	 * correct rq->deadline once the rq's REQ_ATOM_STARTED flag
+	 * is observed.
+	 */
+	smp_mb__before_atomic();
+
+	/*
 	 * Mark us as started and clear complete. Complete might have been
 	 * set if requeue raced with timeout, which then marked it as
 	 * complete. So be sure to clear complete again when we start
 	 * the request, otherwise we'll ignore the completion event.
 	 */
-	if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
-		set_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
+	set_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
 	if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags))
 		clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
 
-- 
1.7.9.5

Re: [PATCH 1/2] blk-mq: fix race between timeout and queue_rq

[Christoph Hellwig]

On Wed, Sep 17, 2014 at 05:47:57PM +0800, Ming Lei wrote:
> Either the request is from requeue or just being allocated from
> tag pool, its REQ_ATOM_STARTED flag has been cleared already, so
> don't test it in blk_mq_start_request().
> 
> One memory barrier is needed between writing rq->deadline and
> setting REQ_ATOM_STARTED so that timeout can't happen too early
> if timeout handler reads obsolete rq->deadline caused by out
> of order of the two writes.

Looks reasonable to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/2] blk-mq: remove unnecessary blk_clear_rq_complete()

[Ming Lei]

From: Ming Lei <ming.lei@canoical.com>

This patch removes two unnecessary blk_clear_rq_complete(),
the REQ_ATOM_COMPLETE flag is cleared inside blk_mq_start_request(),
so:

	- The blk_clear_rq_complete() in blk_flush_restore_request()
	needn't because the request will be freed later, and clearing
	it here may open a small race window with timeout.

	- The blk_clear_rq_complete() in blk_mq_requeue_request() isn't
	necessary too, even though REQ_ATOM_STARTED is cleared in
	__blk_mq_requeue_request(), in theory it still may cause a small
	race window with timeout since the two clear_bit() may be
	reordered.

Signed-off-by: Ming Lei <ming.lei@canoical.com>
---
 block/blk-flush.c |    2 --
 block/blk-mq.c    |    1 -
 2 files changed, 3 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index b439670..db6dfb4 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -127,8 +127,6 @@ static void blk_flush_restore_request(struct request *rq)
 	/* make @rq a normal request */
 	rq->cmd_flags &= ~REQ_FLUSH_SEQ;
 	rq->end_io = rq->flush.saved_end_io;
-
-	blk_clear_rq_complete(rq);
 }
 
 static bool blk_flush_queue_rq(struct request *rq, bool add_front)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 957815e..6e4942d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -430,7 +430,6 @@ static void __blk_mq_requeue_request(struct request *rq)
 void blk_mq_requeue_request(struct request *rq)
 {
 	__blk_mq_requeue_request(rq);
-	blk_clear_rq_complete(rq);
 
 	BUG_ON(blk_queued_rq(rq));
 	blk_mq_add_to_requeue_list(rq, true);
-- 
1.7.9.5

Re: [PATCH 2/2] blk-mq: remove unnecessary blk_clear_rq_complete()

[Christoph Hellwig]

On Wed, Sep 17, 2014 at 05:47:58PM +0800, Ming Lei wrote:
> From: Ming Lei <ming.lei@canoical.com>
> 
> This patch removes two unnecessary blk_clear_rq_complete(),
> the REQ_ATOM_COMPLETE flag is cleared inside blk_mq_start_request(),
> so:
> 
> 	- The blk_clear_rq_complete() in blk_flush_restore_request()
> 	needn't because the request will be freed later, and clearing
> 	it here may open a small race window with timeout.

This one is defintively correct, blk_mq_end_io should take care of this.

> 	- The blk_clear_rq_complete() in blk_mq_requeue_request() isn't
> 	necessary too, even though REQ_ATOM_STARTED is cleared in
> 	__blk_mq_requeue_request(), in theory it still may cause a small
> 	race window with timeout since the two clear_bit() may be
> 	reordered.

Why yo you think it's not nessecary?  The request is not in the drivers
hand at this point, so it should not be marked started.  Maybe I'm missing
something, but this sounds like it could very likely cause regressions.

Re: [PATCH 2/2] blk-mq: remove unnecessary blk_clear_rq_complete()

[Ming Lei]

On Thu, Sep 18, 2014 at 12:48 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Sep 17, 2014 at 05:47:58PM +0800, Ming Lei wrote:
>> From: Ming Lei <ming.lei@canoical.com>
>>
>> This patch removes two unnecessary blk_clear_rq_complete(),
>> the REQ_ATOM_COMPLETE flag is cleared inside blk_mq_start_request(),
>> so:
>>
>>       - The blk_clear_rq_complete() in blk_flush_restore_request()
>>       needn't because the request will be freed later, and clearing
>>       it here may open a small race window with timeout.
>
> This one is defintively correct, blk_mq_end_io should take care of this.
>
>>       - The blk_clear_rq_complete() in blk_mq_requeue_request() isn't
>>       necessary too, even though REQ_ATOM_STARTED is cleared in
>>       __blk_mq_requeue_request(), in theory it still may cause a small
>>       race window with timeout since the two clear_bit() may be
>>       reordered.
>
> Why yo you think it's not nessecary?  The request is not in the drivers

The COMPLETED flag will be cleared in blk_mq_start_request(), so
it needn't to be cleared here, and it is a bit early and might cause
a tiny race window.

> hand at this point, so it should not be marked started.  Maybe I'm missing
> something, but this sounds like it could very likely cause regressions.

The STARTED flag has been cleared in __blk_mq_requeue_request(),
I mean the COMPLETED flag, consider the following situation:

- one req is queued to driver, and the req is completed from
the device just before its timeout expired
- the driver checks the result and finds it need to requeue for some reason
- blk_mq_requeue_request() is called to requeue the req
- COMPLETED is cleared before clearing STARTED because of
writes reorder
- timeout just comes between the two writes
- then the request may be completed another time because of timeout

Thanks,

Re: [PATCH 2/2] blk-mq: remove unnecessary blk_clear_rq_complete()

[Christoph Hellwig]

On Thu, Sep 18, 2014 at 09:33:35AM +0800, Ming Lei wrote:
> > Why yo you think it's not nessecary?  The request is not in the drivers
> 
> The COMPLETED flag will be cleared in blk_mq_start_request(), so
> it needn't to be cleared here, and it is a bit early and might cause
> a tiny race window.
> 
> > hand at this point, so it should not be marked started.  Maybe I'm missing
> > something, but this sounds like it could very likely cause regressions.
> 
> The STARTED flag has been cleared in __blk_mq_requeue_request(),
> I mean the COMPLETED flag, consider the following situation:
> 
> - one req is queued to driver, and the req is completed from
> the device just before its timeout expired
> - the driver checks the result and finds it need to requeue for some reason
> - blk_mq_requeue_request() is called to requeue the req
> - COMPLETED is cleared before clearing STARTED because of
> writes reorder
> - timeout just comes between the two writes
> - then the request may be completed another time because of timeout

Ok, I guess you are right on this one.  Objection cleared.