Re: [PATCH v2] md: improve io stats accounting
From: Guoqing Jiang <hidden>
Date: 2020-07-02 13:55:22
On 7/2/20 12:54 PM, Artur Paszkiewicz wrote:
quoted hunk ↗ jump to hunk
Use generic io accounting functions to manage io stats. There was an attempt to do this earlier in commit 18c0b223cf99 ("md: use generic io stats accounting functions to simplify io stat accounting"), but it did not include a call to generic_end_io_acct() and caused issues with tracking in-flight IOs, so it was later removed in commit 74672d069b29 ("md: fix md io stats accounting broken"). This patch attempts to fix this by using both bio_start_io_acct() and bio_end_io_acct(). To make it possible, a struct md_io is allocated for every new md bio, which includes the io start_time. A new mempool is introduced for this purpose. We override bio->bi_end_io with our own callback and call bio_start_io_acct() before passing the bio to md_handle_request(). When it completes, we call bio_end_io_acct() and the original bi_end_io callback. This adds correct statistics about in-flight IOs and IO processing time, interpreted e.g. in iostat as await, svctm, aqu-sz and %util. It also fixes a situation where too many IOs where reported if a bio was re-submitted to the mddev, because io accounting is now performed only on newly arriving bios. Signed-off-by: Artur Paszkiewicz <redacted> --- v2: - Just override the bi_end_io without having to clone the original bio. - Rebased onto latest md-next. drivers/md/md.c | 56 ++++++++++++++++++++++++++++++++++++++----------- drivers/md/md.h | 1 + 2 files changed, 45 insertions(+), 12 deletions(-)diff --git a/drivers/md/md.c b/drivers/md/md.c index 8bb69c61afe0..25dd3f4116c3 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c@@ -463,12 +463,33 @@ void md_handle_request(struct mddev *mddev, struct bio *bio) } EXPORT_SYMBOL(md_handle_request); +struct md_io { + struct mddev *mddev; + bio_end_io_t *orig_bi_end_io; + void *orig_bi_private; + unsigned long start_time; +}; + +static void md_end_io(struct bio *bio) +{ + struct md_io *md_io = bio->bi_private; + struct mddev *mddev = md_io->mddev; + + bio_end_io_acct(bio, md_io->start_time); + + bio->bi_end_io = md_io->orig_bi_end_io; + bio->bi_private = md_io->orig_bi_private; + + mempool_free(md_io, &mddev->md_io_pool); + + if (bio->bi_end_io) + bio->bi_end_io(bio); +} + static blk_qc_t md_submit_bio(struct bio *bio) { const int rw = bio_data_dir(bio); - const int sgrp = op_stat_group(bio_op(bio)); struct mddev *mddev = bio->bi_disk->private_data; - unsigned int sectors; if (unlikely(test_bit(MD_BROKEN, &mddev->flags)) && (rw == WRITE)) { bio_io_error(bio);@@ -488,21 +509,26 @@ static blk_qc_t md_submit_bio(struct bio *bio) return BLK_QC_T_NONE; } - /* - * save the sectors now since our bio can - * go away inside make_request - */ - sectors = bio_sectors(bio); + if (bio->bi_end_io != md_end_io) { + struct md_io *md_io; + + md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO); + md_io->mddev = mddev; + md_io->start_time = jiffies; + md_io->orig_bi_end_io = bio->bi_end_io; + md_io->orig_bi_private = bio->bi_private; + + bio->bi_end_io = md_end_io; + bio->bi_private = md_io; + + bio_start_io_acct(bio);
It can just be "md_io->start_time = bio_start_io_acct(bio)", with that Acked-by: Guoqing Jiang <redacted> Thanks, Guoqing