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

[Guoqing Jiang <jgq516@gmail.com>]

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