[PATCH 0/2] incremental patches about io stats

[PATCH 0/2] incremental patches about io stats

[Guoqing Jiang]

Hi Song,

The first one addressed Christoph's comment, and I also add another one
to explain why error handling is not needed there. Please review.

Thanks,
Guoqing

Guoqing Jiang (2):
  md: check level before create and exit io_acct_set
  md: add comments in md_integrity_register

 drivers/md/md.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] md: check level before create and exit io_acct_set

[Guoqing Jiang]

The bio_set (io_acct_set) is used by personalities to clone bio and
trace the timestamp of bio. Some personalities such as raid1/10 don't
need the bio_set, so add check to not create it unconditionally.

Also update the comment for md_account_bio to make it more clear.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/md.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 32abcfb8bcad..56b606184c87 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2341,7 +2341,8 @@ int md_integrity_register(struct mddev *mddev)
 
 	pr_debug("md: data integrity enabled on %s\n", mdname(mddev));
 	if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE) ||
-	    bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE)) {
+	    (mddev->level != 1 && mddev->level != 10 &&
+	     bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE))) {
 		pr_err("md: failed to create integrity pool for %s\n",
 		       mdname(mddev));
 		return -EINVAL;
@@ -5570,7 +5571,8 @@ static void md_free(struct kobject *ko)
 
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
-	bioset_exit(&mddev->io_acct_set);
+	if (mddev->level != 1 && mddev->level != 10)
+		bioset_exit(&mddev->io_acct_set);
 	kfree(mddev);
 }
 
@@ -5866,7 +5868,8 @@ int md_run(struct mddev *mddev)
 		if (err)
 			goto exit_bio_set;
 	}
-	if (!bioset_initialized(&mddev->io_acct_set)) {
+	if (mddev->level != 1 && mddev->level != 10 &&
+	    !bioset_initialized(&mddev->io_acct_set)) {
 		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
 				  offsetof(struct md_io_acct, bio_clone), 0);
 		if (err)
@@ -6048,7 +6051,8 @@ int md_run(struct mddev *mddev)
 	module_put(pers->owner);
 	md_bitmap_destroy(mddev);
 abort:
-	bioset_exit(&mddev->io_acct_set);
+	if (mddev->level != 1 && mddev->level != 10)
+		bioset_exit(&mddev->io_acct_set);
 exit_sync_set:
 	bioset_exit(&mddev->sync_set);
 exit_bio_set:
@@ -6276,7 +6280,8 @@ void md_stop(struct mddev *mddev)
 	__md_stop(mddev);
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
-	bioset_exit(&mddev->io_acct_set);
+	if (mddev->level != 1 && mddev->level != 10)
+		bioset_exit(&mddev->io_acct_set);
 }
 
 EXPORT_SYMBOL_GPL(md_stop);
@@ -8593,7 +8598,10 @@ static void md_end_io_acct(struct bio *bio)
 	bio_endio(orig_bio);
 }
 
-/* used by personalities (raid0 and raid5) to account io stats */
+/*
+ * Used by personalities that don't already clone the bio and thus can't
+ * easily add the timestamp to their extended bio structure.
+ */
 void md_account_bio(struct mddev *mddev, struct bio **bio)
 {
 	struct md_io_acct *md_io_acct;
-- 
2.25.1

[PATCH 2/2] md: add comments in md_integrity_register

[Guoqing Jiang]

Given it is not obvious for the error handling, let's try to add some
comments here to make it clear.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/md.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 56b606184c87..2c69905dd5c0 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2343,6 +2343,12 @@ int md_integrity_register(struct mddev *mddev)
 	if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE) ||
 	    (mddev->level != 1 && mddev->level != 10 &&
 	     bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE))) {
+		/*
+		 * No need to handle the failure of bioset_integrity_create,
+		 * because the function is called by md_run() -> pers->run(),
+		 * md_run calls bioset_exit -> bioset_integrity_free in case
+		 * of failure case.
+		 */
 		pr_err("md: failed to create integrity pool for %s\n",
 		       mdname(mddev));
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH 0/2] incremental patches about io stats

[Song Liu]

On Thu, Jun 3, 2021 at 2:21 AM Guoqing Jiang <jgq516@gmail.com> wrote:
>
> Hi Song,
>
> The first one addressed Christoph's comment, and I also add another one
> to explain why error handling is not needed there. Please review.
>

Applied to md-next.Thanks!

Song