| Message ID | 20230609094320.2397604-1-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | md/raid10: Only check QUEUE_FLAG_IO_STAT when issuing io | expand |
Hi, 在 2023/06/09 17:43, linan666@huaweicloud.com 写道: > From: Li Nan <linan122@huawei.com> > > /sys/block/[device]/queue/iostats is used to control whether to count io > stat. Write 0 to it will clear queue_flags QUEUE_FLAG_IO_STAT which means > iostats is disabled. If we disable iostats and later endable it, the io > issued during this period will be counted incorrectly, inflight will be > decreased to -1. > > //T1 set iostats > echo 0 > /sys/block/md0/queue/iostats > clear QUEUE_FLAG_IO_STAT > > //T2 issue io > if (QUEUE_FLAG_IO_STAT) -> false > bio_start_io_acct > inflight++ > > echo 1 > /sys/block/md0/queue/iostats > set QUEUE_FLAG_IO_STAT > > //T3 io end > if (QUEUE_FLAG_IO_STAT) -> true > bio_end_io_acct > inflight-- -> -1 > > Also, if iostats is enabled while issuing io but disabled while io end, > inflight will never be decreased. > > Fix it by checking start_time when io end. Only check QUEUE_FLAG_IO_STAT > while issuing io, just like request based devices. > > Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting") > Signed-off-by: Li Nan <linan122@huawei.com> > --- > drivers/md/raid10.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 381c21f7fb06..bf9dca5c25c3 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -325,7 +325,7 @@ static void raid_end_bio_io(struct r10bio *r10_bio) > if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) > bio->bi_status = BLK_STS_IOERR; > > - if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue)) > + if (r10_bio->start_time) > bio_end_io_acct(bio, r10_bio->start_time); This patch LGTM, can you change this for raid1 as well? raid0 and raid456 is using md_account_bio(), and they don't have such problem. Thanks, Kuai > bio_endio(bio); > /* >
在 2023/6/10 10:32, Yu Kuai 写道: > Hi, > > 在 2023/06/09 17:43, linan666@huaweicloud.com 写道: >> From: Li Nan <linan122@huawei.com> >> >> /sys/block/[device]/queue/iostats is used to control whether to count io >> stat. Write 0 to it will clear queue_flags QUEUE_FLAG_IO_STAT which means >> iostats is disabled. If we disable iostats and later endable it, the io >> issued during this period will be counted incorrectly, inflight will be >> decreased to -1. >> >> //T1 set iostats >> echo 0 > /sys/block/md0/queue/iostats >> clear QUEUE_FLAG_IO_STAT >> >> //T2 issue io >> if (QUEUE_FLAG_IO_STAT) -> false >> bio_start_io_acct >> inflight++ >> >> echo 1 > /sys/block/md0/queue/iostats >> set QUEUE_FLAG_IO_STAT >> >> //T3 io end >> if (QUEUE_FLAG_IO_STAT) -> true >> bio_end_io_acct >> inflight-- -> -1 >> >> Also, if iostats is enabled while issuing io but disabled while io end, >> inflight will never be decreased. >> >> Fix it by checking start_time when io end. Only check QUEUE_FLAG_IO_STAT >> while issuing io, just like request based devices. >> >> Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting") >> Signed-off-by: Li Nan <linan122@huawei.com> >> --- >> drivers/md/raid10.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c >> index 381c21f7fb06..bf9dca5c25c3 100644 >> --- a/drivers/md/raid10.c >> +++ b/drivers/md/raid10.c >> @@ -325,7 +325,7 @@ static void raid_end_bio_io(struct r10bio *r10_bio) >> if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) >> bio->bi_status = BLK_STS_IOERR; >> - if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue)) >> + if (r10_bio->start_time) >> bio_end_io_acct(bio, r10_bio->start_time); > > This patch LGTM, can you change this for raid1 as well? raid0 and > raid456 is using md_account_bio(), and they don't have such problem. > OK, I will fix raid1 later. Thanks for suggestion. > Thanks, > Kuai
On Fri, Jun 9, 2023 at 2:47 AM <linan666@huaweicloud.com> wrote: > > From: Li Nan <linan122@huawei.com> > > /sys/block/[device]/queue/iostats is used to control whether to count io > stat. Write 0 to it will clear queue_flags QUEUE_FLAG_IO_STAT which means > iostats is disabled. If we disable iostats and later endable it, the io > issued during this period will be counted incorrectly, inflight will be > decreased to -1. > > //T1 set iostats > echo 0 > /sys/block/md0/queue/iostats > clear QUEUE_FLAG_IO_STAT > > //T2 issue io > if (QUEUE_FLAG_IO_STAT) -> false > bio_start_io_acct > inflight++ > > echo 1 > /sys/block/md0/queue/iostats > set QUEUE_FLAG_IO_STAT > > //T3 io end > if (QUEUE_FLAG_IO_STAT) -> true > bio_end_io_acct > inflight-- -> -1 > > Also, if iostats is enabled while issuing io but disabled while io end, > inflight will never be decreased. > > Fix it by checking start_time when io end. Only check QUEUE_FLAG_IO_STAT > while issuing io, just like request based devices. > > Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting") > Signed-off-by: Li Nan <linan122@huawei.com> The subject and commit log is a little confusing. I updated it a little bit and applied to md-next. Thanks, Song > --- > drivers/md/raid10.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 381c21f7fb06..bf9dca5c25c3 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -325,7 +325,7 @@ static void raid_end_bio_io(struct r10bio *r10_bio) > if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) > bio->bi_status = BLK_STS_IOERR; > > - if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue)) > + if (r10_bio->start_time) > bio_end_io_acct(bio, r10_bio->start_time); > bio_endio(bio); > /* > -- > 2.39.2 >
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 381c21f7fb06..bf9dca5c25c3 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -325,7 +325,7 @@ static void raid_end_bio_io(struct r10bio *r10_bio) if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) bio->bi_status = BLK_STS_IOERR; - if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue)) + if (r10_bio->start_time) bio_end_io_acct(bio, r10_bio->start_time); bio_endio(bio); /*