[PATCH 0/5] raid1-10 and raid0 cleanups

[PATCH 0/5] raid1-10 and raid0 cleanups

[Marcos Paulo de Souza]

The first patch is just removing some code duplicated in raid1 and raid10, and
the other three patches are just cleanups.

These patches were based in "drivers: md: Unify common definitions of raid1 and
raid10", sent a few weeks ago. Let me know if these patches can be improved.

Thanks

Marcos Paulo de Souza (5):
  md cleanup
  raid1-10: Unify r{1,10}bio_pool_free
  raid0: Remove return statement from void function
  md: raid0: Return md_integrity_register result directly
  md: raid0: Make ret local in raid0_run

 drivers/md/raid0.c    |  8 ++------
 drivers/md/raid1-10.c | 30 ++++++++++++++++++++++++++++++
 drivers/md/raid1.c    | 42 ++++++------------------------------------
 drivers/md/raid10.c   | 38 ++++----------------------------------
 4 files changed, 42 insertions(+), 76 deletions(-)

-- 
2.21.0

[PATCH 1/4] md: raid1-10: Unify r{1,10}bio_pool_free

[Marcos Paulo de Souza]

Avoiding duplicated code, since they just execute a kfree.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid1-10.c |  5 +++++
 drivers/md/raid1.c    | 13 ++++---------
 drivers/md/raid10.c   | 11 +++--------
 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 7d968bf08e54..54db34163968 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -34,6 +34,11 @@ struct resync_pages {
 	struct page	*pages[RESYNC_PAGES];
 };
 
+static void rbio_pool_free(void *rbio, void *data)
+{
+	kfree(rbio);
+}
+
 static inline int resync_alloc_pages(struct resync_pages *rp,
 				     gfp_t gfp_flags)
 {
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index bb052c35bf29..3787c7e45849 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -76,11 +76,6 @@ static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)
 	return kzalloc(size, gfp_flags);
 }
 
-static void r1bio_pool_free(void *r1_bio, void *data)
-{
-	kfree(r1_bio);
-}
-
 #define RESYNC_DEPTH 32
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
 #define RESYNC_WINDOW (RESYNC_BLOCK_SIZE * RESYNC_DEPTH)
@@ -156,7 +151,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 	kfree(rps);
 
 out_free_r1bio:
-	r1bio_pool_free(r1_bio, data);
+	rbio_pool_free(r1_bio, data);
 	return NULL;
 }
 
@@ -176,7 +171,7 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
 	/* resync pages array stored in the 1st bio's .bi_private */
 	kfree(rp);
 
-	r1bio_pool_free(r1bio, data);
+	rbio_pool_free(r1bio, data);
 }
 
 static void put_all_bios(struct r1conf *conf, struct r1bio *r1_bio)
@@ -2931,7 +2926,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		goto abort;
 	conf->poolinfo->raid_disks = mddev->raid_disks * 2;
 	err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, r1bio_pool_alloc,
-			   r1bio_pool_free, conf->poolinfo);
+			   rbio_pool_free, conf->poolinfo);
 	if (err)
 		goto abort;
 
@@ -3216,7 +3211,7 @@ static int raid1_reshape(struct mddev *mddev)
 	newpoolinfo->raid_disks = raid_disks * 2;
 
 	ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
-			   r1bio_pool_free, newpoolinfo);
+			   rbio_pool_free, newpoolinfo);
 	if (ret) {
 		kfree(newpoolinfo);
 		return ret;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 24cb116d950f..c0d9ee4311d4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -107,11 +107,6 @@ static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
 	return kzalloc(size, gfp_flags);
 }
 
-static void r10bio_pool_free(void *r10_bio, void *data)
-{
-	kfree(r10_bio);
-}
-
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
 /* amount of memory to reserve for resync requests */
 #define RESYNC_WINDOW (1024*1024)
@@ -217,7 +212,7 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
 	}
 	kfree(rps);
 out_free_r10bio:
-	r10bio_pool_free(r10_bio, conf);
+	rbio_pool_free(r10_bio, conf);
 	return NULL;
 }
 
@@ -245,7 +240,7 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
 	/* resync pages array stored in the 1st bio's .bi_private */
 	kfree(rp);
 
-	r10bio_pool_free(r10bio, conf);
+	rbio_pool_free(r10bio, conf);
 }
 
 static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
@@ -3660,7 +3655,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	conf->geo = geo;
 	conf->copies = copies;
 	err = mempool_init(&conf->r10bio_pool, NR_RAID_BIOS, r10bio_pool_alloc,
-			   r10bio_pool_free, conf);
+			   rbio_pool_free, conf);
 	if (err)
 		goto out;
 
-- 
2.21.0

[PATCH 1/4] md: raid1-10: Unify r{1,10}bio_pool_free

[Marcos Paulo de Souza]

Avoiding duplicated code, since they just execute a kfree.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid1-10.c |  5 +++++
 drivers/md/raid1.c    | 13 ++++---------
 drivers/md/raid10.c   | 11 +++--------
 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 7d968bf08e54..54db34163968 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -34,6 +34,11 @@ struct resync_pages {
 	struct page	*pages[RESYNC_PAGES];
 };
 
+static void rbio_pool_free(void *rbio, void *data)
+{
+	kfree(rbio);
+}
+
 static inline int resync_alloc_pages(struct resync_pages *rp,
 				     gfp_t gfp_flags)
 {
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index bb052c35bf29..3787c7e45849 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -76,11 +76,6 @@ static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)
 	return kzalloc(size, gfp_flags);
 }
 
-static void r1bio_pool_free(void *r1_bio, void *data)
-{
-	kfree(r1_bio);
-}
-
 #define RESYNC_DEPTH 32
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
 #define RESYNC_WINDOW (RESYNC_BLOCK_SIZE * RESYNC_DEPTH)
@@ -156,7 +151,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 	kfree(rps);
 
 out_free_r1bio:
-	r1bio_pool_free(r1_bio, data);
+	rbio_pool_free(r1_bio, data);
 	return NULL;
 }
 
@@ -176,7 +171,7 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
 	/* resync pages array stored in the 1st bio's .bi_private */
 	kfree(rp);
 
-	r1bio_pool_free(r1bio, data);
+	rbio_pool_free(r1bio, data);
 }
 
 static void put_all_bios(struct r1conf *conf, struct r1bio *r1_bio)
@@ -2931,7 +2926,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		goto abort;
 	conf->poolinfo->raid_disks = mddev->raid_disks * 2;
 	err = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, r1bio_pool_alloc,
-			   r1bio_pool_free, conf->poolinfo);
+			   rbio_pool_free, conf->poolinfo);
 	if (err)
 		goto abort;
 
@@ -3216,7 +3211,7 @@ static int raid1_reshape(struct mddev *mddev)
 	newpoolinfo->raid_disks = raid_disks * 2;
 
 	ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
-			   r1bio_pool_free, newpoolinfo);
+			   rbio_pool_free, newpoolinfo);
 	if (ret) {
 		kfree(newpoolinfo);
 		return ret;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 24cb116d950f..c0d9ee4311d4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -107,11 +107,6 @@ static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
 	return kzalloc(size, gfp_flags);
 }
 
-static void r10bio_pool_free(void *r10_bio, void *data)
-{
-	kfree(r10_bio);
-}
-
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
 /* amount of memory to reserve for resync requests */
 #define RESYNC_WINDOW (1024*1024)
@@ -217,7 +212,7 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
 	}
 	kfree(rps);
 out_free_r10bio:
-	r10bio_pool_free(r10_bio, conf);
+	rbio_pool_free(r10_bio, conf);
 	return NULL;
 }
 
@@ -245,7 +240,7 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
 	/* resync pages array stored in the 1st bio's .bi_private */
 	kfree(rp);
 
-	r10bio_pool_free(r10bio, conf);
+	rbio_pool_free(r10bio, conf);
 }
 
 static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
@@ -3660,7 +3655,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	conf->geo = geo;
 	conf->copies = copies;
 	err = mempool_init(&conf->r10bio_pool, NR_RAID_BIOS, r10bio_pool_alloc,
-			   r10bio_pool_free, conf);
+			   rbio_pool_free, conf);
 	if (err)
 		goto out;
 
-- 
2.21.0

[PATCH 2/4] md: raid0: Remove return statement from void function

This return statement was introduced in commit
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2") and can be
safely removed.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index f3fb5bb8c82a..42b0287104bd 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -609,7 +609,6 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
 static void raid0_status(struct seq_file *seq, struct mddev *mddev)
 {
 	seq_printf(seq, " %dk chunks", mddev->chunk_sectors / 2);
-	return;
 }
 
 static void *raid0_takeover_raid45(struct mddev *mddev)
-- 
2.21.0

[PATCH 2/4] md: raid0: Remove return statement from void function

[Marcos Paulo de Souza]

This return statement was introduced in commit
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2") and can be
safely removed.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index f3fb5bb8c82a..42b0287104bd 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -609,7 +609,6 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
 static void raid0_status(struct seq_file *seq, struct mddev *mddev)
 {
 	seq_printf(seq, " %dk chunks", mddev->chunk_sectors / 2);
-	return;
 }
 
 static void *raid0_takeover_raid45(struct mddev *mddev)
-- 
2.21.0

[PATCH 3/4] md: raid0: Return md_integrity_register result directly

[Marcos Paulo de Souza]

Commit 0c35bd4723e4a39ba2da4c13a22cb97986ee10c8
("md: fix problems with freeing private data after ->run failure")
removed the check for the result of md_integrity_register, so we don't
need to store it anymore, so return it directly.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 42b0287104bd..e72255464c09 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -431,9 +431,7 @@ static int raid0_run(struct mddev *mddev)
 
 	dump_zones(mddev);
 
-	ret = md_integrity_register(mddev);
-
-	return ret;
+	return md_integrity_register(mddev);
 }
 
 static void raid0_free(struct mddev *mddev, void *priv)
-- 
2.21.0

[PATCH 3/4] md: raid0: Return md_integrity_register result directly

[Marcos Paulo de Souza]

Commit 0c35bd4723e4a39ba2da4c13a22cb97986ee10c8
("md: fix problems with freeing private data after ->run failure")
removed the check for the result of md_integrity_register, so we don't
need to store it anymore, so return it directly.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 42b0287104bd..e72255464c09 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -431,9 +431,7 @@ static int raid0_run(struct mddev *mddev)
 
 	dump_zones(mddev);
 
-	ret = md_integrity_register(mddev);
-
-	return ret;
+	return md_integrity_register(mddev);
 }
 
 static void raid0_free(struct mddev *mddev, void *priv)
-- 
2.21.0

[PATCH 4/4] md: raid0: Make ret local in raid0_run

[Marcos Paulo de Souza]

ret variable is only used in a specific situation, so make it local
instead of global.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e72255464c09..b28dbb797f76 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -364,7 +364,6 @@ static void raid0_free(struct mddev *mddev, void *priv);
 static int raid0_run(struct mddev *mddev)
 {
 	struct r0conf *conf;
-	int ret;
 
 	if (mddev->chunk_sectors == 0) {
 		pr_warn("md/raid0:%s: chunk size must be set.\n", mdname(mddev));
@@ -375,7 +374,7 @@ static int raid0_run(struct mddev *mddev)
 
 	/* if private is not null, we are here after takeover */
 	if (mddev->private == NULL) {
-		ret = create_strip_zones(mddev, &conf);
+		int ret = create_strip_zones(mddev, &conf);
 		if (ret < 0)
 			return ret;
 		mddev->private = conf;
-- 
2.21.0

[PATCH 4/4] md: raid0: Make ret local in raid0_run

[Marcos Paulo de Souza]

ret variable is only used in a specific situation, so make it local
instead of global.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/md/raid0.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e72255464c09..b28dbb797f76 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -364,7 +364,6 @@ static void raid0_free(struct mddev *mddev, void *priv);
 static int raid0_run(struct mddev *mddev)
 {
 	struct r0conf *conf;
-	int ret;
 
 	if (mddev->chunk_sectors == 0) {
 		pr_warn("md/raid0:%s: chunk size must be set.\n", mdname(mddev));
@@ -375,7 +374,7 @@ static int raid0_run(struct mddev *mddev)
 
 	/* if private is not null, we are here after takeover */
 	if (mddev->private == NULL) {
-		ret = create_strip_zones(mddev, &conf);
+		int ret = create_strip_zones(mddev, &conf);
 		if (ret < 0)
 			return ret;
 		mddev->private = conf;
-- 
2.21.0

Re: [PATCH 2/4] md: raid0: Remove return statement from void function

[Song Liu]

On Mon, May 20, 2019 at 2:45 PM Marcos Paulo de Souza
<marcos.souza.org@gmail.com> wrote:
>
> This return statement was introduced in commit
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2") and can be
> safely removed.

Wow, that's a really old commit. :)

I think 3/4 and 4/4 of the set makes git-blame more difficult to
follow. Let's not
apply them.

Thanks,
Song

>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
>  drivers/md/raid0.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index f3fb5bb8c82a..42b0287104bd 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -609,7 +609,6 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
>  static void raid0_status(struct seq_file *seq, struct mddev *mddev)
>  {
>         seq_printf(seq, " %dk chunks", mddev->chunk_sectors / 2);
> -       return;
>  }
>
>  static void *raid0_takeover_raid45(struct mddev *mddev)
> --
> 2.21.0
>

Re: [PATCH 2/4] md: raid0: Remove return statement from void function

[Song Liu]

On Mon, May 20, 2019 at 2:45 PM Marcos Paulo de Souza
<marcos.souza.org@gmail.com> wrote:
>
> This return statement was introduced in commit
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2") and can be
> safely removed.

Wow, that's a really old commit. :)

I think 3/4 and 4/4 of the set makes git-blame more difficult to
follow. Let's not
apply them.

Thanks,
Song

>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
>  drivers/md/raid0.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index f3fb5bb8c82a..42b0287104bd 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -609,7 +609,6 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
>  static void raid0_status(struct seq_file *seq, struct mddev *mddev)
>  {
>         seq_printf(seq, " %dk chunks", mddev->chunk_sectors / 2);
> -       return;
>  }
>
>  static void *raid0_takeover_raid45(struct mddev *mddev)
> --
> 2.21.0
>