blk-mq: scheduler and hw queue initialization fixes/enhancements
cover blk-mq: scheduler and hw queue initialization fixes/enhancements
From: Omar Sandoval <[email protected]>
Hi, Jens,
This series has some fixes and enhancements for blk-mq:
- Patch 1 is a cleanup in preparation for the rest of the series
- Patch 2 is a fix necessary for patch 4 when scheduling is enabled, making sure we bring up new hardware queues with scheduler tags
- Patch 3 makes error handling in elevator_switch() more robust, making us fall back to none like you recommended last time
- Patch 4 is the remap fix from last week
- Patch 5 is an extension of patch 2 for multiqueue schedulers that allocate per-hctx data. Nothing in-tree needs it, but Kyber will.
Let me know if you'd prefer deferring this to 4.12 or want to apply 1-4 for 4.11. These are based on block/for-next, so the latter might require a respin.
Thanks!
Omar Sandoval (5):
blk-mq-sched: refactor scheduler initialization
blk-mq-sched: set up scheduler tags when bringing up new queues
blk-mq-sched: fix crash in switch error path
blk-mq: remap queues when adding/removing hardware queues
blk-mq-sched: provide hooks for initializing hardware queue data
block/blk-mq-sched.c | 178 +++++++++++++++++++++++++++++------------------ block/blk-mq-sched.h | 13 ++-- block/blk-mq.c | 25 +++++-- block/blk-sysfs.c | 2 +- block/elevator.c | 114 +++++++++++++++--------------- include/linux/elevator.h | 4 +- 6 files changed, 198 insertions(+), 138 deletions(-)
Signature · 1 line
2.12.2
PATCH 1/5 blk-mq-sched: refactor scheduler initialization
From: Omar Sandoval <[email protected]>
Preparation cleanup for the next couple of fixes, push blk_mq_sched_setup() and e->ops.mq.init_sched() into a helper.
--- block/blk-mq-sched.c | 82 ++++++++++++++++++++++++++++------------------------ block/blk-mq-sched.h | 2 +- block/elevator.c | 32 ++++++++------------ 3 files changed, 57 insertions(+), 59 deletions(-)
block/blk-mq-sched.c +44 -38
@@ -431,11 +431,45 @@ static void blk_mq_sched_free_tags(struct blk_mq_tag_set *set, } } -int blk_mq_sched_setup (struct request_queue *q) +static int blk_mq_sched_alloc_tags (struct request_queue *q, +struct blk_mq_hw_ctx *hctx, +unsigned int hctx_idx) +{ +struct blk_mq_tag_set *set = q->tag_set; +int ret; + + hctx->sched_tags =blk_mq_alloc_rq_map (set, hctx_idx, q->nr_requests, + set->reserved_tags); +if (!hctx->sched_tags) +return -ENOMEM ; + + ret =blk_mq_alloc_rqs (set, hctx->sched_tags, hctx_idx, q->nr_requests); +if (ret) +blk_mq_sched_free_tags (set, hctx, hctx_idx); + +return ret; +} + +void blk_mq_sched_teardown (struct request_queue *q) {struct blk_mq_tag_set *set = q->tag_set;struct blk_mq_hw_ctx *hctx; -int ret, i; +int i; + +queue_for_each_hw_ctx (q, hctx, i) +blk_mq_sched_free_tags (set, hctx, i); +} + +int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e) +{ +struct blk_mq_hw_ctx *hctx; +unsigned int i; +int ret; + +if (!e) { + q->elevator =NULL ; +return 0 ; + }/* * Default to 256, since we don't split into sync/async like the
@@ -443,49 +477,21 @@ int blk_mq_sched_setup(struct request_queue *q)*/ q->nr_requests =2 *BLKDEV_MAX_RQ ; -/* -* We're switching to using an IO scheduler, so setup the hctx -* scheduler tags and switch the request map from the regular -* tags to scheduler tags. First allocate what we need, so we -* can safely fail and fallback, if needed. -*/ - ret =0 ;queue_for_each_hw_ctx (q, hctx, i) { - hctx->sched_tags =blk_mq_alloc_rq_map (set, i, - q->nr_requests, set->reserved_tags); -if (!hctx->sched_tags) { - ret = -ENOMEM ; -break ; - } - ret =blk_mq_alloc_rqs (set, hctx->sched_tags, i, q->nr_requests); + ret =blk_mq_sched_alloc_tags (q, hctx, i);if (ret) -break ; + goto err; } -/* -* If we failed, free what we did allocate -*/ -if (ret) { -queue_for_each_hw_ctx (q, hctx, i) { -if (!hctx->sched_tags) -continue ; -blk_mq_sched_free_tags (set, hctx, i); - } - -return ret; - } + ret = e->ops.mq.init_sched (q, e); +if (ret) + goto err;return 0 ; -} -void blk_mq_sched_teardown (struct request_queue *q) -{ -struct blk_mq_tag_set *set = q->tag_set; -struct blk_mq_hw_ctx *hctx; -int i; - -queue_for_each_hw_ctx (q, hctx, i) -blk_mq_sched_free_tags (set, hctx, i); +err: +blk_mq_sched_teardown (q); +return ret; }int blk_mq_sched_init (struct request_queue *q)
block/blk-mq-sched.h +1 -1
@@ -32,7 +32,7 @@ void blk_mq_sched_move_to_dispatch(struct blk_mq_hw_ctx *hctx,struct list_head *rq_list, struct request *(*get_rq)(struct blk_mq_hw_ctx *)); -int blk_mq_sched_setup (struct request_queue *q); +int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e);void blk_mq_sched_teardown (struct request_queue *q);int blk_mq_sched_init (struct request_queue *q);
block/elevator.c +12 -20
@@ -242,17 +242,12 @@ int elevator_init(struct request_queue *q, char *name) } } -if (e->uses_mq) { - err =blk_mq_sched_setup (q); -if (!err) - err = e->ops.mq.init_sched (q, e); - }else +if (e->uses_mq) + err =blk_mq_init_sched (q, e); +else err = e->ops.sq.elevator_init_fn (q, e); -if (err) { -if (e->uses_mq) -blk_mq_sched_teardown (q); +if (err)elevator_put (e); - }return err; }EXPORT_SYMBOL (elevator_init);
@@ -987,21 +982,18 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e) }/* allocate, init and register new elevator */ -if (new_e) { -if (new_e->uses_mq) { - err =blk_mq_sched_setup (q); -if (!err) - err = new_e->ops.mq.init_sched (q, new_e); - }else - err = new_e->ops.sq.elevator_init_fn (q, new_e); -if (err) - goto fail_init; +if (q->mq_ops) + err =blk_mq_init_sched (q, new_e); +else + err = new_e->ops.sq.elevator_init_fn (q, new_e); +if (err) + goto fail_init; +if (new_e) { err =elv_register_queue (q);if (err) goto fail_register; - }else - q->elevator =NULL ; + }/* done, kill the old one and finish */ if (old) {
Signature · 1 line
2.12.2
PATCH 2/5 blk-mq-sched: set up scheduler tags when bringing up new queues
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
If a new hardware queue is added at runtime, we don't allocate scheduler tags for it, leading to a crash. This hooks up the scheduler framework to blk_mq_{init,exit}_hctx() to make sure everything gets properly initialized/freed.
--- block/blk-mq-sched.c | 22 ++++++++++++++++++++++ block/blk-mq-sched.h | 5 +++++ block/blk-mq.c | 9 ++++++++- 3 files changed, 35 insertions(+), 1 deletion(-)
block/blk-mq-sched.c +22 -0
@@ -460,6 +460,28 @@ void blk_mq_sched_teardown(struct request_queue *q)blk_mq_sched_free_tags (set, hctx, i); } +int blk_mq_sched_init_hctx (struct request_queue *q,struct blk_mq_hw_ctx *hctx, +unsigned int hctx_idx) +{ +struct elevator_queue *e = q->elevator; + +if (!e) +return 0 ; + +return blk_mq_sched_alloc_tags (q, hctx, hctx_idx); +} + +void blk_mq_sched_exit_hctx (struct request_queue *q,struct blk_mq_hw_ctx *hctx, +unsigned int hctx_idx) +{ +struct elevator_queue *e = q->elevator; + +if (!e) +return ; + +blk_mq_sched_free_tags (q->tag_set, hctx, hctx_idx); +} +int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e) {struct blk_mq_hw_ctx *hctx;
block/blk-mq-sched.h +5 -0
@@ -35,6 +35,11 @@ void blk_mq_sched_move_to_dispatch(struct blk_mq_hw_ctx *hctx,int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e);void blk_mq_sched_teardown (struct request_queue *q); +int blk_mq_sched_init_hctx (struct request_queue *q,struct blk_mq_hw_ctx *hctx, +unsigned int hctx_idx); +void blk_mq_sched_exit_hctx (struct request_queue *q,struct blk_mq_hw_ctx *hctx, +unsigned int hctx_idx); +int blk_mq_sched_init (struct request_queue *q); static inline bool
block/blk-mq.c +8 -1
@@ -1839,6 +1839,8 @@ static void blk_mq_exit_hctx(struct request_queue *q, hctx->fq->flush_rq, hctx_idx, flush_start_tag + hctx_idx); +blk_mq_sched_exit_hctx (q, hctx, hctx_idx); +if (set->ops->exit_hctx) set->ops->exit_hctx (hctx, hctx_idx);
@@ -1905,9 +1907,12 @@ static int blk_mq_init_hctx(struct request_queue *q, set->ops->init_hctx(hctx, set->driver_data, hctx_idx)) goto free_bitmap; +if (blk_mq_sched_init_hctx (q, hctx, hctx_idx)) + goto exit_hctx; + hctx->fq =blk_alloc_flush_queue (q, hctx->numa_node, set->cmd_size);if (!hctx->fq) - goto exit_hctx; + goto sched_exit_hctx;if (set->ops->init_request && set->ops->init_request(set->driver_data,
@@ -1922,6 +1927,8 @@ static int blk_mq_init_hctx(struct request_queue *q, free_fq:kfree (hctx->fq); + sched_exit_hctx: +blk_mq_sched_exit_hctx (q, hctx, hctx_idx); exit_hctx:if (set->ops->exit_hctx) set->ops->exit_hctx (hctx, hctx_idx);
Signature · 1 line
2.12.2
PATCH 3/5 blk-mq-sched: fix crash in switch error path
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
In elevator_switch(), if blk_mq_init_sched() fails, we attempt to fall back to the original scheduler. However, at this point, we've already torn down the original scheduler's tags, so this causes a crash. Doing the fallback like the legacy elevator path is much harder for mq, so fix it by just falling back to none, instead.
--- block/blk-mq-sched.c | 13 +++++-- block/blk-mq-sched.h | 2 +- block/blk-mq.c | 2 -- block/blk-sysfs.c | 2 +- block/elevator.c | 94 +++++++++++++++++++++++++++--------------------- include/linux/elevator.h | 2 +- 6 files changed, 67 insertions(+), 48 deletions(-)
block/blk-mq-sched.c +11 -2
@@ -450,7 +450,7 @@ static int blk_mq_sched_alloc_tags(struct request_queue *q,return ret; } -void blk_mq_sched_teardown (struct request_queue *q) +static void blk_mq_sched_tags_teardown (struct request_queue *q) {struct blk_mq_tag_set *set = q->tag_set;struct blk_mq_hw_ctx *hctx;
@@ -512,10 +512,19 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)return 0 ; err: -blk_mq_sched_teardown (q); +blk_mq_sched_tags_teardown (q); + q->elevator =NULL ;return ret; } +void blk_mq_exit_sched (struct request_queue *q,struct elevator_queue *e) +{ +if (e->type->ops.mq.exit_sched) + e->type->ops.mq.exit_sched (e); +blk_mq_sched_tags_teardown (q); + q->elevator =NULL ; +} +int blk_mq_sched_init (struct request_queue *q) {int ret;
block/blk-mq-sched.h +1 -1
@@ -33,7 +33,7 @@ void blk_mq_sched_move_to_dispatch(struct blk_mq_hw_ctx *hctx, struct request *(*get_rq)(struct blk_mq_hw_ctx *));int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e); -void blk_mq_sched_teardown (struct request_queue *q); +void blk_mq_exit_sched (struct request_queue *q,struct elevator_queue *e);int blk_mq_sched_init_hctx (struct request_queue *q,struct blk_mq_hw_ctx *hctx,unsigned int hctx_idx);
block/blk-mq.c +0 -2
@@ -2153,8 +2153,6 @@ void blk_mq_release(struct request_queue *q)struct blk_mq_hw_ctx *hctx;unsigned int i; -blk_mq_sched_teardown (q); -/* hctx kobj stays in hctx */ queue_for_each_hw_ctx (q, hctx, i) {if (!hctx)
block/blk-sysfs.c +1 -1
@@ -803,7 +803,7 @@ static void blk_release_queue(struct kobject *kobj)if (q->elevator) {ioc_clear_queue (q); -elevator_exit (q->elevator); +elevator_exit (q, q->elevator); }blk_free_queue_stats (q->stats);
block/elevator.c +53 -41
144 lines not on this page · raw message
include/linux/elevator.h +1 -1
@@ -211,7 +211,7 @@ extern ssize_t elv_iosched_show(struct request_queue *, char *);extern ssize_t elv_iosched_store (struct request_queue *,const char *,size_t );extern int elevator_init (struct request_queue *,char *); -extern void elevator_exit (struct elevator_queue *); +extern void elevator_exit (struct request_queue *,struct elevator_queue *);extern int elevator_change (struct request_queue *,const char *);extern bool elv_bio_merge_ok (struct request *,struct bio *); extern struct elevator_queue *elevator_alloc(struct request_queue *,
Signature · 1 line
2.12.2
PATCH 4/5 blk-mq: remap queues when adding/removing hardware queues
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
blk_mq_update_nr_hw_queues() used to remap hardware queues, which is the behavior that drivers expect. However, commit 4e68a011428a changed blk_mq_queue_reinit() to not remap queues for the case of CPU hotplugging, inadvertently making blk_mq_update_nr_hw_queues() not remap queues as well. This breaks, for example, NBD's multi-connection mode, leaving the added hardware queues unused. Fix it by making blk_mq_update_nr_hw_queues() explicitly remap the queues.
--- block/blk-mq.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
block/blk-mq.c +10 -4
@@ -2483,6 +2483,14 @@ static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)return 0 ; } +static int blk_mq_update_queue_map (struct blk_mq_tag_set *set) +{ +if (set->ops->map_queues) +return set->ops->map_queues (set); +else +return blk_mq_map_queues (set); +} +/* * Alloc a tag set to be associated with one or more request queues. * May fail with EINVAL for various error conditions. May adjust the
@@ -2537,10 +2545,7 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)if (!set->mq_map) goto out_free_tags; -if (set->ops->map_queues) - ret = set->ops->map_queues (set); -else - ret =blk_mq_map_queues (set); + ret =blk_mq_update_queue_map (set);if (ret) goto out_free_mq_map;
@@ -2632,6 +2637,7 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)blk_mq_freeze_queue (q); set->nr_hw_queues = nr_hw_queues; +blk_mq_update_queue_map (set);list_for_each_entry (q, &set->tag_list, tag_set_list) {blk_mq_realloc_hw_ctxs (set, q);blk_mq_queue_reinit (q, cpu_online_mask);
Signature · 1 line
2.12.2
PATCH 5/5 blk-mq-sched: provide hooks for initializing hardware queue data
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
Schedulers need to be informed when a hardware queue is added or removed at runtime so they can allocate/free per-hardware queue data. So, replace the blk_mq_sched_init_hctx_data() helper, which only makes sense at init time, with .init_hctx() and .exit_hctx() hooks.
--- block/blk-mq-sched.c | 81 +++++++++++++++++++++++++----------------------- block/blk-mq-sched.h | 4 --- include/linux/elevator.h | 2 ++ 3 files changed, 45 insertions(+), 42 deletions(-)
block/blk-mq-sched.c +43 -38
@@ -30,43 +30,6 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q, }EXPORT_SYMBOL_GPL (blk_mq_sched_free_hctx_data); -int blk_mq_sched_init_hctx_data (struct request_queue *q,size_t size, -int (*init)(struct blk_mq_hw_ctx *), -void (*exit)(struct blk_mq_hw_ctx *)) -{ -struct blk_mq_hw_ctx *hctx; -int ret; -int i; - -queue_for_each_hw_ctx (q, hctx, i) { - hctx->sched_data =kmalloc_node (size,GFP_KERNEL , hctx->numa_node); -if (!hctx->sched_data) { - ret = -ENOMEM ; - goto error; - } - -if (init) { - ret =init (hctx); -if (ret) { -/* -* We don't want to give exit() a partially -* initialized sched_data. init() must clean up -* if it fails. -*/ -kfree (hctx->sched_data); - hctx->sched_data =NULL ; - goto error; - } - } - } - -return 0 ; -error: -blk_mq_sched_free_hctx_data (q, exit); -return ret; -} -EXPORT_SYMBOL_GPL (blk_mq_sched_init_hctx_data); - static void __blk_mq_sched_assign_ioc(struct request_queue *q, struct request *rq, struct bio *bio,
@@ -464,11 +427,24 @@ int blk_mq_sched_init_hctx(struct request_queue *q, struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx) {struct elevator_queue *e = q->elevator; +int ret;if (!e)return 0 ; -return blk_mq_sched_alloc_tags (q, hctx, hctx_idx); + ret =blk_mq_sched_alloc_tags (q, hctx, hctx_idx); +if (ret) +return ret; + +if (e->type->ops.mq.init_hctx) { + ret = e->type->ops.mq.init_hctx (hctx, hctx_idx); +if (ret) { +blk_mq_sched_free_tags (q->tag_set, hctx, hctx_idx); +return ret; + } + } + +return 0 ; } void blk_mq_sched_exit_hctx(struct request_queue *q, struct blk_mq_hw_ctx *hctx,
@@ -479,12 +455,18 @@ void blk_mq_sched_exit_hctx(struct request_queue *q, struct blk_mq_hw_ctx *hctx,if (!e)return ; +if (e->type->ops.mq.exit_hctx && hctx->sched_data) { + e->type->ops.mq.exit_hctx (hctx, hctx_idx); + hctx->sched_data =NULL ; + } +blk_mq_sched_free_tags (q->tag_set, hctx, hctx_idx); }int blk_mq_init_sched (struct request_queue *q,struct elevator_type *e) {struct blk_mq_hw_ctx *hctx; +struct elevator_queue *eq;unsigned int i;int ret;
@@ -509,6 +491,18 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)if (ret) goto err; +if (e->ops.mq.init_hctx) { +queue_for_each_hw_ctx (q, hctx, i) { + ret = e->ops.mq.init_hctx (hctx, i); +if (ret) { + eq = q->elevator; +blk_mq_exit_sched (q, eq); +kobject_put (&eq->kobj); +return ret; + } + } + } +return 0 ; err:
@@ -519,6 +513,17 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)void blk_mq_exit_sched (struct request_queue *q,struct elevator_queue *e) { +struct blk_mq_hw_ctx *hctx; +unsigned int i; + +if (e->type->ops.mq.exit_hctx) { +queue_for_each_hw_ctx (q, hctx, i) { +if (hctx->sched_data) { + e->type->ops.mq.exit_hctx (hctx, i); + hctx->sched_data =NULL ; + } + } + }if (e->type->ops.mq.exit_sched) e->type->ops.mq.exit_sched (e);blk_mq_sched_tags_teardown (q);
block/blk-mq-sched.h +0 -4
@@ -4,10 +4,6 @@#include "blk-mq.h" #include "blk-mq-tag.h" -int blk_mq_sched_init_hctx_data (struct request_queue *q,size_t size, -int (*init)(struct blk_mq_hw_ctx *), -void (*exit)(struct blk_mq_hw_ctx *)); -void blk_mq_sched_free_hctx_data (struct request_queue *q,void (*exit)(struct blk_mq_hw_ctx *));
include/linux/elevator.h +2 -0
@@ -93,6 +93,8 @@ struct blk_mq_hw_ctx;struct elevator_mq_ops {int (*init_sched)(struct request_queue *,struct elevator_type *);void (*exit_sched)(struct elevator_queue *); +int (*init_hctx)(struct blk_mq_hw_ctx *,unsigned int ); +void (*exit_hctx)(struct blk_mq_hw_ctx *,unsigned int );bool (*allow_merge)(struct request_queue *,struct request *,struct bio *);bool (*bio_merge)(struct blk_mq_hw_ctx *,struct bio *);
Signature · 1 line
2.12.2
Quoted 18 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
Hi, Jens,
This series has some fixes and enhancements for blk-mq:
- Patch 1 is a cleanup in preparation for the rest of the series
- Patch 2 is a fix necessary for patch 4 when scheduling is enabled, making sure we bring up new hardware queues with scheduler tags
- Patch 3 makes error handling inelevator_switch()more robust, making us fall back to none like you recommended last time
- Patch 4 is the remap fix from last week
- Patch 5 is an extension of patch 2 for multiqueue schedulers that allocate per-hctx data. Nothing in-tree needs it, but Kyber will.
Quoted 3 lines
Let me know if you'd prefer deferring this to 4.12 or want to apply 1-4 for 4.11. These are based on
block/for-next, so the latter might require a respin.
Tested with nbd, didn't crash, you can add
Thanks,
Josef