blk-mq: reimplement timeout handling

Hello,

Changes from [v3]

- Rebased on top of for-4.16/block.

- Integrated Jens's hctx_[un]lock() factoring patch and refreshed the patches accordingly.

- Added comment explaining the use of hctx_lock() instead of rcu_read_lock() in completion path.

Changes from [v2]

- Possible extended looping around seqcount and u64_stat_sync fixed.

- Misplaced MQ_RQ_IDLE state setting fixed.

- RQF_MQ_TIMEOUT_EXPIRED added to prevent firing the same timeout multiple times.

- s/queue_rq_src/srcu/ patch added.

- Other misc changes.

Changes from [v1]

- BLK_EH_RESET_TIMER handling fixed.

- s/request->gstate_seqc/request->gstate_seq/

- READ_ONCE() added to blk_mq_rq_udpate_state().

- Removed left over blk_clear_rq_complete() invocation from blk_mq_rq_timed_out().

Currently, blk-mq timeout path synchronizes against the usual issue/completion path using a complex scheme involving atomic bitflags, REQ_ATOM_*, memory barriers and subtle memory coherence rules. Unfortunatley, it contains quite a few holes.

It's pretty easy to make blk_mq_check_expired() terminate a later instance of a request. If we induce 5 sec delay before time_after_eq() test in blk_mq_check_expired(), shorten the timeout to 2s, and issue back-to-back large IOs, blk-mq starts timing out requests spuriously pretty quickly. Nothing actually timed out. It just made the call on a recycle instance of a request and then terminated a later instance long after the original instance finished. The scenario isn't theoretical either.

This patchset replaces the broken synchronization mechanism with a RCU and generation number based one. Please read the patch description of the second path for more details.

This patchset contains the following eight patches.

0001-blk-mq-move-hctx-lock-unlock-into-a-helper.patch 0002-blk-mq-protect-completion-path-with-RCU.patch 0003-blk-mq-replace-timeout-synchronization-with-a-RCU-an.patch 0004-blk-mq-use-blk_mq_rq_state-instead-of-testing-REQ_AT.patch 0005-blk-mq-make-blk_abort_request-trigger-timeout-path.patch 0006-blk-mq-remove-REQ_ATOM_COMPLETE-usages-from-blk-mq.patch 0007-blk-mq-remove-REQ_ATOM_STARTED.patch 0008-blk-mq-rename-blk_mq_hw_ctx-queue_rq_srcu-to-srcu.patch

and is available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git blk-mq-timeout-v4

diffstat follows. Thanks.

 block/blk-core.c       |    2 
 block/blk-mq-debugfs.c |    4 
 block/blk-mq.c         |  340 ++++++++++++++++++++++++++++---------------------
 block/blk-mq.h         |   49 ++++++-
 block/blk-timeout.c    |   11 -
 block/blk.h            |    7 -
 include/linux/blk-mq.h |    3 
 include/linux/blkdev.h |   25 +++
 8 files changed, 283 insertions(+), 158 deletions(-)

--
tejun

[v3] http://lkml.kernel.org/r/[email protected]
[v2] http://lkml.kernel.org/r/[email protected]
[v1] http://lkml.kernel.org/r/[email protected]

From: Jens Axboe <[email protected]>

Move the RCU vs SRCU logic into lock/unlock helpers, which makes the actual functional bits within the locked region much easier to read.

tj: Reordered in front of timeout revamp patches and added the missing
blk_mq_run_hw_queue() conversion.

---
 block/blk-mq.c | 66 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

index 111e1aa..ddc9261 100644
@@ -557,6 +557,22 @@ static void __blk_mq_complete_request(struct request *rq)
 	put_cpu();
 }
 
+static void hctx_unlock(struct blk_mq_hw_ctx *hctx, int srcu_idx)
+{
+	if (!(hctx->flags & BLK_MQ_F_BLOCKING))
+		rcu_read_unlock();
+	else
+		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
+}
+
+static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)
+{
+	if (!(hctx->flags & BLK_MQ_F_BLOCKING))
+		rcu_read_lock();
+	else
+		*srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
+}
+
 /**
  * blk_mq_complete_request - end I/O on a request
  * @rq:		the request being processed

@@ -1214,17 +1230,11 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 	 */
 	WARN_ON_ONCE(in_interrupt());
 
-	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
-		rcu_read_lock();
-		blk_mq_sched_dispatch_requests(hctx);
-		rcu_read_unlock();
-	} else {
-		might_sleep();
+	might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
 
-		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
-		blk_mq_sched_dispatch_requests(hctx);
-		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
-	}
+	hctx_lock(hctx, &srcu_idx);
+	blk_mq_sched_dispatch_requests(hctx);
+	hctx_unlock(hctx, srcu_idx);
 }
 
 /*

@@ -1296,17 +1306,10 @@ bool blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
 	 * And queue will be rerun in blk_mq_unquiesce_queue() if it is
 	 * quiesced.
 	 */
-	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
-		rcu_read_lock();
-		need_run = !blk_queue_quiesced(hctx->queue) &&
-			blk_mq_hctx_has_pending(hctx);
-		rcu_read_unlock();
-	} else {
-		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
-		need_run = !blk_queue_quiesced(hctx->queue) &&
-			blk_mq_hctx_has_pending(hctx);
-		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
-	}
+	hctx_lock(hctx, &srcu_idx);
+	need_run = !blk_queue_quiesced(hctx->queue) &&
+		blk_mq_hctx_has_pending(hctx);
+	hctx_unlock(hctx, srcu_idx);
 
 	if (need_run) {
 		__blk_mq_delay_run_hw_queue(hctx, async, 0);

@@ -1618,7 +1621,7 @@ static blk_qc_t request_to_qc_t(struct blk_mq_hw_ctx *hctx, struct request *rq)
 
 static void __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 					struct request *rq,
-					blk_qc_t *cookie, bool may_sleep)
+					blk_qc_t *cookie)
 {
 	struct request_queue *q = rq->q;
 	struct blk_mq_queue_data bd = {

@@ -1668,25 +1671,20 @@ static void __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 	}
 
 insert:
-	blk_mq_sched_insert_request(rq, false, run_queue, false, may_sleep);
+	blk_mq_sched_insert_request(rq, false, run_queue, false,
+					hctx->flags & BLK_MQ_F_BLOCKING);
 }
 
 static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 		struct request *rq, blk_qc_t *cookie)
 {
-	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
-		rcu_read_lock();
-		__blk_mq_try_issue_directly(hctx, rq, cookie, false);
-		rcu_read_unlock();
-	} else {
-		unsigned int srcu_idx;
+	int srcu_idx;
 
-		might_sleep();
+	might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
 
-		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
-		__blk_mq_try_issue_directly(hctx, rq, cookie, true);
-		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
-	}
+	hctx_lock(hctx, &srcu_idx);
+	__blk_mq_try_issue_directly(hctx, rq, cookie);
+	hctx_unlock(hctx, srcu_idx);
 }
 
 static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+ICtz dGF0aWMgdm9pZCBoY3R4X3VubG9jayhzdHJ1Y3QgYmxrX21xX2h3X2N0eCAqaGN0eCwgaW50IHNy Y3VfaWR4KQ0KPiArew0KPiArCWlmICghKGhjdHgtPmZsYWdzICYgQkxLX01RX0ZfQkxPQ0tJTkcp KQ0KPiArCQlyY3VfcmVhZF91bmxvY2soKTsNCj4gKwllbHNlDQo+ICsJCXNyY3VfcmVhZF91bmxv Y2soaGN0eC0+cXVldWVfcnFfc3JjdSwgc3JjdV9pZHgpOw0KPiArfQ0KPiArDQo+ICtzdGF0aWMg dm9pZCBoY3R4X2xvY2soc3RydWN0IGJsa19tcV9od19jdHggKmhjdHgsIGludCAqc3JjdV9pZHgp DQo+ICt7DQo+ICsJaWYgKCEoaGN0eC0+ZmxhZ3MgJiBCTEtfTVFfRl9CTE9DS0lORykpDQo+ICsJ CXJjdV9yZWFkX2xvY2soKTsNCj4gKwllbHNlDQo+ICsJCSpzcmN1X2lkeCA9IHNyY3VfcmVhZF9s b2NrKGhjdHgtPnF1ZXVlX3JxX3NyY3UpOw0KPiArfQ0KDQpBIG1pbm9yIGNvbW1lbnQ6IHBsZWFz ZSBjb25zaWRlciB0byByZW9yZGVyIHRoZXNlIHR3byBmdW5jdGlvbnMgc3VjaCB0aGF0IHRoZQ0K bG9jayBmdW5jdGlvbiBhcHBlYXJzIGZpcnN0IGFuZCB0aGUgdW5sb2NrIGZ1bmN0aW9uIHNlY29u ZC4gQW55d2F5Og0KDQpSZXZpZXdlZC1ieTogQmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2No ZUB3ZGMuY29tPg0KDQo=

Currently, blk-mq protects only the issue path with RCU. This patch puts the completion path under the same RCU protection. This will be used to synchronize issue/completion against timeout by later patches, which will also add the comments.

---
 block/blk-mq.c | 5 +++++
 1 file changed, 5 insertions(+)

index ddc9261..6741c3e 100644
@@ -584,11 +584,16 @@ static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)
 void blk_mq_complete_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
+	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, rq->mq_ctx->cpu);
+	int srcu_idx;
 
 	if (unlikely(blk_should_fake_timeout(q)))
 		return;
+
+	hctx_lock(hctx, &srcu_idx);
 	if (!blk_mark_rq_complete(rq))
 		__blk_mq_complete_request(rq);
+	hctx_unlock(hctx, srcu_idx);
 }
 EXPORT_SYMBOL(blk_mq_complete_request);
 

2.9.5

It's not a somewhat false positive, it's a false positive. I haven't seen that bogus warning with the compiler I'm running:

gcc (Ubuntu 7.2.0-1ubuntu1~16.04) 7.2.0

and

gcc (GCC) 7.2.0

Neither of them throw the warning.

Jens Axboe

Are you on non-x86? Really bothers me to have to add a work-around for something that's obviously a false positive.

I forget if we have some gcc/compiler annotation for this, otherwise the good old

int srcu_idx = srcu_idx;

should get the job done.

Jens Axboe

No, plain old x86-64 on Gentoo (*without* crazy hardening or extras). But don't bother spending too much time on this, I can live with the warning even though I found it curious. Not even sure why you don't see this; according to git -Wmaybe-uninitialized is part of the "extrawarn" make flags since ~2016.

Apparently gcc can't see that the first branch in the lock-helper implies that the first branch in the unlock helper will also be taken unconditionally (and how could it? nested branch condition elision?), so it concludes srcu_idx "may" be used uninitialized in unlock, and that's not generally wrong. It just happens to be in this case.

I actually went looking for gcc bugs, pragmas, annotations and whatnot; there are many bugs depending on optimizer, the #pragmas don't work and a potential __attribute__(initialized) was only discussed, but apparently never implemented. \o/

(Narrator: It didn't.)

Isn't there some magic value that will never be used by regular operations? 0 or -1? It should not matter much since it will be consistently overwritten or left alone.

cheers
Holger

Narrator: we retract our previous statement and apologize for the confusion. It works fine when you correctly replace all occurrences, not just one. <:)

Holger

Mapping is the right thing to do. We cache the software queue, which allows us to map back to the same hardware queue. There would be no point in storing both, the mapping is very cheap. No point in bloating the request with redundant information.

Jens Axboe

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEN1 cnJlbnRseSwgYmxrLW1xIHByb3RlY3RzIG9ubHkgdGhlIGlzc3VlIHBhdGggd2l0aCBSQ1UuICBU aGlzIHBhdGNoDQo+IHB1dHMgdGhlIGNvbXBsZXRpb24gcGF0aCB1bmRlciB0aGUgc2FtZSBSQ1Ug cHJvdGVjdGlvbi4gIFRoaXMgd2lsbCBiZQ0KPiB1c2VkIHRvIHN5bmNocm9uaXplIGlzc3VlL2Nv bXBsZXRpb24gYWdhaW5zdCB0aW1lb3V0IGJ5IGxhdGVyIHBhdGNoZXMsDQo+IHdoaWNoIHdpbGwg YWxzbyBhZGQgdGhlIGNvbW1lbnRzLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogVGVqdW4gSGVvIDx0 akBrZXJuZWwub3JnPg0KPiAtLS0NCj4gIGJsb2NrL2Jsay1tcS5jIHwgNSArKysrKw0KPiAgMSBm aWxlIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2Jsb2NrL2Js ay1tcS5jIGIvYmxvY2svYmxrLW1xLmMNCj4gaW5kZXggZGRjOTI2MS4uNjc0MWMzZSAxMDA2NDQN Cj4gLS0tIGEvYmxvY2svYmxrLW1xLmMNCj4gKysrIGIvYmxvY2svYmxrLW1xLmMNCj4gQEAgLTU4 NCwxMSArNTg0LDE2IEBAIHN0YXRpYyB2b2lkIGhjdHhfbG9jayhzdHJ1Y3QgYmxrX21xX2h3X2N0 eCAqaGN0eCwgaW50ICpzcmN1X2lkeCkNCj4gIHZvaWQgYmxrX21xX2NvbXBsZXRlX3JlcXVlc3Qo c3RydWN0IHJlcXVlc3QgKnJxKQ0KPiAgew0KPiAgCXN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxID0g cnEtPnE7DQo+ICsJc3RydWN0IGJsa19tcV9od19jdHggKmhjdHggPSBibGtfbXFfbWFwX3F1ZXVl KHEsIHJxLT5tcV9jdHgtPmNwdSk7DQo+ICsJaW50IHNyY3VfaWR4Ow0KPiAgDQo+ICAJaWYgKHVu bGlrZWx5KGJsa19zaG91bGRfZmFrZV90aW1lb3V0KHEpKSkNCj4gIAkJcmV0dXJuOw0KPiArDQo+ ICsJaGN0eF9sb2NrKGhjdHgsICZzcmN1X2lkeCk7DQo+ICAJaWYgKCFibGtfbWFya19ycV9jb21w bGV0ZShycSkpDQo+ICAJCV9fYmxrX21xX2NvbXBsZXRlX3JlcXVlc3QocnEpOw0KPiArCWhjdHhf dW5sb2NrKGhjdHgsIHNyY3VfaWR4KTsNCj4gIH0NCj4gIEVYUE9SVF9TWU1CT0woYmxrX21xX2Nv bXBsZXRlX3JlcXVlc3QpOw0KDQpIZWxsbyBUZWp1biwNCg0KSSdtIGNvbmNlcm5lZCBhYm91dCB0 aGUgYWRkaXRpb25hbCBDUFUgY3ljbGVzIG5lZWRlZCBmb3IgdGhlIG5ldyBibGtfbXFfbWFwX3F1 ZXVlKCkNCmNhbGwsIGFsdGhvdWdoIEkga25vdyB0aGlzIGNhbGwgaXMgY2hlYXAuIFdvdWxkIHRo ZSB0aW1lb3V0IGNvZGUgcmVhbGx5IGdldCB0aGF0DQptdWNoIG1vcmUgY29tcGxpY2F0ZWQgaWYg dGhlIGhjdHhfbG9jaygpIGFuZCBoY3R4X3VubG9jaygpIGNhbGxzIHdvdWxkIGJlIGNoYW5nZWQN CmludG8gcmN1X3JlYWRfbG9jaygpIGFuZCByY3VfcmVhZF91bmxvY2soKSBjYWxscz8gV291bGQg aXQgYmUgc3VmZmljaWVudCBpZg0KImlmIChoYXNfcmN1KSBzeW5jaHJvbml6ZV9yY3UoKTsiIHdv dWxkIGJlIGNoYW5nZWQgaW50byAic3luY2hyb25pemVfcmN1KCk7IiBpbg0KYmxrX21xX3RpbWVv dXRfd29yaygpPw0KDQpUaGFua3MsDQoNCkJhcnQu

It's a non concern, imho. We do queue mapping all over the place for a variety of reasons, it's total noise, especially since we're calling [s]rcu anyway after that.

Jens Axboe

Hello, Bart.

So, if that is really a concern, let's cache that mapping instead of changing synchronization rules for that.

Code-wise, it won't be too much extra code but I think diverging the sync methods between issue and completion paths is more fragile and likely to invite confusions and mistakes in the future. We have the normal path (issue&completion) synchronizing against the exception path (timeout). I think it's best to keep the sync constructs aligned with that conceptual picture.

Thanks.

tejun

Guys, the map call is FINE. We do it at least once per request anyway, usually multiple times. If it's too expensive, then THAT is what needs fixing, not adding an arbitrary caching of that mapping. Look at the actual code:

	return q->queue_hw_ctx[q->mq_map[cpu]];

that's it. It's just a double index.

So let's put this thing to rest right now - the map call is perfectly fine, especially since the alternatives are either bloating struct request or making the code less maintainable.

Foot -> down.

Jens Axboe

Currently, blk-mq timeout path synchronizes against the usual issue/completion path using a complex scheme involving atomic bitflags, REQ_ATOM_*, memory barriers and subtle memory coherence rules. Unfortunately, it contains quite a few holes.

There's a complex dancing around REQ_ATOM_STARTED and REQ_ATOM_COMPLETE between issue/completion and timeout paths; however, they don't have a synchronization point across request recycle instances and it isn't clear what the barriers add. blk_mq_check_expired() can easily read STARTED from N-2'th iteration, deadline from N-1'th, blk_mark_rq_complete() against Nth instance.

In fact, it's pretty easy to make blk_mq_check_expired() terminate a later instance of a request. If we induce 5 sec delay before time_after_eq() test in blk_mq_check_expired(), shorten the timeout to 2s, and issue back-to-back large IOs, blk-mq starts timing out requests spuriously pretty quickly. Nothing actually timed out. It just made the call on a recycle instance of a request and then terminated a later instance long after the original instance finished. The scenario isn't theoretical either.

This patch replaces the broken synchronization mechanism with a RCU and generation number based one.

1. Each request has a u64 generation + state value, which can be updated only by the request owner. Whenever a request becomes in-flight, the generation number gets bumped up too. This provides the basis for the timeout path to distinguish different recycle instances of the request.

Also, marking a request in-flight and setting its deadline are protected with a seqcount so that the timeout path can fetch both values coherently.

2. The timeout path fetches the generation, state and deadline. If the verdict is timeout, it records the generation into a dedicated request abortion field and does RCU wait.

3. The completion path is also protected by RCU (from the previous patch) and checks whether the current generation number and state match the abortion field. If so, it skips completion.

4. The timeout path, after RCU wait, scans requests again and terminates the ones whose generation and state still match the ones requested for abortion.

By now, the timeout path knows that either the generation number and state changed if it lost the race or the completion will yield to it and can safely timeout the request.

While it's more lines of code, it's conceptually simpler, doesn't depend on direct use of subtle memory ordering or coherence, and hopefully doesn't terminate the wrong instance.

While this change makes REQ_ATOM_COMPLETE synchronization unnecessary between issue/complete and timeout paths, REQ_ATOM_COMPLETE isn't removed yet as it's still used in other places. Future patches will move all state tracking to the new mechanism and remove all bitops in the hot paths.

v2: - Fixed BLK_EH_RESET_TIMER handling as pointed out by Jianchao.
- s/request->gstate_seqc/request->gstate_seq/ as suggested by Peter.
- READ_ONCE() added in blk_mq_rq_update_state() as suggested by Peter.

v3: - Fixed possible extended seqcount / u64_stats_sync read looping
spotted by Peter.
- MQ_RQ_IDLE was incorrectly being set in complete_request instead of free_request. Fixed.

v4: - Rebased on top of hctx_lock() refactoring patch.
- Added comment explaining the use of hctx_lock() in completion path.

---
 block/blk-core.c       |   2 +
 block/blk-mq.c         | 223 +++++++++++++++++++++++++++++++++----------------
 block/blk-mq.h         |  46 ++++++++++
 block/blk-timeout.c    |   2 +-
 block/blk.h            |   6 --
 include/linux/blk-mq.h |   1 +
 include/linux/blkdev.h |  23 +++++
 7 files changed, 224 insertions(+), 79 deletions(-)

index 2e0d041..f843ae4f 100644
@@ -126,6 +126,8 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
 	rq->part = NULL;
+	seqcount_init(&rq->gstate_seq);
+	u64_stats_init(&rq->aborted_gstate_sync);
 }
 EXPORT_SYMBOL(blk_rq_init);
 

index 764ecf9..6427be7 100644
@@ -208,7 +208,7 @@ void blk_add_timer(struct request *req)
 	if (!req->timeout)
 		req->timeout = q->rq_timeout;
 
-	WRITE_ONCE(req->deadline, jiffies + req->timeout);
+	req->deadline = jiffies + req->timeout;
 
 	/*
 	 * Only the non-mq case needs to add the request to a protected list.

index 3f14469..9cb2739 100644
@@ -123,12 +123,6 @@ void blk_account_io_done(struct request *req);
  * Internal atomic flags for request handling
  */
 enum rq_atomic_flags {
-	/*
-	 * Keep these two bits first - not because we depend on the
-	 * value of them, but we do depend on them being in the same
-	 * byte of storage to ensure ordering on writes. Keeping them
-	 * first will achieve that nicely.
-	 */
 	REQ_ATOM_COMPLETE = 0,
 	REQ_ATOM_STARTED,
 

index 95c9a5c..460798d 100644
@@ -51,6 +51,7 @@ struct blk_mq_hw_ctx {
 	unsigned int		queue_num;
 
 	atomic_t		nr_active;
+	unsigned int		nr_expired;
 
 	struct hlist_node	cpuhp_dead;
 	struct kobject		kobj;

index 46e606f..2206607 100644
@@ -27,6 +27,8 @@
 #include <linux/percpu-refcount.h>
 #include <linux/scatterlist.h>
 #include <linux/blkzoned.h>
+#include <linux/seqlock.h>
+#include <linux/u64_stats_sync.h>
 
 struct module;
 struct scsi_ioctl_command;

@@ -230,6 +232,27 @@ struct request {
 
 	unsigned short write_hint;
 
+	/*
+	 * On blk-mq, the lower bits of ->gstate carry the MQ_RQ_* state
+	 * value and the upper bits the generation number which is
+	 * monotonically incremented and used to distinguish the reuse
+	 * instances.
+	 *
+	 * ->gstate_seq allows updates to ->gstate and other fields
+	 * (currently ->deadline) during request start to be read
+	 * atomically from the timeout path, so that it can operate on a
+	 * coherent set of information.
+	 */
+	seqcount_t gstate_seq;
+	u64 gstate;
+
+	/*
+	 * ->aborted_gstate is used by the timeout to claim a specific
+	 * recycle instance of this request.  See blk_mq_timeout_work().
+	 */
+	struct u64_stats_sync aborted_gstate_sync;
+	u64 aborted_gstate;
+
 	unsigned long deadline;
 	struct list_head timeout_list;
 

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+ICtz dGF0aWMgdm9pZCBibGtfbXFfcnFfdXBkYXRlX2Fib3J0ZWRfZ3N0YXRlKHN0cnVjdCByZXF1ZXN0 ICpycSwgdTY0IGdzdGF0ZSkNCj4gK3sNCj4gKwl1bnNpZ25lZCBsb25nIGZsYWdzOw0KPiArDQo+ ICsJbG9jYWxfaXJxX3NhdmUoZmxhZ3MpOw0KPiArCXU2NF9zdGF0c191cGRhdGVfYmVnaW4oJnJx LT5hYm9ydGVkX2dzdGF0ZV9zeW5jKTsNCj4gKwlycS0+YWJvcnRlZF9nc3RhdGUgPSBnc3RhdGU7 DQo+ICsJdTY0X3N0YXRzX3VwZGF0ZV9lbmQoJnJxLT5hYm9ydGVkX2dzdGF0ZV9zeW5jKTsNCj4g Kwlsb2NhbF9pcnFfcmVzdG9yZShmbGFncyk7DQo+ICt9DQoNClBsZWFzZSBhZGQgYSBjb21tZW50 IHRoYXQgZXhwbGFpbnMgdGhlIHB1cnBvc2Ugb2YgbG9jYWxfaXJxX3NhdmUoKSBhbmQNCmxvY2Fs X2lycV9yZXN0b3JlKCkuIFBsZWFzZSBhbHNvIGV4cGxhaW4gd2h5IHlvdSBjaG9zZSB0byBkaXNh YmxlIGludGVycnVwdHMNCmluc3RlYWQgb2YgZGlzYWJsaW5nIHByZWVtcHRpb24uIEkgdGhpbmsg dGhhdCBkaXNhYmxpbmcgcHJlZW1wdGlvbiBzaG91bGQgYmUNCnN1ZmZpY2llbnQgc2luY2UgdGhp cyBpcyB0aGUgb25seSBjb2RlIHRoYXQgdXBkYXRlcyBycS0+YWJvcnRlZF9nc3RhdGUgYW5kDQpz aW5jZSB0aGlzIGZ1bmN0aW9uIGlzIGFsd2F5cyBjYWxsZWQgZnJvbSB0aHJlYWQgY29udGV4dC4N Cg0KPiBAQCAtODAxLDYgKzg0MCwxMiBAQCB2b2lkIGJsa19tcV9ycV90aW1lZF9vdXQoc3RydWN0 IHJlcXVlc3QgKnJlcSwgYm9vbCByZXNlcnZlZCkNCj4gIAkJX19ibGtfbXFfY29tcGxldGVfcmVx dWVzdChyZXEpOw0KPiAgCQlicmVhazsNCj4gIAljYXNlIEJMS19FSF9SRVNFVF9USU1FUjoNCj4g KwkJLyoNCj4gKwkJICogQXMgbm90aGluZyBwcmV2ZW50cyBmcm9tIGNvbXBsZXRpb24gaGFwcGVu aW5nIHdoaWxlDQo+ICsJCSAqIC0+YWJvcnRlZF9nc3RhdGUgaXMgc2V0LCB0aGlzIG1heSBsZWFk IHRvIGlnbm9yZWQNCj4gKwkJICogY29tcGxldGlvbnMgYW5kIGZ1cnRoZXIgc3B1cmlvdXMgdGlt ZW91dHMuDQo+ICsJCSAqLw0KPiArCQlibGtfbXFfcnFfdXBkYXRlX2Fib3J0ZWRfZ3N0YXRlKHJl cSwgMCk7DQo+ICAJCWJsa19hZGRfdGltZXIocmVxKTsNCj4gIAkJYmxrX2NsZWFyX3JxX2NvbXBs ZXRlKHJlcSk7DQo+ICAJCWJyZWFrOw0KDQpJcyB0aGUgcmFjZSB0aGF0IHRoZSBjb21tZW50IHJl ZmVycyB0byBhZGRyZXNzZWQgYnkgb25lIG9mIHRoZSBsYXRlciBwYXRjaGVzPw0KDQpUaGFua3Ms DQoNCkJhcnQu

Will do.

blk_mq_complete_request() can read it from the irq context. If that happens between update_begin and end, it'll end up looping infinitely.

No, but it's not a new race. It has always been there and I suppose doesn't lead to practical problems - the race window is pretty small and the effect isn't critical. I'm just documenting the existing race condition. Will note that in the description.

Thanks.

tejun

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEBA IC0yMzAsNiArMjMyLDI3IEBAIHN0cnVjdCByZXF1ZXN0IHsNCj4gIA0KPiAgCXVuc2lnbmVkIHNo b3J0IHdyaXRlX2hpbnQ7DQo+ICANCj4gKwkvKg0KPiArCSAqIE9uIGJsay1tcSwgdGhlIGxvd2Vy IGJpdHMgb2YgLT5nc3RhdGUgY2FycnkgdGhlIE1RX1JRXyogc3RhdGUNCj4gKwkgKiB2YWx1ZSBh bmQgdGhlIHVwcGVyIGJpdHMgdGhlIGdlbmVyYXRpb24gbnVtYmVyIHdoaWNoIGlzDQo+ICsJICog bW9ub3RvbmljYWxseSBpbmNyZW1lbnRlZCBhbmQgdXNlZCB0byBkaXN0aW5ndWlzaCB0aGUgcmV1 c2UNCj4gKwkgKiBpbnN0YW5jZXMuDQo+ICsJICoNCj4gKwkgKiAtPmdzdGF0ZV9zZXEgYWxsb3dz IHVwZGF0ZXMgdG8gLT5nc3RhdGUgYW5kIG90aGVyIGZpZWxkcw0KPiArCSAqIChjdXJyZW50bHkg LT5kZWFkbGluZSkgZHVyaW5nIHJlcXVlc3Qgc3RhcnQgdG8gYmUgcmVhZA0KPiArCSAqIGF0b21p Y2FsbHkgZnJvbSB0aGUgdGltZW91dCBwYXRoLCBzbyB0aGF0IGl0IGNhbiBvcGVyYXRlIG9uIGEN Cj4gKwkgKiBjb2hlcmVudCBzZXQgb2YgaW5mb3JtYXRpb24uDQo+ICsJICovDQo+ICsJc2VxY291 bnRfdCBnc3RhdGVfc2VxOw0KPiArCXU2NCBnc3RhdGU7DQo+ICsNCj4gKwkvKg0KPiArCSAqIC0+ YWJvcnRlZF9nc3RhdGUgaXMgdXNlZCBieSB0aGUgdGltZW91dCB0byBjbGFpbSBhIHNwZWNpZmlj DQo+ICsJICogcmVjeWNsZSBpbnN0YW5jZSBvZiB0aGlzIHJlcXVlc3QuICBTZWUgYmxrX21xX3Rp bWVvdXRfd29yaygpLg0KPiArCSAqLw0KPiArCXN0cnVjdCB1NjRfc3RhdHNfc3luYyBhYm9ydGVk X2dzdGF0ZV9zeW5jOw0KPiArCXU2NCBhYm9ydGVkX2dzdGF0ZTsNCj4gKw0KPiAgCXVuc2lnbmVk IGxvbmcgZGVhZGxpbmU7DQo+ICAJc3RydWN0IGxpc3RfaGVhZCB0aW1lb3V0X2xpc3Q7DQoNCkRv ZXMgImdzdGF0ZSIgcGVyaGFwcyBzdGFuZCBmb3IgImdlbmVyYXRpb24gbnVtYmVyIGFuZCBzdGF0 ZSI/IElmIHNvLCBwbGVhc2UNCm1lbnRpb24gdGhpcyBpbiBvbmUgb2YgdGhlIGFib3ZlIGNvbW1l bnRzLg0KDQpUaGFua3MsDQoNCkJhcnQu

Yeah, will do.

Thanks.

tejun

blk_mq_check_inflight() and blk_mq_poll_hybrid_sleep() test REQ_ATOM_COMPLETE to determine the request state. Both uses are speculative and we can test REQ_ATOM_STARTED and blk_mq_rq_state() for equivalent results. Replace the tests. This will allow removing REQ_ATOM_COMPLETE usages from blk-mq.

---
 block/blk-mq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 6587f0c..41bfd27 100644
@@ -95,8 +95,7 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 {
 	struct mq_inflight *mi = priv;
 
-	if (test_bit(REQ_ATOM_STARTED, &rq->atomic_flags) &&
-	    !test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags)) {
+	if (blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT) {
 		/*
 		 * index[0] counts the specific partition that was asked
 		 * for. index[1] counts the ones that are active on the

@@ -3017,7 +3016,8 @@ static bool blk_mq_poll_hybrid_sleep(struct request_queue *q,
 
 	hrtimer_init_sleeper(&hs, current);
 	do {
-		if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags))
+		if (test_bit(REQ_ATOM_STARTED, &rq->atomic_flags) &&
+		    blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT)
 			break;
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		hrtimer_start_expires(&hs.timer, mode);

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IGJs a19tcV9jaGVja19pbmZsaWdodCgpIGFuZCBibGtfbXFfcG9sbF9oeWJyaWRfc2xlZXAoKSB0ZXN0 DQo+IFJFUV9BVE9NX0NPTVBMRVRFIHRvIGRldGVybWluZSB0aGUgcmVxdWVzdCBzdGF0ZS4gIEJv dGggdXNlcyBhcmUNCj4gc3BlY3VsYXRpdmUgYW5kIHdlIGNhbiB0ZXN0IFJFUV9BVE9NX1NUQVJU RUQgYW5kIGJsa19tcV9ycV9zdGF0ZSgpIGZvcg0KPiBlcXVpdmFsZW50IHJlc3VsdHMuICBSZXBs YWNlIHRoZSB0ZXN0cy4gIFRoaXMgd2lsbCBhbGxvdyByZW1vdmluZw0KPiBSRVFfQVRPTV9DT01Q TEVURSB1c2FnZXMgZnJvbSBibGstbXEuDQoNClJldmlld2VkLWJ5OiBCYXJ0IFZhbiBBc3NjaGUg PGJhcnQudmFuYXNzY2hlQHdkYy5jb20+DQoNCg==

With issue/complete and timeout paths now using the generation number and state based synchronization, blk_abort_request() is the only one which depends on REQ_ATOM_COMPLETE for arbitrating completion.

There's no reason for blk_abort_request() to be a completely separate path. This patch makes blk_abort_request() piggyback on the timeout path instead of trying to terminate the request directly.

This removes the last dependency on REQ_ATOM_COMPLETE in blk-mq.

Note that this makes blk_abort_request() asynchronous - it initiates abortion but the actual termination will happen after a short while, even when the caller owns the request. AFAICS, SCSI and ATA should be fine with that and I think mtip32xx and dasd should be safe but not completely sure. It'd be great if people who know the drivers take a look.

---
 block/blk-mq.c      | 2 +-
 block/blk-mq.h      | 2 --
 block/blk-timeout.c | 8 ++++----
 3 files changed, 5 insertions(+), 7 deletions(-)

index 41bfd27..bed4cb8 100644
@@ -814,7 +814,7 @@ struct blk_mq_timeout_data {
 	unsigned int nr_expired;
 };
 
-void blk_mq_rq_timed_out(struct request *req, bool reserved)
+static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 {
 	const struct blk_mq_ops *ops = req->q->mq_ops;
 	enum blk_eh_timer_return ret = BLK_EH_RESET_TIMER;

index cf01f6f..6b2d616 100644
@@ -94,8 +94,6 @@ extern int blk_mq_sysfs_register(struct request_queue *q);
 extern void blk_mq_sysfs_unregister(struct request_queue *q);
 extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
 
-extern void blk_mq_rq_timed_out(struct request *req, bool reserved);
-
 void blk_mq_release(struct request_queue *q);
 
 /**

index 6427be7..d580af3 100644
@@ -156,12 +156,12 @@ void blk_timeout_work(struct work_struct *work)
  */
 void blk_abort_request(struct request *req)
 {
-	if (blk_mark_rq_complete(req))
-		return;
-
 	if (req->q->mq_ops) {
-		blk_mq_rq_timed_out(req, false);
+		req->deadline = jiffies;
+		mod_timer(&req->q->timeout, 0);
 	} else {
+		if (blk_mark_rq_complete(req))
+			return;
 		blk_delete_timer(req);
 		blk_rq_timed_out(req);
 	}

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEBA IC0xNTYsMTIgKzE1NiwxMiBAQCB2b2lkIGJsa190aW1lb3V0X3dvcmsoc3RydWN0IHdvcmtfc3Ry dWN0ICp3b3JrKQ0KPiAgICovDQo+ICB2b2lkIGJsa19hYm9ydF9yZXF1ZXN0KHN0cnVjdCByZXF1 ZXN0ICpyZXEpDQo+ICB7DQo+IC0JaWYgKGJsa19tYXJrX3JxX2NvbXBsZXRlKHJlcSkpDQo+IC0J CXJldHVybjsNCj4gLQ0KPiAgCWlmIChyZXEtPnEtPm1xX29wcykgew0KPiAtCQlibGtfbXFfcnFf dGltZWRfb3V0KHJlcSwgZmFsc2UpOw0KPiArCQlyZXEtPmRlYWRsaW5lID0gamlmZmllczsNCj4g KwkJbW9kX3RpbWVyKCZyZXEtPnEtPnRpbWVvdXQsIDApOw0KPiAgCX0gZWxzZSB7DQo+ICsJCWlm IChibGtfbWFya19ycV9jb21wbGV0ZShyZXEpKQ0KPiArCQkJcmV0dXJuOw0KPiAgCQlibGtfZGVs ZXRlX3RpbWVyKHJlcSk7DQo+ICAJCWJsa19ycV90aW1lZF9vdXQocmVxKTsNCj4gIAl9DQoNCk90 aGVyIHJlcS0+ZGVhZGxpbmUgd3JpdGVzIGFyZSBwcm90ZWN0ZWQgYnkgcHJlZW1wdF9kaXNhYmxl KCksDQp3cml0ZV9zZXFjb3VudF9iZWdpbigmcnEtPmdzdGF0ZV9zZXEpLCB3cml0ZV9zZXFjb3Vu dF9lbmQoJnJxLT5nc3RhdGVfc2VxKQ0KYW5kIHByZWVtcHRfZW5hYmxlKCkuIEkgdGhpbmsgaXQn cyBmaW5lIHRoYXQgdGhlIGFib3ZlIHJlcS0+ZGVhZGxpbmUgc3RvcmUNCmRvZXMgbm90IGhhdmUg dGhhdCBwcm90ZWN0aW9uIGJ1dCBJIGFsc28gdGhpbmsgdGhhdCB0aGF0IGRlc2VydmVzIGEgY29t bWVudC4NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

Will add.

Thanks.

tejun

After the recent updates to use generation number and state based synchronization, blk-mq no longer depends on REQ_ATOM_COMPLETE except to avoid firing the same timeout multiple times.

Remove all REQ_ATOM_COMPLETE usages and use a new rq_flags flag RQF_MQ_TIMEOUT_EXPIRED to avoid firing the same timeout multiple times. This removes atomic bitops from hot paths too.

v2: Removed blk_clear_rq_complete() from blk_mq_rq_timed_out().

v3: Added RQF_MQ_TIMEOUT_EXPIRED flag.

---
 block/blk-mq.c         | 15 +++++++--------
 block/blk-timeout.c    |  1 +
 include/linux/blkdev.h |  2 ++
 3 files changed, 10 insertions(+), 8 deletions(-)

index bed4cb8..050d982 100644
@@ -628,8 +628,7 @@ void blk_mq_complete_request(struct request *rq)
 	 * hctx_lock() covers both issue and completion paths.
 	 */
 	hctx_lock(hctx, &srcu_idx);
-	if (blk_mq_rq_aborted_gstate(rq) != rq->gstate &&
-	    !blk_mark_rq_complete(rq))
+	if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
 		__blk_mq_complete_request(rq);
 	hctx_unlock(hctx, srcu_idx);
 }

@@ -679,8 +678,6 @@ void blk_mq_start_request(struct request *rq)
 	preempt_enable();
 
 	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);
 
 	if (q->dma_drain_size && blk_rq_bytes(rq)) {
 		/*

@@ -831,6 +828,8 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 	if (!test_bit(REQ_ATOM_STARTED, &req->atomic_flags))
 		return;
 
+	req->rq_flags |= RQF_MQ_TIMEOUT_EXPIRED;
+
 	if (ops->timeout)
 		ret = ops->timeout(req, reserved);
 

@@ -846,7 +845,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 		 */
 		blk_mq_rq_update_aborted_gstate(req, 0);
 		blk_add_timer(req);
-		blk_clear_rq_complete(req);
 		break;
 	case BLK_EH_NOT_HANDLED:
 		break;

@@ -865,7 +863,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 
 	might_sleep();
 
-	if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+	if ((rq->rq_flags & RQF_MQ_TIMEOUT_EXPIRED) ||
+	    !test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
 		return;
 
 	/* read coherent snapshots of @rq->state_gen and @rq->deadline */

@@ -900,8 +899,8 @@ static void blk_mq_terminate_expired(struct blk_mq_hw_ctx *hctx,
 	 * now guaranteed to see @rq->aborted_gstate and yield.  If
 	 * @rq->aborted_gstate still matches @rq->gstate, @rq is ours.
 	 */
-	if (READ_ONCE(rq->gstate) == rq->aborted_gstate &&
-	    !blk_mark_rq_complete(rq))
+	if (!(rq->rq_flags & RQF_MQ_TIMEOUT_EXPIRED) &&
+	    READ_ONCE(rq->gstate) == rq->aborted_gstate)
 		blk_mq_rq_timed_out(rq, reserved);
 }
 

index d580af3..25c4ffa 100644
@@ -209,6 +209,7 @@ void blk_add_timer(struct request *req)
 		req->timeout = q->rq_timeout;
 
 	req->deadline = jiffies + req->timeout;
+	req->rq_flags &= ~RQF_MQ_TIMEOUT_EXPIRED;
 
 	/*
 	 * Only the non-mq case needs to add the request to a protected list.

index 2206607..e2a4afc 100644
@@ -125,6 +125,8 @@ typedef __u32 __bitwise req_flags_t;
 #define RQF_SPECIAL_PAYLOAD	((__force req_flags_t)(1 << 18))
 /* The per-zone write lock is held for this request */
 #define RQF_ZONE_WRITE_LOCKED	((__force req_flags_t)(1 << 19))
+/* timeout is expired */
+#define RQF_MQ_TIMEOUT_EXPIRED	((__force req_flags_t)(1 << 20))
 
 /* flags that prevent us from merging requests: */
 #define RQF_NOMERGE_FLAGS \

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEFm dGVyIHRoZSByZWNlbnQgdXBkYXRlcyB0byB1c2UgZ2VuZXJhdGlvbiBudW1iZXIgYW5kIHN0YXRl IGJhc2VkDQo+IHN5bmNocm9uaXphdGlvbiwgYmxrLW1xIG5vIGxvbmdlciBkZXBlbmRzIG9uIFJF UV9BVE9NX0NPTVBMRVRFIGV4Y2VwdA0KPiB0byBhdm9pZCBmaXJpbmcgdGhlIHNhbWUgdGltZW91 dCBtdWx0aXBsZSB0aW1lcy4NCj4gDQo+IFJlbW92ZSBhbGwgUkVRX0FUT01fQ09NUExFVEUgdXNh Z2VzIGFuZCB1c2UgYSBuZXcgcnFfZmxhZ3MgZmxhZw0KPiBSUUZfTVFfVElNRU9VVF9FWFBJUkVE IHRvIGF2b2lkIGZpcmluZyB0aGUgc2FtZSB0aW1lb3V0IG11bHRpcGxlDQo+IHRpbWVzLiAgVGhp cyByZW1vdmVzIGF0b21pYyBiaXRvcHMgZnJvbSBob3QgcGF0aHMgdG9vLg0KPiANCj4gdjI6IFJl bW92ZWQgYmxrX2NsZWFyX3JxX2NvbXBsZXRlKCkgZnJvbSBibGtfbXFfcnFfdGltZWRfb3V0KCku DQo+IA0KPiB2MzogQWRkZWQgUlFGX01RX1RJTUVPVVRfRVhQSVJFRCBmbGFnLg0KDQpJIHRoaW5r IGl0J3MgdW5mb3J0dW5hdGUgdGhhdCB0aGlzIHBhdGNoIHNwcmVhZHMgdGhlIHJlcXVlc3Qgc3Rh dGUgb3ZlciB0d28NCnN0cnVjdCByZXF1ZXN0IG1lbWJlcnMsIG5hbWVseSB0aGUgbG93ZXIgdHdv IGJpdHMgb2YgZ3N0YXRlIGFuZCB0aGUNClJRRl9NUV9USU1FT1VUX0VYUElSRUQgZmxhZyBpbiBy ZXEtPnJxX2ZsYWdzLiBQbGVhc2UgY29uc2lkZXIgdG8gZHJvcCB0aGUNClJRRl9NUV9USU1FT1VU X0VYUElSRUQgZmxhZyBhbmQgdG8gcmVwcmVzZW50IHRoZSAidGltZW91dCBoYXMgYmVlbiBwcm9j ZXNzZWQiDQpzdGF0ZSBhcyBhIE1RX1JRXyogc3RhdGUgaW4gZ3N0YXRlLiBUaGF0IHdpbGwgYXZv aWQgdGhhdCBldmVyeSBzdGF0ZSB1cGRhdGUNCmhhcyB0byBiZSByZXZpZXdlZCB3aGV0aGVyIG9y IG5vdCBwZXJoYXBzIGFuIHVwZGF0ZSBvZiB0aGUNClJRRl9NUV9USU1FT1VUX0VYUElSRUQgZmxh ZyBpcyBtaXNzaW5nLg0KDQpUaGFua3MsDQoNCkJhcnQu

After the recent updates to use generation number and state based synchronization, we can easily replace REQ_ATOM_STARTED usages by adding an extra state to distinguish completed but not yet freed state.

Add MQ_RQ_COMPLETE and replace REQ_ATOM_STARTED usages with blk_mq_rq_state() tests. REQ_ATOM_STARTED no longer has any users left and is removed.

---
 block/blk-mq-debugfs.c |  4 +---
 block/blk-mq.c         | 37 ++++++++-----------------------------
 block/blk-mq.h         |  1 +
 block/blk.h            |  1 -
 4 files changed, 10 insertions(+), 33 deletions(-)

index b56a4f3..8adc837 100644
@@ -271,7 +271,6 @@ static const char *const cmd_flag_name[] = {
 #define RQF_NAME(name) [ilog2((__force u32)RQF_##name)] = #name
 static const char *const rqf_name[] = {
 	RQF_NAME(SORTED),
-	RQF_NAME(STARTED),
 	RQF_NAME(QUEUED),
 	RQF_NAME(SOFTBARRIER),
 	RQF_NAME(FLUSH_SEQ),

@@ -295,7 +294,6 @@ static const char *const rqf_name[] = {
 #define RQAF_NAME(name) [REQ_ATOM_##name] = #name
 static const char *const rqaf_name[] = {
 	RQAF_NAME(COMPLETE),
-	RQAF_NAME(STARTED),
 	RQAF_NAME(POLL_SLEPT),
 };
 #undef RQAF_NAME

@@ -409,7 +407,7 @@ static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
 	const struct show_busy_params *params = data;
 
 	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
-	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+	    blk_mq_rq_state(rq) != MQ_RQ_IDLE)
 		__blk_mq_debugfs_rq_show(params->m,
 					 list_entry_rq(&rq->queuelist));
 }

index 050d982..54ecb2c 100644
@@ -483,7 +483,6 @@ void blk_mq_free_request(struct request *rq)
 		blk_put_rl(blk_rq_rl(rq));
 
 	blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
-	clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
 	clear_bit(REQ_ATOM_POLL_SLEPT, &rq->atomic_flags);
 	if (rq->tag != -1)
 		blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag);

@@ -531,6 +530,7 @@ static void __blk_mq_complete_request(struct request *rq)
 	int cpu;
 
 	WARN_ON_ONCE(blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT);
+	blk_mq_rq_update_state(rq, MQ_RQ_COMPLETE);
 
 	if (rq->internal_tag != -1)
 		blk_mq_sched_completed_request(rq);

@@ -636,7 +636,7 @@ EXPORT_SYMBOL(blk_mq_complete_request);
 
 int blk_mq_request_started(struct request *rq)
 {
-	return test_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
+	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
 }
 EXPORT_SYMBOL_GPL(blk_mq_request_started);
 

@@ -655,7 +655,6 @@ void blk_mq_start_request(struct request *rq)
 	}
 
 	WARN_ON_ONCE(blk_mq_rq_state(rq) != MQ_RQ_IDLE);
-	WARN_ON_ONCE(test_bit(REQ_ATOM_STARTED, &rq->atomic_flags));
 
 	/*
 	 * Mark @rq in-flight which also advances the generation number,

@@ -677,8 +676,6 @@ void blk_mq_start_request(struct request *rq)
 	write_seqcount_end(&rq->gstate_seq);
 	preempt_enable();
 
-	set_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
-
 	if (q->dma_drain_size && blk_rq_bytes(rq)) {
 		/*
 		 * Make sure space for the drain appears.  We know we can do

@@ -691,13 +688,9 @@ void blk_mq_start_request(struct request *rq)
 EXPORT_SYMBOL(blk_mq_start_request);
 
 /*
- * When we reach here because queue is busy, REQ_ATOM_COMPLETE
- * flag isn't set yet, so there may be race with timeout handler,
- * but given rq->deadline is just set in .queue_rq() under
- * this situation, the race won't be possible in reality because
- * rq->timeout should be set as big enough to cover the window
- * between blk_mq_start_request() called from .queue_rq() and
- * clearing REQ_ATOM_STARTED here.
+ * When we reach here because queue is busy, it's safe to change the state
+ * to IDLE without checking @rq->aborted_gstate because we should still be
+ * holding the RCU read lock and thus protected against timeout.
  */
 static void __blk_mq_requeue_request(struct request *rq)
 {

@@ -709,7 +702,7 @@ static void __blk_mq_requeue_request(struct request *rq)
 	wbt_requeue(q->rq_wb, &rq->issue_stat);
 	blk_mq_sched_requeue_request(rq);
 
-	if (test_and_clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) {
+	if (blk_mq_rq_state(rq) != MQ_RQ_IDLE) {
 		blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
 		if (q->dma_drain_size && blk_rq_bytes(rq))
 			rq->nr_phys_segments--;

@@ -816,18 +809,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 	const struct blk_mq_ops *ops = req->q->mq_ops;
 	enum blk_eh_timer_return ret = BLK_EH_RESET_TIMER;
 
-	/*
-	 * We know that complete is set at this point. If STARTED isn't set
-	 * anymore, then the request isn't active and the "timeout" should
-	 * just be ignored. This can happen due to the bitflag ordering.
-	 * Timeout first checks if STARTED is set, and if it is, assumes
-	 * the request is active. But if we race with completion, then
-	 * both flags will get cleared. So check here again, and ignore
-	 * a timeout event with a request that isn't active.
-	 */
-	if (!test_bit(REQ_ATOM_STARTED, &req->atomic_flags))
-		return;
-
 	req->rq_flags |= RQF_MQ_TIMEOUT_EXPIRED;
 
 	if (ops->timeout)

@@ -863,8 +844,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 
 	might_sleep();
 
-	if ((rq->rq_flags & RQF_MQ_TIMEOUT_EXPIRED) ||
-	    !test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+	if (rq->rq_flags & RQF_MQ_TIMEOUT_EXPIRED)
 		return;
 
 	/* read coherent snapshots of @rq->state_gen and @rq->deadline */

@@ -3015,8 +2995,7 @@ static bool blk_mq_poll_hybrid_sleep(struct request_queue *q,
 
 	hrtimer_init_sleeper(&hs, current);
 	do {
-		if (test_bit(REQ_ATOM_STARTED, &rq->atomic_flags) &&
-		    blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT)
+		if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE)
 			break;
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		hrtimer_start_expires(&hs.timer, mode);

index 6b2d616..8591a54 100644
@@ -34,6 +34,7 @@ struct blk_mq_ctx {
 enum mq_rq_state {
 	MQ_RQ_IDLE		= 0,
 	MQ_RQ_IN_FLIGHT		= 1,
+	MQ_RQ_COMPLETE		= 2,
 
 	MQ_RQ_STATE_BITS	= 2,
 	MQ_RQ_STATE_MASK	= (1 << MQ_RQ_STATE_BITS) - 1,

index 9cb2739..a68dbe3 100644
@@ -124,7 +124,6 @@ void blk_account_io_done(struct request *req);
  */
 enum rq_atomic_flags {
 	REQ_ATOM_COMPLETE = 0,
-	REQ_ATOM_STARTED,
 
 	REQ_ATOM_POLL_SLEPT,
 };

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEFm dGVyIHRoZSByZWNlbnQgdXBkYXRlcyB0byB1c2UgZ2VuZXJhdGlvbiBudW1iZXIgYW5kIHN0YXRl IGJhc2VkDQo+IHN5bmNocm9uaXphdGlvbiwgd2UgY2FuIGVhc2lseSByZXBsYWNlIFJFUV9BVE9N X1NUQVJURUQgdXNhZ2VzIGJ5DQo+IGFkZGluZyBhbiBleHRyYSBzdGF0ZSB0byBkaXN0aW5ndWlz aCBjb21wbGV0ZWQgYnV0IG5vdCB5ZXQgZnJlZWQNCj4gc3RhdGUuDQo+IA0KPiBBZGQgTVFfUlFf Q09NUExFVEUgYW5kIHJlcGxhY2UgUkVRX0FUT01fU1RBUlRFRCB1c2FnZXMgd2l0aA0KPiBibGtf bXFfcnFfc3RhdGUoKSB0ZXN0cy4gIFJFUV9BVE9NX1NUQVJURUQgbm8gbG9uZ2VyIGhhcyBhbnkg dXNlcnMNCj4gbGVmdCBhbmQgaXMgcmVtb3ZlZC4NCg0KUmV2aWV3ZWQtYnk6IEJhcnQgVmFuIEFz c2NoZSA8YmFydC52YW5hc3NjaGVAd2RjLmNvbT4NCg0K

The RCU protection has been expanded to cover both queueing and completion paths making ->queue_rq_srcu a misnomer. Rename it to ->srcu as suggested by Bart.

---
 block/blk-mq.c         | 14 +++++++-------
 include/linux/blk-mq.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

index 54ecb2c..071f396 100644
@@ -219,7 +219,7 @@ void blk_mq_quiesce_queue(struct request_queue *q)
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (hctx->flags & BLK_MQ_F_BLOCKING)
-			synchronize_srcu(hctx->queue_rq_srcu);
+			synchronize_srcu(hctx->srcu);
 		else
 			rcu = true;
 	}

@@ -564,7 +564,7 @@ static void hctx_unlock(struct blk_mq_hw_ctx *hctx, int srcu_idx)
 	if (!(hctx->flags & BLK_MQ_F_BLOCKING))
 		rcu_read_unlock();
 	else
-		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
+		srcu_read_unlock(hctx->srcu, srcu_idx);
 }
 
 static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)

@@ -572,7 +572,7 @@ static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)
 	if (!(hctx->flags & BLK_MQ_F_BLOCKING))
 		rcu_read_lock();
 	else
-		*srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
+		*srcu_idx = srcu_read_lock(hctx->srcu);
 }
 
 static void blk_mq_rq_update_aborted_gstate(struct request *rq, u64 gstate)

@@ -931,7 +931,7 @@ static void blk_mq_timeout_work(struct work_struct *work)
 			if (!(hctx->flags & BLK_MQ_F_BLOCKING))
 				has_rcu = true;
 			else
-				synchronize_srcu(hctx->queue_rq_srcu);
+				synchronize_srcu(hctx->srcu);
 
 			hctx->nr_expired = 0;
 		}

@@ -2094,7 +2094,7 @@ static void blk_mq_exit_hctx(struct request_queue *q,
 		set->ops->exit_hctx(hctx, hctx_idx);
 
 	if (hctx->flags & BLK_MQ_F_BLOCKING)
-		cleanup_srcu_struct(hctx->queue_rq_srcu);
+		cleanup_srcu_struct(hctx->srcu);
 
 	blk_mq_remove_cpuhp(hctx);
 	blk_free_flush_queue(hctx->fq);

@@ -2167,7 +2167,7 @@ static int blk_mq_init_hctx(struct request_queue *q,
 		goto free_fq;
 
 	if (hctx->flags & BLK_MQ_F_BLOCKING)
-		init_srcu_struct(hctx->queue_rq_srcu);
+		init_srcu_struct(hctx->srcu);
 
 	blk_mq_debugfs_register_hctx(q, hctx);
 

@@ -2456,7 +2456,7 @@ static int blk_mq_hw_ctx_size(struct blk_mq_tag_set *tag_set)
 {
 	int hw_ctx_size = sizeof(struct blk_mq_hw_ctx);
 
-	BUILD_BUG_ON(ALIGN(offsetof(struct blk_mq_hw_ctx, queue_rq_srcu),
+	BUILD_BUG_ON(ALIGN(offsetof(struct blk_mq_hw_ctx, srcu),
 			   __alignof__(struct blk_mq_hw_ctx)) !=
 		     sizeof(struct blk_mq_hw_ctx));
 

index 460798d..8efcf49 100644
@@ -66,7 +66,7 @@ struct blk_mq_hw_ctx {
 #endif
 
 	/* Must be the last member - see also blk_mq_hw_ctx_size(). */
-	struct srcu_struct	queue_rq_srcu[0];
+	struct srcu_struct	srcu[0];
 };
 
 struct blk_mq_tag_set {

2.9.5

T24gTW9uLCAyMDE4LTAxLTA4IGF0IDExOjE1IC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IFRo ZSBSQ1UgcHJvdGVjdGlvbiBoYXMgYmVlbiBleHBhbmRlZCB0byBjb3ZlciBib3RoIHF1ZXVlaW5n IGFuZA0KPiBjb21wbGV0aW9uIHBhdGhzIG1ha2luZyAtPnF1ZXVlX3JxX3NyY3UgYSBtaXNub21l ci4gIFJlbmFtZSBpdCB0bw0KPiAtPnNyY3UgYXMgc3VnZ2VzdGVkIGJ5IEJhcnQuDQoNClJldmll d2VkLWJ5OiBCYXJ0IFZhbiBBc3NjaGUgPGJhcnQudmFuYXNzY2hlQHdkYy5jb20+DQoNCg==