blk-mq: use a delayed work item for timeouts

RFC PATCH New Christoph Hellwig 2 participants 5 messages Oct 12, 2015

Subsystems (3)

SCSI Block layer NVMe

lore

Share & tools

lore

Christoph Hellwig

author maintainer

2015-10-12 19:29 UTC #0 lore raw

RFC PATCH 1/1 blk-mq: use a delayed work item for timeouts

For some pending NVMe work I'd really love to be able to get my timeouts from process context. So far it seems only SCSI and NVMe use the blk-mq timeout handler, and both don't seem to be particularly excited about being called from time context. Does anyone have an objection against the patch below that switches it to use a delayed work item? I could make use of this quickly for NVMe, but for SCSI we still have to deal with the old request code which can't be switched to a delayed work as easily.

---
 block/blk-mq.c         | 11 ++++++-----
 include/linux/blkdev.h |  5 ++++-
 2 files changed, 10 insertions(+), 6 deletions(-)

block/blk-mq.c +6 -5

index d921cd5..a7ae387 100644
@@ -635,9 +635,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	}
 }
 
-static void blk_mq_rq_timer(unsigned long priv)
+static void blk_mq_rq_timer_work(struct work_struct *work)
 {
-	struct request_queue *q = (struct request_queue *)priv;
+	struct request_queue *q =
+		container_of(work, struct request_queue, timeout_work.work);
 	struct blk_mq_timeout_data data = {
 		.next		= 0,
 		.next_set	= 0,

@@ -648,7 +649,7 @@ static void blk_mq_rq_timer(unsigned long priv)
 
 	if (data.next_set) {
 		data.next = blk_rq_timeout(round_jiffies_up(data.next));
-		mod_timer(&q->timeout, data.next);
+		mod_delayed_work(system_wq, &q->timeout_work, data.next);
 	} else {
 		struct blk_mq_hw_ctx *hctx;

@@ -2008,7 +2009,7 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 			    PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
 		goto err_hctxs;
 
-	setup_timer(&q->timeout, blk_mq_rq_timer, (unsigned long) q);
+	INIT_DELAYED_WORK(&q->timeout_work, blk_mq_rq_timer_work);
 	blk_queue_rq_timeout(q, set->timeout ? set->timeout : 30 * HZ);
 
 	q->nr_queues = nr_cpu_ids;

@@ -2173,7 +2174,7 @@ static int blk_mq_queue_reinit_notify(struct notifier_block *nb,
 		 * timeout handler can't touch hw queue during the
 		 * reinitialization
 		 */
-		del_timer_sync(&q->timeout);
+		cancel_delayed_work_sync(&q->timeout_work);
 	}
 
 	list_for_each_entry(q, &all_q_list, all_q_node)

include/linux/blkdev.h +4 -1

index 19c2e94..ecce48f 100644
@@ -401,7 +401,10 @@ struct request_queue {
 	unsigned int		request_fn_active;
 
 	unsigned int		rq_timeout;
-	struct timer_list	timeout;
+	union {
+		struct timer_list	timeout;	/* legacy */
+		struct delayed_work	timeout_work;	/* blk-mq */
+	};
 	struct list_head	timeout_list;
 
 	struct list_head	icq_list;

Signature · 1 line

1.9.1

Jens Axboe

2015-10-12 19:34 UTC #1 lore raw

RFC PATCH 1/1 blk-mq: use a delayed work item for timeouts

Quoted 8 lines · Christoph Hellwig

For some pending NVMe work I'd really love to be able to get my timeouts from process context. So far it seems only SCSI and NVMe use the blk-mq timeout handler, and both don't seem to be particularly excited about being called from time context. Does anyone have an objection against the patch below that switches it to use a delayed work item? I could make use of this quickly for NVMe, but for SCSI we still have to deal with the old request code which can't be switched to a delayed work as easily.

No that's definitely fine with me, imho most error handling callbacks should be in process context for ease of use in the driver.

Signature · 1 line

Jens Axboe

Jens Axboe

2015-10-12 20:08 UTC #2 lore raw

RFC PATCH 1/1 blk-mq: use a delayed work item for timeouts

Quoted 10 lines · Jens Axboe

Quoted 8 lines · Christoph Hellwig
For some pending NVMe work I'd really love to be able to get my timeouts from process context. So far it seems only SCSI and NVMe use the blk-mq timeout handler, and both don't seem to be particularly excited about being called from time context. Does anyone have an objection against the patch below that switches it to use a delayed work item? I could make use of this quickly for NVMe, but for SCSI we still have to deal with the old request code which can't be switched to a delayed work as easily.

Quoted 2 lines

No that's definitely fine with me, imho most error handling callbacks should be in process context for ease of use in the driver.

Took a closer look. The patch looks incomplete. The hot path for blk-mq is blk_add_timer(), looks like you left that one alone in the conversion?

Might be easier to just leave the timer alone, and if it actually fires _and_ we have to do something, punt to a workqueue instead of invoking the timeout handler directly.

Signature · 1 line

Jens Axboe

Christoph Hellwig

author maintainer

2015-10-12 20:22 UTC #3 lore raw

RFC PATCH 1/1 blk-mq: use a delayed work item for timeouts

Quoted 5 lines · Jens Axboe

Quoted 2 lines
No that's definitely fine with me, imho most error handling callbacks should be in process context for ease of use in the driver.
Took a closer look. The patch looks incomplete. The hot path for blk-mq is blk_add_timer(), looks like you left that one alone in the conversion?

Oh, damn. I had that part in my initial version that also crudely converted the old request code and dropped a bit too much. That should defintively do the queue_deayed_work.

Quoted 3 lines

Might be easier to just leave the timer alone, and if it actually fires _and_ we have to do something, punt to a workqueue instead of invoking the timeout handler directly.

queue_delayed_work just assigns two additional fields, then sets timer->experies and does an add_timer. So it's the generic implementation of your above scheme. I'd much rather use the generic version if possible instead of trying to recreate it.

Jens Axboe

2015-10-12 20:26 UTC #4 lore raw

RFC PATCH 1/1 blk-mq: use a delayed work item for timeouts

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Jens Axboe
Quoted 2 lines
No that's definitely fine with me, imho most error handling callbacks should be in process context for ease of use in the driver.
Took a closer look. The patch looks incomplete. The hot path for blk-mq is blk_add_timer(), looks like you left that one alone in the conversion?
Oh, damn. I had that part in my initial version that also crudely converted the old request code and dropped a bit too much. That should defintively do the queue_deayed_work.

Yep

Quoted 8 lines

Quoted 3 lines
Might be easier to just leave the timer alone, and if it actually fires _and_ we have to do something, punt to a workqueue instead of invoking the timeout handler directly.
queue_delayed_work just assigns two additional fields, then sets timer->experies and does an add_timer. So it's the generic implementation of your above scheme. I'd much rather use the generic version if possible instead of trying to recreate it.

I agree, converting to delayed work in general is the cleaner solution. The hot path is really NOT doing anything at all, that's the usual path. If it isn't, then we've screwed up. And the conversion to delayed_work_pending() from timer_pending() looks fine as well, that's another important piece.

Signature · 1 line

Jens Axboe