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

[Guoqing Jiang <jgq516@gmail.com>]

On 5/27/21 12:00 AM, Song Liu wrote:
> On Wed, May 26, 2021 at 12:53 AM Guoqing Jiang <jgq516@gmail.com> wrote:
>>
>>
>> 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.
> I think it is better to handle it within this function. Does it have
> any downside
> to call bioset_integrity_free(&mddev->bio_set) here?
>
> [...]

md_run has to deal with failure path by call bioset_exit, which
already call bioset_integrity_free implicitly. Why the additional
call of bioset_integrity_free would be helpful? Or do you want
to remove bioset_exit from md_run as well?

>>>> +
>>>> +       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.
> iostats was on by default before this set, as we didn't check
> blk_queue_io_stat().
> So it is better to keep the same behavior.

Could you point the place where md enables iostats before the set?
I can't find relevant code for it.

Thanks,
Guoqing