[PATCH 0/2] blk-mq: two fixes about updating hw queues

[PATCH 0/2] blk-mq: two fixes about updating hw queues

[Jianchao Wang]

Hi Jens

This patch set fixes two defects during update hw queues.

1st patch refactor the blk-mq debugfs and sysfs register during
blk_mq_update_nr_hw_queues.

2nd patch should be able to fix the panic reported in follwing link
https://marc.info/?l=linux-block&m=153799465603723&w=2

Jianchao Wang (2)
blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues
blk-mq: fallback to previous nr_hw_queues when updating fails

block/blk-mq.c | 79 ++++++++++++++++++++++++++++++++++------------------------
1 file changed, 47 insertions(+), 32 deletions(-)

Thanks
Jianchao

[PATCH 1/2] blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues

[Jianchao Wang]

blk-mq debugfs and sysfs entries need to be removed before updating
queue map, otherwise, we get get wrong result there. This patch fixes
it and remove the redundant debugfs and sysfs register/unregister
operations during __blk_mq_update_nr_hw_queues.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 block/blk-mq.c | 39 ++++++++++++---------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 85a1c1a..6356455 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2137,8 +2137,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
 		struct blk_mq_tag_set *set,
 		struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx)
 {
-	blk_mq_debugfs_unregister_hctx(hctx);
-
 	if (blk_mq_hw_queue_mapped(hctx))
 		blk_mq_tag_idle(hctx);
 
@@ -2165,6 +2163,7 @@ static void blk_mq_exit_hw_queues(struct request_queue *q,
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (i == nr_queue)
 			break;
+		blk_mq_debugfs_unregister_hctx(hctx);
 		blk_mq_exit_hctx(q, set, hctx, i);
 	}
 }
@@ -2222,8 +2221,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
 	if (hctx->flags & BLK_MQ_F_BLOCKING)
 		init_srcu_struct(hctx->srcu);
 
-	blk_mq_debugfs_register_hctx(q, hctx);
-
 	return 0;
 
  free_fq:
@@ -2512,8 +2509,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	int i, j;
 	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
 
-	blk_mq_sysfs_unregister(q);
-
 	/* protect against switching io scheduler  */
 	mutex_lock(&q->sysfs_lock);
 	for (i = 0; i < set->nr_hw_queues; i++) {
@@ -2561,7 +2556,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	}
 	q->nr_hw_queues = i;
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_sysfs_register(q);
 }
 
 struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
@@ -2659,25 +2653,6 @@ void blk_mq_free_queue(struct request_queue *q)
 	blk_mq_exit_hw_queues(q, set, set->nr_hw_queues);
 }
 
-/* Basically redo blk_mq_init_queue with queue frozen */
-static void blk_mq_queue_reinit(struct request_queue *q)
-{
-	WARN_ON_ONCE(!atomic_read(&q->mq_freeze_depth));
-
-	blk_mq_debugfs_unregister_hctxs(q);
-	blk_mq_sysfs_unregister(q);
-
-	/*
-	 * redo blk_mq_init_cpu_queues and blk_mq_init_hw_queues. FIXME: maybe
-	 * we should change hctx numa_node according to the new topology (this
-	 * involves freeing and re-allocating memory, worth doing?)
-	 */
-	blk_mq_map_swqueue(q);
-
-	blk_mq_sysfs_register(q);
-	blk_mq_debugfs_register_hctxs(q);
-}
-
 static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
 {
 	int i;
@@ -2987,11 +2962,21 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		if (!blk_mq_elv_switch_none(&head, q))
 			goto switch_back;
 
+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
+		blk_mq_debugfs_unregister_hctxs(q);
+		blk_mq_sysfs_unregister(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);
+		blk_mq_map_swqueue(q);
+	}
+
+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
+		blk_mq_sysfs_register(q);
+		blk_mq_debugfs_register_hctxs(q);
 	}
 
 switch_back:
-- 
2.7.4

Re: [PATCH 1/2] blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues

[Ming Lei]

On Thu, Sep 27, 2018 at 06:43:03PM +0800, Jianchao Wang wrote:
> blk-mq debugfs and sysfs entries need to be removed before updating
> queue map, otherwise, we get get wrong result there. This patch fixes
> it and remove the redundant debugfs and sysfs register/unregister
> operations during __blk_mq_update_nr_hw_queues.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> ---
>  block/blk-mq.c | 39 ++++++++++++---------------------------
>  1 file changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 85a1c1a..6356455 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2137,8 +2137,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
>  		struct blk_mq_tag_set *set,
>  		struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx)
>  {
> -	blk_mq_debugfs_unregister_hctx(hctx);
> -
>  	if (blk_mq_hw_queue_mapped(hctx))
>  		blk_mq_tag_idle(hctx);
>  
> @@ -2165,6 +2163,7 @@ static void blk_mq_exit_hw_queues(struct request_queue *q,
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		if (i == nr_queue)
>  			break;
> +		blk_mq_debugfs_unregister_hctx(hctx);
>  		blk_mq_exit_hctx(q, set, hctx, i);
>  	}
>  }
> @@ -2222,8 +2221,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
>  	if (hctx->flags & BLK_MQ_F_BLOCKING)
>  		init_srcu_struct(hctx->srcu);
>  
> -	blk_mq_debugfs_register_hctx(q, hctx);
> -
>  	return 0;
>  
>   free_fq:
> @@ -2512,8 +2509,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  	int i, j;
>  	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
>  
> -	blk_mq_sysfs_unregister(q);
> -
>  	/* protect against switching io scheduler  */
>  	mutex_lock(&q->sysfs_lock);
>  	for (i = 0; i < set->nr_hw_queues; i++) {
> @@ -2561,7 +2556,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  	}
>  	q->nr_hw_queues = i;
>  	mutex_unlock(&q->sysfs_lock);
> -	blk_mq_sysfs_register(q);
>  }
>  
>  struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
> @@ -2659,25 +2653,6 @@ void blk_mq_free_queue(struct request_queue *q)
>  	blk_mq_exit_hw_queues(q, set, set->nr_hw_queues);
>  }

Looks both blk_mq_sysfs_register() and blk_mq_debugfs_register_hctx()
are removed from the path of blk_mq_init_allocated_queue(), so where are
the two registered for a freshly new queue?

Thanks,
Ming

Re: [PATCH 1/2] blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues

[jianchao.wang]

Hi Ming

On 09/29/2018 10:49 AM, Ming Lei wrote:
> Looks both blk_mq_sysfs_register() and blk_mq_debugfs_register_hctx()
> are removed from the path of blk_mq_init_allocated_queue(), so where are
> the two registered for a freshly new queue?

Initially, both of them are registered by following path.
__device_add_disk
  -> blk_register_queue

	if (q->mq_ops) {
		__blk_mq_register_dev(dev, q);
		blk_mq_debugfs_register(q);
	}

blk_mq_init_allocated_queue won't do any thing.

Thanks
Jianchao

Re: [PATCH 1/2] blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues

[Ming Lei]

On Thu, Sep 27, 2018 at 06:43:03PM +0800, Jianchao Wang wrote:
> blk-mq debugfs and sysfs entries need to be removed before updating
> queue map, otherwise, we get get wrong result there. This patch fixes
> it and remove the redundant debugfs and sysfs register/unregister
> operations during __blk_mq_update_nr_hw_queues.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> ---
>  block/blk-mq.c | 39 ++++++++++++---------------------------
>  1 file changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 85a1c1a..6356455 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2137,8 +2137,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
>  		struct blk_mq_tag_set *set,
>  		struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx)
>  {
> -	blk_mq_debugfs_unregister_hctx(hctx);
> -
>  	if (blk_mq_hw_queue_mapped(hctx))
>  		blk_mq_tag_idle(hctx);
>  
> @@ -2165,6 +2163,7 @@ static void blk_mq_exit_hw_queues(struct request_queue *q,
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		if (i == nr_queue)
>  			break;
> +		blk_mq_debugfs_unregister_hctx(hctx);
>  		blk_mq_exit_hctx(q, set, hctx, i);
>  	}
>  }
> @@ -2222,8 +2221,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
>  	if (hctx->flags & BLK_MQ_F_BLOCKING)
>  		init_srcu_struct(hctx->srcu);
>  
> -	blk_mq_debugfs_register_hctx(q, hctx);
> -
>  	return 0;
>  
>   free_fq:
> @@ -2512,8 +2509,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  	int i, j;
>  	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
>  
> -	blk_mq_sysfs_unregister(q);
> -
>  	/* protect against switching io scheduler  */
>  	mutex_lock(&q->sysfs_lock);
>  	for (i = 0; i < set->nr_hw_queues; i++) {
> @@ -2561,7 +2556,6 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  	}
>  	q->nr_hw_queues = i;
>  	mutex_unlock(&q->sysfs_lock);
> -	blk_mq_sysfs_register(q);
>  }
>  
>  struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
> @@ -2659,25 +2653,6 @@ void blk_mq_free_queue(struct request_queue *q)
>  	blk_mq_exit_hw_queues(q, set, set->nr_hw_queues);
>  }
>  
> -/* Basically redo blk_mq_init_queue with queue frozen */
> -static void blk_mq_queue_reinit(struct request_queue *q)
> -{
> -	WARN_ON_ONCE(!atomic_read(&q->mq_freeze_depth));
> -
> -	blk_mq_debugfs_unregister_hctxs(q);
> -	blk_mq_sysfs_unregister(q);
> -
> -	/*
> -	 * redo blk_mq_init_cpu_queues and blk_mq_init_hw_queues. FIXME: maybe
> -	 * we should change hctx numa_node according to the new topology (this
> -	 * involves freeing and re-allocating memory, worth doing?)
> -	 */
> -	blk_mq_map_swqueue(q);
> -
> -	blk_mq_sysfs_register(q);
> -	blk_mq_debugfs_register_hctxs(q);
> -}
> -
>  static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
>  {
>  	int i;
> @@ -2987,11 +2962,21 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
>  		if (!blk_mq_elv_switch_none(&head, q))
>  			goto switch_back;
>  
> +	list_for_each_entry(q, &set->tag_list, tag_set_list) {
> +		blk_mq_debugfs_unregister_hctxs(q);
> +		blk_mq_sysfs_unregister(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);
> +		blk_mq_map_swqueue(q);
> +	}
> +
> +	list_for_each_entry(q, &set->tag_list, tag_set_list) {
> +		blk_mq_sysfs_register(q);
> +		blk_mq_debugfs_register_hctxs(q);
>  	}
>  
>  switch_back:
> -- 
> 2.7.4
> 

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH 2/2] blk-mq: fallback to previous nr_hw_queues when updating fails

[Jianchao Wang]

When we try to increate the nr_hw_queues, we may fail due to
shortage of memory or other reason, then blk_mq_realloc_hw_ctxs stops
and some entries in q->queue_hw_ctx are left with NULL. However,
because queue map has been updated with new nr_hw_queues, some cpus
have been mapped to hw queue which just encounters allocation failure,
thus blk_mq_map_queue could return NULL. This will cause panic in
following blk_mq_map_swqueue.

To fix it, let blk_mq_realloc_hw_ctxs return false to
skip blk_mq_map_swqueue and fallback to previous nr_hw_queues in
case of increasing nr_hw_queues failure.

Reported-by: syzbot+83e8cbe702263932d9d4@syzkaller.appspotmail.com
Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 block/blk-mq.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 6356455..c867ede 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2503,10 +2503,10 @@ static int blk_mq_hw_ctx_size(struct blk_mq_tag_set *tag_set)
 	return hw_ctx_size;
 }
 
-static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
+static bool blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 						struct request_queue *q)
 {
-	int i, j;
+	int i, j, end;
 	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
 
 	/* protect against switching io scheduler  */
@@ -2542,7 +2542,24 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 		}
 		blk_mq_hctx_kobj_init(hctxs[i]);
 	}
-	for (j = i; j < q->nr_hw_queues; j++) {
+
+	if (i != set->nr_hw_queues) {
+		/*
+		 * Increasing nr_hw_queues fails. Free the newly allocated
+		 * hctxs and keep the previous q->nr_hw_queues.
+		 */
+		j = q->nr_hw_queues;
+		end = i;
+	} else {
+		/*
+		 * If nr_hw_queues is decreased, free the redundant hctxs.
+		 */
+		j = i;
+		end = q->nr_hw_queues;
+		q->nr_hw_queues = set->nr_hw_queues;
+	}
+
+	for (; j < end; j++) {
 		struct blk_mq_hw_ctx *hctx = hctxs[j];
 
 		if (hctx) {
@@ -2554,8 +2571,9 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 
 		}
 	}
-	q->nr_hw_queues = i;
 	mutex_unlock(&q->sysfs_lock);
+
+	return (q->nr_hw_queues == set->nr_hw_queues);
 }
 
 struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
@@ -2939,6 +2957,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 {
 	struct request_queue *q;
 	LIST_HEAD(head);
+	int prev_nr_hw_queues;
 
 	lockdep_assert_held(&set->tag_list_lock);
 
@@ -2967,10 +2986,21 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		blk_mq_sysfs_unregister(q);
 	}
 
+	prev_nr_hw_queues = set->nr_hw_queues;
 	set->nr_hw_queues = nr_hw_queues;
+again:
 	blk_mq_update_queue_map(set);
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		blk_mq_realloc_hw_ctxs(set, q);
+		/*
+		 * If increasing nr_hw_queues fail, fallback to previous
+		 * nr_hw_queues.
+		 */
+		if (!blk_mq_realloc_hw_ctxs(set, q)) {
+			pr_warn("updating nr_hw_queues to %d fails, fallback to %d.\n",
+					nr_hw_queues, prev_nr_hw_queues);
+			set->nr_hw_queues = prev_nr_hw_queues;
+			goto again;
+		}
 		blk_mq_map_swqueue(q);
 	}
 
-- 
2.7.4

Re: [PATCH 0/2] blk-mq: two fixes about updating hw queues

[jianchao.wang]

Would anyone please take a look at this patch ?

Thanks in advance
Jianchao

On 09/27/2018 06:43 PM, Jianchao Wang wrote:
> Hi Jens
> 
> This patch set fixes two defects during update hw queues.
> 
> 1st patch refactor the blk-mq debugfs and sysfs register during
> blk_mq_update_nr_hw_queues.
> 
> 2nd patch should be able to fix the panic reported in follwing link
> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dblock-26m-3D153799465603723-26w-3D2&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=JoGIFiPF4RF8WH8rquW8Qswr2FYvPOVaTxlgmjKHsDk&s=TQn_KiIX2sfmXP_9IYy2IFFRxBj4jBKVaSSU-I1z9g8&e=
> 
> Jianchao Wang (2)
> blk-mq: adjust debugfs and sysfs register when updating nr_hw_queues
> blk-mq: fallback to previous nr_hw_queues when updating fails
> 
> block/blk-mq.c | 79 ++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 47 insertions(+), 32 deletions(-)
> 
> Thanks
> Jianchao
>