[PATCH V3 0/6] md: io stats accounting

[PATCH V3 0/6] md: io stats accounting

[Guoqing Jiang]

V3 changes:

1. For raid0 and raid5, move clone bio into personality layers.
2. add two patches for raid5 to avoid copy chunk read bio twice.

V2 changes:

1. add accounting_bio to md_personality.
2. cleanup in case bioset_integrity_create fails.
3. use bio_end_io_acct.
4. remove patch for enable io accounting for multipath.
5. add one patch to rename print_msg.
6. add one patch to deprecate linear, multipath and faulty.

Guoqing Jiang (8):
  md: revert io stats accounting
  md: add io accounting for raid0 and raid5
  md/raid5: move checking badblock before clone bio in
    raid5_read_one_chunk
  md/raid5: avoid redundant bio clone in raid5_read_one_chunk
  md/raid1: rename print_msg with r1bio_existed
  md/raid1: enable io accounting
  md/raid10: enable io accounting
  md: mark some personalities as deprecated

 drivers/md/Kconfig        |  6 +--
 drivers/md/md-faulty.c    |  2 +-
 drivers/md/md-linear.c    |  2 +-
 drivers/md/md-multipath.c |  2 +-
 drivers/md/md.c           | 89 +++++++++++++++++++--------------------
 drivers/md/md.h           |  9 +++-
 drivers/md/raid0.c        |  3 ++
 drivers/md/raid1.c        | 15 +++++--
 drivers/md/raid1.h        |  1 +
 drivers/md/raid10.c       |  6 +++
 drivers/md/raid10.h       |  1 +
 drivers/md/raid5.c        | 34 +++++++++------
 12 files changed, 101 insertions(+), 69 deletions(-)

-- 
2.25.1

[PATCH V3 1/8] md: revert io stats accounting

[Guoqing Jiang]

The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
double fault problem per the report [1], and also it is not correct to
change ->bi_end_io if md don't own it, so let's revert it.

And io stats accounting will be replemented in later commits.

[1]. https://lore.kernel.org/linux-raid/3bf04253-3fad-434a-63a7-20214e38cf26@gmail.com/T/#t

Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/md.c | 45 ---------------------------------------------
 drivers/md/md.h |  1 -
 2 files changed, 46 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 49f897fbb89b..7ba00e4c862d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -441,30 +441,6 @@ 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;
-	struct block_device *orig_bi_bdev;
-	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_remapped(bio, md_io->start_time, md_io->orig_bi_bdev);
-
-	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);
@@ -489,21 +465,6 @@ static blk_qc_t md_submit_bio(struct bio *bio)
 		return BLK_QC_T_NONE;
 	}
 
-	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->orig_bi_end_io = bio->bi_end_io;
-		md_io->orig_bi_private = bio->bi_private;
-		md_io->orig_bi_bdev = bio->bi_bdev;
-
-		bio->bi_end_io = md_end_io;
-		bio->bi_private = md_io;
-
-		md_io->start_time = bio_start_io_acct(bio);
-	}
-
 	/* bio could be mergeable after passing to underlayer */
 	bio->bi_opf &= ~REQ_NOMERGE;
 
@@ -5608,7 +5569,6 @@ static void md_free(struct kobject *ko)
 
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
-	mempool_exit(&mddev->md_io_pool);
 	kfree(mddev);
 }
 
@@ -5705,11 +5665,6 @@ static int md_alloc(dev_t dev, char *name)
 		 */
 		mddev->hold_active = UNTIL_STOP;
 
-	error = mempool_init_kmalloc_pool(&mddev->md_io_pool, BIO_POOL_SIZE,
-					  sizeof(struct md_io));
-	if (error)
-		goto abort;
-
 	error = -ENOMEM;
 	mddev->queue = blk_alloc_queue(NUMA_NO_NODE);
 	if (!mddev->queue)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index fb7eab58cfd5..4da240ffe2c5 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -487,7 +487,6 @@ struct mddev {
 	struct bio_set			sync_set; /* for sync operations like
 						   * metadata and bitmap writes
 						   */
-	mempool_t			md_io_pool;
 
 	/* Generic flush handling.
 	 * The last to finish preflush schedules a worker to submit
-- 
2.25.1

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

[Guoqing Jiang]

We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
don't own clone infrastructure to accounting io. And the bioset is added
to mddev instead of to raid0 and raid5 layer, because with this way, we
can put common functions to md.h and reuse them in raid0 and raid5.

Also struct md_io_acct is added accordingly which includes io start_time,
the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
to get related io status.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/md.c    | 44 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/md/md.h    |  8 ++++++++
 drivers/md/raid0.c |  3 +++
 drivers/md/raid5.c |  9 +++++++++
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 7ba00e4c862d..87786f180525 100644
--- 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)) {
 		pr_err("md: failed to create integrity pool for %s\n",
 		       mdname(mddev));
 		return -EINVAL;
@@ -5569,6 +5570,7 @@ static void md_free(struct kobject *ko)
 
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
+	bioset_exit(&mddev->io_acct_set);
 	kfree(mddev);
 }
 
@@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
 		if (err)
 			return err;
 	}
+	if (!bioset_initialized(&mddev->io_acct_set)) {
+		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
+				  offsetof(struct md_io_acct, bio_clone), 0);
+		if (err)
+			return err;
+	}
 
 	spin_lock(&pers_lock);
 	pers = find_pers(mddev->level, mddev->clevel);
@@ -6041,6 +6049,7 @@ int md_run(struct mddev *mddev)
 abort:
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
+	bioset_exit(&mddev->io_acct_set);
 	return err;
 }
 EXPORT_SYMBOL_GPL(md_run);
@@ -6264,6 +6273,7 @@ void md_stop(struct mddev *mddev)
 	__md_stop(mddev);
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
+	bioset_exit(&mddev->io_acct_set);
 }
 
 EXPORT_SYMBOL_GPL(md_stop);
@@ -8568,6 +8578,38 @@ void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
 }
 EXPORT_SYMBOL_GPL(md_submit_discard_bio);
 
+static void md_end_io_acct(struct bio *bio)
+{
+	struct md_io_acct *md_io_acct = bio->bi_private;
+	struct bio *orig_bio = md_io_acct->orig_bio;
+
+	orig_bio->bi_status = bio->bi_status;
+
+	bio_end_io_acct(orig_bio, md_io_acct->start_time);
+	bio_put(bio);
+	bio_endio(orig_bio);
+}
+
+/* used by personalities (raid0 and raid5) to account io stats */
+void md_account_bio(struct mddev *mddev, struct bio **bio)
+{
+	struct md_io_acct *md_io_acct;
+	struct bio *clone;
+
+	if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
+		return;
+
+	clone = bio_clone_fast(*bio, GFP_NOIO, &mddev->io_acct_set);
+	md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
+	md_io_acct->orig_bio = *bio;
+	md_io_acct->start_time = bio_start_io_acct(*bio);
+
+	clone->bi_end_io = md_end_io_acct;
+	clone->bi_private = md_io_acct;
+	*bio = clone;
+}
+EXPORT_SYMBOL_GPL(md_account_bio);
+
 /* md_allow_write(mddev)
  * Calling this ensures that the array is marked 'active' so that writes
  * may proceed without blocking.  It is important to call this before
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 4da240ffe2c5..4191f22acce4 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -487,6 +487,7 @@ struct mddev {
 	struct bio_set			sync_set; /* for sync operations like
 						   * metadata and bitmap writes
 						   */
+	struct bio_set			io_acct_set; /* for raid0 and raid5 io accounting */
 
 	/* Generic flush handling.
 	 * The last to finish preflush schedules a worker to submit
@@ -683,6 +684,12 @@ struct md_thread {
 	void			*private;
 };
 
+struct md_io_acct {
+	struct bio *orig_bio;
+	unsigned long start_time;
+	struct bio bio_clone;
+};
+
 #define THREAD_WAKEUP  0
 
 static inline void safe_put_page(struct page *p)
@@ -714,6 +721,7 @@ extern void md_error(struct mddev *mddev, struct md_rdev *rdev);
 extern void md_finish_reshape(struct mddev *mddev);
 void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
 			struct bio *bio, sector_t start, sector_t size);
+void md_account_bio(struct mddev *mddev, struct bio **bio);
 
 extern bool __must_check md_flush_request(struct mddev *mddev, struct bio *bio);
 extern void md_super_write(struct mddev *mddev, struct md_rdev *rdev,
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e5d7411cba9b..62c8b6adac70 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -546,6 +546,9 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
 		bio = split;
 	}
 
+	if (bio->bi_pool != &mddev->bio_set)
+		md_account_bio(mddev, &bio);
+
 	orig_sector = sector;
 	zone = find_zone(mddev->private, &sector);
 	switch (conf->layout) {
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 841e1c1aa5e6..58e9dbc0f683 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5468,6 +5468,7 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 	sector_t sector = raid_bio->bi_iter.bi_sector;
 	unsigned chunk_sects = mddev->chunk_sectors;
 	unsigned sectors = chunk_sects - (sector & (chunk_sects-1));
+	struct r5conf *conf = mddev->private;
 
 	if (sectors < bio_sectors(raid_bio)) {
 		struct r5conf *conf = mddev->private;
@@ -5477,6 +5478,9 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 		raid_bio = split;
 	}
 
+	if (raid_bio->bi_pool != &conf->bio_split)
+		md_account_bio(mddev, &raid_bio);
+
 	if (!raid5_read_one_chunk(mddev, raid_bio))
 		return raid_bio;
 
@@ -5756,6 +5760,7 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	DEFINE_WAIT(w);
 	bool do_prepare;
 	bool do_flush = false;
+	bool do_clone = false;
 
 	if (unlikely(bi->bi_opf & REQ_PREFLUSH)) {
 		int ret = log_handle_flush_request(conf, bi);
@@ -5784,6 +5789,7 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	if (rw == READ && mddev->degraded == 0 &&
 	    mddev->reshape_position == MaxSector) {
 		bi = chunk_aligned_read(mddev, bi);
+		do_clone = true;
 		if (!bi)
 			return true;
 	}
@@ -5798,6 +5804,9 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	last_sector = bio_end_sector(bi);
 	bi->bi_next = NULL;
 
+	if (!do_clone)
+		md_account_bio(mddev, &bi);
+
 	prepare_to_wait(&conf->wait_for_overlap, &w, TASK_UNINTERRUPTIBLE);
 	for (; logical_sector < last_sector; logical_sector += RAID5_STRIPE_SECTORS(conf)) {
 		int previous;
-- 
2.25.1

[PATCH V3 3/8] md/raid5: move checking badblock before clone bio in raid5_read_one_chunk

[Guoqing Jiang]

We don't need to clone bio if the relevant region has badblock.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/raid5.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 58e9dbc0f683..5a05277f4be7 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5427,6 +5427,13 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
 	atomic_inc(&rdev->nr_pending);
 	rcu_read_unlock();
 
+	if (is_badblock(rdev, sector, bio_sectors(raid_bio), &first_bad,
+			&bad_sectors)) {
+		bio_put(raid_bio);
+		rdev_dec_pending(rdev, mddev);
+		return 0;
+	}
+
 	align_bio = bio_clone_fast(raid_bio, GFP_NOIO, &mddev->bio_set);
 	bio_set_dev(align_bio, rdev->bdev);
 	align_bio->bi_end_io = raid5_align_endio;
@@ -5435,13 +5442,6 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
 
 	raid_bio->bi_next = (void *)rdev;
 
-	if (is_badblock(rdev, sector, bio_sectors(align_bio), &first_bad,
-			&bad_sectors)) {
-		bio_put(align_bio);
-		rdev_dec_pending(rdev, mddev);
-		return 0;
-	}
-
 	/* No reshape active, so we can trust rdev->data_offset */
 	align_bio->bi_iter.bi_sector += rdev->data_offset;
 
-- 
2.25.1

[PATCH V3 4/8] md/raid5: avoid redundant bio clone in raid5_read_one_chunk

[Guoqing Jiang]

After enable io accounting, chunk read bio could be cloned twice which
is not good. To avoid such inefficiency, let's clone align_bio from
io_acct_set too, then we need only call md_account_bio in make_request
unconditionally.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/raid5.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 5a05277f4be7..f83623ac8c34 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5364,11 +5364,13 @@ static struct bio *remove_bio_from_retry(struct r5conf *conf,
  */
 static void raid5_align_endio(struct bio *bi)
 {
-	struct bio* raid_bi  = bi->bi_private;
+	struct md_io_acct *md_io_acct = bi->bi_private;
+	struct bio *raid_bi = md_io_acct->orig_bio;
 	struct mddev *mddev;
 	struct r5conf *conf;
 	struct md_rdev *rdev;
 	blk_status_t error = bi->bi_status;
+	unsigned long start_time = md_io_acct->start_time;
 
 	bio_put(bi);
 
@@ -5380,6 +5382,8 @@ static void raid5_align_endio(struct bio *bi)
 	rdev_dec_pending(rdev, conf->mddev);
 
 	if (!error) {
+		if (blk_queue_io_stat(raid_bi->bi_bdev->bd_disk->queue))
+			bio_end_io_acct(raid_bi, start_time);
 		bio_endio(raid_bi);
 		if (atomic_dec_and_test(&conf->active_aligned_reads))
 			wake_up(&conf->wait_for_quiescent);
@@ -5398,6 +5402,7 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
 	struct md_rdev *rdev;
 	sector_t sector, end_sector, first_bad;
 	int bad_sectors, dd_idx;
+	struct md_io_acct *md_io_acct;
 
 	if (!in_chunk_boundary(mddev, raid_bio)) {
 		pr_debug("%s: non aligned\n", __func__);
@@ -5434,14 +5439,18 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
 		return 0;
 	}
 
-	align_bio = bio_clone_fast(raid_bio, GFP_NOIO, &mddev->bio_set);
+	align_bio = bio_clone_fast(raid_bio, GFP_NOIO, &mddev->io_acct_set);
+	md_io_acct = container_of(align_bio, struct md_io_acct, bio_clone);
+	raid_bio->bi_next = (void *)rdev;
+	if (blk_queue_io_stat(raid_bio->bi_bdev->bd_disk->queue))
+		md_io_acct->start_time = bio_start_io_acct(raid_bio);
+	md_io_acct->orig_bio = raid_bio;
+
 	bio_set_dev(align_bio, rdev->bdev);
 	align_bio->bi_end_io = raid5_align_endio;
-	align_bio->bi_private = raid_bio;
+	align_bio->bi_private = md_io_acct;
 	align_bio->bi_iter.bi_sector = sector;
 
-	raid_bio->bi_next = (void *)rdev;
-
 	/* No reshape active, so we can trust rdev->data_offset */
 	align_bio->bi_iter.bi_sector += rdev->data_offset;
 
@@ -5468,7 +5477,6 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 	sector_t sector = raid_bio->bi_iter.bi_sector;
 	unsigned chunk_sects = mddev->chunk_sectors;
 	unsigned sectors = chunk_sects - (sector & (chunk_sects-1));
-	struct r5conf *conf = mddev->private;
 
 	if (sectors < bio_sectors(raid_bio)) {
 		struct r5conf *conf = mddev->private;
@@ -5478,9 +5486,6 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 		raid_bio = split;
 	}
 
-	if (raid_bio->bi_pool != &conf->bio_split)
-		md_account_bio(mddev, &raid_bio);
-
 	if (!raid5_read_one_chunk(mddev, raid_bio))
 		return raid_bio;
 
@@ -5760,7 +5765,6 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	DEFINE_WAIT(w);
 	bool do_prepare;
 	bool do_flush = false;
-	bool do_clone = false;
 
 	if (unlikely(bi->bi_opf & REQ_PREFLUSH)) {
 		int ret = log_handle_flush_request(conf, bi);
@@ -5789,7 +5793,6 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	if (rw == READ && mddev->degraded == 0 &&
 	    mddev->reshape_position == MaxSector) {
 		bi = chunk_aligned_read(mddev, bi);
-		do_clone = true;
 		if (!bi)
 			return true;
 	}
@@ -5804,9 +5807,7 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	last_sector = bio_end_sector(bi);
 	bi->bi_next = NULL;
 
-	if (!do_clone)
-		md_account_bio(mddev, &bi);
-
+	md_account_bio(mddev, &bi);
 	prepare_to_wait(&conf->wait_for_overlap, &w, TASK_UNINTERRUPTIBLE);
 	for (; logical_sector < last_sector; logical_sector += RAID5_STRIPE_SECTORS(conf)) {
 		int previous;
-- 
2.25.1

[PATCH V3 5/8] md/raid1: rename print_msg with r1bio_existed

[Guoqing Jiang]

The caller of raid1_read_request could pass NULL or a valid pointer for
"struct r1bio *r1_bio", so it actually means whether r1_bio is existed
or not.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/raid1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index ced076ba560e..696da6b8b7ed 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1210,7 +1210,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	const unsigned long do_sync = (bio->bi_opf & REQ_SYNC);
 	int max_sectors;
 	int rdisk;
-	bool print_msg = !!r1_bio;
+	bool r1bio_existed = !!r1_bio;
 	char b[BDEVNAME_SIZE];
 
 	/*
@@ -1220,7 +1220,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	 */
 	gfp_t gfp = r1_bio ? (GFP_NOIO | __GFP_HIGH) : GFP_NOIO;
 
-	if (print_msg) {
+	if (r1bio_existed) {
 		/* Need to get the block device name carefully */
 		struct md_rdev *rdev;
 		rcu_read_lock();
@@ -1252,7 +1252,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 
 	if (rdisk < 0) {
 		/* couldn't find anywhere to read from */
-		if (print_msg) {
+		if (r1bio_existed) {
 			pr_crit_ratelimited("md/raid1:%s: %s: unrecoverable I/O read error for block %llu\n",
 					    mdname(mddev),
 					    b,
@@ -1263,7 +1263,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	}
 	mirror = conf->mirrors + rdisk;
 
-	if (print_msg)
+	if (r1bio_existed)
 		pr_info_ratelimited("md/raid1:%s: redirecting sector %llu to other mirror: %s\n",
 				    mdname(mddev),
 				    (unsigned long long)r1_bio->sector,
-- 
2.25.1

[PATCH V3 6/8] md/raid1: enable io accounting

[Guoqing Jiang]

For raid1, we record the start time between split bio and clone bio,
and finish the accounting in the final endio.

Also introduce start_time in r1bio accordingly.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/raid1.c | 7 +++++++
 drivers/md/raid1.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 696da6b8b7ed..51f2547c2007 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -300,6 +300,8 @@ static void call_bio_endio(struct r1bio *r1_bio)
 	if (!test_bit(R1BIO_Uptodate, &r1_bio->state))
 		bio->bi_status = BLK_STS_IOERR;
 
+	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+		bio_end_io_acct(bio, r1_bio->start_time);
 	bio_endio(bio);
 }
 
@@ -1292,6 +1294,9 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 
 	r1_bio->read_disk = rdisk;
 
+	if (!r1bio_existed && blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+		r1_bio->start_time = bio_start_io_acct(bio);
+
 	read_bio = bio_clone_fast(bio, gfp, &mddev->bio_set);
 
 	r1_bio->bios[rdisk] = read_bio;
@@ -1461,6 +1466,8 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		r1_bio->sectors = max_sectors;
 	}
 
+	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+		r1_bio->start_time = bio_start_io_acct(bio);
 	atomic_set(&r1_bio->remaining, 1);
 	atomic_set(&r1_bio->behind_remaining, 0);
 
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index b7eb09e8c025..ccf10e59b116 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -158,6 +158,7 @@ struct r1bio {
 	sector_t		sector;
 	int			sectors;
 	unsigned long		state;
+	unsigned long		start_time;
 	struct mddev		*mddev;
 	/*
 	 * original bio going to /dev/mdx
-- 
2.25.1

[PATCH V3 7/8] md/raid10: enable io accounting

[Guoqing Jiang]

For raid10, we record the start time between split bio and clone bio,
and finish the accounting in the final endio.

Also introduce start_time in r10bio accordingly.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/raid10.c | 6 ++++++
 drivers/md/raid10.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 13f5e6b2a73d..16977e8e075d 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -297,6 +297,8 @@ 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))
+		bio_end_io_acct(bio, r10_bio->start_time);
 	bio_endio(bio);
 	/*
 	 * Wake up any possible resync thread that waits for the device
@@ -1184,6 +1186,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
 	}
 	slot = r10_bio->read_slot;
 
+	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+		r10_bio->start_time = bio_start_io_acct(bio);
 	read_bio = bio_clone_fast(bio, gfp, &mddev->bio_set);
 
 	r10_bio->devs[slot].bio = read_bio;
@@ -1483,6 +1487,8 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
 		r10_bio->master_bio = bio;
 	}
 
+	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+		r10_bio->start_time = bio_start_io_acct(bio);
 	atomic_set(&r10_bio->remaining, 1);
 	md_bitmap_startwrite(mddev->bitmap, r10_bio->sector, r10_bio->sectors, 0);
 
diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
index 1461fd55311b..c34bb196790e 100644
--- a/drivers/md/raid10.h
+++ b/drivers/md/raid10.h
@@ -124,6 +124,7 @@ struct r10bio {
 	sector_t		sector;	/* virtual sector number */
 	int			sectors;
 	unsigned long		state;
+	unsigned long		start_time;
 	struct mddev		*mddev;
 	/*
 	 * original bio going to /dev/mdx
-- 
2.25.1

[PATCH V3 8/8] md: mark some personalities as deprecated

[Guoqing Jiang]

Mark the three personalities (linear, fault and multipath) as deprecated
because:

1. people can use dm multipath or nvme multipath.
2. linear is already deprecated in MODULE_ALIAS.
3. no one actively using fault.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
---
 drivers/md/Kconfig        | 6 +++---
 drivers/md/md-faulty.c    | 2 +-
 drivers/md/md-linear.c    | 2 +-
 drivers/md/md-multipath.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index f2014385d48b..0602e82a9516 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -47,7 +47,7 @@ config MD_AUTODETECT
 	  If unsure, say Y.
 
 config MD_LINEAR
-	tristate "Linear (append) mode"
+	tristate "Linear (append) mode (deprecated)"
 	depends on BLK_DEV_MD
 	help
 	  If you say Y here, then your multiple devices driver will be able to
@@ -158,7 +158,7 @@ config MD_RAID456
 	  If unsure, say Y.
 
 config MD_MULTIPATH
-	tristate "Multipath I/O support"
+	tristate "Multipath I/O support (deprecated)"
 	depends on BLK_DEV_MD
 	help
 	  MD_MULTIPATH provides a simple multi-path personality for use
@@ -169,7 +169,7 @@ config MD_MULTIPATH
 	  If unsure, say N.
 
 config MD_FAULTY
-	tristate "Faulty test module for MD"
+	tristate "Faulty test module for MD (deprecated)"
 	depends on BLK_DEV_MD
 	help
 	  The "faulty" module allows for a block device that occasionally returns
diff --git a/drivers/md/md-faulty.c b/drivers/md/md-faulty.c
index fda4cb3f936f..c0dc6f2ef4a3 100644
--- a/drivers/md/md-faulty.c
+++ b/drivers/md/md-faulty.c
@@ -357,7 +357,7 @@ static void raid_exit(void)
 module_init(raid_init);
 module_exit(raid_exit);
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("Fault injection personality for MD");
+MODULE_DESCRIPTION("Fault injection personality for MD (deprecated)");
 MODULE_ALIAS("md-personality-10"); /* faulty */
 MODULE_ALIAS("md-faulty");
 MODULE_ALIAS("md-level--5");
diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
index 63ed8329a98d..1ff51647a682 100644
--- a/drivers/md/md-linear.c
+++ b/drivers/md/md-linear.c
@@ -312,7 +312,7 @@ static void linear_exit (void)
 module_init(linear_init);
 module_exit(linear_exit);
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("Linear device concatenation personality for MD");
+MODULE_DESCRIPTION("Linear device concatenation personality for MD (deprecated)");
 MODULE_ALIAS("md-personality-1"); /* LINEAR - deprecated*/
 MODULE_ALIAS("md-linear");
 MODULE_ALIAS("md-level--1");
diff --git a/drivers/md/md-multipath.c b/drivers/md/md-multipath.c
index 776bbe542db5..e7d6486f090f 100644
--- a/drivers/md/md-multipath.c
+++ b/drivers/md/md-multipath.c
@@ -471,7 +471,7 @@ static void __exit multipath_exit (void)
 module_init(multipath_init);
 module_exit(multipath_exit);
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("simple multi-path personality for MD");
+MODULE_DESCRIPTION("simple multi-path personality for MD (deprecated)");
 MODULE_ALIAS("md-personality-7"); /* MULTIPATH */
 MODULE_ALIAS("md-multipath");
 MODULE_ALIAS("md-level--4");
-- 
2.25.1

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

[Song Liu]

On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang <jgq516@gmail.com> wrote:
>
> We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
> don't own clone infrastructure to accounting io. And the bioset is added
> to mddev instead of to raid0 and raid5 layer, because with this way, we
> can put common functions to md.h and reuse them in raid0 and raid5.
>
> Also struct md_io_acct is added accordingly which includes io start_time,
> the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
> to get related io status.
>
> Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>

Applied the set to md-next, with some changes on this one. Please take a look at
these changes.

Thanks,
Song

[...]

> --- 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.

>                 pr_err("md: failed to create integrity pool for %s\n",
>                        mdname(mddev));
>                 return -EINVAL;
> @@ -5569,6 +5570,7 @@ static void md_free(struct kobject *ko)
>
>         bioset_exit(&mddev->bio_set);
>         bioset_exit(&mddev->sync_set);
> +       bioset_exit(&mddev->io_acct_set);
>         kfree(mddev);
>  }
>
> @@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
>                 if (err)
>                         return err;
>         }
> +       if (!bioset_initialized(&mddev->io_acct_set)) {
> +               err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
> +                                 offsetof(struct md_io_acct, bio_clone), 0);
> +               if (err)
> +                       return err;
> +       }

And here (for the other two bioset_initialized calls).

>
>         spin_lock(&pers_lock);
>         pers = find_pers(mddev->level, mddev->clevel);

[...]

> +
> +       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.

[...]

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

[Guoqing Jiang]

On 5/26/21 2:32 PM, Song Liu wrote:
> On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang <jgq516@gmail.com> 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.

>
>>                  pr_err("md: failed to create integrity pool for %s\n",
>>                         mdname(mddev));
>>                  return -EINVAL;
>> @@ -5569,6 +5570,7 @@ static void md_free(struct kobject *ko)
>>
>>          bioset_exit(&mddev->bio_set);
>>          bioset_exit(&mddev->sync_set);
>> +       bioset_exit(&mddev->io_acct_set);
>>          kfree(mddev);
>>   }
>>
>> @@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
>>                  if (err)
>>                          return err;
>>          }
>> +       if (!bioset_initialized(&mddev->io_acct_set)) {
>> +               err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
>> +                                 offsetof(struct md_io_acct, bio_clone), 0);
>> +               if (err)
>> +                       return err;
>> +       }
> And here (for the other two bioset_initialized calls).

Yes, it looks correct to me.

>
>>          spin_lock(&pers_lock);
>>          pers = find_pers(mddev->level, mddev->clevel);
> [...]
>
>> +
>> +       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.

Thanks,
Guoqing

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

[Song Liu]

On Wed, May 26, 2021 at 12:53 AM Guoqing Jiang <jgq516@gmail.com> wrote:
>
>
>
> On 5/26/21 2:32 PM, Song Liu wrote:
> > On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang <jgq516@gmail.com> 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

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

[Guoqing Jiang]

On 5/27/21 12:00 AM, Song Liu wrote:
> On Wed, May 26, 2021 at 12:53 AM Guoqing Jiang <jgq516@gmail.com> wrote:
>>
>>
>> On 5/26/21 2:32 PM, Song Liu wrote:
>>> On Tue, May 25, 2021 at 2:47 AM Guoqing Jiang <jgq516@gmail.com> 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?
>
> [...]

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?

>>>> +
>>>> +       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

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

[Song Liu]

On Wed, May 26, 2021 at 7:00 PM Guoqing Jiang <jgq516@gmail.com> wrote:
>

[...]

> >> (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?

I guess you are right. Removing this one.

>
> >>>> +
> >>>> +       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.

Before this set, we did not set QUEUE_FLAG_IO_STAT, and we didn't
check blk_queue_io_stat() either. So even with /sys/block/mdX/queue/iostats
of 0, we still get iostats for the md device. By setting QUEUE_FLAG_IO_STAT
with the patch, the users still get iostats working by default.

Does this make sense?
Thanks,
Song

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

[Guoqing Jiang]

Hi Song,

On 5/27/21 2:14 PM, Song Liu wrote:
>> Could you point the place where md enables iostats before the set?
>> I can't find relevant code for it.
> Before this set, we did not set QUEUE_FLAG_IO_STAT, and we didn't
> check blk_queue_io_stat() either. So even with /sys/block/mdX/queue/iostats
> of 0, we still get iostats for the md device. By setting QUEUE_FLAG_IO_STAT
> with the patch, the users still get iostats working by default.

I am okay with enable it by default,  though users need to know the
parameter given it affects the performance.

Thanks,
Guoqing

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

[Christoph Hellwig]

On Tue, May 25, 2021 at 05:46:17PM +0800, Guoqing Jiang wrote:
> We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
> don't own clone infrastructure to accounting io. And the bioset is added
> to mddev instead of to raid0 and raid5 layer, because with this way, we
> can put common functions to md.h and reuse them in raid0 and raid5.
> 
> Also struct md_io_acct is added accordingly which includes io start_time,
> the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
> to get related io status.
> 
> Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
> ---
>  drivers/md/md.c    | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/md/md.h    |  8 ++++++++
>  drivers/md/raid0.c |  3 +++
>  drivers/md/raid5.c |  9 +++++++++
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 7ba00e4c862d..87786f180525 100644
> --- 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)) {

Don't we need to create this new only for raid0 and raid5?
Shouldn't they call helpers to create it?

> @@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
>  		if (err)
>  			return err;
>  	}
> +	if (!bioset_initialized(&mddev->io_acct_set)) {
> +		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
> +				  offsetof(struct md_io_acct, bio_clone), 0);
> +		if (err)
> +			return err;
> +	}

Can someone explain why we are having these bioset_initialized checks
here (also for the existing one)?  This just smells like very sloppy
life time rules.

> +/* used by personalities (raid0 and raid5) to account io stats */

Instead of mentioning the personalities this migt better explain
something like ".. by personalities that don't already clone the
bio and thus can't easily add the timestamp to their extended bio
structure"

> +void md_account_bio(struct mddev *mddev, struct bio **bio)
> +{
> +	struct md_io_acct *md_io_acct;
> +	struct bio *clone;
> +
> +	if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
> +		return;
> +
> +	clone = bio_clone_fast(*bio, GFP_NOIO, &mddev->io_acct_set);
> +	md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
> +	md_io_acct->orig_bio = *bio;
> +	md_io_acct->start_time = bio_start_io_acct(*bio);
> +
> +	clone->bi_end_io = md_end_io_acct;
> +	clone->bi_private = md_io_acct;
> +	*bio = clone;

I would find a calling conventions that returns the allocated clone
(or the original bio if there is no accounting) more logical.

> +	struct bio_set			io_acct_set; /* for raid0 and raid5 io accounting */

crazy long line.

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

[Guoqing Jiang]

On 5/27/21 11:25 PM, Christoph Hellwig wrote:

>   
>   	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)) {
> Don't we need to create this new only for raid0 and raid5?
> Shouldn't they call helpers to create it?

Good catch, will add a check for level.

>> @@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
>>   		if (err)
>>   			return err;
>>   	}
>> +	if (!bioset_initialized(&mddev->io_acct_set)) {
>> +		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
>> +				  offsetof(struct md_io_acct, bio_clone), 0);
>> +		if (err)
>> +			return err;
>> +	}
> Can someone explain why we are having these bioset_initialized checks
> here (also for the existing one)?  This just smells like very sloppy
> life time rules.

My understanding is that md_run is not only called when array is
created/assembled, for example, it can also be called in md_ioctl,
which means you can't call bioset_init unconditionally. Others may
have better explanation.

BTW, besides md, dm is another user of bioset_initialized.

>> +/* used by personalities (raid0 and raid5) to account io stats */
> Instead of mentioning the personalities this migt better explain
> something like ".. by personalities that don't already clone the
> bio and thus can't easily add the timestamp to their extended bio
> structure"

Ok, thanks for rephrasing.

>> +void md_account_bio(struct mddev *mddev, struct bio **bio)
>> +{
>> +	struct md_io_acct *md_io_acct;
>> +	struct bio *clone;
>> +
>> +	if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
>> +		return;
>> +
>> +	clone = bio_clone_fast(*bio, GFP_NOIO, &mddev->io_acct_set);
>> +	md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
>> +	md_io_acct->orig_bio = *bio;
>> +	md_io_acct->start_time = bio_start_io_acct(*bio);
>> +
>> +	clone->bi_end_io = md_end_io_acct;
>> +	clone->bi_private = md_io_acct;
>> +	*bio = clone;
> I would find a calling conventions that returns the allocated clone
> (or the original bio if there is no accounting) more logical.

Not sure if I follow, do you want the function return "struct bio *"
instead of "void"? I don't think there is fundamental difference
with current behavior.

>> +	struct bio_set			io_acct_set; /* for raid0 and raid5 io accounting */
> crazy long line.

At lease it aligns with above line and checkpatch doesn't complain
either.

Thanks,
Guoqing

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

[Guoqing Jiang]

From: Guoqing Jiang <jgq516@gmail.com>

We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
don't own clone infrastructure to accounting io. And the bioset is added
to mddev instead of to raid0 and raid5 layer, because with this way, we
can put common functions to md.h and reuse them in raid0 and raid5.

Also struct md_io_acct is added accordingly which includes io start_time,
the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
to get related io status.

Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
Signed-off-by: Song Liu <song@kernel.org>
---
Hi Song,

Please consider apply the updated patch which has minor changes based on
Christoph's comment.

1. don't create io_acct_set for raid1 and raid10.
2. update comment for md_account_bio.

Thanks,
Guoqing

 drivers/md/md.c    | 55 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/md/md.h    |  8 +++++++
 drivers/md/raid0.c |  3 +++
 drivers/md/raid5.c |  9 ++++++++
 4 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 7ba00e4c862d..17a4aa8271f7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2340,7 +2340,9 @@ 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) ||
+	    (mddev->level != 1 && mddev->level != 10 &&
+	     bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE))) {
 		pr_err("md: failed to create integrity pool for %s\n",
 		       mdname(mddev));
 		return -EINVAL;
@@ -5569,6 +5571,7 @@ static void md_free(struct kobject *ko)
 
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
+	bioset_exit(&mddev->io_acct_set);
 	kfree(mddev);
 }
 
@@ -5862,7 +5865,13 @@ int md_run(struct mddev *mddev)
 	if (!bioset_initialized(&mddev->sync_set)) {
 		err = bioset_init(&mddev->sync_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
 		if (err)
-			return err;
+			goto exit_bio_set;
+	}
+	if (!bioset_initialized(&mddev->io_acct_set)) {
+		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
+				  offsetof(struct md_io_acct, bio_clone), 0);
+		if (err)
+			goto exit_sync_set;
 	}
 
 	spin_lock(&pers_lock);
@@ -5990,6 +5999,7 @@ int md_run(struct mddev *mddev)
 			blk_queue_flag_set(QUEUE_FLAG_NONROT, mddev->queue);
 		else
 			blk_queue_flag_clear(QUEUE_FLAG_NONROT, mddev->queue);
+		blk_queue_flag_set(QUEUE_FLAG_IO_STAT, mddev->queue);
 	}
 	if (pers->sync_request) {
 		if (mddev->kobj.sd &&
@@ -6039,8 +6049,11 @@ int md_run(struct mddev *mddev)
 	module_put(pers->owner);
 	md_bitmap_destroy(mddev);
 abort:
-	bioset_exit(&mddev->bio_set);
+	bioset_exit(&mddev->io_acct_set);
+exit_sync_set:
 	bioset_exit(&mddev->sync_set);
+exit_bio_set:
+	bioset_exit(&mddev->bio_set);
 	return err;
 }
 EXPORT_SYMBOL_GPL(md_run);
@@ -6264,6 +6277,7 @@ void md_stop(struct mddev *mddev)
 	__md_stop(mddev);
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
+	bioset_exit(&mddev->io_acct_set);
 }
 
 EXPORT_SYMBOL_GPL(md_stop);
@@ -8568,6 +8582,41 @@ void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
 }
 EXPORT_SYMBOL_GPL(md_submit_discard_bio);
 
+static void md_end_io_acct(struct bio *bio)
+{
+	struct md_io_acct *md_io_acct = bio->bi_private;
+	struct bio *orig_bio = md_io_acct->orig_bio;
+
+	orig_bio->bi_status = bio->bi_status;
+
+	bio_end_io_acct(orig_bio, md_io_acct->start_time);
+	bio_put(bio);
+	bio_endio(orig_bio);
+}
+
+/*
+ * Used by personalities that don't already clone the bio and thus can't
+ * easily add the timestamp to their extended bio structure.
+ */
+void md_account_bio(struct mddev *mddev, struct bio **bio)
+{
+	struct md_io_acct *md_io_acct;
+	struct bio *clone;
+
+	if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
+		return;
+
+	clone = bio_clone_fast(*bio, GFP_NOIO, &mddev->io_acct_set);
+	md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
+	md_io_acct->orig_bio = *bio;
+	md_io_acct->start_time = bio_start_io_acct(*bio);
+
+	clone->bi_end_io = md_end_io_acct;
+	clone->bi_private = md_io_acct;
+	*bio = clone;
+}
+EXPORT_SYMBOL_GPL(md_account_bio);
+
 /* md_allow_write(mddev)
  * Calling this ensures that the array is marked 'active' so that writes
  * may proceed without blocking.  It is important to call this before
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 4da240ffe2c5..4191f22acce4 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -487,6 +487,7 @@ struct mddev {
 	struct bio_set			sync_set; /* for sync operations like
 						   * metadata and bitmap writes
 						   */
+	struct bio_set			io_acct_set; /* for raid0 and raid5 io accounting */
 
 	/* Generic flush handling.
 	 * The last to finish preflush schedules a worker to submit
@@ -683,6 +684,12 @@ struct md_thread {
 	void			*private;
 };
 
+struct md_io_acct {
+	struct bio *orig_bio;
+	unsigned long start_time;
+	struct bio bio_clone;
+};
+
 #define THREAD_WAKEUP  0
 
 static inline void safe_put_page(struct page *p)
@@ -714,6 +721,7 @@ extern void md_error(struct mddev *mddev, struct md_rdev *rdev);
 extern void md_finish_reshape(struct mddev *mddev);
 void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
 			struct bio *bio, sector_t start, sector_t size);
+void md_account_bio(struct mddev *mddev, struct bio **bio);
 
 extern bool __must_check md_flush_request(struct mddev *mddev, struct bio *bio);
 extern void md_super_write(struct mddev *mddev, struct md_rdev *rdev,
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e5d7411cba9b..62c8b6adac70 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -546,6 +546,9 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
 		bio = split;
 	}
 
+	if (bio->bi_pool != &mddev->bio_set)
+		md_account_bio(mddev, &bio);
+
 	orig_sector = sector;
 	zone = find_zone(mddev->private, &sector);
 	switch (conf->layout) {
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 841e1c1aa5e6..58e9dbc0f683 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5468,6 +5468,7 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 	sector_t sector = raid_bio->bi_iter.bi_sector;
 	unsigned chunk_sects = mddev->chunk_sectors;
 	unsigned sectors = chunk_sects - (sector & (chunk_sects-1));
+	struct r5conf *conf = mddev->private;
 
 	if (sectors < bio_sectors(raid_bio)) {
 		struct r5conf *conf = mddev->private;
@@ -5477,6 +5478,9 @@ static struct bio *chunk_aligned_read(struct mddev *mddev, struct bio *raid_bio)
 		raid_bio = split;
 	}
 
+	if (raid_bio->bi_pool != &conf->bio_split)
+		md_account_bio(mddev, &raid_bio);
+
 	if (!raid5_read_one_chunk(mddev, raid_bio))
 		return raid_bio;
 
@@ -5756,6 +5760,7 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	DEFINE_WAIT(w);
 	bool do_prepare;
 	bool do_flush = false;
+	bool do_clone = false;
 
 	if (unlikely(bi->bi_opf & REQ_PREFLUSH)) {
 		int ret = log_handle_flush_request(conf, bi);
@@ -5784,6 +5789,7 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	if (rw == READ && mddev->degraded == 0 &&
 	    mddev->reshape_position == MaxSector) {
 		bi = chunk_aligned_read(mddev, bi);
+		do_clone = true;
 		if (!bi)
 			return true;
 	}
@@ -5798,6 +5804,9 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
 	last_sector = bio_end_sector(bi);
 	bi->bi_next = NULL;
 
+	if (!do_clone)
+		md_account_bio(mddev, &bi);
+
 	prepare_to_wait(&conf->wait_for_overlap, &w, TASK_UNINTERRUPTIBLE);
 	for (; logical_sector < last_sector; logical_sector += RAID5_STRIPE_SECTORS(conf)) {
 		int previous;
-- 
2.25.1

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

[Guoqing Jiang]

On 6/1/21 9:19 AM, Guoqing Jiang wrote:
> From: Guoqing Jiang <jgq516@gmail.com>
>
> We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
> don't own clone infrastructure to accounting io. And the bioset is added
> to mddev instead of to raid0 and raid5 layer, because with this way, we
> can put common functions to md.h and reuse them in raid0 and raid5.
>
> Also struct md_io_acct is added accordingly which includes io start_time,
> the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
> to get related io status.
>
> Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> Hi Song,
>
> Please consider apply the updated patch which has minor changes based on
> Christoph's comment.
>
> 1. don't create io_acct_set for raid1 and raid10.
> 2. update comment for md_account_bio.

Pls ignore this given it didn't check all the places before io_acct_set.
Do you want an incremental patch against for-next branch or a fresh
one to replace current patch in the tree?

Thanks,
Guoqing

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

[Song Liu]

On Wed, Jun 2, 2021 at 6:17 PM Guoqing Jiang <jgq516@gmail.com> wrote:
>
>
>
> On 6/1/21 9:19 AM, Guoqing Jiang wrote:
> > From: Guoqing Jiang <jgq516@gmail.com>
> >
> > We introduce a new bioset (io_acct_set) for raid0 and raid5 since they
> > don't own clone infrastructure to accounting io. And the bioset is added
> > to mddev instead of to raid0 and raid5 layer, because with this way, we
> > can put common functions to md.h and reuse them in raid0 and raid5.
> >
> > Also struct md_io_acct is added accordingly which includes io start_time,
> > the origin bio and cloned bio. Then we can call bio_{start,end}_io_acct
> > to get related io status.
> >
> > Signed-off-by: Guoqing Jiang <jiangguoqing@kylinos.cn>
> > Signed-off-by: Song Liu <song@kernel.org>
> > ---
> > Hi Song,
> >
> > Please consider apply the updated patch which has minor changes based on
> > Christoph's comment.
> >
> > 1. don't create io_acct_set for raid1 and raid10.
> > 2. update comment for md_account_bio.
>
> Pls ignore this given it didn't check all the places before io_acct_set.
> Do you want an incremental patch against for-next branch or a fresh
> one to replace current patch in the tree?

Hi Guoqing,

Thanks for the hard work on this. Please send a fix patch on top of current
md-next.

Song