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

[Guoqing Jiang <jgq516@xxxxxxxxx>]

On 5/26/21 2:32 PM, Song Liu wrote:
> On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang <jgq516@xxxxxxxxx> wrote:
>
>
> > --- 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.

> >                  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.

> >          spin_lock(&pers_lock);
> >          pers = find_pers(mddev->level, mddev->clevel);
> [...]
>
> > +
> > +       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