Re: [PATCH V3 2/8] md: add io accounting for raid0 and raid5

On 5/26/21 2:32 PM, Song Liu wrote:

On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang [off-list ref] wrote:

quoted

--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -2340,7 +2340,8 @@ int md_integrity_register(struct mddev *mddev)
                                bdev_get_integrity(reference->bdev));

         pr_debug("md: data integrity enabled on %s\n", mdname(mddev));
-       if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE)) {
+       if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE) ||
+           bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE)) {

Added better error handling here.

No need to do it here, because md_integrity_register is called from
md_run() -> pers->run(), if above returns failure, then the path
(bioset_exit -> bioset_integrity_free) is triggered.

I thought we probably need a comment here given it is not explicit.

quoted

                 pr_err("md: failed to create integrity pool for %s\n",
                        mdname(mddev));
                 return -EINVAL;

@@ -5569,6 +5570,7 @@ static void md_free(struct kobject *ko)

         bioset_exit(&mddev->bio_set);
         bioset_exit(&mddev->sync_set);
+       bioset_exit(&mddev->io_acct_set);
         kfree(mddev);
  }

@@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
                 if (err)
                         return err;
         }
+       if (!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)
+                       return err;
+       }

And here (for the other two bioset_initialized calls).

Yes, it looks correct to me.

quoted

         spin_lock(&pers_lock);
         pers = find_pers(mddev->level, mddev->clevel);

[...]

quoted

+
+       if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
+               return;

Added blk_queue_flag_set(QUEUE_FLAG_IO_STAT, mddev->queue); to md_run.
We still need it as md doesn't use mq. Without it, the default iostats is 0.

It enables io accounting by default, so raid5 and raid0 users have to
disable it if they don't want the additional latency.

Thanks,
Guoqing