[PATCH] blk-mq: remap queues when adding/removing hardware queues

[PATCH] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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_queue_reinit() optionally remap queues, which we do when updating
the number of hardware queues but not when hotplugging.

Fixes: 4e68a011428a ("blk-mq: don't redistribute hardware queues on a CPU hotplug event")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
The only callers of blk_mq_update_nr_hw_queues() are nbd and nbd. I *think*
nbd_dev_add() also wants this remap behavior.

 block/blk-mq.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 061fc2cc88d3..1abbf7c83193 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2343,14 +2343,27 @@ void blk_mq_free_queue(struct request_queue *q)
 	blk_mq_exit_hw_queues(q, set, set->nr_hw_queues);
 }
 
+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);
+}
+
+
 /* Basically redo blk_mq_init_queue with queue frozen */
 static void blk_mq_queue_reinit(struct request_queue *q,
-				const struct cpumask *online_mask)
+				const struct cpumask *online_mask,
+				bool remap_queues)
 {
 	WARN_ON_ONCE(!atomic_read(&q->mq_freeze_depth));
 
 	blk_mq_sysfs_unregister(q);
 
+	if (remap_queues)
+		blk_mq_update_queue_map(q->tag_set);
+
 	/*
 	 * redo blk_mq_init_cpu_queues and blk_mq_init_hw_queues. FIXME: maybe
 	 * we should change hctx numa_node according to new topology (this
@@ -2387,7 +2400,7 @@ static void blk_mq_queue_reinit_work(void)
 		blk_mq_freeze_queue_wait(q);
 
 	list_for_each_entry(q, &all_q_list, all_q_node)
-		blk_mq_queue_reinit(q, &cpuhp_online_new);
+		blk_mq_queue_reinit(q, &cpuhp_online_new, false);
 
 	list_for_each_entry(q, &all_q_list, all_q_node)
 		blk_mq_unfreeze_queue(q);
@@ -2532,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;
 
@@ -2629,11 +2639,12 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
 	set->nr_hw_queues = nr_hw_queues;
 	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);
+		blk_mq_queue_reinit(q, cpu_online_mask, true);
 	}
 
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
 		blk_mq_unfreeze_queue(q);
+
 }
 EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 
-- 
2.12.1

Re: [PATCH] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

On Fri, Mar 31, 2017 at 11:59:24AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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_queue_reinit() optionally remap queues, which we do when updating
> the number of hardware queues but not when hotplugging.
> 
> Fixes: 4e68a011428a ("blk-mq: don't redistribute hardware queues on a CPU hotplug event")
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> The only callers of blk_mq_update_nr_hw_queues() are nbd and nbd. I *think*
> nbd_dev_add() also wants this remap behavior.

Uh, I meant nbd and nvme, and nvme_dev_add().

Re: [PATCH] blk-mq: remap queues when adding/removing hardware queues

[Keith Busch]

On Fri, Mar 31, 2017 at 11:59:24AM -0700, Omar Sandoval wrote:
> @@ -2629,11 +2639,12 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
>  	set->nr_hw_queues = nr_hw_queues;
>  	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);
> +		blk_mq_queue_reinit(q, cpu_online_mask, true);

I think you want to call blk_mq_update_queue_map directly outside this
loop rather than for each queue through blk_mq_queue_reinit. We only
need to map the queues once per tagset rather than per queue.

Re: [PATCH] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

On Fri, Mar 31, 2017 at 04:30:44PM -0400, Keith Busch wrote:
> On Fri, Mar 31, 2017 at 11:59:24AM -0700, Omar Sandoval wrote:
> > @@ -2629,11 +2639,12 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
> >  	set->nr_hw_queues = nr_hw_queues;
> >  	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);
> > +		blk_mq_queue_reinit(q, cpu_online_mask, true);
> 
> I think you want to call blk_mq_update_queue_map directly outside this
> loop rather than for each queue through blk_mq_queue_reinit. We only
> need to map the queues once per tagset rather than per queue.

Right, thanks, I'll do that. I figure you're the person to ask,
nvme_add_dev() does want the remap to happen, right?

Re: [PATCH] blk-mq: remap queues when adding/removing hardware queues

[Keith Busch]

On Fri, Mar 31, 2017 at 01:30:15PM -0700, Omar Sandoval wrote:
> On Fri, Mar 31, 2017 at 04:30:44PM -0400, Keith Busch wrote:
> > On Fri, Mar 31, 2017 at 11:59:24AM -0700, Omar Sandoval wrote:
> > > @@ -2629,11 +2639,12 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
> > >  	set->nr_hw_queues = nr_hw_queues;
> > >  	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);
> > > +		blk_mq_queue_reinit(q, cpu_online_mask, true);
> > 
> > I think you want to call blk_mq_update_queue_map directly outside this
> > loop rather than for each queue through blk_mq_queue_reinit. We only
> > need to map the queues once per tagset rather than per queue.
> 
> Right, thanks, I'll do that. I figure you're the person to ask,
> nvme_add_dev() does want the remap to happen, right?

Yep, nvme may want to change the queue count if you alter either the CPU
topology or some device specific setting to reprovision hardware queues.

[PATCH v2] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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.

Fixes: 4e68a011428a ("blk-mq: don't redistribute hardware queues on a CPU hotplug event")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 061fc2cc88d3..70fa2b0d385a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2478,6 +2478,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
@@ -2532,10 +2540,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;
 
@@ -2627,6 +2632,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);
@@ -2634,6 +2640,7 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
 
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
 		blk_mq_unfreeze_queue(q);
+
 }
 EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 
-- 
2.12.1

Re: [PATCH v2] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

On Fri, Mar 31, 2017 at 01:43:41PM -0700, Omar Sandoval wrote:
> @@ -2634,6 +2640,7 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
>  
>  	list_for_each_entry(q, &set->tag_list, tag_set_list)
>  		blk_mq_unfreeze_queue(q);
> +

Stupid whitespace damage...

[PATCH v3] blk-mq: remap queues when adding/removing hardware queues

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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.

Fixes: 4e68a011428a ("blk-mq: don't redistribute hardware queues on a CPU hotplug event")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 061fc2cc88d3..dbc0f2f745e3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2478,6 +2478,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
@@ -2532,10 +2540,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;
 
@@ -2627,6 +2632,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);
-- 
2.12.1

Re: [PATCH v3] blk-mq: remap queues when adding/removing hardware queues

[Keith Busch]

On Fri, Mar 31, 2017 at 01:48:35PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.
> 
> Fixes: 4e68a011428a ("blk-mq: don't redistribute hardware queues on a CPU hotplug event")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

This looks good to me. 

Reviewed-by: Keith Busch <keith.busch@intel.com>

Re: [PATCH v3] blk-mq: remap queues when adding/removing hardware queues

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3] blk-mq: remap queues when adding/removing hardware queues

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>