[PATCH v3 0/2] Optimize r1conf->r1bio_pool

[PATCH v3 0/2] Optimize r1conf->r1bio_pool

[Wang Jinchao]

The r1conf->r1bio_pool field was previously a struct, 
which caused a bug after raid1_reshape.
These patch changes it to a pointer type 
and removes struct pool_info and related code.

Wang Jinchao (2):
  md/raid1: change r1conf->r1bio_pool to a pointer type
  md/raid1: remove struct pool_info and related code

 drivers/md/raid1.c | 84 +++++++++++++++++-----------------------------
 drivers/md/raid1.h | 22 +-----------
 2 files changed, 31 insertions(+), 75 deletions(-)
-- 
v1: Replace mempool_init() with mempool_create()
v2: Replace mempool_init() with mempool_create_kmalloc_pool()
v3: Fix checkpatch errors and a bug in calculating new_r1bio_size

The idea was proposed by Kuai, and I implemented it.
Thanks for the guidance and review.

-- 
2.43.0

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

[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_kmalloc_pool() to
avoid referencing a stack-based wait queue.

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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index fd4ce2a4136f..8249cbb89fec 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,9 +1305,8 @@ 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);
-	/* Ensure no bio records IO_BLOCKED */
-	memset(r1_bio->bios, 0, conf->raid_disks * sizeof(r1_bio->bios[0]));
+	r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
+	memset(r1_bio, 0, offsetof(struct r1bio, bios[conf->raid_disks * 2]));
 	init_r1bio(r1_bio, mddev, bio);
 	return r1_bio;
 }
@@ -3084,6 +3083,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	int i;
 	struct raid1_info *disk;
 	struct md_rdev *rdev;
+	size_t r1bio_size;
 	int err = -ENOMEM;
 
 	conf = kzalloc(sizeof(struct r1conf), GFP_KERNEL);
@@ -3124,9 +3124,10 @@ 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)
+
+	r1bio_size = offsetof(struct r1bio, bios[mddev->raid_disks * 2]);
+	conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS, r1bio_size);
+	if (!conf->r1bio_pool)
 		goto abort;
 
 	err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
@@ -3197,7 +3198,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 +3311,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 +3367,14 @@ 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;
+	size_t new_r1bio_size;
 	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 +3406,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,
-			   rbio_pool_free, newpoolinfo);
-	if (ret) {
+	new_r1bio_size = offsetof(struct r1bio, bios[raid_disks * 2]);
+	newpool = mempool_create_kmalloc_pool(NR_RAID_BIOS, new_r1bio_size);
+	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 +3426,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 +3457,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

[PATCH v3 2/2] md/raid1: remove struct pool_info and related code

[Wang Jinchao]

The struct pool_info was originally introduced mainly to support reshape
operations, serving as a parameter for mempool_init() when raid_disks
changes. Now that mempool_create_kmalloc_pool() is sufficient for this
purpose, struct pool_info and its related code are no longer needed.

Remove struct pool_info and all associated code.

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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 8249cbb89fec..3a31e230727c 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -127,10 +127,9 @@ static inline struct r1bio *get_resync_r1bio(struct bio *bio)
 	return get_resync_pages(bio)->raid_bio;
 }
 
-static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)
+static void *r1bio_pool_alloc(gfp_t gfp_flags, struct r1conf *conf)
 {
-	struct pool_info *pi = data;
-	int size = offsetof(struct r1bio, bios[pi->raid_disks]);
+	int size = offsetof(struct r1bio, bios[conf->raid_disks * 2]);
 
 	/* allocate a r1bio with room for raid_disks entries in the bios array */
 	return kzalloc(size, gfp_flags);
@@ -145,18 +144,18 @@ static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)
 
 static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 {
-	struct pool_info *pi = data;
+	struct r1conf *conf = data;
 	struct r1bio *r1_bio;
 	struct bio *bio;
 	int need_pages;
 	int j;
 	struct resync_pages *rps;
 
-	r1_bio = r1bio_pool_alloc(gfp_flags, pi);
+	r1_bio = r1bio_pool_alloc(gfp_flags, conf);
 	if (!r1_bio)
 		return NULL;
 
-	rps = kmalloc_array(pi->raid_disks, sizeof(struct resync_pages),
+	rps = kmalloc_array(conf->raid_disks * 2, sizeof(struct resync_pages),
 			    gfp_flags);
 	if (!rps)
 		goto out_free_r1bio;
@@ -164,7 +163,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 	/*
 	 * Allocate bios : 1 for reading, n-1 for writing
 	 */
-	for (j = pi->raid_disks ; j-- ; ) {
+	for (j = conf->raid_disks * 2; j-- ; ) {
 		bio = bio_kmalloc(RESYNC_PAGES, gfp_flags);
 		if (!bio)
 			goto out_free_bio;
@@ -177,11 +176,11 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 	 * If this is a user-requested check/repair, allocate
 	 * RESYNC_PAGES for each bio.
 	 */
-	if (test_bit(MD_RECOVERY_REQUESTED, &pi->mddev->recovery))
-		need_pages = pi->raid_disks;
+	if (test_bit(MD_RECOVERY_REQUESTED, &conf->mddev->recovery))
+		need_pages = conf->raid_disks * 2;
 	else
 		need_pages = 1;
-	for (j = 0; j < pi->raid_disks; j++) {
+	for (j = 0; j < conf->raid_disks * 2; j++) {
 		struct resync_pages *rp = &rps[j];
 
 		bio = r1_bio->bios[j];
@@ -207,7 +206,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 		resync_free_pages(&rps[j]);
 
 out_free_bio:
-	while (++j < pi->raid_disks) {
+	while (++j < conf->raid_disks * 2) {
 		bio_uninit(r1_bio->bios[j]);
 		kfree(r1_bio->bios[j]);
 	}
@@ -220,12 +219,12 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 
 static void r1buf_pool_free(void *__r1_bio, void *data)
 {
-	struct pool_info *pi = data;
+	struct r1conf *conf = data;
 	int i;
 	struct r1bio *r1bio = __r1_bio;
 	struct resync_pages *rp = NULL;
 
-	for (i = pi->raid_disks; i--; ) {
+	for (i = conf->raid_disks * 2; i--; ) {
 		rp = get_resync_pages(r1bio->bios[i]);
 		resync_free_pages(rp);
 		bio_uninit(r1bio->bios[i]);
@@ -2745,7 +2744,7 @@ static int init_resync(struct r1conf *conf)
 	BUG_ON(mempool_initialized(&conf->r1buf_pool));
 
 	return mempool_init(&conf->r1buf_pool, buffs, r1buf_pool_alloc,
-			    r1buf_pool_free, conf->poolinfo);
+			    r1buf_pool_free, conf);
 }
 
 static struct r1bio *raid1_alloc_init_r1buf(struct r1conf *conf)
@@ -2755,7 +2754,7 @@ static struct r1bio *raid1_alloc_init_r1buf(struct r1conf *conf)
 	struct bio *bio;
 	int i;
 
-	for (i = conf->poolinfo->raid_disks; i--; ) {
+	for (i = conf->raid_disks * 2; i--; ) {
 		bio = r1bio->bios[i];
 		rps = bio->bi_private;
 		bio_reset(bio, NULL, 0);
@@ -3120,11 +3119,6 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	if (!conf->tmppage)
 		goto abort;
 
-	conf->poolinfo = kzalloc(sizeof(*conf->poolinfo), GFP_KERNEL);
-	if (!conf->poolinfo)
-		goto abort;
-	conf->poolinfo->raid_disks = mddev->raid_disks * 2;
-
 	r1bio_size = offsetof(struct r1bio, bios[mddev->raid_disks * 2]);
 	conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS, r1bio_size);
 	if (!conf->r1bio_pool)
@@ -3134,8 +3128,6 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	if (err)
 		goto abort;
 
-	conf->poolinfo->mddev = mddev;
-
 	err = -EINVAL;
 	spin_lock_init(&conf->device_lock);
 	conf->raid_disks = mddev->raid_disks;
@@ -3201,7 +3193,6 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		mempool_destroy(conf->r1bio_pool);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
-		kfree(conf->poolinfo);
 		kfree(conf->nr_pending);
 		kfree(conf->nr_waiting);
 		kfree(conf->nr_queued);
@@ -3314,7 +3305,6 @@ static void raid1_free(struct mddev *mddev, void *priv)
 	mempool_destroy(conf->r1bio_pool);
 	kfree(conf->mirrors);
 	safe_put_page(conf->tmppage);
-	kfree(conf->poolinfo);
 	kfree(conf->nr_pending);
 	kfree(conf->nr_waiting);
 	kfree(conf->nr_queued);
@@ -3368,7 +3358,6 @@ static int raid1_reshape(struct mddev *mddev)
 	 * devices have the higher raid_disk numbers.
 	 */
 	mempool_t *newpool, *oldpool;
-	struct pool_info *newpoolinfo;
 	size_t new_r1bio_size;
 	struct raid1_info *newmirrors;
 	struct r1conf *conf = mddev->private;
@@ -3400,23 +3389,15 @@ static int raid1_reshape(struct mddev *mddev)
 			return -EBUSY;
 	}
 
-	newpoolinfo = kmalloc(sizeof(*newpoolinfo), GFP_KERNEL);
-	if (!newpoolinfo)
-		return -ENOMEM;
-	newpoolinfo->mddev = mddev;
-	newpoolinfo->raid_disks = raid_disks * 2;
-
 	new_r1bio_size = offsetof(struct r1bio, bios[raid_disks * 2]);
 	newpool = mempool_create_kmalloc_pool(NR_RAID_BIOS, new_r1bio_size);
 	if (!newpool) {
-		kfree(newpoolinfo);
 		return -ENOMEM;
 	}
 	newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
 					 raid_disks, 2),
 			     GFP_KERNEL);
 	if (!newmirrors) {
-		kfree(newpoolinfo);
 		mempool_destroy(newpool);
 		return -ENOMEM;
 	}
@@ -3442,8 +3423,6 @@ static int raid1_reshape(struct mddev *mddev)
 	}
 	kfree(conf->mirrors);
 	conf->mirrors = newmirrors;
-	kfree(conf->poolinfo);
-	conf->poolinfo = newpoolinfo;
 
 	spin_lock_irqsave(&conf->device_lock, flags);
 	mddev->degraded += (raid_disks - conf->raid_disks);
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index 652c347b1a70..d236ef179cfb 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -49,22 +49,6 @@ struct raid1_info {
 	sector_t	seq_start;
 };
 
-/*
- * memory pools need a pointer to the mddev, so they can force an unplug
- * when memory is tight, and a count of the number of drives that the
- * pool was allocated for, so they know how much to allocate and free.
- * mddev->raid_disks cannot be used, as it can change while a pool is active
- * These two datums are stored in a kmalloced struct.
- * The 'raid_disks' here is twice the raid_disks in r1conf.
- * This allows space for each 'real' device can have a replacement in the
- * second half of the array.
- */
-
-struct pool_info {
-	struct mddev *mddev;
-	int	raid_disks;
-};
-
 struct r1conf {
 	struct mddev		*mddev;
 	struct raid1_info	*mirrors;	/* twice 'raid_disks' to
@@ -114,10 +98,6 @@ struct r1conf {
 	 */
 	int			recovery_disabled;
 
-	/* poolinfo contains information about the content of the
-	 * mempools - it changes when the array grows or shrinks
-	 */
-	struct pool_info	*poolinfo;
 	mempool_t		*r1bio_pool;
 	mempool_t		r1buf_pool;
 
-- 
2.43.0

Re: [PATCH v3 2/2] md/raid1: remove struct pool_info and related code

[Yu Kuai]

在 2025/06/24 9:55, Wang Jinchao 写道:
> The struct pool_info was originally introduced mainly to support reshape
> operations, serving as a parameter for mempool_init() when raid_disks
> changes. Now that mempool_create_kmalloc_pool() is sufficient for this
> purpose, struct pool_info and its related code are no longer needed.
> 
> Remove struct pool_info and all associated code.
> 
> Signed-off-by: Wang Jinchao<wangjinchao600@gmail.com>
> ---
>   drivers/md/raid1.c | 49 +++++++++++++---------------------------------
>   drivers/md/raid1.h | 20 -------------------
>   2 files changed, 14 insertions(+), 55 deletions(-)

Nice cleanup.
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

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

[Yu Kuai]

Hi,

在 2025/06/24 9:55, 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_kmalloc_pool() to
> avoid referencing a stack-based wait queue.
> 
> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
> ---
>   drivers/md/raid1.c | 39 ++++++++++++++++++---------------------
>   drivers/md/raid1.h |  2 +-
>   2 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index fd4ce2a4136f..8249cbb89fec 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,9 +1305,8 @@ 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);
> -	/* Ensure no bio records IO_BLOCKED */
> -	memset(r1_bio->bios, 0, conf->raid_disks * sizeof(r1_bio->bios[0]));
> +	r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
> +	memset(r1_bio, 0, offsetof(struct r1bio, bios[conf->raid_disks * 2]));
>   	init_r1bio(r1_bio, mddev, bio);
>   	return r1_bio;
>   }
> @@ -3084,6 +3083,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
>   	int i;
>   	struct raid1_info *disk;
>   	struct md_rdev *rdev;
> +	size_t r1bio_size;
>   	int err = -ENOMEM;
>   
>   	conf = kzalloc(sizeof(struct r1conf), GFP_KERNEL);
> @@ -3124,9 +3124,10 @@ 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)
> +
> +	r1bio_size = offsetof(struct r1bio, bios[mddev->raid_disks * 2]);

The local variable doesn't look necessary, it's just used once anyway.
> +	conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS, r1bio_size);
> +	if (!conf->r1bio_pool)
>   		goto abort;
>   
>   	err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
> @@ -3197,7 +3198,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 +3311,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 +3367,14 @@ 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;
> +	size_t new_r1bio_size;
>   	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 +3406,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,
> -			   rbio_pool_free, newpoolinfo);
> -	if (ret) {
> +	new_r1bio_size = offsetof(struct r1bio, bios[raid_disks * 2]);
same here. Otherwise looks good to me.

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> +	newpool = mempool_create_kmalloc_pool(NR_RAID_BIOS, new_r1bio_size);
> +	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 +3426,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 +3457,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 v3 1/2] md/raid1: change r1conf->r1bio_pool to a pointer type

[Wang Jinchao]

On 6/28/25 11:17, Yu Kuai wrote:
> Hi,
> 
> 在 2025/06/24 9:55, 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_kmalloc_pool() to
>> avoid referencing a stack-based wait queue.
>>
>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>> ---
>>   drivers/md/raid1.c | 39 ++++++++++++++++++---------------------
>>   drivers/md/raid1.h |  2 +-
>>   2 files changed, 19 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index fd4ce2a4136f..8249cbb89fec 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,9 +1305,8 @@ 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);
>> -    /* Ensure no bio records IO_BLOCKED */
>> -    memset(r1_bio->bios, 0, conf->raid_disks * sizeof(r1_bio->bios[0]));
>> +    r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
>> +    memset(r1_bio, 0, offsetof(struct r1bio, bios[conf->raid_disks * 
>> 2]));
>>       init_r1bio(r1_bio, mddev, bio);
>>       return r1_bio;
>>   }
>> @@ -3084,6 +3083,7 @@ static struct r1conf *setup_conf(struct mddev 
>> *mddev)
>>       int i;
>>       struct raid1_info *disk;
>>       struct md_rdev *rdev;
>> +    size_t r1bio_size;
>>       int err = -ENOMEM;
>>       conf = kzalloc(sizeof(struct r1conf), GFP_KERNEL);
>> @@ -3124,9 +3124,10 @@ 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)
>> +
>> +    r1bio_size = offsetof(struct r1bio, bios[mddev->raid_disks * 2]);
> 
> The local variable doesn't look necessary, it's just used once anyway.
>> +    conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS, 
>> r1bio_size);
>> +    if (!conf->r1bio_pool)
>>           goto abort;
>>       err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
>> @@ -3197,7 +3198,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 +3311,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 +3367,14 @@ 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;
>> +    size_t new_r1bio_size;
>>       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 +3406,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,
>> -               rbio_pool_free, newpoolinfo);
>> -    if (ret) {
>> +    new_r1bio_size = offsetof(struct r1bio, bios[raid_disks * 2]);
> same here. Otherwise looks good to me.
> 
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
>> +    newpool = mempool_create_kmalloc_pool(NR_RAID_BIOS, new_r1bio_size);
>> +    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 +3426,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 +3457,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;
>>
> 
Thanks for pointing that out.

I originally introduced the local variable to avoid these checkpatch.pl 
messages:
     CHECK: Alignment should match open parenthesis
     WARNING: line length of xxx exceeds 100 columns
But I agree that using a temporary variable in this case adds 
unnecessary noise, since the value is only used once.

Based on your review and a re-read of the kernel documentation, I guess 
that CHECK:-level warnings are not strictly require fixing—especially 
when fixing them would harm clarity. Please let me know if I’ve 
misunderstood it.

I'll drop the local variable and update the patch accordingly in the 
next version.

Thanks again for the feedback.

-- 
Best regards,
Jinchao