md/raid1: change r1conf->r1bio_pool to a pointer type

[PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Wang Jinchao 7 months, 3 weeks ago

In raid1_reshape(), newpool is a stack variable.
mempool_init() initializes newpool->wait with the stack address.
After assigning newpool to conf->r1bio_pool, the wait queue
need to be reinitialized, which is not ideal.

Change raid1_conf->r1bio_pool to a pointer type and
replace mempool_init() with mempool_create() to
avoid referencing a stack-based wait queue.

Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
---
 drivers/md/raid1.c | 31 +++++++++++++------------------
 drivers/md/raid1.h |  2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index fd4ce2a4136f..4d4833915b5f 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -255,7 +255,7 @@ static void free_r1bio(struct r1bio *r1_bio)
 	struct r1conf *conf = r1_bio->mddev->private;
 
 	put_all_bios(conf, r1_bio);
-	mempool_free(r1_bio, &conf->r1bio_pool);
+	mempool_free(r1_bio, conf->r1bio_pool);
 }
 
 static void put_buf(struct r1bio *r1_bio)
@@ -1305,7 +1305,7 @@ alloc_r1bio(struct mddev *mddev, struct bio *bio)
 	struct r1conf *conf = mddev->private;
 	struct r1bio *r1_bio;
 
-	r1_bio = mempool_alloc(&conf->r1bio_pool, GFP_NOIO);
+	r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
 	/* Ensure no bio records IO_BLOCKED */
 	memset(r1_bio->bios, 0, conf->raid_disks * sizeof(r1_bio->bios[0]));
 	init_r1bio(r1_bio, mddev, bio);
@@ -3124,9 +3124,9 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	if (!conf->poolinfo)
 		goto abort;
 	conf->poolinfo->raid_disks = mddev->raid_disks * 2;
-	err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, r1bio_pool_alloc,
-			   rbio_pool_free, conf->poolinfo);
-	if (err)
+	conf->r1bio_pool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
+					  rbio_pool_free, conf->poolinfo);
+	if (!conf->r1bio_pool)
 		goto abort;
 
 	err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
@@ -3197,7 +3197,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 
  abort:
 	if (conf) {
-		mempool_exit(&conf->r1bio_pool);
+		mempool_destroy(conf->r1bio_pool);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
 		kfree(conf->poolinfo);
@@ -3310,7 +3310,7 @@ static void raid1_free(struct mddev *mddev, void *priv)
 {
 	struct r1conf *conf = priv;
 
-	mempool_exit(&conf->r1bio_pool);
+	mempool_destroy(conf->r1bio_pool);
 	kfree(conf->mirrors);
 	safe_put_page(conf->tmppage);
 	kfree(conf->poolinfo);
@@ -3366,17 +3366,13 @@ static int raid1_reshape(struct mddev *mddev)
 	 * At the same time, we "pack" the devices so that all the missing
 	 * devices have the higher raid_disk numbers.
 	 */
-	mempool_t newpool, oldpool;
+	mempool_t *newpool, *oldpool;
 	struct pool_info *newpoolinfo;
 	struct raid1_info *newmirrors;
 	struct r1conf *conf = mddev->private;
 	int cnt, raid_disks;
 	unsigned long flags;
 	int d, d2;
-	int ret;
-
-	memset(&newpool, 0, sizeof(newpool));
-	memset(&oldpool, 0, sizeof(oldpool));
 
 	/* Cannot change chunk_size, layout, or level */
 	if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
@@ -3408,18 +3404,18 @@ static int raid1_reshape(struct mddev *mddev)
 	newpoolinfo->mddev = mddev;
 	newpoolinfo->raid_disks = raid_disks * 2;
 
-	ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
+	newpool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
 			   rbio_pool_free, newpoolinfo);
-	if (ret) {
+	if (!newpool) {
 		kfree(newpoolinfo);
-		return ret;
+		return -ENOMEM;
 	}
 	newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
 					 raid_disks, 2),
 			     GFP_KERNEL);
 	if (!newmirrors) {
 		kfree(newpoolinfo);
-		mempool_exit(&newpool);
+		mempool_destroy(newpool);
 		return -ENOMEM;
 	}
 
@@ -3428,7 +3424,6 @@ static int raid1_reshape(struct mddev *mddev)
 	/* ok, everything is stopped */
 	oldpool = conf->r1bio_pool;
 	conf->r1bio_pool = newpool;
-	init_waitqueue_head(&conf->r1bio_pool.wait);
 
 	for (d = d2 = 0; d < conf->raid_disks; d++) {
 		struct md_rdev *rdev = conf->mirrors[d].rdev;
@@ -3460,7 +3455,7 @@ static int raid1_reshape(struct mddev *mddev)
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 	md_wakeup_thread(mddev->thread);
 
-	mempool_exit(&oldpool);
+	mempool_destroy(oldpool);
 	return 0;
 }
 
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index 33f318fcc268..652c347b1a70 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -118,7 +118,7 @@ struct r1conf {
 	 * mempools - it changes when the array grows or shrinks
 	 */
 	struct pool_info	*poolinfo;
-	mempool_t		r1bio_pool;
+	mempool_t		*r1bio_pool;
 	mempool_t		r1buf_pool;
 
 	struct bio_set		bio_split;
-- 
2.43.0

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Wang Jinchao 7 months, 2 weeks ago

On 6/18/25 19:41, Wang Jinchao wrote:
>> In raid1_reshape(), newpool is a stack variable.
>> mempool_init() initializes newpool->wait with the stack address.
>> After assigning newpool to conf->r1bio_pool, the wait queue
>> need to be reinitialized, which is not ideal.
>> 
>> Change raid1_conf->r1bio_pool to a pointer type and
>> replace mempool_init() with mempool_create() to
>> avoid referencing a stack-based wait queue.
>>

>Can you also switch to kmalloc pool in this patch?

>Thanks,
>Kuai

Hi Kuai,

Comparing mempool_create_kmalloc_pool() and mempool_create(), the former 
requires the pool element size as a parameter, while the latter uses 
r1bio_pool_alloc() to allocate new elements, with the size calculated 
based on poolinfo->raid_disks.
The key point is poolinfo, which is used for both r1bio_pool and r1buf_pool.
If we change from mempool_create() to mempool_create_kmalloc_pool(), we 
would need to introduce a new concept, such as r1bio_pool_size, and 
store it somewhere. In this case, the original conf->poolinfo would lose 
its meaning and become just r1buf_poolinfo.
So I think keeping poolinfo is a better fit for the pool in RAID1.

By the way, I did not receive your email in my Gmail inbox; I found your 
message on lore.org. The last email I received from you was on June 16, 
so I am not sure what the problem is.
I also sent you an email mentioning that not using poolinfo makes 
rollback in raid1_reshape more difficult.
I wonder whether you received it, or maybe I missed your reply.

I am looking forward to your discussion. I want to gain a deeper 
understanding and contribute more to md/raid.

Thanks.

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Yu Kuai 7 months, 2 weeks ago

Hi,

在 2025/06/23 11:18, Wang Jinchao 写道:
> Comparing mempool_create_kmalloc_pool() and mempool_create(), the former 
> requires the pool element size as a parameter, while the latter uses 
> r1bio_pool_alloc() to allocate new elements, with the size calculated 
> based on poolinfo->raid_disks.
> The key point is poolinfo, which is used for both r1bio_pool and 
> r1buf_pool.
> If we change from mempool_create() to mempool_create_kmalloc_pool(), we 
> would need to introduce a new concept, such as r1bio_pool_size, and 
> store it somewhere. In this case, the original conf->poolinfo would lose 
> its meaning and become just r1buf_poolinfo.
> So I think keeping poolinfo is a better fit for the pool in RAID1.
> 

I said multiple times it's a fixed size and won't change, you don't need
to store it. Not sure if you get this. :(

conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS,
			offsetof(struct r1bio, bios[mddev->raid_disks *2]);

Thanks,
Kuai

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Wang Jinchao 7 months, 2 weeks ago

On 6/23/25 11:26, Yu Kuai wrote:
> Hi,
> 
> 在 2025/06/23 11:18, Wang Jinchao 写道:
>> Comparing mempool_create_kmalloc_pool() and mempool_create(), the 
>> former requires the pool element size as a parameter, while the latter 
>> uses r1bio_pool_alloc() to allocate new elements, with the size 
>> calculated based on poolinfo->raid_disks.
>> The key point is poolinfo, which is used for both r1bio_pool and 
>> r1buf_pool.
>> If we change from mempool_create() to mempool_create_kmalloc_pool(), 
>> we would need to introduce a new concept, such as r1bio_pool_size, and 
>> store it somewhere. In this case, the original conf->poolinfo would 
>> lose its meaning and become just r1buf_poolinfo.
>> So I think keeping poolinfo is a better fit for the pool in RAID1.
>>
> 
> I said multiple times it's a fixed size and won't change, you don't need
> to store it. Not sure if you get this. :(
> 
> conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS,
>              offsetof(struct r1bio, bios[mddev->raid_disks *2]);
> 
> Thanks,
> Kuai
> 

This time I got it.
I used to think it was a pointer, but now I realize it’s actually a 
pointer cast from a fixed value.
I can change it to use mempool_create_kmalloc_pool now.
I will also reconsider your three previous suggestions.
Thanks for your patience.

---
Jinchao

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Yu Kuai 7 months, 3 weeks ago

Hi,

在 2025/06/18 19:41, Wang Jinchao 写道:
> In raid1_reshape(), newpool is a stack variable.
> mempool_init() initializes newpool->wait with the stack address.
> After assigning newpool to conf->r1bio_pool, the wait queue
> need to be reinitialized, which is not ideal.
> 
> Change raid1_conf->r1bio_pool to a pointer type and
> replace mempool_init() with mempool_create() to
> avoid referencing a stack-based wait queue.

Can you also switch to kmalloc pool in this patch?

Thanks,
Kuai

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Su Yue 7 months, 3 weeks ago

On Wed 18 Jun 2025 at 19:41, Wang Jinchao 
<wangjinchao600@gmail.com> wrote:

> In raid1_reshape(), newpool is a stack variable.
> mempool_init() initializes newpool->wait with the stack address.
> After assigning newpool to conf->r1bio_pool, the wait queue
> need to be reinitialized, which is not ideal.
>
> Change raid1_conf->r1bio_pool to a pointer type and
> replace mempool_init() with mempool_create() to
> avoid referencing a stack-based wait queue.
>
> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
> ---
>  drivers/md/raid1.c | 31 +++++++++++++------------------
>  drivers/md/raid1.h |  2 +-
>  2 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index fd4ce2a4136f..4d4833915b5f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -255,7 +255,7 @@ static void free_r1bio(struct r1bio *r1_bio)
>  	struct r1conf *conf = r1_bio->mddev->private;
>
>  	put_all_bios(conf, r1_bio);
> -	mempool_free(r1_bio, &conf->r1bio_pool);
> +	mempool_free(r1_bio, conf->r1bio_pool);
>  }
>
>  static void put_buf(struct r1bio *r1_bio)
> @@ -1305,7 +1305,7 @@ alloc_r1bio(struct mddev *mddev, struct 
> bio *bio)
>  	struct r1conf *conf = mddev->private;
>  	struct r1bio *r1_bio;
>
> -	r1_bio = mempool_alloc(&conf->r1bio_pool, GFP_NOIO);
> +	r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
>  	/* Ensure no bio records IO_BLOCKED */
>  	memset(r1_bio->bios, 0, conf->raid_disks * 
>  sizeof(r1_bio->bios[0]));
>  	init_r1bio(r1_bio, mddev, bio);
> @@ -3124,9 +3124,9 @@ static struct r1conf *setup_conf(struct 
> mddev *mddev)
>  	if (!conf->poolinfo)
>  		goto abort;
>  	conf->poolinfo->raid_disks = mddev->raid_disks * 2;
> -	err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, 
> r1bio_pool_alloc,
> -			   rbio_pool_free, conf->poolinfo);
> -	if (err)
> +	conf->r1bio_pool = mempool_create(NR_RAID_BIOS, 
> r1bio_pool_alloc,
> +					  rbio_pool_free, conf->poolinfo);
> +	if (!conf->r1bio_pool)
>
err should be set to -ENOMEM.

--
Su

>  		goto abort;
>
>  	err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
> @@ -3197,7 +3197,7 @@ static struct r1conf *setup_conf(struct 
> mddev *mddev)
>
>   abort:
>  	if (conf) {
> -		mempool_exit(&conf->r1bio_pool);
> +		mempool_destroy(conf->r1bio_pool);
>  		kfree(conf->mirrors);
>  		safe_put_page(conf->tmppage);
>  		kfree(conf->poolinfo);
> @@ -3310,7 +3310,7 @@ static void raid1_free(struct mddev 
> *mddev, void *priv)
>  {
>  	struct r1conf *conf = priv;
>
> -	mempool_exit(&conf->r1bio_pool);
> +	mempool_destroy(conf->r1bio_pool);
>  	kfree(conf->mirrors);
>  	safe_put_page(conf->tmppage);
>  	kfree(conf->poolinfo);
> @@ -3366,17 +3366,13 @@ static int raid1_reshape(struct mddev 
> *mddev)
>  	 * At the same time, we "pack" the devices so that all the 
>  missing
>  	 * devices have the higher raid_disk numbers.
>  	 */
> -	mempool_t newpool, oldpool;
> +	mempool_t *newpool, *oldpool;
>  	struct pool_info *newpoolinfo;
>  	struct raid1_info *newmirrors;
>  	struct r1conf *conf = mddev->private;
>  	int cnt, raid_disks;
>  	unsigned long flags;
>  	int d, d2;
> -	int ret;
> -
> -	memset(&newpool, 0, sizeof(newpool));
> -	memset(&oldpool, 0, sizeof(oldpool));
>
>  	/* Cannot change chunk_size, layout, or level */
>  	if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
> @@ -3408,18 +3404,18 @@ static int raid1_reshape(struct mddev 
> *mddev)
>  	newpoolinfo->mddev = mddev;
>  	newpoolinfo->raid_disks = raid_disks * 2;
>
> -	ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
> +	newpool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
>  			   rbio_pool_free, newpoolinfo);
> -	if (ret) {
> +	if (!newpool) {
>  		kfree(newpoolinfo);
> -		return ret;
> +		return -ENOMEM;
>  	}
>  	newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
>  					 raid_disks, 2),
>  			     GFP_KERNEL);
>  	if (!newmirrors) {
>  		kfree(newpoolinfo);
> -		mempool_exit(&newpool);
> +		mempool_destroy(newpool);
>  		return -ENOMEM;
>  	}
>
> @@ -3428,7 +3424,6 @@ static int raid1_reshape(struct mddev 
> *mddev)
>  	/* ok, everything is stopped */
>  	oldpool = conf->r1bio_pool;
>  	conf->r1bio_pool = newpool;
> -	init_waitqueue_head(&conf->r1bio_pool.wait);
>
>  	for (d = d2 = 0; d < conf->raid_disks; d++) {
>  		struct md_rdev *rdev = conf->mirrors[d].rdev;
> @@ -3460,7 +3455,7 @@ static int raid1_reshape(struct mddev 
> *mddev)
>  	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>  	md_wakeup_thread(mddev->thread);
>
> -	mempool_exit(&oldpool);
> +	mempool_destroy(oldpool);
>  	return 0;
>  }
>
> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> index 33f318fcc268..652c347b1a70 100644
> --- a/drivers/md/raid1.h
> +++ b/drivers/md/raid1.h
> @@ -118,7 +118,7 @@ struct r1conf {
>  	 * mempools - it changes when the array grows or shrinks
>  	 */
>  	struct pool_info	*poolinfo;
> -	mempool_t		r1bio_pool;
> +	mempool_t		*r1bio_pool;
>  	mempool_t		r1buf_pool;
>
>  	struct bio_set		bio_split;

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Wang Jinchao 7 months, 3 weeks ago

On 6/19/25 08:56, Su Yue wrote:
> On Wed 18 Jun 2025 at 19:41, Wang Jinchao <wangjinchao600@gmail.com> wrote:
> 
>> In raid1_reshape(), newpool is a stack variable.
>> mempool_init() initializes newpool->wait with the stack address.
>> After assigning newpool to conf->r1bio_pool, the wait queue
>> need to be reinitialized, which is not ideal.
>>
>> Change raid1_conf->r1bio_pool to a pointer type and
>> replace mempool_init() with mempool_create() to
>> avoid referencing a stack-based wait queue.
>>
>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>> ---
>>  drivers/md/raid1.c | 31 +++++++++++++------------------
>>  drivers/md/raid1.h |  2 +-
>>  2 files changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index fd4ce2a4136f..4d4833915b5f 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -255,7 +255,7 @@ static void free_r1bio(struct r1bio *r1_bio)
>>      struct r1conf *conf = r1_bio->mddev->private;
>>
>>      put_all_bios(conf, r1_bio);
>> -    mempool_free(r1_bio, &conf->r1bio_pool);
>> +    mempool_free(r1_bio, conf->r1bio_pool);
>>  }
>>
>>  static void put_buf(struct r1bio *r1_bio)
>> @@ -1305,7 +1305,7 @@ alloc_r1bio(struct mddev *mddev, struct bio *bio)
>>      struct r1conf *conf = mddev->private;
>>      struct r1bio *r1_bio;
>>
>> -    r1_bio = mempool_alloc(&conf->r1bio_pool, GFP_NOIO);
>> +    r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
>>      /* Ensure no bio records IO_BLOCKED */
>>      memset(r1_bio->bios, 0, conf->raid_disks *  sizeof(r1_bio- 
>> >bios[0]));
>>      init_r1bio(r1_bio, mddev, bio);
>> @@ -3124,9 +3124,9 @@ static struct r1conf *setup_conf(struct mddev 
>> *mddev)
>>      if (!conf->poolinfo)
>>          goto abort;
>>      conf->poolinfo->raid_disks = mddev->raid_disks * 2;
>> -    err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, 
>> r1bio_pool_alloc,
>> -               rbio_pool_free, conf->poolinfo);
>> -    if (err)
>> +    conf->r1bio_pool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
>> +                      rbio_pool_free, conf->poolinfo);
>> +    if (!conf->r1bio_pool)
>>
> err should be set to -ENOMEM.
> 
At the beginning of the function, err is initialized to -ENOMEM.

> -- 
> Su
> 
>>          goto abort;
>>
>>      err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
>> @@ -3197,7 +3197,7 @@ static struct r1conf *setup_conf(struct mddev 
>> *mddev)
>>
>>   abort:
>>      if (conf) {
>> -        mempool_exit(&conf->r1bio_pool);
>> +        mempool_destroy(conf->r1bio_pool);
>>          kfree(conf->mirrors);
>>          safe_put_page(conf->tmppage);
>>          kfree(conf->poolinfo);
>> @@ -3310,7 +3310,7 @@ static void raid1_free(struct mddev *mddev, void 
>> *priv)
>>  {
>>      struct r1conf *conf = priv;
>>
>> -    mempool_exit(&conf->r1bio_pool);
>> +    mempool_destroy(conf->r1bio_pool);
>>      kfree(conf->mirrors);
>>      safe_put_page(conf->tmppage);
>>      kfree(conf->poolinfo);
>> @@ -3366,17 +3366,13 @@ static int raid1_reshape(struct mddev *mddev)
>>       * At the same time, we "pack" the devices so that all the  missing
>>       * devices have the higher raid_disk numbers.
>>       */
>> -    mempool_t newpool, oldpool;
>> +    mempool_t *newpool, *oldpool;
>>      struct pool_info *newpoolinfo;
>>      struct raid1_info *newmirrors;
>>      struct r1conf *conf = mddev->private;
>>      int cnt, raid_disks;
>>      unsigned long flags;
>>      int d, d2;
>> -    int ret;
>> -
>> -    memset(&newpool, 0, sizeof(newpool));
>> -    memset(&oldpool, 0, sizeof(oldpool));
>>
>>      /* Cannot change chunk_size, layout, or level */
>>      if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
>> @@ -3408,18 +3404,18 @@ static int raid1_reshape(struct mddev *mddev)
>>      newpoolinfo->mddev = mddev;
>>      newpoolinfo->raid_disks = raid_disks * 2;
>>
>> -    ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
>> +    newpool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
>>                 rbio_pool_free, newpoolinfo);
>> -    if (ret) {
>> +    if (!newpool) {
>>          kfree(newpoolinfo);
>> -        return ret;
>> +        return -ENOMEM;
>>      }
>>      newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
>>                       raid_disks, 2),
>>                   GFP_KERNEL);
>>      if (!newmirrors) {
>>          kfree(newpoolinfo);
>> -        mempool_exit(&newpool);
>> +        mempool_destroy(newpool);
>>          return -ENOMEM;
>>      }
>>
>> @@ -3428,7 +3424,6 @@ static int raid1_reshape(struct mddev *mddev)
>>      /* ok, everything is stopped */
>>      oldpool = conf->r1bio_pool;
>>      conf->r1bio_pool = newpool;
>> -    init_waitqueue_head(&conf->r1bio_pool.wait);
>>
>>      for (d = d2 = 0; d < conf->raid_disks; d++) {
>>          struct md_rdev *rdev = conf->mirrors[d].rdev;
>> @@ -3460,7 +3455,7 @@ static int raid1_reshape(struct mddev *mddev)
>>      set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>>      md_wakeup_thread(mddev->thread);
>>
>> -    mempool_exit(&oldpool);
>> +    mempool_destroy(oldpool);
>>      return 0;
>>  }
>>
>> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
>> index 33f318fcc268..652c347b1a70 100644
>> --- a/drivers/md/raid1.h
>> +++ b/drivers/md/raid1.h
>> @@ -118,7 +118,7 @@ struct r1conf {
>>       * mempools - it changes when the array grows or shrinks
>>       */
>>      struct pool_info    *poolinfo;
>> -    mempool_t        r1bio_pool;
>> +    mempool_t        *r1bio_pool;
>>      mempool_t        r1buf_pool;
>>
>>      struct bio_set        bio_split;

Re: [PATCH] md/raid1: change r1conf->r1bio_pool to a pointer type

Posted by Su Yue 7 months, 3 weeks ago

On Thu 19 Jun 2025 at 10:01, Wang Jinchao 
<wangjinchao600@gmail.com> wrote:

> On 6/19/25 08:56, Su Yue wrote:
>> On Wed 18 Jun 2025 at 19:41, Wang Jinchao 
>> <wangjinchao600@gmail.com> wrote:
>>
>>> In raid1_reshape(), newpool is a stack variable.
>>> mempool_init() initializes newpool->wait with the stack 
>>> address.
>>> After assigning newpool to conf->r1bio_pool, the wait queue
>>> need to be reinitialized, which is not ideal.
>>>
>>> Change raid1_conf->r1bio_pool to a pointer type and
>>> replace mempool_init() with mempool_create() to
>>> avoid referencing a stack-based wait queue.
>>>
>>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>>> ---
>>>  drivers/md/raid1.c | 31 +++++++++++++------------------
>>>  drivers/md/raid1.h |  2 +-
>>>  2 files changed, 14 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>> index fd4ce2a4136f..4d4833915b5f 100644
>>> --- a/drivers/md/raid1.c
>>> +++ b/drivers/md/raid1.c
>>> @@ -255,7 +255,7 @@ static void free_r1bio(struct r1bio 
>>> *r1_bio)
>>>      struct r1conf *conf = r1_bio->mddev->private;
>>>
>>>      put_all_bios(conf, r1_bio);
>>> -    mempool_free(r1_bio, &conf->r1bio_pool);
>>> +    mempool_free(r1_bio, conf->r1bio_pool);
>>>  }
>>>
>>>  static void put_buf(struct r1bio *r1_bio)
>>> @@ -1305,7 +1305,7 @@ alloc_r1bio(struct mddev *mddev, struct 
>>> bio *bio)
>>>      struct r1conf *conf = mddev->private;
>>>      struct r1bio *r1_bio;
>>>
>>> -    r1_bio = mempool_alloc(&conf->r1bio_pool, GFP_NOIO);
>>> +    r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
>>>      /* Ensure no bio records IO_BLOCKED */
>>>      memset(r1_bio->bios, 0, conf->raid_disks * 
>>>  sizeof(r1_bio-  >bios[0]));
>>>      init_r1bio(r1_bio, mddev, bio);
>>> @@ -3124,9 +3124,9 @@ static struct r1conf *setup_conf(struct 
>>> mddev *mddev)
>>>      if (!conf->poolinfo)
>>>          goto abort;
>>>      conf->poolinfo->raid_disks = mddev->raid_disks * 2;
>>> -    err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, 
>>> r1bio_pool_alloc,
>>> -               rbio_pool_free, conf->poolinfo);
>>> -    if (err)
>>> +    conf->r1bio_pool = mempool_create(NR_RAID_BIOS, 
>>> r1bio_pool_alloc,
>>> +                      rbio_pool_free, conf->poolinfo);
>>> +    if (!conf->r1bio_pool)
>>>
>> err should be set to -ENOMEM.
>>
> At the beginning of the function, err is initialized to -ENOMEM.
>
Alright...

--
Su
>> -- Su
>>
>>>          goto abort;
>>>
>>>      err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
>>> @@ -3197,7 +3197,7 @@ static struct r1conf *setup_conf(struct 
>>> mddev *mddev)
>>>
>>>   abort:
>>>      if (conf) {
>>> -        mempool_exit(&conf->r1bio_pool);
>>> +        mempool_destroy(conf->r1bio_pool);
>>>          kfree(conf->mirrors);
>>>          safe_put_page(conf->tmppage);
>>>          kfree(conf->poolinfo);
>>> @@ -3310,7 +3310,7 @@ static void raid1_free(struct mddev 
>>> *mddev, void *priv)
>>>  {
>>>      struct r1conf *conf = priv;
>>>
>>> -    mempool_exit(&conf->r1bio_pool);
>>> +    mempool_destroy(conf->r1bio_pool);
>>>      kfree(conf->mirrors);
>>>      safe_put_page(conf->tmppage);
>>>      kfree(conf->poolinfo);
>>> @@ -3366,17 +3366,13 @@ static int raid1_reshape(struct mddev 
>>> *mddev)
>>>       * At the same time, we "pack" the devices so that all 
>>> the  missing
>>>       * devices have the higher raid_disk numbers.
>>>       */
>>> -    mempool_t newpool, oldpool;
>>> +    mempool_t *newpool, *oldpool;
>>>      struct pool_info *newpoolinfo;
>>>      struct raid1_info *newmirrors;
>>>      struct r1conf *conf = mddev->private;
>>>      int cnt, raid_disks;
>>>      unsigned long flags;
>>>      int d, d2;
>>> -    int ret;
>>> -
>>> -    memset(&newpool, 0, sizeof(newpool));
>>> -    memset(&oldpool, 0, sizeof(oldpool));
>>>
>>>      /* Cannot change chunk_size, layout, or level */
>>>      if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
>>> @@ -3408,18 +3404,18 @@ static int raid1_reshape(struct mddev 
>>> *mddev)
>>>      newpoolinfo->mddev = mddev;
>>>      newpoolinfo->raid_disks = raid_disks * 2;
>>>
>>> -    ret = mempool_init(&newpool, NR_RAID_BIOS, 
>>> r1bio_pool_alloc,
>>> +    newpool = mempool_create(NR_RAID_BIOS, r1bio_pool_alloc,
>>>                 rbio_pool_free, newpoolinfo);
>>> -    if (ret) {
>>> +    if (!newpool) {
>>>          kfree(newpoolinfo);
>>> -        return ret;
>>> +        return -ENOMEM;
>>>      }
>>>      newmirrors = kzalloc(array3_size(sizeof(struct 
>>> raid1_info),
>>>                       raid_disks, 2),
>>>                   GFP_KERNEL);
>>>      if (!newmirrors) {
>>>          kfree(newpoolinfo);
>>> -        mempool_exit(&newpool);
>>> +        mempool_destroy(newpool);
>>>          return -ENOMEM;
>>>      }
>>>
>>> @@ -3428,7 +3424,6 @@ static int raid1_reshape(struct mddev 
>>> *mddev)
>>>      /* ok, everything is stopped */
>>>      oldpool = conf->r1bio_pool;
>>>      conf->r1bio_pool = newpool;
>>> -    init_waitqueue_head(&conf->r1bio_pool.wait);
>>>
>>>      for (d = d2 = 0; d < conf->raid_disks; d++) {
>>>          struct md_rdev *rdev = conf->mirrors[d].rdev;
>>> @@ -3460,7 +3455,7 @@ static int raid1_reshape(struct mddev 
>>> *mddev)
>>>      set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>>>      md_wakeup_thread(mddev->thread);
>>>
>>> -    mempool_exit(&oldpool);
>>> +    mempool_destroy(oldpool);
>>>      return 0;
>>>  }
>>>
>>> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
>>> index 33f318fcc268..652c347b1a70 100644
>>> --- a/drivers/md/raid1.h
>>> +++ b/drivers/md/raid1.h
>>> @@ -118,7 +118,7 @@ struct r1conf {
>>>       * mempools - it changes when the array grows or shrinks
>>>       */
>>>      struct pool_info    *poolinfo;
>>> -    mempool_t        r1bio_pool;
>>> +    mempool_t        *r1bio_pool;
>>>      mempool_t        r1buf_pool;
>>>
>>>      struct bio_set        bio_split;