[PATCH 1/2] blk-mq: remove blk_mq_alloc_tag_set_tags

[PATCH 1/2] blk-mq: remove blk_mq_alloc_tag_set_tags

[Christoph Hellwig]

There is no point in trying to share any code with the realloc case when
all that is needed by the initial tagset allocation is a simple
kcalloc_node.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7ff3415c8eadc..8c630dbdf107e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4403,12 +4403,6 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
 	return 0;
 }
 
-static int blk_mq_alloc_tag_set_tags(struct blk_mq_tag_set *set,
-				int new_nr_hw_queues)
-{
-	return blk_mq_realloc_tag_set_tags(set, 0, new_nr_hw_queues);
-}
-
 /*
  * Alloc a tag set to be associated with one or more request queues.
  * May fail with EINVAL for various error conditions. May adjust the
@@ -4471,11 +4465,13 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 			goto out_free_srcu;
 	}
 
-	ret = blk_mq_alloc_tag_set_tags(set, set->nr_hw_queues);
-	if (ret)
+	ret = -ENOMEM;
+	set->tags = kcalloc_node(set->nr_hw_queues,
+				 sizeof(struct blk_mq_tags *), GFP_KERNEL,
+				 set->numa_node);
+	if (!set->tags)
 		goto out_cleanup_srcu;
 
-	ret = -ENOMEM;
 	for (i = 0; i < set->nr_maps; i++) {
 		set->map[i].mq_map = kcalloc_node(nr_cpu_ids,
 						  sizeof(set->map[i].mq_map[0]),
-- 
2.30.2

[PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Christoph Hellwig]

Use set->nr_hw_queues for the current number of tags, and remove the
duplicate set->nr_hw_queues update in the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8c630dbdf107e..9fa0b9a1435f2 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4381,11 +4381,11 @@ static void blk_mq_update_queue_map(struct blk_mq_tag_set *set)
 }
 
 static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
-				  int cur_nr_hw_queues, int new_nr_hw_queues)
+				       int new_nr_hw_queues)
 {
 	struct blk_mq_tags **new_tags;
 
-	if (cur_nr_hw_queues >= new_nr_hw_queues)
+	if (set->nr_hw_queues >= new_nr_hw_queues)
 		return 0;
 
 	new_tags = kcalloc_node(new_nr_hw_queues, sizeof(struct blk_mq_tags *),
@@ -4394,7 +4394,7 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
 		return -ENOMEM;
 
 	if (set->tags)
-		memcpy(new_tags, set->tags, cur_nr_hw_queues *
+		memcpy(new_tags, set->tags, set->nr_hw_queues *
 		       sizeof(*set->tags));
 	kfree(set->tags);
 	set->tags = new_tags;
@@ -4710,11 +4710,9 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	}
 
 	prev_nr_hw_queues = set->nr_hw_queues;
-	if (blk_mq_realloc_tag_set_tags(set, set->nr_hw_queues, nr_hw_queues) <
-	    0)
+	if (blk_mq_realloc_tag_set_tags(set, nr_hw_queues) < 0)
 		goto reregister;
 
-	set->nr_hw_queues = nr_hw_queues;
 fallback:
 	blk_mq_update_queue_map(set);
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-- 
2.30.2

Re: [PATCH 1/2] blk-mq: remove blk_mq_alloc_tag_set_tags

[Chaitanya Kulkarni]

On 11/9/22 02:08, Christoph Hellwig wrote:
> There is no point in trying to share any code with the realloc case when
> all that is needed by the initial tagset allocation is a simple
> kcalloc_node.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---


Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Chaitanya Kulkarni]

On 11/9/22 02:08, Christoph Hellwig wrote:
> Use set->nr_hw_queues for the current number of tags, and remove the
> duplicate set->nr_hw_queues update in the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/2] blk-mq: remove blk_mq_alloc_tag_set_tags

[Jens Axboe]

On Wed, 9 Nov 2022 11:08:10 +0100, Christoph Hellwig wrote:
> There is no point in trying to share any code with the realloc case when
> all that is needed by the initial tagset allocation is a simple
> kcalloc_node.
> 
> 

Applied, thanks!

[1/2] blk-mq: remove blk_mq_alloc_tag_set_tags
      commit: 5ee20298ff25e883d0668507b3216992a2e9e6cd
[2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags
      commit: ee9d55210c2fe40ab6600b8009de2243b2ad1a4a

Best regards,
-- 
Jens Axboe

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Shinichiro Kawasaki]

On Nov 09, 2022 / 11:08, Christoph Hellwig wrote:
> Use set->nr_hw_queues for the current number of tags, and remove the
> duplicate set->nr_hw_queues update in the caller.

Chaitanya found that blktests test cases block/029 and block/030 fail on
linux-block/for-next branch [1]. The test cases modify null_blk devices'
submit_queues number during test, and it caused the failures.

[1] https://lore.kernel.org/linux-block/5183bc2c-746b-5806-9ace-31a3a7000e6d@nvidia.com/

I bisected and confirmed that this patch is the trigger. As I noted below,
one of its hunks looks wrong for me.

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8c630dbdf107e..9fa0b9a1435f2 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4381,11 +4381,11 @@ static void blk_mq_update_queue_map(struct blk_mq_tag_set *set)
>  }
>  
>  static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
> -				  int cur_nr_hw_queues, int new_nr_hw_queues)
> +				       int new_nr_hw_queues)
>  {
>  	struct blk_mq_tags **new_tags;
>  
> -	if (cur_nr_hw_queues >= new_nr_hw_queues)
> +	if (set->nr_hw_queues >= new_nr_hw_queues)
>  		return 0;
>

When the condition of the if statement above is true, set->nr_hw_queues is no
longer updated with new_nr_hw_queues. In nullb_update_nr_hw_queues(), null_blk
calls blk_mq_update_nr_hw_queues() and refers set->nr_hw_queues. With unexpected
set->nr_hw_queues value, null_blk fails to update submit_queues number.

With a quick fix below, the blktests failures were avoided. Could you take look
and see if this is the right fix?

diff --git a/block/blk-mq.c b/block/blk-mq.c
index a3a5fb4d4ef6..604e19be9648 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4384,8 +4384,10 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
 {
        struct blk_mq_tags **new_tags;

-       if (set->nr_hw_queues >= new_nr_hw_queues)
+       if (set->nr_hw_queues >= new_nr_hw_queues) {
+               set->nr_hw_queues = new_nr_hw_queues;
                return 0;
+       }

        new_tags = kcalloc_node(new_nr_hw_queues, sizeof(struct blk_mq_tags *),
                                GFP_KERNEL, set->numa_node);

-- 
Shin'ichiro Kawasaki

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Christoph Hellwig]

The fix looks good, but could be simplified a bit more:

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ee16b4c34c6a5..cdd8efcec1916 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4381,7 +4381,7 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
 	struct blk_mq_tags **new_tags;
 
 	if (set->nr_hw_queues >= new_nr_hw_queues)
-		return 0;
+		goto done;
 
 	new_tags = kcalloc_node(new_nr_hw_queues, sizeof(struct blk_mq_tags *),
 				GFP_KERNEL, set->numa_node);
@@ -4393,8 +4393,8 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
 		       sizeof(*set->tags));
 	kfree(set->tags);
 	set->tags = new_tags;
+done:
 	set->nr_hw_queues = new_nr_hw_queues;
-
 	return 0;
 }
 

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Shinichiro Kawasaki]

On Nov 21, 2022 / 08:58, Christoph Hellwig wrote:
> The fix looks good, but could be simplified a bit more:
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ee16b4c34c6a5..cdd8efcec1916 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4381,7 +4381,7 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
>  	struct blk_mq_tags **new_tags;
>  
>  	if (set->nr_hw_queues >= new_nr_hw_queues)
> -		return 0;
> +		goto done;
>  
>  	new_tags = kcalloc_node(new_nr_hw_queues, sizeof(struct blk_mq_tags *),
>  				GFP_KERNEL, set->numa_node);
> @@ -4393,8 +4393,8 @@ static int blk_mq_realloc_tag_set_tags(struct blk_mq_tag_set *set,
>  		       sizeof(*set->tags));
>  	kfree(set->tags);
>  	set->tags = new_tags;
> +done:
>  	set->nr_hw_queues = new_nr_hw_queues;
> -
>  	return 0;
>  }

Thanks, this fix is the better. And I reconfirmed it avoids the block/029 and
block/030 failures.

I guess it is too late for Jens to fold-in this fix in the for-next branch.
Christoph, would you prepare a formal fix patch? Or if it helps, I can send out
the patch with your authorship and SoB tag (with Co-developed-by tag of mine).

-- 
Shin'ichiro Kawasaki

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Christoph Hellwig]

On Mon, Nov 21, 2022 at 11:14:44AM +0000, Shinichiro Kawasaki wrote:
> Thanks, this fix is the better. And I reconfirmed it avoids the block/029 and
> block/030 failures.
> 
> I guess it is too late for Jens to fold-in this fix in the for-next branch.
> Christoph, would you prepare a formal fix patch? Or if it helps, I can send out
> the patch with your authorship and SoB tag (with Co-developed-by tag of mine).

I can send the patch.  But I'd also be perfectly happy with you
sending it as the auther, as my version is just a tiny incremental
cleanup.

Re: [PATCH 2/2] blk-mq: simplify blk_mq_realloc_tag_set_tags

[Shinichiro Kawasaki]

On Nov 21, 2022 / 14:19, Christoph Hellwig wrote:
> On Mon, Nov 21, 2022 at 11:14:44AM +0000, Shinichiro Kawasaki wrote:
> > Thanks, this fix is the better. And I reconfirmed it avoids the block/029 and
> > block/030 failures.
> > 
> > I guess it is too late for Jens to fold-in this fix in the for-next branch.
> > Christoph, would you prepare a formal fix patch? Or if it helps, I can send out
> > the patch with your authorship and SoB tag (with Co-developed-by tag of mine).
> 
> I can send the patch.  But I'd also be perfectly happy with you
> sending it as the auther, as my version is just a tiny incremental
> cleanup.

Thanks. I'll post the fix patch.

-- 
Shin'ichiro Kawasaki