blk-mq: reimplement timeout handling

Changes from the last version[1]

- 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 six patches.

0001-blk-mq-protect-completion-path-with-RCU.patch 0002-blk-mq-replace-timeout-synchronization-with-a-RCU-an.patch 0003-blk-mq-use-blk_mq_rq_state-instead-of-testing-REQ_AT.patch 0004-blk-mq-make-blk_abort_request-trigger-timeout-path.patch 0005-blk-mq-remove-REQ_ATOM_COMPLETE-usages-from-blk-mq.patch 0006-blk-mq-remove-REQ_ATOM_STARTED.patch

and is available in the following git branch.

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

diffstat follows. Thanks.

 block/blk-core.c       |    2 
 block/blk-mq-debugfs.c |    4 
 block/blk-mq.c         |  255 ++++++++++++++++++++++++++++---------------------
 block/blk-mq.h         |   48 ++++++++-
 block/blk-timeout.c    |    9 -
 block/blk.h            |    7 -
 include/linux/blk-mq.h |    1 
 include/linux/blkdev.h |   23 ++++
 8 files changed, 226 insertions(+), 123 deletions(-)

--
tejun

[1] http://lkml.kernel.org/r/[email protected]

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 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

index 1109747..acf4fbb 100644
@@ -568,11 +568,23 @@ static void __blk_mq_complete_request(struct request *rq)
 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;
-	if (!blk_mark_rq_complete(rq))
-		__blk_mq_complete_request(rq);
+
+	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
+		rcu_read_lock();
+		if (!blk_mark_rq_complete(rq))
+			__blk_mq_complete_request(rq);
+		rcu_read_unlock();
+	} else {
+		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
+		if (!blk_mark_rq_complete(rq))
+			__blk_mq_complete_request(rq);
+		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
+	}
 }
 EXPORT_SYMBOL(blk_mq_complete_request);
 

2.9.5

Hello tejun
Sorry for missing the V2, same comment again.

Hello,

Sure, but it's just a lot cleaner to use the same to protect both issue and completion; otherwise, whoever who wants to synchronize against them have to do awkward double rcu locking.

Thanks.

tejun

It's fair. Thanks for your detailed response. That's really appreciated.

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+ICsJ fSBlbHNlIHsNCj4gKwkJc3JjdV9pZHggPSBzcmN1X3JlYWRfbG9jayhoY3R4LT5xdWV1ZV9ycV9z cmN1KTsNCj4gKwkJaWYgKCFibGtfbWFya19ycV9jb21wbGV0ZShycSkpDQo+ICsJCQlfX2Jsa19t cV9jb21wbGV0ZV9yZXF1ZXN0KHJxKTsNCj4gKwkJc3JjdV9yZWFkX3VubG9jayhoY3R4LT5xdWV1 ZV9ycV9zcmN1LCBzcmN1X2lkeCk7DQoNCkhlbGxvIFRlanVuLA0KDQpUaGUgbmFtZSBxdWV1ZV9y cV9zcmN1IHdhcyBjaG9zZW4gdG8gcmVmbGVjdCB0aGUgb3JpZ2luYWwgdXNlIG9mIHRoYXQgc3Ry dWN0dXJlLA0KbmFtZWx5IHRvIHByb3RlY3QgLnF1ZXVlX3JxKCkgY2FsbHMuIFlvdXIgcGF0Y2gg c2VyaWVzIGJyb2FkZW5zIHRoZSB1c2Ugb2YgdGhhdA0Kc3JjdSBzdHJ1Y3R1cmUgc28gSSB3b3Vs ZCBhcHByZWNpYXRlIGl0IGlmIGl0IHdvdWxkIGJlIHJlbmFtZWQsIGUuZy4gaW50byAic3JjdSIu DQpTZWUgYWxzbyBjb21taXQgNmE4M2U3NGQyMTRhICgiYmxrLW1xOiBJbnRyb2R1Y2UgYmxrX21x X3F1aWVzY2VfcXVldWUoKSIpLg0KDQpUaGFua3MsDQoNCkJhcnQu

Yeah, will add a patch to rename it.

Ah yeah, it'd be nice to have the [s]rcu synchronize calls factored out.

Thanks.

tejun

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.

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 synchornization 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.

---
 block/blk-core.c       |   2 +
 block/blk-mq.c         | 206 +++++++++++++++++++++++++++++++------------------
 block/blk-mq.h         |  45 +++++++++++
 block/blk-timeout.c    |   2 +-
 block/blk.h            |   6 --
 include/linux/blk-mq.h |   1 +
 include/linux/blkdev.h |  23 ++++++
 7 files changed, 204 insertions(+), 81 deletions(-)

index b888175..6034623 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 8089ca1..2d6fd11 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;

@@ -228,6 +230,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

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+ICsv Kg0KPiArICogQml0cyBmb3IgcmVxdWVzdC0+Z3N0YXRlLiAgVGhlIGxvd2VyIHR3byBiaXRzIGNh cnJ5IE1RX1JRXyogc3RhdGUgdmFsdWUNCj4gKyAqIGFuZCB0aGUgdXBwZXIgYml0cyB0aGUgZ2Vu ZXJhdGlvbiBudW1iZXIuDQo+ICsgKi8NCj4gK2VudW0gbXFfcnFfc3RhdGUgew0KPiArCU1RX1JR X0lETEUJCT0gMCwNCj4gKwlNUV9SUV9JTl9GTElHSFQJCT0gMSwNCj4gKw0KPiArCU1RX1JRX1NU QVRFX0JJVFMJPSAyLA0KPiArCU1RX1JRX1NUQVRFX01BU0sJPSAoMSA8PCBNUV9SUV9TVEFURV9C SVRTKSAtIDEsDQo+ICsJTVFfUlFfR0VOX0lOQwkJPSAxIDw8IE1RX1JRX1NUQVRFX0JJVFMsDQo+ ICt9Ow0KPiArDQo+IEBAIC04NSw2ICs5OCwzOCBAQCBleHRlcm4gdm9pZCBibGtfbXFfcnFfdGlt ZWRfb3V0KHN0cnVjdCByZXF1ZXN0ICpyZXEsIGJvb2wgcmVzZXJ2ZWQpOw0KPiArLyoqDQo+ICsg KiBibGtfbXFfcnFfc3RhdGUoKSAtIHJlYWQgdGhlIGN1cnJlbnQgTVFfUlFfKiBzdGF0ZSBvZiBh IHJlcXVlc3QNCj4gKyAqIEBycTogdGFyZ2V0IHJlcXVlc3QuDQo+ICsgKi8NCj4gK3N0YXRpYyBp bmxpbmUgaW50IGJsa19tcV9ycV9zdGF0ZShzdHJ1Y3QgcmVxdWVzdCAqcnEpDQo+ICt7DQo+ICsJ cmV0dXJuIFJFQURfT05DRShycS0+Z3N0YXRlKSAmIE1RX1JRX1NUQVRFX01BU0s7DQo+ICt9DQo+ ICsNCj4gKy8qKg0KPiArICogYmxrX21xX3JxX3VwZGF0ZV9zdGF0ZSgpIC0gc2V0IHRoZSBjdXJy ZW50IE1RX1JRXyogc3RhdGUgb2YgYSByZXF1ZXN0DQo+ICsgKiBAcnE6IHRhcmdldCByZXF1ZXN0 Lg0KPiArICogQHN0YXRlOiBuZXcgc3RhdGUgdG8gc2V0Lg0KPiArICoNCj4gKyAqIFNldCBAcnEn cyBzdGF0ZSB0byBAc3RhdGUuICBUaGUgY2FsbGVyIGlzIHJlc3BvbnNpYmxlIGZvciBlbnN1cmlu ZyB0aGF0DQo+ICsgKiB0aGVyZSBhcmUgbm8gb3RoZXIgdXBkYXRlcnMuICBBIHJlcXVlc3QgY2Fu IHRyYW5zaXRpb24gaW50byBJTl9GTElHSFQNCj4gKyAqIG9ubHkgZnJvbSBJRExFIGFuZCBkb2lu ZyBzbyBpbmNyZW1lbnRzIHRoZSBnZW5lcmF0aW9uIG51bWJlci4NCj4gKyAqLw0KPiArc3RhdGlj IGlubGluZSB2b2lkIGJsa19tcV9ycV91cGRhdGVfc3RhdGUoc3RydWN0IHJlcXVlc3QgKnJxLA0K PiArCQkJCQkgIGVudW0gbXFfcnFfc3RhdGUgc3RhdGUpDQo+ICt7DQo+ICsJdTY0IG5ld192YWwg PSAoUkVBRF9PTkNFKHJxLT5nc3RhdGUpICYgfk1RX1JRX1NUQVRFX01BU0spIHwgc3RhdGU7DQo+ ICsNCj4gKwlpZiAoc3RhdGUgPT0gTVFfUlFfSU5fRkxJR0hUKSB7DQo+ICsJCVdBUk5fT05fT05D RShibGtfbXFfcnFfc3RhdGUocnEpICE9IE1RX1JRX0lETEUpOw0KPiArCQluZXdfdmFsICs9IE1R X1JRX0dFTl9JTkM7DQo+ICsJfQ0KPiArDQo+ICsJLyogYXZvaWQgZXhwb3NpbmcgaW50ZXJpbSB2 YWx1ZXMgKi8NCj4gKwlXUklURV9PTkNFKHJxLT5nc3RhdGUsIG5ld192YWwpOw0KPiArfQ0KDQpI ZWxsbyBUZWp1biwNCg0KSGF2ZSB5b3UgY29uc2lkZXJlZCB0aGUgZm9sbG93aW5nIGluc3RlYWQg b2YgaW50cm9kdWNpbmcgTVFfUlFfSURMRSBhbmQNCk1RX1JRX0lOX0ZMSUdIVD8gSSB0aGluayB0 aGlzIGNvdWxkIGhlbHAgdG8gbGltaXQgdGhlIG51bWJlciBvZiBuZXcgYXRvbWljDQpvcGVyYXRp b25zIGludHJvZHVjZWQgaW4gdGhlIGhvdCBwYXRoIGJ5IHRoaXMgcGF0Y2ggc2VyaWVzLg0KDQpz dGF0aWMgaW5saW5lIGJvb2wgYmxrX21xX3JxX2luX2ZsaWdodChzdHJ1Y3QgcmVxdWVzdCAqcnEp DQp7DQoJcmV0dXJuIGxpc3RfZW1wdHkoJnJxLT5xdWV1ZWxpc3QpOw0KfQ0KDQpUaGFua3MsDQoN CkJhcnQu

Hello, Bart.

But nothing in the hot paths is atomic.

And the fact that we encode the generation number and state into a single variable contributes to not needing atomic operations. Breaking up the state and generation like the above would need more synchronization, not less.

Thanks.

tejun

Hi Tejun

Hi, Jianchao.

Awesome, thanks for testing!

tejun

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IHJ1 bGVzLiAgVW5mb3J0dW5hdGxleSwgaXQgY29udGFpbnMgcXVpdGUgYSBmZXcgaG9sZXMuDQogICAg ICAgICAgXl5eXl5eXl5eXl5eXg0KICAgICAgICAgIFVuZm9ydHVuYXRlbHk/DQoNCj4gV2hpbGUg dGhpcyBjaGFuZ2UgbWFrZXMgUkVRX0FUT01fQ09NUExFVEUgc3luY2hvcm5pemF0aW9uIHVubmVj ZXNzYXJ5DQogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIF5eXl5e Xl5eXl5eXl5eXg0KICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBz eW5jaHJvbml6YXRpb24/DQoNCj4gLS0tIGEvYmxvY2svYmxrLWNvcmUuYw0KPiArKysgYi9ibG9j ay9ibGstY29yZS5jDQo+IEBAIC0xMjYsNiArMTI2LDggQEAgdm9pZCBibGtfcnFfaW5pdChzdHJ1 Y3QgcmVxdWVzdF9xdWV1ZSAqcSwgc3RydWN0IHJlcXVlc3QgKnJxKQ0KPiAgCXJxLT5zdGFydF90 aW1lID0gamlmZmllczsNCj4gIAlzZXRfc3RhcnRfdGltZV9ucyhycSk7DQo+ICAJcnEtPnBhcnQg PSBOVUxMOw0KPiArCXNlcWNvdW50X2luaXQoJnJxLT5nc3RhdGVfc2VxKTsNCj4gKwl1NjRfc3Rh dHNfaW5pdCgmcnEtPmFib3J0ZWRfZ3N0YXRlX3N5bmMpOw0KPiAgfQ0KPiAgRVhQT1JUX1NZTUJP TChibGtfcnFfaW5pdCk7DQoNClNvcnJ5IGJ1dCB0aGUgYWJvdmUgY2hhbmdlIGxvb2tzIHVnbHkg dG8gbWUuIE15IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCANCmJsa19ycV9pbml0KCkgaXMgb25seSB1 c2VkIGluc2lkZSB0aGUgYmxvY2sgbGF5ZXIgdG8gaW5pdGlhbGl6ZSBsZWdhY3kgYmxvY2sNCmxh eWVyIHJlcXVlc3RzIHdoaWxlIGdzdGF0ZV9zZXEgYW5kIGFib3J0ZWRfZ3N0YXRlX3N5bmMgYXJl IG9ubHkgcmVsZXZhbnQNCmZvciBibGstbXEgcmVxdWVzdHMuIFdvdWxkbid0IGl0IGJlIGJldHRl ciB0byBhdm9pZCB0aGF0IGJsa19ycV9pbml0KCkgaXMNCmNhbGxlZCBmb3IgYmxrLW1xIHJlcXVl c3RzIHN1Y2ggdGhhdCB0aGUgYWJvdmUgY2hhbmdlIGNhbiBiZSBsZWZ0IG91dD8gVGhlDQpvbmx5 IGNhbGxlcnMgb3V0c2lkZSB0aGUgYmxvY2sgbGF5ZXIgY29yZSBvZiBibGtfcnFfaW5pdCgpIEkg a25vdyBvZiBhcmUNCmlkZV9wcmVwX3NlbnNlKCkgYW5kIHNjc2lfaW9jdGxfcmVzZXQoKS4gSSBj YW4gaGVscCB3aXRoIGNvbnZlcnRpbmcgdGhlIFNDU0kNCmNvZGUgaWYgeW91IHdhbnQuDQoNCj4g Kwl3cml0ZV9zZXFjb3VudF9iZWdpbigmcnEtPmdzdGF0ZV9zZXEpOw0KPiArCWJsa19tcV9ycV91 cGRhdGVfc3RhdGUocnEsIE1RX1JRX0lOX0ZMSUdIVCk7DQo+ICsJYmxrX2FkZF90aW1lcihycSk7 DQo+ICsJd3JpdGVfc2VxY291bnRfZW5kKCZycS0+Z3N0YXRlX3NlcSk7DQoNCk15IHVuZGVyc3Rh bmRpbmcgaXMgdGhhdCBib3RoIHdyaXRlX3NlcWNvdW50X2JlZ2luKCkgYW5kIHdyaXRlX3NlcWNv dW50X2VuZCgpDQp0cmlnZ2VyIGEgd3JpdGUgbWVtb3J5IGJhcnJpZXIuIElzIGEgc2VxY291bnQg cmVhbGx5IGZhc3RlciB0aGFuIGEgc3BpbmxvY2s/DQoNCj4gDQo+IEBAIC03OTIsNiArODExLDE0 IEBAIHZvaWQgYmxrX21xX3JxX3RpbWVkX291dChzdHJ1Y3QgcmVxdWVzdCAqcmVxLCBib29sIHJl c2VydmVkKQ0KPiAgCQlfX2Jsa19tcV9jb21wbGV0ZV9yZXF1ZXN0KHJlcSk7DQo+ICAJCWJyZWFr Ow0KPiAgCWNhc2UgQkxLX0VIX1JFU0VUX1RJTUVSOg0KPiArCQkvKg0KPiArCQkgKiBBcyBub3Ro aW5nIHByZXZlbnRzIGZyb20gY29tcGxldGlvbiBoYXBwZW5pbmcgd2hpbGUNCj4gKwkJICogLT5h Ym9ydGVkX2dzdGF0ZSBpcyBzZXQsIHRoaXMgbWF5IGxlYWQgdG8gaWdub3JlZA0KPiArCQkgKiBj b21wbGV0aW9ucyBhbmQgZnVydGhlciBzcHVyaW91cyB0aW1lb3V0cy4NCj4gKwkJICovDQo+ICsJ CXU2NF9zdGF0c191cGRhdGVfYmVnaW4oJnJlcS0+YWJvcnRlZF9nc3RhdGVfc3luYyk7DQo+ICsJ CXJlcS0+YWJvcnRlZF9nc3RhdGUgPSAwOw0KPiArCQl1NjRfc3RhdHNfdXBkYXRlX2VuZCgmcmVx LT5hYm9ydGVkX2dzdGF0ZV9zeW5jKTsNCg0KSWYgYSBibGstbXEgcmVxdWVzdCBpcyByZXN1Ym1p dHRlZCAyKio2MiB0aW1lcywgY2FuIHRoYXQgcmVzdWx0IGluIHRoZSBhYm92ZQ0KY29kZSBzZXR0 aW5nIGFib3J0ZWRfZ3N0YXRlIHRvIHRoZSBzYW1lIHZhbHVlIGFzIGdzdGF0ZT8gSXNuJ3QgdGhh dCBhIGJ1Zz8NCklmIHNvLCBob3cgYWJvdXQgc2V0dGluZyBhYm9ydGVkX2dzdGF0ZSBpbiB0aGUg YWJvdmUgY29kZSB0byBlLmcuIGdzdGF0ZSBeICgyKio2Myk/DQoNCj4gQEAgLTIyOCw2ICsyMzAs MjcgQEAgc3RydWN0IHJlcXVlc3Qgew0KPiAgDQo+ICAJdW5zaWduZWQgc2hvcnQgd3JpdGVfaGlu dDsNCj4gIA0KPiArCS8qDQo+ICsJICogT24gYmxrLW1xLCB0aGUgbG93ZXIgYml0cyBvZiAtPmdz dGF0ZSBjYXJyeSB0aGUgTVFfUlFfKiBzdGF0ZQ0KPiArCSAqIHZhbHVlIGFuZCB0aGUgdXBwZXIg Yml0cyB0aGUgZ2VuZXJhdGlvbiBudW1iZXIgd2hpY2ggaXMNCj4gKwkgKiBtb25vdG9uaWNhbGx5 IGluY3JlbWVudGVkIGFuZCB1c2VkIHRvIGRpc3Rpbmd1aXNoIHRoZSByZXVzZQ0KPiArCSAqIGlu c3RhbmNlcy4NCj4gKwkgKg0KPiArCSAqIC0+Z3N0YXRlX3NlcSBhbGxvd3MgdXBkYXRlcyB0byAt PmdzdGF0ZSBhbmQgb3RoZXIgZmllbGRzDQo+ICsJICogKGN1cnJlbnRseSAtPmRlYWRsaW5lKSBk dXJpbmcgcmVxdWVzdCBzdGFydCB0byBiZSByZWFkDQo+ICsJICogYXRvbWljYWxseSBmcm9tIHRo ZSB0aW1lb3V0IHBhdGgsIHNvIHRoYXQgaXQgY2FuIG9wZXJhdGUgb24gYQ0KPiArCSAqIGNvaGVy ZW50IHNldCBvZiBpbmZvcm1hdGlvbi4NCj4gKwkgKi8NCj4gKwlzZXFjb3VudF90IGdzdGF0ZV9z ZXE7DQo+ICsJdTY0IGdzdGF0ZTsNCj4gKw0KPiArCS8qDQo+ICsJICogLT5hYm9ydGVkX2dzdGF0 ZSBpcyB1c2VkIGJ5IHRoZSB0aW1lb3V0IHRvIGNsYWltIGEgc3BlY2lmaWMNCj4gKwkgKiByZWN5 Y2xlIGluc3RhbmNlIG9mIHRoaXMgcmVxdWVzdC4gIFNlZSBibGtfbXFfdGltZW91dF93b3JrKCku DQo+ICsJICovDQo+ICsJc3RydWN0IHU2NF9zdGF0c19zeW5jIGFib3J0ZWRfZ3N0YXRlX3N5bmM7 DQo+ICsJdTY0IGFib3J0ZWRfZ3N0YXRlOw0KPiArDQo+ICAJdW5zaWduZWQgbG9uZyBkZWFkbGlu ZTsNCj4gIAlzdHJ1Y3QgbGlzdF9oZWFkIHRpbWVvdXRfbGlzdDsNCg0KV2h5IGFyZSBnc3RhdGUg YW5kIGFib3J0ZWRfZ3N0YXRlIDY0LWJpdCB2YXJpYWJsZXM/IFdoYXQgbWFrZXMgeW91IHRoaW5r IHRoYXQNCjMyIGJpdHMgd291bGQgbm90IGJlIGVub3VnaD8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

Hello,

lol, believe it or not, my english spelling is a lot better than my korean. Will fix them.

This is also used by flush path. We probably should clean that up, but let's worry about that later cuz flush handling has enough of its own complications.

Write memory barrier has no cost on x86 and usually pretty low cost elsewhere too and they're likely in the same cacheline as other rq fields.

A request gets aborted only if the state is in-flight, 0 isn't that. Also, how many years would 2^62 times be?

Because 32 bits puts it in the rance where a false hit is still theoretically possible in a reasonable amount of time.

Thanks.

tejun

T24gVGh1LCAyMDE3LTEyLTE0IGF0IDExOjE5IC0wODAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K PiBPbiBUaHUsIERlYyAxNCwgMjAxNyBhdCAwNjo1MToxMVBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj aGUgd3JvdGU6DQo+ID4gT24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBI ZW8gd3JvdGU6DQo+ID4gPiAtLS0gYS9ibG9jay9ibGstY29yZS5jDQo+ID4gPiArKysgYi9ibG9j ay9ibGstY29yZS5jDQo+ID4gPiBAQCAtMTI2LDYgKzEyNiw4IEBAIHZvaWQgYmxrX3JxX2luaXQo c3RydWN0IHJlcXVlc3RfcXVldWUgKnEsIHN0cnVjdCByZXF1ZXN0ICpycSkNCj4gPiA+ICAJcnEt PnN0YXJ0X3RpbWUgPSBqaWZmaWVzOw0KPiA+ID4gIAlzZXRfc3RhcnRfdGltZV9ucyhycSk7DQo+ ID4gPiAgCXJxLT5wYXJ0ID0gTlVMTDsNCj4gPiA+ICsJc2VxY291bnRfaW5pdCgmcnEtPmdzdGF0 ZV9zZXEpOw0KPiA+ID4gKwl1NjRfc3RhdHNfaW5pdCgmcnEtPmFib3J0ZWRfZ3N0YXRlX3N5bmMp Ow0KPiA+ID4gIH0NCj4gPiA+ICBFWFBPUlRfU1lNQk9MKGJsa19ycV9pbml0KTsNCj4gPiANCj4g PiBTb3JyeSBidXQgdGhlIGFib3ZlIGNoYW5nZSBsb29rcyB1Z2x5IHRvIG1lLiBNeSB1bmRlcnN0 YW5kaW5nIGlzIHRoYXQgDQo+ID4gYmxrX3JxX2luaXQoKSBpcyBvbmx5IHVzZWQgaW5zaWRlIHRo ZSBibG9jayBsYXllciB0byBpbml0aWFsaXplIGxlZ2FjeSBibG9jaw0KPiA+IGxheWVyIHJlcXVl c3RzIHdoaWxlIGdzdGF0ZV9zZXEgYW5kIGFib3J0ZWRfZ3N0YXRlX3N5bmMgYXJlIG9ubHkgcmVs ZXZhbnQNCj4gPiBmb3IgYmxrLW1xIHJlcXVlc3RzLiBXb3VsZG4ndCBpdCBiZSBiZXR0ZXIgdG8g YXZvaWQgdGhhdCBibGtfcnFfaW5pdCgpIGlzDQo+ID4gY2FsbGVkIGZvciBibGstbXEgcmVxdWVz dHMgc3VjaCB0aGF0IHRoZSBhYm92ZSBjaGFuZ2UgY2FuIGJlIGxlZnQgb3V0PyBUaGUNCj4gPiBv bmx5IGNhbGxlcnMgb3V0c2lkZSB0aGUgYmxvY2sgbGF5ZXIgY29yZSBvZiBibGtfcnFfaW5pdCgp IEkga25vdyBvZiBhcmUNCj4gPiBpZGVfcHJlcF9zZW5zZSgpIGFuZCBzY3NpX2lvY3RsX3Jlc2V0 KCkuIEkgY2FuIGhlbHAgd2l0aCBjb252ZXJ0aW5nIHRoZSBTQ1NJDQo+ID4gY29kZSBpZiB5b3Ug d2FudC4NCj4gDQo+IFRoaXMgaXMgYWxzbyB1c2VkIGJ5IGZsdXNoIHBhdGguICBXZSBwcm9iYWJs eSBzaG91bGQgY2xlYW4gdGhhdCB1cCwNCj4gYnV0IGxldCdzIHdvcnJ5IGFib3V0IHRoYXQgbGF0 ZXIgY3V6IGZsdXNoIGhhbmRsaW5nIGhhcyBlbm91Z2ggb2YgaXRzDQo+IG93biBjb21wbGljYXRp b25zLg0KDQpXZSBtYXkgaGF2ZSBhIGRpZmZlcmVudCBvcGluaW9uIGFib3V0IHRoaXMgYnV0IEkg dGhpbmsgaXQgaXMgbW9yZSB0aGFuIGENCmRldGFpbC4gVGhpcyBwYXRjaCBuZWVkcyBnc3RhdGVf c2VxIGFuZCBhYm9ydGVkX2dzdGF0ZV9zeW5jIHRvIGJlIHByZXNlcnZlZA0KYWNyb3NzIHJlcXVl c3Qgc3RhdGUgdHJhbnNpdGlvbnMgZnJvbSBNUV9SUV9JTl9GTElHSFQgdG8gTVJfUlFfSURMRS4N CmJsa19tcV9pbml0X3JlcXVlc3QoKSBpcyBjYWxsZWQgYXQgcmVxdWVzdCBhbGxvY2F0aW9uIHRp bWUgc28gaXQncyB0aGUgcmlnaHQNCmNvbnRleHQgdG8gaW5pdGlhbGl6ZSBnc3RhdGVfc2VxIGFu ZCBhYm9ydGVkX2dzdGF0ZV9zeW5jLiBibGtfcnFfaW5pdCgpDQpob3dldmVyIGlzIGNhbGxlZCBi ZWZvcmUgYSBldmVyeSB1c2Ugb2YgYSByZXF1ZXN0LiBTb3JyeSBidXQgSSdtIG5vdA0KZW50aHVz aWFzdCBhYm91dCB0aGUgY29kZSBpbiBibGtfcnFfaW5pdCgpIHRoYXQgcmVpbml0aWFsaXplcyBz dGF0ZQ0KaW5mb3JtYXRpb24gdGhhdCBzaG91bGQgc3Vydml2ZSByZXF1ZXN0IHJldXNlLg0KDQpC YXJ0Lg==

Hello, Bart.

On Thu, Dec 14, 2017 at 09:13:32PM +0000, Bart Van Assche wrote:
...

If it wasn't clear, me neither. I think what'd be better is making those paths use the usual request allocation path instead of custom one but for flush handling, that's not gonna be trivial, so let's deal with that later.

Thanks.

tejun

Yes lots, no atomic operations and no waiting.

The only constraint for write_seqlock is that there must not be any concurrency.

But now that I look at this again, TJ, why can't the below happen?

	write_seqlock_begin();
	blk_mq_rq_update_state(rq, IN_FLIGHT);
	blk_add_timer(rq);
	<timer-irq>
		read_seqcount_begin()
			while (seq & 1)
				cpurelax();
		// life-lock
	</timer-irq>
	write_seqlock_end();

T24gVGh1LCAyMDE3LTEyLTE0IGF0IDIxOjIwICswMTAwLCBQZXRlciBaaWpsc3RyYSB3cm90ZToN Cj4gT24gVGh1LCBEZWMgMTQsIDIwMTcgYXQgMDY6NTE6MTFQTSArMDAwMCwgQmFydCBWYW4gQXNz Y2hlIHdyb3RlOg0KPiA+IE9uIFR1ZSwgMjAxNy0xMi0xMiBhdCAxMTowMSAtMDgwMCwgVGVqdW4g SGVvIHdyb3RlOg0KPiA+ID4gKwl3cml0ZV9zZXFjb3VudF9iZWdpbigmcnEtPmdzdGF0ZV9zZXEp Ow0KPiA+ID4gKwlibGtfbXFfcnFfdXBkYXRlX3N0YXRlKHJxLCBNUV9SUV9JTl9GTElHSFQpOw0K PiA+ID4gKwlibGtfYWRkX3RpbWVyKHJxKTsNCj4gPiA+ICsJd3JpdGVfc2VxY291bnRfZW5kKCZy cS0+Z3N0YXRlX3NlcSk7DQo+ID4gDQo+ID4gTXkgdW5kZXJzdGFuZGluZyBpcyB0aGF0IGJvdGgg d3JpdGVfc2VxY291bnRfYmVnaW4oKSBhbmQgd3JpdGVfc2VxY291bnRfZW5kKCkNCj4gPiB0cmln Z2VyIGEgd3JpdGUgbWVtb3J5IGJhcnJpZXIuIElzIGEgc2VxY291bnQgcmVhbGx5IGZhc3RlciB0 aGFuIGEgc3BpbmxvY2s/DQo+IA0KPiBZZXMgbG90cywgbm8gYXRvbWljIG9wZXJhdGlvbnMgYW5k IG5vIHdhaXRpbmcuDQo+IA0KPiBUaGUgb25seSBjb25zdHJhaW50IGZvciB3cml0ZV9zZXFsb2Nr IGlzIHRoYXQgdGhlcmUgbXVzdCBub3QgYmUgYW55DQo+IGNvbmN1cnJlbmN5Lg0KPiANCj4gQnV0 IG5vdyB0aGF0IEkgbG9vayBhdCB0aGlzIGFnYWluLCBUSiwgd2h5IGNhbid0IHRoZSBiZWxvdyBo YXBwZW4/DQo+IA0KPiAJd3JpdGVfc2VxbG9ja19iZWdpbigpOw0KPiAJYmxrX21xX3JxX3VwZGF0 ZV9zdGF0ZShycSwgSU5fRkxJR0hUKTsNCj4gCWJsa19hZGRfdGltZXIocnEpOw0KPiAJPHRpbWVy LWlycT4NCj4gCQlyZWFkX3NlcWNvdW50X2JlZ2luKCkNCj4gCQkJd2hpbGUgKHNlcSAmIDEpDQo+ IAkJCQljcHVyZWxheCgpOw0KPiAJCS8vIGxpZmUtbG9jaw0KPiAJPC90aW1lci1pcnE+DQo+IAl3 cml0ZV9zZXFsb2NrX2VuZCgpOw0KDQpIZWxsbyBQZXRlciwNCg0KU29tZSB0aW1lIGFnbyB0aGUg YmxvY2sgbGF5ZXIgd2FzIGNoYW5nZWQgdG8gaGFuZGxlIHRpbWVvdXRzIGluIHRocmVhZCBjb250 ZXh0DQppbnN0ZWFkIG9mIGludGVycnVwdCBjb250ZXh0LiBTZWUgYWxzbyBjb21taXQgMjg3OTIy ZWIwYjE4ICgiYmxvY2s6IGRlZmVyDQp0aW1lb3V0cyB0byBhIHdvcmtxdWV1ZSIpLg0KDQpCYXJ0 Lg==

That only makes it a little better:

	Task-A					Worker

	write_seqcount_begin()
	blk_mq_rw_update_state(rq, IN_FLIGHT)
	blk_add_timer(rq)
	<timer>
		schedule_work()
	</timer>
	<context-switch to worker>
						read_seqcount_begin()
							while(seq & 1)
								cpu_relax();

Now normally this isn't fatal because Worker will simply spin its entire time slice away and we'll eventually schedule our Task-A back in, which will complete the seqcount and things will work.

But if, for some reason, our Worker was to have RT priority higher than our Task-A we'd be up some creek without no paddles.

We don't happen to have preemption of IRQs off here? That would fix things nicely.

Hi Peter

The current seqcount read side is as below:

	do {
		start = read_seqcount_begin(&rq->gstate_seq);
		gstate = READ_ONCE(rq->gstate);
		deadline = rq->deadline;
	} while (read_seqcount_retry(&rq->gstate_seq, start));

read_seqcount_retry() doesn't check the bit 0, but whether the saved value from read_seqcount_begin() is equal to the current value of seqcount. pls refer:
static inline int __read_seqcount_retry(const seqcount_t *s, unsigned start)

{
	return unlikely(s->sequence != start);
}

Thanks
Jianchao

static inline unsigned read_seqcount_begin(const seqcount_t *s)

{
	seqcount_lockdep_reader_access(s);
	return raw_read_seqcount_begin(s);
}

static inline unsigned raw_read_seqcount_begin(const seqcount_t *s)

{
	unsigned ret = __read_seqcount_begin(s);
	smp_rmb();
	return ret;
}

static inline unsigned __read_seqcount_begin(const seqcount_t *s)

{
	unsigned ret;

repeat:

	ret = READ_ONCE(s->sequence);
	if (unlikely(ret & 1)) {
		cpu_relax();
		goto repeat;
	}
	return ret;
}

Really thanks for kindly pointing out.
jianchao

Most kthreads, including kworkers, are very frequently SCHED_FIFO here.

-Mike

Hello, Peter.

Ah, you're right. For both gstate_seq and aborted_gstate_sync, we can push all synchronization to the timeout side - ie. gstate_seq read can yield, pause or synchronize_rcu and hte aborted_gstate_sync can disable irq around update.

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 b4e733b..3a08d7a 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

@@ -2958,7 +2957,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

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.

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

index 3a08d7a..73d6444 100644
@@ -785,7 +785,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 154b2fa..1fa83fa 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..051924f 100644
@@ -156,12 +156,13 @@ 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

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+ICB2 b2lkIGJsa19hYm9ydF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0ICpyZXEpDQo+ICB7DQo+IC0JaWYg KGJsa19tYXJrX3JxX2NvbXBsZXRlKHJlcSkpDQo+IC0JCXJldHVybjsNCj4gIA0KPiAgCWlmIChy ZXEtPnEtPm1xX29wcykgew0KPiAtCQlibGtfbXFfcnFfdGltZWRfb3V0KHJlcSwgZmFsc2UpOw0K PiArCQlyZXEtPmRlYWRsaW5lID0gamlmZmllczsNCj4gKwkJbW9kX3RpbWVyKCZyZXEtPnEtPnRp bWVvdXQsIDApOw0KPiAgCX0gZWxzZSB7DQo+ICsJCWlmIChibGtfbWFya19ycV9jb21wbGV0ZShy ZXEpKQ0KPiArCQkJcmV0dXJuOw0KPiAgCQlibGtfZGVsZXRlX3RpbWVyKHJlcSk7DQo+ICAJCWJs a19ycV90aW1lZF9vdXQocmVxKTsNCj4gIAl9DQoNClRoaXMgcGF0Y2ggbWFrZXMgYmxrX2Fib3J0 X3JlcXVlc3QoKSBhc3luY2hyb25vdXMgZm9yIGJsay1tcS4gSGF2ZSBhbGwgY2FsbGVycw0KYmVl biBhdWRpdGVkIHRvIHZlcmlmeSB3aGV0aGVyIHRoaXMgY2hhbmdlIGlzIHNhZmU/DQoNClRoYW5r cywNCg0KQmFydC4=

Hello,

I *think* so. For all the ata related parts, I know they're. mtip32xx and dasd_ioctl, it seems safe, but I can't tell for sure. Will cc the respective maintainers on the next posting.

Thanks.

tejun

After the recent updates to use generation number and state based synchronization, blk-mq no longer depends on REQ_ATOM_COMPLETE for anything.

Remove all REQ_ATOM_COMPLETE usages. This removes atomic bitops from hot paths too.

v2: Removed blk_clear_rq_complete() from blk_mq_rq_timed_out().

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

index 73d6444..7269552 100644
@@ -596,14 +596,12 @@ void blk_mq_complete_request(struct request *rq)
 	 */
 	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
 		rcu_read_lock();
-		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);
 		rcu_read_unlock();
 	} else {
 		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
-		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);
 		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
 	}

@@ -650,8 +648,6 @@ void blk_mq_start_request(struct request *rq)
 	write_seqcount_end(&rq->gstate_seq);
 
 	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)) {
 		/*

@@ -819,7 +815,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 		req->aborted_gstate = 0;
 		u64_stats_update_end(&req->aborted_gstate_sync);
 		blk_add_timer(req);
-		blk_clear_rq_complete(req);
 		break;
 	case BLK_EH_NOT_HANDLED:
 		break;

@@ -870,8 +865,7 @@ 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 (READ_ONCE(rq->gstate) == rq->aborted_gstate)
 		blk_mq_rq_timed_out(rq, reserved);
 }
 

2.9.5

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         | 39 ++++++++-------------------------------
 block/blk-mq.h         |  1 +
 block/blk.h            |  1 -
 4 files changed, 10 insertions(+), 35 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 7269552..bf53132 100644
@@ -482,7 +482,7 @@ void blk_mq_free_request(struct request *rq)
 	if (blk_rq_rl(rq))
 		blk_put_rl(blk_rq_rl(rq));
 
-	clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
+	blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
 	clear_bit(REQ_ATOM_POLL_SLEPT, &rq->atomic_flags);
 	if (rq->tag != -1)
 		blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag);

@@ -530,7 +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_IDLE);
+	blk_mq_rq_update_state(rq, MQ_RQ_COMPLETE);
 
 	if (rq->internal_tag != -1)
 		blk_mq_sched_completed_request(rq);

@@ -610,7 +610,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);
 

@@ -629,7 +629,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,

@@ -647,8 +646,6 @@ void blk_mq_start_request(struct request *rq)
 	blk_add_timer(rq);
 	write_seqcount_end(&rq->gstate_seq);
 
-	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

@@ -661,13 +658,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)
 {

@@ -679,7 +672,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--;

@@ -786,18 +779,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;
-
 	if (ops->timeout)
 		ret = ops->timeout(req, reserved);
 

@@ -831,9 +812,6 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	unsigned long gstate, deadline;
 	int start;
 
-	if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
-		return;
-
 	/* read coherent snapshots of @rq->state_gen and @rq->deadline */
 	do {
 		start = read_seqcount_begin(&rq->gstate_seq);

@@ -2951,8 +2929,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 1fa83fa..523cad0 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

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEBA IC00MDksNyArNDA3LDcgQEAgc3RhdGljIHZvaWQgaGN0eF9zaG93X2J1c3lfcnEoc3RydWN0IHJl cXVlc3QgKnJxLCB2b2lkICpkYXRhLCBib29sIHJlc2VydmVkKQ0KPiAgICAgICAgIGNvbnN0IHN0 cnVjdCBzaG93X2J1c3lfcGFyYW1zICpwYXJhbXMgPSBkYXRhOw0KPiAgDQo+ICAgICAgICAgaWYg KGJsa19tcV9tYXBfcXVldWUocnEtPnEsIHJxLT5tcV9jdHgtPmNwdSkgPT0gcGFyYW1zLT5oY3R4 ICYmDQo+IC0gICAgICAgICAgIHRlc3RfYml0KFJFUV9BVE9NX1NUQVJURUQsICZycS0+YXRvbWlj X2ZsYWdzKSkNCj4gKyAgICAgICAgICAgYmxrX21xX3JxX3N0YXRlKHJxKSAhPSBNUV9SUV9JRExF KQ0KPiAgICAgICAgICAgICAgICAgX19ibGtfbXFfZGVidWdmc19ycV9zaG93KHBhcmFtcy0+bSwN Cj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBsaXN0X2VudHJ5X3Jx KCZycS0+cXVldWVsaXN0KSk7DQoNClRoZSBhYm92ZSBjb2RlIHNob3VsZCBzaG93IGFsbCByZXF1 ZXN0cyBvd25lZCBieSB0aGUgYmxvY2sgZHJpdmVyLiBQYXRjaA0KImJsay1tcS1kZWJ1Z2ZzOiBB bHNvIHNob3cgcmVxdWVzdHMgdGhhdCBoYXZlIG5vdCB5ZXQgYmVlbiBzdGFydGVkIiAobm90IHll dA0KaW4gSmVucycgdHJlZSkgY2hhbmdlcyB0aGUgUkVRX0FUT01fU1RBUlRFRCB0ZXN0IGludG8g bGlzdF9lbXB0eSgmcnEtPnF1ZXVlbGlzdCkuDQpDYW4gdGhhdCBjaGFuZ2UgYmUgc3VwcG9ydGVk IHdpdGggdGhlIGV4aXN0aW5nIE1RX1JRXyogc3RhdGVzIG9yIHdpbGwgYSBuZXcNCnN0YXRlIGhh dmUgdG8gYmUgaW50cm9kdWNlZCB0byBzdXBwb3J0IHRoaXM/IFNlZSBhbHNvDQpodHRwczovL21h cmMuaW5mby8/bD1saW51eC1ibG9jayZtPTE1MTI1MjE4ODQxMTk5MS4NCg0KVGhhbmtzLA0KDQpC YXJ0Lg==

If list_empty() test was correct before, it'd be correct now too.

Thnaks.

tejun

I like this a lot, it's a lot less fragile and more intuitive/readable than what we have now. And apparently less error prone... I'll do some testing with this.

BTW, since youadd a few more BLK_MQ_F_BLOCKING checks, I think something like the below would be a good cleanup on top of this.

From: Jens Axboe <[email protected]>
Subject: [PATCH] blk-mq: move hctx lock/unlock into a helper

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

---

index 663069dca4d6..dec3d1bb0559 100644
@@ -572,6 +572,22 @@ static u64 blk_mq_rq_aborted_gstate(struct request *rq)
 	return aborted_gstate;
 }
 
+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

@@ -594,17 +610,10 @@ void blk_mq_complete_request(struct request *rq)
 	 * claiming @rq and we lost.  This is synchronized through RCU.
 	 * See blk_mq_timeout_work() for details.
 	 */
-	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
-		rcu_read_lock();
-		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
-			__blk_mq_complete_request(rq);
-		rcu_read_unlock();
-	} else {
-		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
-		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
-			__blk_mq_complete_request(rq);
-		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
-	}
+	hctx_lock(hctx, &srcu_idx);
+	if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
+		__blk_mq_complete_request(rq);
+	hctx_unlock(hctx, srcu_idx);
 }
 EXPORT_SYMBOL(blk_mq_complete_request);
 

@@ -1243,17 +1252,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);
 }
 
 /*

@@ -1624,7 +1627,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 = {

@@ -1674,25 +1677,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)

Jens Axboe

Hello, Jens.

Great.

Yeah, that's a lot better. It might also be a good idea to add lockdep_assert_hctx_locked() for verification and documentation.

Thanks.

tejun

T24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IEN1 cnJlbnRseSwgYmxrLW1xIHRpbWVvdXQgcGF0aCBzeW5jaHJvbml6ZXMgYWdhaW5zdCB0aGUgdXN1 YWwNCj4gaXNzdWUvY29tcGxldGlvbiBwYXRoIHVzaW5nIGEgY29tcGxleCBzY2hlbWUgaW52b2x2 aW5nIGF0b21pYw0KPiBiaXRmbGFncywgUkVRX0FUT01fKiwgbWVtb3J5IGJhcnJpZXJzIGFuZCBz dWJ0bGUgbWVtb3J5IGNvaGVyZW5jZQ0KPiBydWxlcy4gIFVuZm9ydHVuYXRsZXksIGl0IGNvbnRh aW5zIHF1aXRlIGEgZmV3IGhvbGVzLg0KDQpIZWxsbyBUZWp1biwNCg0KQW4gYXR0ZW1wdCB0byBy dW4gU0NTSSBJL08gd2l0aCB0aGlzIHBhdGNoIHNlcmllcyBhcHBsaWVkIHJlc3VsdGVkIGluDQp0 aGUgZm9sbG93aW5nOg0KDQpCVUc6IHVuYWJsZSB0byBoYW5kbGUga2VybmVsIE5VTEwgcG9pbnRl ciBkZXJlZmVyZW5jZSBhdCAgICAgICAgICAgKG51bGwpDQpJUDogc2NzaV90aW1lc19vdXQrMHgx Yy8weDJkMA0KUEdEIDAgUDREIDANCk9vcHM6IDAwMDAgWyMxXSBQUkVFTVBUIFNNUA0KQ1BVOiAx IFBJRDogNDM3IENvbW06IGt3b3JrZXIvMToxSCBUYWludGVkOiBHICAgICAgICBXICAgICAgICA0 LjE1LjAtcmM0LWRiZysgIzENCkhhcmR3YXJlIG5hbWU6IERlbGwgSW5jLiBQb3dlckVkZ2UgUjcy MC8wVldUOTAsIEJJT1MgMi41LjQgMDEvMjIvMjAxNg0KV29ya3F1ZXVlOiBrYmxvY2tkIGJsa19t cV90aW1lb3V0X3dvcmsNClJJUDogMDAxMDpzY3NpX3RpbWVzX291dCsweDFjLzB4MmQwDQpSU1A6 IDAwMTg6ZmZmZmM5MDAwN2VmM2Q1OCBFRkxBR1M6IDAwMDEwMjQ2DQpSQVg6IDAwMDAwMDAwMDAw MDAwMDAgUkJYOiBmZmZmODgwODc4ZWFiMDAwIFJDWDogMDAwMDAwMDAwMDAwMDAwMA0KUkRYOiAw MDAwMDAwMDAwMDAwMDAwIFJTSTogMDAwMDAwMDAwMDAwMDAwMCBSREk6IGZmZmY4ODA4NzhlYWIw MDANClJCUDogZmZmZjg4MDg3OGVhYjFhMCBSMDg6IGZmZmZmZmZmZmZmZmZmZmYgUjA5OiAwMDAw MDAwMDAwMDAwMDAxDQpSMTA6IDAwMDAwMDAwMDAwMDAwMDAgUjExOiAwMDAwMDAwMDAwMDAwMDAw IFIxMjogMDAwMDAwMDAwMDAwMDAwNA0KUjEzOiAwMDAwMDAwMDAwMDAwMDAwIFIxNDogZmZmZjg4 MDg1ZTRhNWNlOCBSMTU6IGZmZmY4ODA4NzhlOWY4NDgNCkZTOiAgMDAwMDAwMDAwMDAwMDAwMCgw MDAwKSBHUzpmZmZmODgwOTNmNjAwMDAwKDAwMDApIGtubEdTOjAwMDAwMDAwMDAwMDAwMDANCkNT OiAgMDAxMCBEUzogMDAwMCBFUzogMDAwMCBDUjA6IDAwMDAwMDAwODAwNTAwMzMNCkNSMjogMDAw MDAwMDAwMDAwMDAwMCBDUjM6IDAwMDAwMDAwMDFjMGYwMDIgQ1I0OiAwMDAwMDAwMDAwMDYwNmUw DQpDYWxsIFRyYWNlOg0KIGJsa19tcV90ZXJtaW5hdGVfZXhwaXJlZCsweDM2LzB4NzANCiBidF9p dGVyKzB4NDMvMHg1MA0KIGJsa19tcV9xdWV1ZV90YWdfYnVzeV9pdGVyKzB4ZWUvMHgyMDANCiBi bGtfbXFfdGltZW91dF93b3JrKzB4MTg2LzB4MmUwDQogcHJvY2Vzc19vbmVfd29yaysweDIyMS8w eDZlMA0KIHdvcmtlcl90aHJlYWQrMHgzYS8weDM5MA0KIGt0aHJlYWQrMHgxMWMvMHgxNDANCiBy ZXRfZnJvbV9mb3JrKzB4MjQvMHgzMA0KUklQOiBzY3NpX3RpbWVzX291dCsweDFjLzB4MmQwIFJT UDogZmZmZmM5MDAwN2VmM2Q1OA0KQ1IyOiAwMDAwMDAwMDAwMDAwMDAwDQoNCihnZGIpIGxpc3Qg KihzY3NpX3RpbWVzX291dCsweDFjKQ0KMHhmZmZmZmZmZjgxNDdhZGJjIGlzIGluIHNjc2lfdGlt ZXNfb3V0IChkcml2ZXJzL3Njc2kvc2NzaV9lcnJvci5jOjI4NSkuDQoyODAgICAgICAqLw0KMjgx ICAgICBlbnVtIGJsa19laF90aW1lcl9yZXR1cm4gc2NzaV90aW1lc19vdXQoc3RydWN0IHJlcXVl c3QgKnJlcSkNCjI4MiAgICAgew0KMjgzICAgICAgICAgICAgIHN0cnVjdCBzY3NpX2NtbmQgKnNj bWQgPSBibGtfbXFfcnFfdG9fcGR1KHJlcSk7DQoyODQgICAgICAgICAgICAgZW51bSBibGtfZWhf dGltZXJfcmV0dXJuIHJ0biA9IEJMS19FSF9OT1RfSEFORExFRDsNCjI4NSAgICAgICAgICAgICBz dHJ1Y3QgU2NzaV9Ib3N0ICpob3N0ID0gc2NtZC0+ZGV2aWNlLT5ob3N0Ow0KMjg2DQoyODcgICAg ICAgICAgICAgdHJhY2Vfc2NzaV9kaXNwYXRjaF9jbWRfdGltZW91dChzY21kKTsNCjI4OCAgICAg ICAgICAgICBzY3NpX2xvZ19jb21wbGV0aW9uKHNjbWQsIFRJTUVPVVRfRVJST1IpOw0KMjg5DQoN CihnZGIpIGRpc2FzIC9zIHNjc2lfdGltZXNfb3V0DQpbIC4uLiBdDQoyODMgICAgICAgICAgICAg c3RydWN0IHNjc2lfY21uZCAqc2NtZCA9IGJsa19tcV9ycV90b19wZHUocmVxKTsNCjI4NCAgICAg ICAgICAgICBlbnVtIGJsa19laF90aW1lcl9yZXR1cm4gcnRuID0gQkxLX0VIX05PVF9IQU5ETEVE Ow0KMjg1ICAgICAgICAgICAgIHN0cnVjdCBTY3NpX0hvc3QgKmhvc3QgPSBzY21kLT5kZXZpY2Ut Pmhvc3Q7DQogICAweGZmZmZmZmZmODE0N2FkYjIgPCsxOD46ICAgIG1vdiAgICAweDFkOCglcmRp KSwlcmF4DQogICAweGZmZmZmZmZmODE0N2FkYjkgPCsyNT46ICAgIG1vdiAgICAlcmRpLCVyYngN CiAgIDB4ZmZmZmZmZmY4MTQ3YWRiYyA8KzI4PjogICAgbW92ICAgICglcmF4KSwlcjEzDQogICAw eGZmZmZmZmZmODE0N2FkYmYgPCszMT46ICAgIG5vcGwgICAweDAoJXJheCwlcmF4LDEpDQoNCkJh cnQu

Can you please try the v3? There were a couple bugs that I missed while testing earlier versions.

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

Thanks.

tejun

T24gV2VkLCAyMDE3LTEyLTIwIGF0IDE2OjA4IC0wODAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K PiBPbiBXZWQsIERlYyAyMCwgMjAxNyBhdCAxMTo0MTowMlBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj aGUgd3JvdGU6DQo+ID4gT24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBI ZW8gd3JvdGU6DQo+ID4gPiBDdXJyZW50bHksIGJsay1tcSB0aW1lb3V0IHBhdGggc3luY2hyb25p emVzIGFnYWluc3QgdGhlIHVzdWFsDQo+ID4gPiBpc3N1ZS9jb21wbGV0aW9uIHBhdGggdXNpbmcg YSBjb21wbGV4IHNjaGVtZSBpbnZvbHZpbmcgYXRvbWljDQo+ID4gPiBiaXRmbGFncywgUkVRX0FU T01fKiwgbWVtb3J5IGJhcnJpZXJzIGFuZCBzdWJ0bGUgbWVtb3J5IGNvaGVyZW5jZQ0KPiA+ID4g cnVsZXMuICBVbmZvcnR1bmF0bGV5LCBpdCBjb250YWlucyBxdWl0ZSBhIGZldyBob2xlcy4NCj4g PiANCj4gPiBIZWxsbyBUZWp1biwNCj4gPiANCj4gPiBBbiBhdHRlbXB0IHRvIHJ1biBTQ1NJIEkv TyB3aXRoIHRoaXMgcGF0Y2ggc2VyaWVzIGFwcGxpZWQgcmVzdWx0ZWQgaW4NCj4gPiB0aGUgZm9s bG93aW5nOg0KPiANCj4gQ2FuIHlvdSBwbGVhc2UgdHJ5IHRoZSB2Mz8gIFRoZXJlIHdlcmUgYSBj b3VwbGUgYnVncyB0aGF0IEkgbWlzc2VkDQo+IHdoaWxlIHRlc3RpbmcgZWFybGllciB2ZXJzaW9u cy4NCj4gDQo+ICBodHRwOi8vbGttbC5rZXJuZWwub3JnL3IvMjAxNzEyMTYxMjA3MjYuNTE3MTUz LTEtdGpAa2VybmVsLm9yZw0KDQpXaWxsIGRvLiBCdXQgcGxlYXNlIENjOiBsaW51eC1ibG9jayBp biBjYXNlIHlvdSB3b3VsZCBwb3N0IGEgdjQgb2YgdGhpcyBwYXRjaA0Kc2VyaWVzLiBJIHNlYXJj aGVkIHRoZSBsaW51eC1ibG9jayBmb2xkZXIgb2YgbXkgbWFpbGJveCBmb3IgdGhlIGxhdGVzdCB2 ZXJzaW9uDQpvZiB0aGlzIHBhdGNoIHNlcmllcyBhbmQgdGhhdCBpcyBob3cgSSBlbmRlZCB1cCB0 ZXN0aW5nIHYyIGluc3RlYWQgb2YgdjMgLi4uDQoNCkJhcnQu