blk-mq: Factor out [s]rcu synchronization

Factor out [s]rcu synchronization in blk_mq_timeout_work() into blk_mq_timeout_sync_rcu(). This is to add another user in the future and doesn't cause any functional changes.

---
Hello,

We were tracking this down in the following thread

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

but lost the reproducer in the middle and couldn't fully verify these two patches fix the problem; however, the identified race is real and a bug, so I think it'd be best to apply these two patches.

Given the lack of further reports on this front, I don't think it's necessary to rush these patches. I think it'd be best to apply these once the merge window closes. If we need to backport these to -stable, we can tag them later.

Thanks.

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

@@ -876,13 +876,34 @@ static void blk_mq_check_expired(struct
 	    time_after_eq(jiffies, deadline)) {
 		blk_mq_rq_update_aborted_gstate(rq, gstate);
 		data->nr_expired++;
-		hctx->nr_expired++;
+		hctx->need_sync_rcu = true;
 	} else if (!data->next_set || time_after(data->next, deadline)) {
 		data->next = deadline;
 		data->next_set = 1;
 	}
 }
 
+static void blk_mq_timeout_sync_rcu(struct request_queue *q)
+{
+	struct blk_mq_hw_ctx *hctx;
+	bool has_rcu = false;
+	int i;
+
+	queue_for_each_hw_ctx(q, hctx, i) {
+		if (!hctx->need_sync_rcu)
+			continue;
+
+		if (!(hctx->flags & BLK_MQ_F_BLOCKING))
+			has_rcu = true;
+		else
+			synchronize_srcu(hctx->srcu);
+
+		hctx->need_sync_rcu = false;
+	}
+	if (has_rcu)
+		synchronize_rcu();
+}
+
 static void blk_mq_terminate_expired(struct blk_mq_hw_ctx *hctx,
 		struct request *rq, void *priv, bool reserved)
 {

@@ -930,27 +951,13 @@ static void blk_mq_timeout_work(struct w
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &data);
 
 	if (data.nr_expired) {
-		bool has_rcu = false;
-
 		/*
 		 * Wait till everyone sees ->aborted_gstate.  The
 		 * sequential waits for SRCUs aren't ideal.  If this ever
 		 * becomes a problem, we can add per-hw_ctx rcu_head and
 		 * wait in parallel.
 		 */
-		queue_for_each_hw_ctx(q, hctx, i) {
-			if (!hctx->nr_expired)
-				continue;
-
-			if (!(hctx->flags & BLK_MQ_F_BLOCKING))
-				has_rcu = true;
-			else
-				synchronize_srcu(hctx->srcu);
-
-			hctx->nr_expired = 0;
-		}
-		if (has_rcu)
-			synchronize_rcu();
+		blk_mq_timeout_sync_rcu(q);
 
 		/* terminate the ones we won */
 		blk_mq_queue_tag_busy_iter(q, blk_mq_terminate_expired, NULL);

@@ -51,7 +51,7 @@ struct blk_mq_hw_ctx {
 	unsigned int		queue_num;
 
 	atomic_t		nr_active;
-	unsigned int		nr_expired;
+	bool			need_sync_rcu;
 
 	struct hlist_node	cpuhp_dead;
 	struct kobject		kobj;

When a request is handed over from normal execution to timeout, we synchronize using ->aborted_gstate and RCU grace periods; however, when a request is being returned from timeout handling to normal execution for BLK_EH_RESET_TIMER, we were skipping the same synchronization.

This means that it theoretically is possible for a returned request's completion and recycling compete against the reordered and delayed writes from timeout path.

This patch adds an equivalent synchronization when a request is returned from timeout path to normal completion path.

---
 block/blk-mq.c         |   49 ++++++++++++++++++++++++++++++++++++++++---------
 block/blk-timeout.c    |    2 +-
 include/linux/blkdev.h |    4 +++-
 3 files changed, 44 insertions(+), 11 deletions(-)

@@ -818,7 +818,8 @@ struct blk_mq_timeout_data {
 	unsigned int nr_expired;
 };
 
-static void blk_mq_rq_timed_out(struct request *req, bool reserved)
+static void blk_mq_rq_timed_out(struct blk_mq_hw_ctx *hctx, struct request *req,
+				int *nr_resets, bool reserved)
 {
 	const struct blk_mq_ops *ops = req->q->mq_ops;
 	enum blk_eh_timer_return ret = BLK_EH_RESET_TIMER;

@@ -833,13 +834,10 @@ static void blk_mq_rq_timed_out(struct r
 		__blk_mq_complete_request(req);
 		break;
 	case BLK_EH_RESET_TIMER:
-		/*
-		 * As nothing prevents from completion happening while
-		 * ->aborted_gstate is set, this may lead to ignored
-		 * completions and further spurious timeouts.
-		 */
-		blk_mq_rq_update_aborted_gstate(req, 0);
 		blk_add_timer(req);
+		req->rq_flags |= RQF_MQ_TIMEOUT_RESET;
+		(*nr_resets)++;
+		hctx->need_sync_rcu = true;
 		break;
 	case BLK_EH_NOT_HANDLED:
 		break;

@@ -916,7 +914,26 @@ static void blk_mq_terminate_expired(str
 	 */
 	if (!(rq->rq_flags & RQF_MQ_TIMEOUT_EXPIRED) &&
 	    READ_ONCE(rq->gstate) == rq->aborted_gstate)
-		blk_mq_rq_timed_out(rq, reserved);
+		blk_mq_rq_timed_out(hctx, rq, priv, reserved);
+}
+
+static void blk_mq_finish_timeout_reset(struct blk_mq_hw_ctx *hctx,
+		struct request *rq, void *priv, bool reserved)
+{
+	/*
+	 * @rq's timer reset has gone through rcu synchronization and is
+	 * visible now.  Allow normal completions again by resetting
+	 * ->aborted_gstate.  Don't clear RQF_MQ_TIMEOUT_RESET here as
+	 * there's no memory ordering around ->aborted_gstate making it the
+	 * only field safe to update.  Let blk_add_timer() clear it later
+	 * when the request is recycled or times out again.
+	 *
+	 * As nothing prevents from completion happening while
+	 * ->aborted_gstate is set, this may lead to ignored completions
+	 * and further spurious timeouts.
+	 */
+	if (rq->rq_flags & RQF_MQ_TIMEOUT_RESET)
+		blk_mq_rq_update_aborted_gstate(rq, 0);
 }
 
 static void blk_mq_timeout_work(struct work_struct *work)

@@ -951,6 +968,8 @@ static void blk_mq_timeout_work(struct w
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &data);
 
 	if (data.nr_expired) {
+		int nr_resets = 0;
+
 		/*
 		 * Wait till everyone sees ->aborted_gstate.  The
 		 * sequential waits for SRCUs aren't ideal.  If this ever

@@ -960,7 +979,19 @@ static void blk_mq_timeout_work(struct w
 		blk_mq_timeout_sync_rcu(q);
 
 		/* terminate the ones we won */
-		blk_mq_queue_tag_busy_iter(q, blk_mq_terminate_expired, NULL);
+		blk_mq_queue_tag_busy_iter(q, blk_mq_terminate_expired,
+					   &nr_resets);
+
+		/*
+		 * For BLK_EH_RESET_TIMER, release the requests after
+		 * blk_add_timer() from above is visible to avoid timer
+		 * reset racing against recycling.
+		 */
+		if (nr_resets) {
+			blk_mq_timeout_sync_rcu(q);
+			blk_mq_queue_tag_busy_iter(q,
+					blk_mq_finish_timeout_reset, NULL);
+		}
 	}
 
 	if (data.next_set) {

@@ -216,7 +216,7 @@ void blk_add_timer(struct request *req)
 		req->timeout = q->rq_timeout;
 
 	blk_rq_set_deadline(req, jiffies + req->timeout);
-	req->rq_flags &= ~RQF_MQ_TIMEOUT_EXPIRED;
+	req->rq_flags &= ~(RQF_MQ_TIMEOUT_EXPIRED | RQF_MQ_TIMEOUT_RESET);
 
 	/*
 	 * Only the non-mq case needs to add the request to a protected list.

@@ -127,8 +127,10 @@ typedef __u32 __bitwise req_flags_t;
 #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))
+/* timeout is expired */
+#define RQF_MQ_TIMEOUT_RESET	((__force req_flags_t)(1 << 21))
 /* already slept for hybrid poll */
-#define RQF_MQ_POLL_SLEPT	((__force req_flags_t)(1 << 21))
+#define RQF_MQ_POLL_SLEPT	((__force req_flags_t)(1 << 22))
 
 /* flags that prevent us from merging requests: */
 #define RQF_NOMERGE_FLAGS \

Oh, that's not a new race. That's the same non-critical race which always existed. It's just being documented.

Thanks.

tejun

T24gTW9uLCAyMDE4LTA0LTAyIGF0IDE0OjEwIC0wNzAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IE9u IE1vbiwgQXByIDAyLCAyMDE4IGF0IDAyOjA4OjM3UE0gLTA3MDAsIEJhcnQgVmFuIEFzc2NoZSB3 cm90ZToNCj4gPiBPbiAwNC8wMi8xOCAxMjowMSwgVGVqdW4gSGVvIHdyb3RlOg0KPiA+ID4gKwkg KiBBcyBub3RoaW5nIHByZXZlbnRzIGZyb20gY29tcGxldGlvbiBoYXBwZW5pbmcgd2hpbGUNCj4g PiA+ICsJICogLT5hYm9ydGVkX2dzdGF0ZSBpcyBzZXQsIHRoaXMgbWF5IGxlYWQgdG8gaWdub3Jl ZCBjb21wbGV0aW9ucw0KPiA+ID4gKwkgKiBhbmQgZnVydGhlciBzcHVyaW91cyB0aW1lb3V0cy4N Cj4gPiA+ICsJICovDQo+ID4gPiArCWlmIChycS0+cnFfZmxhZ3MgJiBSUUZfTVFfVElNRU9VVF9S RVNFVCkNCj4gPiA+ICsJCWJsa19tcV9ycV91cGRhdGVfYWJvcnRlZF9nc3RhdGUocnEsIDApOw0K PiA+IA0KPiA+IEhlbGxvIFRlanVuLA0KPiA+IA0KPiA+IFNpbmNlIHRoaXMgcGF0Y2ggZml4ZXMg b25lIHJhY2UgYnV0IGludHJvZHVjZXMgYW5vdGhlciByYWNlLCBpcyB0aGlzDQo+ID4gcGF0Y2gg cmVhbGx5IGFuIGltcHJvdmVtZW50Pw0KPiANCj4gT2gsIHRoYXQncyBub3QgYSBuZXcgcmFjZS4g IFRoYXQncyB0aGUgc2FtZSBub24tY3JpdGljYWwgcmFjZSB3aGljaA0KPiBhbHdheXMgZXhpc3Rl ZC4gIEl0J3MganVzdCBiZWluZyBkb2N1bWVudGVkLg0KDQpIZWxsbyBUZWp1biwNCg0KSSB0aGlu ayBpdCBjYW4gaGFwcGVuIHRoYXQgdGhlIGFib3ZlIGNvZGUgc2VlcyB0aGF0IChycS0+cnFfZmxh Z3MgJg0KUlFGX01RX1RJTUVPVVRfUkVTRVQpICE9IDAsIHRoYXQgYmxrX21xX3N0YXJ0X3JlcXVl c3QoKSBleGVjdXRlcyB0aGUNCmZvbGxvd2luZyBjb2RlOg0KDQoJYmxrX21xX3JxX3VwZGF0ZV9z dGF0ZShycSwgTVFfUlFfSU5fRkxJR0hUKTsNCglibGtfYWRkX3RpbWVyKHJxKTsNCg0KYW5kIHRo YXQgc3Vic2VxdWVudGx5IGJsa19tcV9ycV91cGRhdGVfYWJvcnRlZF9nc3RhdGUocnEsIDApIGlz IGNhbGxlZCwNCndoaWNoIHdpbGwgY2F1c2UgdGhlIG5leHQgY29tcGxldGlvbiB0byBiZSBsb3N0 LiBJcyBmaXhpbmcgb25lIG9jY3VycmVuY2UNCm9mIGEgcmFjZSBhbmQgcmVpbnRyb2R1Y2luZyBp dCBpbiBhbm90aGVyIGNvZGUgcGF0aCByZWFsbHkgYW4gaW1wcm92ZW1lbnQ/DQoNClRoYW5rcywN Cg0KQmFydC4NCg0KDQoNCg==

Hello,

...

I'm not following at all. How would blk_mq_start_request() get called on the request while it's still owned by the timeout handler? That gstate clearing is what transfers the ownership back to the non-timeout path.

Thanks.

tejun

T24gTW9uLCAyMDE4LTA0LTAyIGF0IDE0OjEwIC0wNzAwLCBUZWp1biBIZW8gd3JvdGU6DQo+IE9u IE1vbiwgQXByIDAyLCAyMDE4IGF0IDAyOjA4OjM3UE0gLTA3MDAsIEJhcnQgVmFuIEFzc2NoZSB3 cm90ZToNCj4gPiBPbiAwNC8wMi8xOCAxMjowMSwgVGVqdW4gSGVvIHdyb3RlOg0KPiA+ID4gKwkg KiBBcyBub3RoaW5nIHByZXZlbnRzIGZyb20gY29tcGxldGlvbiBoYXBwZW5pbmcgd2hpbGUNCj4g PiA+ICsJICogLT5hYm9ydGVkX2dzdGF0ZSBpcyBzZXQsIHRoaXMgbWF5IGxlYWQgdG8gaWdub3Jl ZCBjb21wbGV0aW9ucw0KPiA+ID4gKwkgKiBhbmQgZnVydGhlciBzcHVyaW91cyB0aW1lb3V0cy4N Cj4gPiA+ICsJICovDQo+ID4gPiArCWlmIChycS0+cnFfZmxhZ3MgJiBSUUZfTVFfVElNRU9VVF9S RVNFVCkNCj4gPiA+ICsJCWJsa19tcV9ycV91cGRhdGVfYWJvcnRlZF9nc3RhdGUocnEsIDApOw0K PiA+IA0KPiA+IEhlbGxvIFRlanVuLA0KPiA+IA0KPiA+IFNpbmNlIHRoaXMgcGF0Y2ggZml4ZXMg b25lIHJhY2UgYnV0IGludHJvZHVjZXMgYW5vdGhlciByYWNlLCBpcyB0aGlzDQo+ID4gcGF0Y2gg cmVhbGx5IGFuIGltcHJvdmVtZW50Pw0KPiANCj4gT2gsIHRoYXQncyBub3QgYSBuZXcgcmFjZS4g IFRoYXQncyB0aGUgc2FtZSBub24tY3JpdGljYWwgcmFjZSB3aGljaA0KPiBhbHdheXMgZXhpc3Rl ZC4gIEl0J3MganVzdCBiZWluZyBkb2N1bWVudGVkLg0KDQpUaGlzIHBhdGNoIGluY3JlYXNlcyB0 aGUgdGltZSBkdXJpbmcgd2hpY2ggLmFib3J0ZWRfZ3N0YXRlID09IC5nc3RhdGUgaWYgdGhlDQp0 aW1lb3V0IGlzIHJlc2V0LiBEb2VzIHRoYXQgaW5jcmVhc2UgdGhlIGNoYW5jZSB0aGF0IGEgY29t cGxldGlvbiB3aWxsIGJlIG1pc3NlZA0KaWYgdGhlIHJlcXVlc3QgdGltZW91dCBpcyByZXNldD8N Cg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg==

Hello,

It sure does, but we're comparing an outright kernel bug vs. an inherently opportunistic mechanism being a bit more lossy. I think the answer is pretty clear.

Thanks.

tejun

T24gTW9uLCAyMDE4LTA0LTAyIGF0IDE1OjAxIC0wNzAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K PiBPbiBNb24sIEFwciAwMiwgMjAxOCBhdCAwOTo1Njo0MVBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj aGUgd3JvdGU6DQo+ID4gVGhpcyBwYXRjaCBpbmNyZWFzZXMgdGhlIHRpbWUgZHVyaW5nIHdoaWNo IC5hYm9ydGVkX2dzdGF0ZSA9PSAuZ3N0YXRlIGlmIHRoZQ0KPiA+IHRpbWVvdXQgaXMgcmVzZXQu IERvZXMgdGhhdCBpbmNyZWFzZSB0aGUgY2hhbmNlIHRoYXQgYSBjb21wbGV0aW9uIHdpbGwgYmUg bWlzc2VkDQo+ID4gaWYgdGhlIHJlcXVlc3QgdGltZW91dCBpcyByZXNldD8NCj4gDQo+IEl0IHN1 cmUgZG9lcywgYnV0IHdlJ3JlIGNvbXBhcmluZyBhbiBvdXRyaWdodCBrZXJuZWwgYnVnIHZzLiBh bg0KPiBpbmhlcmVudGx5IG9wcG9ydHVuaXN0aWMgbWVjaGFuaXNtIGJlaW5nIGEgYml0IG1vcmUg bG9zc3kuICBJIHRoaW5rDQo+IHRoZSBhbnN3ZXIgaXMgcHJldHR5IGNsZWFyLg0KDQpIZWxsbyBU ZWp1biwNCg0KUGxlYXNlIGVsYWJvcmF0ZSB3aGF0IHlvdXIgbG9uZy10ZXJtIGdvYWwgaXMgZm9y IHRoZSBibGstbXEgdGltZW91dCBoYW5kbGVyLg0KVGhlIGxlZ2FjeSBibG9jayBsYXllciBzdXNw ZW5kcyByZXF1ZXN0IHN0YXRlIGNoYW5nZXMgd2hpbGUgYSB0aW1lb3V0IGlzDQpiZWluZyBwcm9j ZXNzZWQgYnkgaG9sZGluZyB0aGUgcmVxdWVzdCBxdWV1ZSBsb2NrIHdoaWxlIHJlcXVlc3RzIGFy ZSBiZWluZw0KcHJvY2Vzc2VkLCB3aGlsZSBwcm9jZXNzaW5nIGEgdGltZW91dCBhbmQgd2hpbGUg Y2FsbGluZyBxLT5ycV90aW1lZF9vdXRfZm4ocnEpLg0KRG8geW91IHRoaW5rIGl0IGlzIHBvc3Np YmxlIHRvIG1ha2UgdGhlIGJsay1tcSBjb3JlIHN1c3BlbmQgcmVxdWVzdCBwcm9jZXNzaW5nDQp3 aGlsZSBwcm9jZXNzaW5nIGEgdGltZW91dCB3aXRob3V0IGludHJvZHVjaW5nIGxvY2tpbmcgaW4N CmJsa19tcV9jb21wbGV0ZV9yZXF1ZXN0KCk/IElmIHlvdSBkbyBub3QgcGxhbiB0byBhZGQgbG9j a2luZyBpbg0KYmxrX21xX2NvbXBsZXRlX3JlcXVlc3QoKSwgZG8geW91IHRoaW5rIGl0IGlzIHBv c3NpYmxlIHRvIGZpeCBhbGwgdGhlIHJhY2VzIHdlDQpkaXNjdXNzZWQgaW4gcHJldmlvdXMgZS1t YWlscz8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg==

Hello,

Hmm... I don't really have any plans beyond what's been posted.

I don't know of multiple race conditions. What am I missing? AFAIK, there's one non-critical race condition which has always been there. We have a larger race window for that case but don't yet know whether that's problematic or not. If that actually is problematic, we can figure out a way to solve that but such effort / added complexity doesn't seem justified yet. No?

Thanks.

tejun

T24gTW9uLCAyMDE4LTA0LTAyIGF0IDE1OjE2IC0wNzAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K PiBBRkFJSywNCj4gdGhlcmUncyBvbmUgbm9uLWNyaXRpY2FsIHJhY2UgY29uZGl0aW9uIHdoaWNo IGhhcyBhbHdheXMgYmVlbiB0aGVyZS4NCj4gV2UgaGF2ZSBhIGxhcmdlciByYWNlIHdpbmRvdyBm b3IgdGhhdCBjYXNlIGJ1dCBkb24ndCB5ZXQga25vdyB3aGV0aGVyDQo+IHRoYXQncyBwcm9ibGVt YXRpYyBvciBub3QuICBJZiB0aGF0IGFjdHVhbGx5IGlzIHByb2JsZW1hdGljLCB3ZSBjYW4NCj4g ZmlndXJlIG91dCBhIHdheSB0byBzb2x2ZSB0aGF0IGJ1dCBzdWNoIGVmZm9ydCAvIGFkZGVkIGNv bXBsZXhpdHkNCj4gZG9lc24ndCBzZWVtIGp1c3RpZmllZCB5ZXQuICBObz8NCg0KSGVsbG8gVGVq dW4sDQoNClNvbWUgaW1wb3J0YW50IGJsb2NrIGRyaXZlcnMgc3lzdGVtYXRpY2FsbHkgcmV0dXJu IEJMS19FSF9SRVNFVF9USU1FUg0KZnJvbSB0aGVpciB0aW1lb3V0IGhhbmRsZXIsIGUuZy4gdGhl IHZpcnRpby1zY3NpIGluaXRpYXRvciBkcml2ZXIuIFdoYXQNCndpbGwgdGhlIGNvbnNlcXVlbmNl IGJlIGZvciB0aGVzZSBkcml2ZXJzIGlmIGEgYmxrX21xX2NvbXBsZXRlX3JlcXVlc3QoKQ0KY2Fs bCBpcyBpZ25vcmVkPyBXaWxsIHRoZSByZXF1ZXN0IGZvciB3aGljaCB0aGUgY29tcGxldGlvbiB3 YXMgaWdub3JlZA0KZ2V0IHN0dWNrPw0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg0KDQo=