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

On 5/27/21 12:00 AM, Song Liu wrote:

On Wed, May 26, 2021 at 12:53 AM Guoqing Jiang [off-list ref] wrote:

quoted

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

quoted

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.

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?

quoted

quoted

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.

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