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

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

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?

[...]

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.

Thanks,
Song