On Wed, May 26, 2021 at 12:53 AM Guoqing Jiang <jgq516@xxxxxxxxx> wrote:
>
>
>
> 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.
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?
[...]
> >> +
> >> + 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
