blk-mq: move timeout handling from queue to tagset

RFC PATCH In review Keith Busch 4 participants 5 messages Jul 18, 2018 – Jul 19, 2018

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Keith Busch

author

2018-07-18 17:00 UTC #0 lore raw

RFC PATCH 1/1 blk-mq: move timeout handling from queue to tagset

This patch removes the per-request_queue timeout handling and uses the tagset instead. This simplifies the timeout handling since a shared tag set can handle all timed out requests in a single work queue rather than iterate the same set in different work for all the users of that set.

The long term objective of this is to aid blk-mq drivers with shared tagsets. These drivers typically want their timeout error handling to be single threaded, and this patch provides that context so they wouldn't need to sync with other request queues requiring the same error handling.

Timeout handling per-queue was a bit racy with completions anyway:
a tag active for one request_queue while iterating could be freed and reallocated to a different request_queue, so a queue's timeout handler may be operating on a request allocated to a different queue. Though no real harm was done with how the callbacks used those requests, this patch fixes that race.

And since the timeout code takes a reference on requests, the request can never exit the queue while timeout code is considering. We no longer need to enter each request_queue in the timeout work so this patch removes that unnecessary code.

Signed-off-by Keith Busch <[email protected]>

---
 block/blk-core.c       |  5 ++---
 block/blk-mq.c         | 45 ++++++++++++++++++++-------------------------
 block/blk-timeout.c    | 16 ++++++++++------
 include/linux/blk-mq.h |  2 ++
 4 files changed, 34 insertions(+), 34 deletions(-)

block/blk-core.c +2 -3

index f84a9b7b6f5a..9cf7ff6baba0 100644
@@ -404,9 +404,6 @@ EXPORT_SYMBOL(blk_stop_queue);
  */
 void blk_sync_queue(struct request_queue *q)
 {
-	del_timer_sync(&q->timeout);
-	cancel_work_sync(&q->timeout_work);
-
 	if (q->mq_ops) {
 		struct blk_mq_hw_ctx *hctx;
 		int i;

@@ -415,6 +412,8 @@ void blk_sync_queue(struct request_queue *q)
 		queue_for_each_hw_ctx(q, hctx, i)
 			cancel_delayed_work_sync(&hctx->run_work);
 	} else {
+		del_timer_sync(&q->timeout);
+		cancel_work_sync(&q->timeout_work);
 		cancel_delayed_work_sync(&q->delay_work);
 	}
 }

block/blk-mq.c +20 ~~-25~~

87 lines not on this page · raw message

block/blk-timeout.c +10 -6

index f2cfd56e1606..fe26aa5305c9 100644
@@ -193,9 +193,13 @@ void blk_add_timer(struct request *req)
 {
 	struct request_queue *q = req->q;
 	unsigned long expiry;
+	struct timer_list *timer;
 
-	if (!q->mq_ops)
+	if (!q->mq_ops) {
 		lockdep_assert_held(q->queue_lock);
+		timer = &q->timeout;
+	} else
+		timer = &q->tag_set->timer;
 
 	/* blk-mq has its own handler, so we don't need ->rq_timed_out_fn */
 	if (!q->mq_ops && !q->rq_timed_out_fn)

@@ -227,9 +231,9 @@ void blk_add_timer(struct request *req)
 	 */
 	expiry = blk_rq_timeout(round_jiffies_up(blk_rq_deadline(req)));
 
-	if (!timer_pending(&q->timeout) ||
-	    time_before(expiry, q->timeout.expires)) {
-		unsigned long diff = q->timeout.expires - expiry;
+	if (!timer_pending(timer) ||
+	    time_before(expiry, timer->expires)) {
+		unsigned long diff = timer->expires - expiry;
 
 		/*
 		 * Due to added timer slack to group timers, the timer

@@ -238,8 +242,8 @@ void blk_add_timer(struct request *req)
 		 * modifying the timer because expires for value X
 		 * will be X + something.
 		 */
-		if (!timer_pending(&q->timeout) || (diff >= HZ / 2))
-			mod_timer(&q->timeout, expiry);
+		if (!timer_pending(timer) || (diff >= HZ / 2))
+			mod_timer(timer, expiry);
 	}
 
 }

include/linux/blk-mq.h +2 -0

index e3147eb74222..9b0fd11ce89a 100644
@@ -86,6 +86,8 @@ struct blk_mq_tag_set {
 
 	struct blk_mq_tags	**tags;
 
+	struct timer_list	timer;
+	struct work_struct	timeout_work;
 	struct mutex		tag_list_lock;
 	struct list_head	tag_list;
 };

Signature · 1 line

2.14.4

Bart Van Assche

2018-07-18 17:18 UTC #1 lore raw

Quoted 9 lines · Keith Busch

void blk_sync_queue(struct request_queue *q)
{
block/blk-core.c +0 -3
-	del_timer_sync(&q->timeout);
-	cancel_work_sync(&q->timeout_work);
-
 	if (q->mq_ops) {
 		struct blk_mq_hw_ctx *hctx;
 		int i;
@@ -415,6 +-412,8 @@ void blk_sync_queue(struct r=

equest_queue *q)

Review

Quoted 2 lines

(quoted diff) +0 -0

 		queue_for_each_hw_ctx(q, hctx, i)
 			cancel_delayed_work_sync(&hctx->run_work)=

;

Review

Quoted 4 lines

(quoted diff) +2 -0

 	} else {
+-		del_timer_sync(&q->timeout);
+-		cancel_work_sync(&q->timeout_work);
 		cancel_delayed_work_sync(&q->delay_work)�

Ds-

Quoted 2 lines

}
}

What is the impact of this change on the md driver, which is the only drive= r
that calls blk_sync_queue() directly? What will happen if timeout p= rocessing
happens concurrently with or after blk_sync_queue() has returned?

Quoted 1 line

static void blk_mq_timeout_work(struct work_struct +=

ACo-work)

Review

Quoted 3 lines

{

(quoted diff) +0 -2

-	struct request_queue *q =
-		container_of(work, struct request_queue, timeout_work)=

;

Quoted 2 lines

+-	struct blk_mq_tag_set *set =
+-		container_of(work, struct blk_mq_tag_set, timeout=

_work);

block/blk-mq.c static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,

Quoted 11 lines

+- struct request_queue *q;

block/blk-mq.c +0 -0

 	unsigned long next = 0;
 	struct blk_mq_hw_ctx *hctx;
 	int i;

=20

block/blk-mq.c +0 -6

-	/* A deadlock might occur if a request is stuck requiring a
-	 * timeout at the same time a queue freeze is waiting
-	 * completion, since the timeout code would not be able to
-	 * acquire the queue reference here.
-	 *
-	 * That's why we don't use blk_queue_enter here; in=

stead, we use

Review

Quoted 1 line

(quoted diff) +0 -1

-	 * percpu_ref_tryget directly, because we need to be ab=

le to

Review

Quoted 1 line

(quoted diff) +0 -1

-	 * obtain a reference even in the short window between the queu=

Review

Quoted 2 lines

(quoted diff) +0 -2

-	 * starting to freeze, by dropping the first reference in
-	 * blk_freeze_queue_start, and the moment the last =

request is

Review

Quoted 1 line

(quoted diff) +0 -1

-	 * consumed, marked by the instant q_usage_counter reac=

hes

block/blk-mq.c static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,

Quoted 3 lines

block/blk-mq.c +0 -3

-	 * zero.
-	 */
-	if (!percpu_ref_tryget(&q->q_usage_coun=

ter))

block/blk-mq.c static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,

Quoted 3 lines

block/blk-mq.c +0 -3

-		return;
-
-	blk_mq_queue_tag_busy_iter(q, blk_mq_ch=

eck_expired, &next);

Quoted 1 line

+- blk_mq_tagset_busy_iter(set, blk_mq_check+=

AF8-expired, &next);

Review

Quoted 9 lines

=20

(quoted diff) +5 -2

 	if (next != 0) {
-		mod_timer(&q->timeout, next);
-	} else {
+-		mod_timer(&set->timer, next);
+-		return;
+-	}
+-
+-	list_for_each_entry(q, &set->tag_list, tag=

_set_list) {

Quoted 4 lines

block/blk-mq.c +0 -0

 		/*
 		 * Request timeouts are handled as a forward rolling timer. If
 		 * we end up here it means that no requests are pending and

@@ -881,7 +-868,6 @@ static void blk_mq_timeout�

F8-work(struct work_struct *work)

block/blk-mq.c static void blk_mq_timeout_work(struct work_struct *work) · line 871

Quoted 5 lines

block/blk-mq.c +0 -1

 				blk_mq_tag_idle(hctx);
 		}
 	}
-	blk_queue_exit(q);
 }

What prevents that a request queue is removed from set->tag_list wh= ile the above
loop examines tag_list? Can blk_cleanup_queue() queue be called=
from the context
of another thread while this loop is examining hardware queues? =20

Quoted 1 line

+- timer_setup(&set->timer, blk_mq_timed_out+=

AF8-timer, 0);

Quoted 1 line

+- INIT_WORK(&set->timeout_work, blk_mq_timeo=

ut_work);

Quoted 4 lines

[ ... ]
--- a/include/linux/blk-mq.h
+-+-+- b/include/linux/blk-mq.h
@@ -86,6 +-86,8 @@ struct blk_mq_tag_set {

Quoted 5 lines

=20
struct blk_mq_tags **tags;
=20
+-	struct timer_list	timer;
+-	struct work_struct	timeout_work;

Can the timer and timeout_work data structures be replaced by a single delayed_work instance?

Thanks,

Bart.

Keith Busch

2018-07-18 17:45 UTC #2 lore raw

Quoted 11 lines · Bart Van Assche

Quoted 10 lines · Keith Busch

block/blk-core.c +1 -2

-	cancel_work_sync(&q->timeout_work);
-
 	if (q->mq_ops) {
 		struct blk_mq_hw_ctx *hctx;
 		int i;

@@ -415,6 +412,8 @@ void blk_sync_queue(struct request_queue *q)
 		queue_for_each_hw_ctx(q, hctx, i)
 			cancel_delayed_work_sync(&hctx->run_work);
 	} else {
+		del_timer_sync(&q->timeout);

Quoted 8 lines

block/blk-core.c void blk_sync_queue(struct request_queue *q) · line 419
Quoted 4 lines
block/blk-core.c +1 -0
+		cancel_work_sync(&q->timeout_work);
 		cancel_delayed_work_sync(&q->delay_work);
 	}
 }
What is the impact of this change on the md driver, which is the only driver that calls blk_sync_queue() directly? What will happen if timeout processing happens concurrently with or after blk_sync_queue() has returned?

That's a make_request_fn stacking driver, right? There should be no impact in that case, since the change above affects only mq.

I'm actually a little puzzled why md calls blk_sync_queue. Are the queue timers ever used for bio-based drivers?

Quoted 6 lines

block/blk-mq.c +1 -0

+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		/*
 		 * Request timeouts are handled as a forward rolling timer. If
 		 * we end up here it means that no requests are pending and

@@ -881,7 +868,6 @@ static void blk_mq_timeout_work(struct work_struct *work)
 				blk_mq_tag_idle(hctx);

Quoted 8 lines

block/blk-mq.c static void blk_mq_timeout_work(struct work_struct *work) · line 871
Quoted 4 lines
block/blk-mq.c +0 -1
 		}
 	}
-	blk_queue_exit(q);
 }
What prevents that a request queue is removed from set->tag_list while the above loop examines tag_list? Can blk_cleanup_queue() queue be called from the context of another thread while this loop is examining hardware queues?

Good point. I missed that this needs to hold the tag_list_lock.

Quoted 7 lines

block/blk-mq.c +2 -0

+	timer_setup(&set->timer, blk_mq_timed_out_timer, 0);
+	INIT_WORK(&set->timeout_work, blk_mq_timeout_work);
[ ... ]

include/linux/blk-mq.h +0 -0

@@ -86,6 +86,8 @@ struct blk_mq_tag_set {

Quoted 7 lines

include/linux/blk-mq.h struct blk_mq_tag_set { · line 90
Quoted 4 lines
include/linux/blk-mq.h +2 -0
 	struct blk_mq_tags	**tags;
 
+	struct timer_list	timer;
+	struct work_struct	timeout_work;
Can the timer and timeout_work data structures be replaced by a single delayed_work instance?

I think so. I wanted to keep blk_add_timer relatively unchanged for this proposal, so I followed the existing pattern with the timer kicking the work. I don't see why that extra indirection is necessary, so I think it's a great idea. Unless anyone knows a reason not to, we can collapse this into a single delayed work for both mq and legacy as a prep patch before this one.

Thanks for the feedback!

Bart Van Assche

2018-07-18 18:34 UTC #3 lore raw

Quoted 3 lines · Keith Busch

Quoted 2 lines · Bart Van Assche
Review
Quoted 1 line · Keith Busch
(quoted diff) +0 -1
-	cancel_work_sync(&q->timeout_work)+=

ADs-

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
block/blk-core.c +0 -1
-
 	if (q->mq_ops) {
 		struct blk_mq_hw_ctx *hctx;
 		int i;
@@ -415,6 +-412,8 @@ void blk_sync_qu=

eue(struct request_queue *q)

Quoted 2 lines

Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
 		queue_for_each_hw_ctx(q, hctx, i)
 			cancel_delayed_work_sync(&hctx->r=

un_work);

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
} else {
+-		del_timer_sync(&q->timeout);
+-		cancel_work_sync(&q->timeout_work=

);

Quoted 1 line

Quoted 1 line
Quoted 1 line
cancel_delayed_work_sync(&q->delay=

_work);

Quoted 4 lines

Quoted 4 lines
Quoted 2 lines
}
}
=20
What is the impact of this change on the md driver, which is th=

e only driver

Quoted 1 line

Quoted 1 line
that calls blk_sync_queue() directly? What will happen =

if timeout processing

Quoted 1 line

Quoted 1 line
happens concurrently with or after blk_sync_queue() has=

returned?

Quoted 2 lines

=20
That's a make_request_fn stacking driver, right? There should=

Quoted 3 lines

no impact in that case, since the change above affects only mq. =20
I'm actually a little puzzled why md calls blk_sync_queue. Ar=

e the

Quoted 1 line

queue timers ever used for bio-based drivers?

Hello Keith,

For all md drivers that I verified calling md_make_request() trigge= rs one or
more generic_make_request() calls for the underlying devices. So it= 's not
clear to me why the md driver calls blk_sync_queue().

Bart.=

jianchao.wang

2018-07-19 09:15 UTC #4 lore raw

In reply to Keith Busch

Hi Keith

Quoted 9 lines · Keith Busch

Quoted 9 lines

block/blk-mq.c static void blk_mq_timeout_work(struct work_struct *work) · line 884

Quoted 9 lines

block/blk-mq.c +1 -1

+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		/*
 		 * Request timeouts are handled as a forward rolling timer. If
 		 * we end up here it means that no requests are pending and

@@ -881,7 +868,6 @@ static void blk_mq_timeout_work(struct work_struct *work)
 				blk_mq_tag_idle(hctx);
 		}
 	}
-	blk_queue_exit(q);

The tags sharing fairness mechanism between different request_queues cannot work well here. When timer is per-request_queue, if there is no request on one request_queue, it could be idled. But now, with per-tagset timer, we cannot detect the idle one at all.

Quoted 14 lines

Quoted 8 lines
Quoted 8 lines
block/blk-mq.c +2 -0
+	timer_setup(&set->timer, blk_mq_timed_out_timer, 0);
+	INIT_WORK(&set->timeout_work, blk_mq_timeout_work);
[ ... ]
include/linux/blk-mq.h +0 -0
@@ -86,6 +86,8 @@ struct blk_mq_tag_set {
 
Quoted 6 lines
include/linux/blk-mq.h struct blk_mq_tag_set { · line 90
Quoted 4 lines
include/linux/blk-mq.h +2 -0
 	struct blk_mq_tags	**tags;
 
+	struct timer_list	timer;
+	struct work_struct	timeout_work;
Can the timer and timeout_work data structures be replaced by a single delayed_work instance?

Quoted 6 lines

I think so. I wanted to keep blk_add_timer relatively unchanged for this proposal, so I followed the existing pattern with the timer kicking the work. I don't see why that extra indirection is necessary, so I think it's a great idea. Unless anyone knows a reason not to, we can collapse this into a single delayed work for both mq and legacy as a prep patch before this one.

mod_delayed_work_on is very tricky in our scenario. It will grab the pending work entry and queue it again.

delayed_work.timer trigger
queue_work timeout_work delayed_work.timer not pending
mod_delayed_work_on
grab the pending timeout_work
re-arm the timer

The timeout_work would not be run.

Thanks
Jianchao