blk-mq: move timeout handling from queue to tagset
Subsystems (2)
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.
--- 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
@@ -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
@@ -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
@@ -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
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
-0queue_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
-0unsigned 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=
e
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
-2if (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
-1blk_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 @@ structblk_mq_tag_set{
-
Quoted 5 lines
=20
structblk_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.
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 afterblk_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
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_listwhile the above loop examinestag_list? Canblk_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
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
-0struct blk_mq_tags **tags; +struct timer_list timer; +struct work_struct timeout_work;Can the timer and
timeout_workdata structures be replaced by a singledelayed_workinstance?
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!
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
-0queue_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 amake_request_fnstacking driver, right? There should=
be
Quoted 3 lines
no impact in that case, since the change above affects only mq. =20
I'm actually a little puzzled why md callsblk_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.=
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
-0struct blk_mq_tags **tags; +struct timer_list timer; +struct work_struct timeout_work;Can the timer and
timeout_workdata structures be replaced by a singledelayed_workinstance?
Quoted 6 lines
I think so. I wanted to keep
blk_add_timerrelatively 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 triggerqueue_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