[PATCH 00/14] dm: improve bio-based IO accounting

[PATCH 00/14] dm: improve bio-based IO accounting

[Mike Snitzer]

Hi,

All the changes from this patchset are available here:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.18

This work is based on Jens' for-5.18/block

For the linux-block crowd, I'm spamming you purely to seek review of
the last patch in this patchset (but welcome review of the entire set
if you're willing). Given all the DM changes that precede the last
block patch I'd appreciate review so it can be merged for 5.18 via
linux-dm.git.

DM is the only consumer of the recently added bio_start_io_acct_time
that the last patch enhances and renames to bio_start_io_acct_remapped

This patchset's primary purpose is to add the dm_submit_bio_remap()
interface to improve the bio-based IO accounting for DM targets that
take ownership of bios and use their own workqueues to process/remap
and later submit the bios.

Motivation is to fix the relatively useless nature of dm-crypt's
buffered IO stats.  DM core shouldn't immediately start IO accounting
for bios that dm-crypt goes on to queue in workqueues. The IO should
only have its IO started once submitted.  Otherwise the iostats for
dm-crypt just looks like an upfront flood of IO but then offer little
indication that anything is happening.  Given dm-crypt's cpu intensive
nature it takes time to complete IO but unless you look at the
underlying devices' iostats you wouldn't see it occurring.

Mike

Mike Snitzer (14):
  dm: rename split functions
  dm: fold __clone_and_map_data_bio into __split_and_process_bio
  dm: refactor dm_split_and_process_bio a bit
  dm: reduce code duplication in __map_bio
  dm: remove impossible BUG_ON in __send_empty_flush
  dm: remove unused mapped_device argument from free_tio
  dm: remove code only needed before submit_bio recursion
  dm: record old_sector in dm_target_io before calling map function
  dm: prep for following changes
  dm: add dm_submit_bio_remap interface
  dm crypt: use dm_submit_bio_remap
  dm delay: dm_submit_bio_remap
  dm: improve correctness and efficiency of bio-based IO accounting
  block: add bio_start_io_acct_remapped for the benefit of DM

 block/blk-core.c              |  24 ++---
 drivers/md/dm-core.h          |   4 +-
 drivers/md/dm-crypt.c         |   7 +-
 drivers/md/dm-delay.c         |   3 +-
 drivers/md/dm.c               | 224 ++++++++++++++++++++++--------------------
 include/linux/blkdev.h        |  16 ++-
 include/linux/device-mapper.h |   7 ++
 7 files changed, 156 insertions(+), 129 deletions(-)

-- 
2.15.0

[PATCH 01/14] dm: rename split functions

[Mike Snitzer]

Rename __split_and_process_bio to dm_split_and_process_bio.
Rename __split_and_process_non_flush to __split_and_process_bio.

Also fix a stale comment and whitespace.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ab9cc91931f9..2cecb8832936 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1359,7 +1359,7 @@ static bool __process_abnormal_io(struct clone_info *ci, struct dm_target *ti,
 /*
  * Select the correct strategy for processing a non-flush bio.
  */
-static int __split_and_process_non_flush(struct clone_info *ci)
+static int __split_and_process_bio(struct clone_info *ci)
 {
 	struct dm_target *ti;
 	unsigned len;
@@ -1395,8 +1395,8 @@ static void init_clone_info(struct clone_info *ci, struct mapped_device *md,
 /*
  * Entry point to split a bio into clones and submit them to the targets.
  */
-static void __split_and_process_bio(struct mapped_device *md,
-					struct dm_table *map, struct bio *bio)
+static void dm_split_and_process_bio(struct mapped_device *md,
+				     struct dm_table *map, struct bio *bio)
 {
 	struct clone_info ci;
 	int error = 0;
@@ -1409,19 +1409,19 @@ static void __split_and_process_bio(struct mapped_device *md,
 	} else if (op_is_zone_mgmt(bio_op(bio))) {
 		ci.bio = bio;
 		ci.sector_count = 0;
-		error = __split_and_process_non_flush(&ci);
+		error = __split_and_process_bio(&ci);
 	} else {
 		ci.bio = bio;
 		ci.sector_count = bio_sectors(bio);
-		error = __split_and_process_non_flush(&ci);
+		error = __split_and_process_bio(&ci);
 		if (ci.sector_count && !error) {
 			/*
 			 * Remainder must be passed to submit_bio_noacct()
 			 * so that it gets handled *after* bios already submitted
 			 * have been completely processed.
 			 * We take a clone of the original to store in
-			 * ci.io->orig_bio to be used by end_io_acct() and
-			 * for dec_pending to use for completion handling.
+			 * ci.io->orig_bio to be used by end_io_acct() and for
+			 * dm_io_dec_pending() to use for completion handling.
 			 */
 			struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count,
 						  GFP_NOIO, &md->queue->bio_split);
@@ -1470,7 +1470,7 @@ static void dm_submit_bio(struct bio *bio)
 	if (is_abnormal_io(bio))
 		blk_queue_split(&bio);
 
-	__split_and_process_bio(md, map, bio);
+	dm_split_and_process_bio(md, map, bio);
 out:
 	dm_put_live_table(md, srcu_idx);
 }
@@ -2283,11 +2283,11 @@ static int __dm_suspend(struct mapped_device *md, struct dm_table *map,
 	/*
 	 * Here we must make sure that no processes are submitting requests
 	 * to target drivers i.e. no one may be executing
-	 * __split_and_process_bio from dm_submit_bio.
+	 * dm_split_and_process_bio from dm_submit_bio.
 	 *
-	 * To get all processes out of __split_and_process_bio in dm_submit_bio,
+	 * To get all processes out of dm_split_and_process_bio in dm_submit_bio,
 	 * we take the write lock. To prevent any process from reentering
-	 * __split_and_process_bio from dm_submit_bio and quiesce the thread
+	 * dm_split_and_process_bio from dm_submit_bio and quiesce the thread
 	 * (dm_wq_work), we set DMF_BLOCK_IO_FOR_SUSPEND and call
 	 * flush_workqueue(md->wq).
 	 */
-- 
2.15.0

Re: [PATCH 01/14] dm: rename split functions

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:19PM -0500, Mike Snitzer wrote:
> Rename __split_and_process_bio to dm_split_and_process_bio.
> Rename __split_and_process_non_flush to __split_and_process_bio.
> 
> Also fix a stale comment and whitespace.
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/14] dm: fold __clone_and_map_data_bio into __split_and_process_bio

[Mike Snitzer]

Fold __clone_and_map_data_bio into its only caller.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 2cecb8832936..2f1942b61d48 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1188,25 +1188,6 @@ static void bio_setup_sector(struct bio *bio, sector_t sector, unsigned len)
 	bio->bi_iter.bi_size = to_bytes(len);
 }
 
-/*
- * Creates a bio that consists of range of complete bvecs.
- */
-static int __clone_and_map_data_bio(struct clone_info *ci, struct dm_target *ti,
-				    sector_t sector, unsigned *len)
-{
-	struct bio *bio = ci->bio, *clone;
-
-	clone = alloc_tio(ci, ti, 0, len, GFP_NOIO);
-	bio_advance(clone, to_bytes(sector - clone->bi_iter.bi_sector));
-	clone->bi_iter.bi_size = to_bytes(*len);
-
-	if (bio_integrity(bio))
-		bio_integrity_trim(clone);
-
-	__map_bio(clone);
-	return 0;
-}
-
 static void alloc_multiple_bios(struct bio_list *blist, struct clone_info *ci,
 				struct dm_target *ti, unsigned num_bios,
 				unsigned *len)
@@ -1361,6 +1342,7 @@ static bool __process_abnormal_io(struct clone_info *ci, struct dm_target *ti,
  */
 static int __split_and_process_bio(struct clone_info *ci)
 {
+	struct bio *clone;
 	struct dm_target *ti;
 	unsigned len;
 	int r;
@@ -1374,9 +1356,13 @@ static int __split_and_process_bio(struct clone_info *ci)
 
 	len = min_t(sector_t, max_io_len(ti, ci->sector), ci->sector_count);
 
-	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
-	if (r < 0)
-		return r;
+	clone = alloc_tio(ci, ti, 0, &len, GFP_NOIO);
+	bio_advance(clone, to_bytes(ci->sector - clone->bi_iter.bi_sector));
+	clone->bi_iter.bi_size = to_bytes(len);
+	if (bio_integrity(clone))
+		bio_integrity_trim(clone);
+
+	__map_bio(clone);
 
 	ci->sector += len;
 	ci->sector_count -= len;
-- 
2.15.0

Re: [PATCH 02/14] dm: fold __clone_and_map_data_bio into __split_and_process_bio

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:20PM -0500, Mike Snitzer wrote:
> Fold __clone_and_map_data_bio into its only caller.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/14] dm: refactor dm_split_and_process_bio a bit

[Mike Snitzer]

Remove needless branching. Leaves code to catch malformed
op_is_zone_mgmt bios (they shouldn't have a payload).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 51 ++++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 2f1942b61d48..56734aae718d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1375,7 +1375,13 @@ static void init_clone_info(struct clone_info *ci, struct mapped_device *md,
 {
 	ci->map = map;
 	ci->io = alloc_io(md, bio);
+	ci->bio = bio;
 	ci->sector = bio->bi_iter.bi_sector;
+	ci->sector_count = bio_sectors(bio);
+
+	/* Shouldn't happen but sector_count was being set to 0 so... */
+	if (WARN_ON_ONCE(op_is_zone_mgmt(bio_op(bio)) && ci->sector_count))
+		ci->sector_count = 0;
 }
 
 /*
@@ -1392,34 +1398,29 @@ static void dm_split_and_process_bio(struct mapped_device *md,
 	if (bio->bi_opf & REQ_PREFLUSH) {
 		error = __send_empty_flush(&ci);
 		/* dm_io_dec_pending submits any data associated with flush */
-	} else if (op_is_zone_mgmt(bio_op(bio))) {
-		ci.bio = bio;
-		ci.sector_count = 0;
-		error = __split_and_process_bio(&ci);
-	} else {
-		ci.bio = bio;
-		ci.sector_count = bio_sectors(bio);
-		error = __split_and_process_bio(&ci);
-		if (ci.sector_count && !error) {
-			/*
-			 * Remainder must be passed to submit_bio_noacct()
-			 * so that it gets handled *after* bios already submitted
-			 * have been completely processed.
-			 * We take a clone of the original to store in
-			 * ci.io->orig_bio to be used by end_io_acct() and for
-			 * dm_io_dec_pending() to use for completion handling.
-			 */
-			struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count,
-						  GFP_NOIO, &md->queue->bio_split);
-			ci.io->orig_bio = b;
+		goto out;
+	}
 
-			bio_chain(b, bio);
-			trace_block_split(b, bio->bi_iter.bi_sector);
-			submit_bio_noacct(bio);
-		}
+	error = __split_and_process_bio(&ci);
+	if (ci.sector_count && !error) {
+		/*
+		 * Remainder must be passed to submit_bio_noacct()
+		 * so that it gets handled *after* bios already submitted
+		 * have been completely processed.
+		 * We take a clone of the original to store in
+		 * ci.io->orig_bio to be used by end_io_acct() and for
+		 * dm_io_dec_pending() to use for completion handling.
+		 */
+		struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count,
+					  GFP_NOIO, &md->queue->bio_split);
+		ci.io->orig_bio = b;
+
+		bio_chain(b, bio);
+		trace_block_split(b, bio->bi_iter.bi_sector);
+		submit_bio_noacct(bio);
 	}
+out:
 	start_io_acct(ci.io);
-
 	/* drop the extra reference count */
 	dm_io_dec_pending(ci.io, errno_to_blk_status(error));
 }
-- 
2.15.0

Re: [PATCH 03/14] dm: refactor dm_split_and_process_bio a bit

[Christoph Hellwig]

> +	error = __split_and_process_bio(&ci);
> +	if (ci.sector_count && !error) {

Maybe turn thisin to

	if (error || !ci.sector_count)
		goto out;

	ci.io->orig_bio = bio_split(bio, bio_sectors(bio) - ci.sector_count,
				    GFP_NOIO, &md->queue->bio_split);
	bio_chain(ci.io->orig_bio, bio);
	trace_block_split(ci.io->orig_bio, bio->bi_iter.bi_sector);
	submit_bio_noacct(bio);

and remove another layer of indentation uing the existing label?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 04/14] dm: reduce code duplication in __map_bio

[Mike Snitzer]

Error path code (for handling DM_MAPIO_REQUEUE and DM_MAPIO_KILL) is
effectively identical.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 56734aae718d..cc014e56252e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1161,20 +1161,14 @@ static void __map_bio(struct bio *clone)
 		submit_bio_noacct(clone);
 		break;
 	case DM_MAPIO_KILL:
-		if (unlikely(swap_bios_limit(ti, clone))) {
-			struct mapped_device *md = io->md;
-			up(&md->swap_bios_semaphore);
-		}
-		free_tio(clone);
-		dm_io_dec_pending(io, BLK_STS_IOERR);
-		break;
 	case DM_MAPIO_REQUEUE:
-		if (unlikely(swap_bios_limit(ti, clone))) {
-			struct mapped_device *md = io->md;
-			up(&md->swap_bios_semaphore);
-		}
+		if (unlikely(swap_bios_limit(ti, clone)))
+			up(&io->md->swap_bios_semaphore);
 		free_tio(clone);
-		dm_io_dec_pending(io, BLK_STS_DM_REQUEUE);
+		if (r == DM_MAPIO_KILL)
+			dm_io_dec_pending(io, BLK_STS_IOERR);
+		else
+			dm_io_dec_pending(io, BLK_STS_DM_REQUEUE);
 		break;
 	default:
 		DMWARN("unimplemented target map return value: %d", r);
-- 
2.15.0

Re: [PATCH 04/14] dm: reduce code duplication in __map_bio

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:22PM -0500, Mike Snitzer wrote:
> Error path code (for handling DM_MAPIO_REQUEUE and DM_MAPIO_KILL) is
> effectively identical.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/14] dm: remove impossible BUG_ON in __send_empty_flush

[Mike Snitzer]

The flush_bio in question was just initialized to be empty, so there
is no way bio_has_data() will retrun true.  So remove stale BUG_ON().

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index cc014e56252e..1985fc3f2a95 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1255,7 +1255,6 @@ static int __send_empty_flush(struct clone_info *ci)
 	ci->bio = &flush_bio;
 	ci->sector_count = 0;
 
-	BUG_ON(bio_has_data(ci->bio));
 	while ((ti = dm_table_get_target(ci->map, target_nr++)))
 		__send_duplicate_bios(ci, ti, ti->num_flush_bios, NULL);
 
-- 
2.15.0

Re: [PATCH 05/14] dm: remove impossible BUG_ON in __send_empty_flush

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:23PM -0500, Mike Snitzer wrote:
> The flush_bio in question was just initialized to be empty, so there
> is no way bio_has_data() will retrun true.  So remove stale BUG_ON().

s/retrun/return/

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 06/14] dm: remove unused mapped_device argument from free_tio

[Mike Snitzer]

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1985fc3f2a95..f091bbf8a8dc 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -539,7 +539,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
 	return io;
 }
 
-static void free_io(struct mapped_device *md, struct dm_io *io)
+static void free_io(struct dm_io *io)
 {
 	bio_put(&io->tio.clone);
 }
@@ -825,7 +825,7 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 		io_error = io->status;
 		start_time = io->start_time;
 		stats_aux = io->stats_aux;
-		free_io(md, io);
+		free_io(io);
 		end_io_acct(md, bio, start_time, &stats_aux);
 
 		if (io_error == BLK_STS_DM_REQUEUE)
-- 
2.15.0

Re: [PATCH 06/14] dm: remove unused mapped_device argument from free_tio

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:24PM -0500, Mike Snitzer wrote:
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/14] dm: remove code only needed before submit_bio recursion

[Mike Snitzer]

Commit 8615cb65bd63 ("dm: remove useless loop in
__split_and_process_bio") showcased that we no longer loop.

Remove the bio_advance() in __split_and_process_bio() that was only
needed when looping was possible.

Similarly there is no need to advance the bio, using ci->sector
cursor, in __send_duplicate_bios().

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f091bbf8a8dc..5950d518e544 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1176,12 +1176,6 @@ static void __map_bio(struct bio *clone)
 	}
 }
 
-static void bio_setup_sector(struct bio *bio, sector_t sector, unsigned len)
-{
-	bio->bi_iter.bi_sector = sector;
-	bio->bi_iter.bi_size = to_bytes(len);
-}
-
 static void alloc_multiple_bios(struct bio_list *blist, struct clone_info *ci,
 				struct dm_target *ti, unsigned num_bios,
 				unsigned *len)
@@ -1224,14 +1218,14 @@ static void __send_duplicate_bios(struct clone_info *ci, struct dm_target *ti,
 	case 1:
 		clone = alloc_tio(ci, ti, 0, len, GFP_NOIO);
 		if (len)
-			bio_setup_sector(clone, ci->sector, *len);
+			clone->bi_iter.bi_size = to_bytes(*len);
 		__map_bio(clone);
 		break;
 	default:
 		alloc_multiple_bios(&blist, ci, ti, num_bios, len);
 		while ((clone = bio_list_pop(&blist))) {
 			if (len)
-				bio_setup_sector(clone, ci->sector, *len);
+				clone->bi_iter.bi_size = to_bytes(*len);
 			__map_bio(clone);
 		}
 		break;
@@ -1350,7 +1344,6 @@ static int __split_and_process_bio(struct clone_info *ci)
 	len = min_t(sector_t, max_io_len(ti, ci->sector), ci->sector_count);
 
 	clone = alloc_tio(ci, ti, 0, &len, GFP_NOIO);
-	bio_advance(clone, to_bytes(ci->sector - clone->bi_iter.bi_sector));
 	clone->bi_iter.bi_size = to_bytes(len);
 	if (bio_integrity(clone))
 		bio_integrity_trim(clone);
-- 
2.15.0

Re: [PATCH 07/14] dm: remove code only needed before submit_bio recursion

[Christoph Hellwig]

>  static void alloc_multiple_bios(struct bio_list *blist, struct clone_info *ci,
>  				struct dm_target *ti, unsigned num_bios,
>  				unsigned *len)
> @@ -1224,14 +1218,14 @@ static void __send_duplicate_bios(struct clone_info *ci, struct dm_target *ti,
>  	case 1:
>  		clone = alloc_tio(ci, ti, 0, len, GFP_NOIO);
>  		if (len)
> -			bio_setup_sector(clone, ci->sector, *len);
> +			clone->bi_iter.bi_size = to_bytes(*len);
>  		__map_bio(clone);
>  		break;
>  	default:
>  		alloc_multiple_bios(&blist, ci, ti, num_bios, len);
>  		while ((clone = bio_list_pop(&blist))) {
>  			if (len)
> -				bio_setup_sector(clone, ci->sector, *len);
> +				clone->bi_iter.bi_size = to_bytes(*len);
>  			__map_bio(clone);
>  		}
>  		break;
> @@ -1350,7 +1344,6 @@ static int __split_and_process_bio(struct clone_info *ci)
>  	len = min_t(sector_t, max_io_len(ti, ci->sector), ci->sector_count);
>  
>  	clone = alloc_tio(ci, ti, 0, &len, GFP_NOIO);
> -	bio_advance(clone, to_bytes(ci->sector - clone->bi_iter.bi_sector));
>  	clone->bi_iter.bi_size = to_bytes(len);

Maybe move the clone->bi_iter.bi_size assignment into alloc_tio as well?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/14] dm: record old_sector in dm_target_io before calling map function

[Mike Snitzer]

Prep for being able to defer trace_block_bio_remap() until when the
bio is remapped and submitted by the DM target.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-core.h | 1 +
 drivers/md/dm.c      | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 72d18c3fbf1f..f40be01cca81 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -214,6 +214,7 @@ struct dm_target_io {
 	unsigned int target_bio_nr;
 	unsigned int *len_ptr;
 	bool inside_dm_io;
+	sector_t old_sector;
 	struct bio clone;
 };
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 5950d518e544..3bd872b0e891 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -567,6 +567,7 @@ static struct bio *alloc_tio(struct clone_info *ci, struct dm_target *ti,
 	tio->ti = ti;
 	tio->target_bio_nr = target_bio_nr;
 	tio->len_ptr = len;
+	tio->old_sector = 0;
 
 	return &tio->clone;
 }
@@ -1120,7 +1121,6 @@ static void __map_bio(struct bio *clone)
 {
 	struct dm_target_io *tio = clone_to_tio(clone);
 	int r;
-	sector_t sector;
 	struct dm_io *io = tio->io;
 	struct dm_target *ti = tio->ti;
 
@@ -1132,7 +1132,7 @@ static void __map_bio(struct bio *clone)
 	 * this io.
 	 */
 	dm_io_inc_pending(io);
-	sector = clone->bi_iter.bi_sector;
+	tio->old_sector = clone->bi_iter.bi_sector;
 
 	if (unlikely(swap_bios_limit(ti, clone))) {
 		struct mapped_device *md = io->md;
@@ -1157,7 +1157,8 @@ static void __map_bio(struct bio *clone)
 		break;
 	case DM_MAPIO_REMAPPED:
 		/* the bio has been remapped so dispatch it */
-		trace_block_bio_remap(clone, bio_dev(io->orig_bio), sector);
+		trace_block_bio_remap(clone, bio_dev(io->orig_bio),
+				      tio->old_sector);
 		submit_bio_noacct(clone);
 		break;
 	case DM_MAPIO_KILL:
-- 
2.15.0

Re: [PATCH 08/14] dm: record old_sector in dm_target_io before calling map function

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:26PM -0500, Mike Snitzer wrote:
> Prep for being able to defer trace_block_bio_remap() until when the
> bio is remapped and submitted by the DM target.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/14] dm: prep for following changes

[Mike Snitzer]

Rename dm_io struct's 'endio_lock' member to 'lock' to reuse spinlock
to protect new member used to flag if IO accounting has been started.

Also move kicking of the suspend queue out to dm_io_dec_pending (the
only caller) since end_io_acct will soon only be called if IO
accounting was started.

Some comment tweaks and removal of local variables. No functional
change.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-core.h |  2 +-
 drivers/md/dm.c      | 32 ++++++++++++++------------------
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index f40be01cca81..8dd196aec130 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -230,7 +230,7 @@ struct dm_io {
 	atomic_t io_count;
 	struct bio *orig_bio;
 	unsigned long start_time;
-	spinlock_t endio_lock;
+	spinlock_t lock;
 	struct dm_stats_aux stats_aux;
 	/* last member of dm_target_io is 'struct bio' */
 	struct dm_target_io tio;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3bd872b0e891..8c0e96b8e1a5 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -487,12 +487,12 @@ EXPORT_SYMBOL_GPL(dm_start_time_ns_from_clone);
 
 static void start_io_acct(struct dm_io *io)
 {
-	struct mapped_device *md = io->md;
 	struct bio *bio = io->orig_bio;
 
 	bio_start_io_acct_time(bio, io->start_time);
-	if (unlikely(dm_stats_used(&md->stats)))
-		dm_stats_account_io(&md->stats, bio_data_dir(bio),
+
+	if (unlikely(dm_stats_used(&io->md->stats)))
+		dm_stats_account_io(&io->md->stats, bio_data_dir(bio),
 				    bio->bi_iter.bi_sector, bio_sectors(bio),
 				    false, 0, &io->stats_aux);
 }
@@ -500,18 +500,12 @@ static void start_io_acct(struct dm_io *io)
 static void end_io_acct(struct mapped_device *md, struct bio *bio,
 			unsigned long start_time, struct dm_stats_aux *stats_aux)
 {
-	unsigned long duration = jiffies - start_time;
-
 	bio_end_io_acct(bio, start_time);
 
 	if (unlikely(dm_stats_used(&md->stats)))
 		dm_stats_account_io(&md->stats, bio_data_dir(bio),
 				    bio->bi_iter.bi_sector, bio_sectors(bio),
-				    true, duration, stats_aux);
-
-	/* nudge anyone waiting on suspend queue */
-	if (unlikely(wq_has_sleeper(&md->wait)))
-		wake_up(&md->wait);
+				    true, jiffies - start_time, stats_aux);
 }
 
 static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
@@ -532,7 +526,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
 	atomic_set(&io->io_count, 1);
 	io->orig_bio = bio;
 	io->md = md;
-	spin_lock_init(&io->endio_lock);
+	spin_lock_init(&io->lock);
 
 	io->start_time = jiffies;
 
@@ -796,10 +790,10 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 
 	/* Push-back supersedes any I/O errors */
 	if (unlikely(error)) {
-		spin_lock_irqsave(&io->endio_lock, flags);
+		spin_lock_irqsave(&io->lock, flags);
 		if (!(io->status == BLK_STS_DM_REQUEUE && __noflush_suspending(md)))
 			io->status = error;
-		spin_unlock_irqrestore(&io->endio_lock, flags);
+		spin_unlock_irqrestore(&io->lock, flags);
 	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
@@ -829,6 +823,10 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 		free_io(io);
 		end_io_acct(md, bio, start_time, &stats_aux);
 
+		/* nudge anyone waiting on suspend queue */
+		if (unlikely(wq_has_sleeper(&md->wait)))
+			wake_up(&md->wait);
+
 		if (io_error == BLK_STS_DM_REQUEUE)
 			return;
 
@@ -1127,9 +1125,7 @@ static void __map_bio(struct bio *clone)
 	clone->bi_end_io = clone_endio;
 
 	/*
-	 * Map the clone.  If r == 0 we don't need to do
-	 * anything, the target has assumed ownership of
-	 * this io.
+	 * Map the clone.
 	 */
 	dm_io_inc_pending(io);
 	tio->old_sector = clone->bi_iter.bi_sector;
@@ -1154,6 +1150,7 @@ static void __map_bio(struct bio *clone)
 
 	switch (r) {
 	case DM_MAPIO_SUBMITTED:
+		/* target has assumed ownership of this io */
 		break;
 	case DM_MAPIO_REMAPPED:
 		/* the bio has been remapped so dispatch it */
@@ -1301,10 +1298,9 @@ static bool is_abnormal_io(struct bio *bio)
 static bool __process_abnormal_io(struct clone_info *ci, struct dm_target *ti,
 				  int *result)
 {
-	struct bio *bio = ci->bio;
 	unsigned num_bios = 0;
 
-	switch (bio_op(bio)) {
+	switch (bio_op(ci->bio)) {
 	case REQ_OP_DISCARD:
 		num_bios = ti->num_discard_bios;
 		break;
-- 
2.15.0

Re: [PATCH 09/14] dm: prep for following changes

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:27PM -0500, Mike Snitzer wrote:
> Rename dm_io struct's 'endio_lock' member to 'lock' to reuse spinlock
> to protect new member used to flag if IO accounting has been started.
> 
> Also move kicking of the suspend queue out to dm_io_dec_pending (the
> only caller) since end_io_acct will soon only be called if IO
> accounting was started.
> 
> Some comment tweaks and removal of local variables. No functional
> change.

The changes looks fine to me, but for the reader it would be much nicer
if the lock rename, kicking changes and local variable tweaks were split
into separate patches.

[PATCH 10/14] dm: add dm_submit_bio_remap interface

[Mike Snitzer]

Switch from early bio-based IO accounting (at the time DM clones each
incoming bio) to late IO accounting just before each remapped bio is
issued to underlying device via submit_bio_noacct().

Allows more precise bio-based IO accounting for DM targets that use
their own workqueues to perform additional processing of each bio in
conjunction with their DM_MAPIO_SUBMITTED return from their map
function.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-core.h          |  1 +
 drivers/md/dm.c               | 93 +++++++++++++++++++++++++++++++++++++++----
 include/linux/device-mapper.h |  7 ++++
 3 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 8dd196aec130..3ecd6f294f53 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -230,6 +230,7 @@ struct dm_io {
 	atomic_t io_count;
 	struct bio *orig_bio;
 	unsigned long start_time;
+	unsigned long io_acct_time;
 	spinlock_t lock;
 	struct dm_stats_aux stats_aux;
 	/* last member of dm_target_io is 'struct bio' */
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8c0e96b8e1a5..ad512f40716e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -485,21 +485,54 @@ u64 dm_start_time_ns_from_clone(struct bio *bio)
 }
 EXPORT_SYMBOL_GPL(dm_start_time_ns_from_clone);
 
-static void start_io_acct(struct dm_io *io)
+static void __start_io_acct(struct dm_io *io, struct bio *bio)
 {
-	struct bio *bio = io->orig_bio;
+	unsigned long flags;
 
-	bio_start_io_acct_time(bio, io->start_time);
+	/* Ensure IO accounting is only ever started once */
+	spin_lock_irqsave(&io->lock, flags);
+	if (smp_load_acquire(&io->io_acct_time)) {
+		spin_unlock_irqrestore(&io->lock, flags);
+		return;
+	}
+	smp_store_release(&io->io_acct_time, jiffies);
+	spin_unlock_irqrestore(&io->lock, flags);
 
+	bio_start_io_acct_time(bio, io->start_time);
 	if (unlikely(dm_stats_used(&io->md->stats)))
 		dm_stats_account_io(&io->md->stats, bio_data_dir(bio),
 				    bio->bi_iter.bi_sector, bio_sectors(bio),
 				    false, 0, &io->stats_aux);
 }
 
+static void start_io_acct(struct dm_io *io, struct bio *bio)
+{
+	/* Only start_io_acct() once for this IO */
+	if (smp_load_acquire(&io->io_acct_time))
+		return;
+
+	__start_io_acct(io, bio);
+}
+
+static void clone_and_start_io_acct(struct dm_io *io, struct bio *bio)
+{
+	struct bio io_acct_clone;
+
+	/* Only clone_and_start_io_acct() once for this IO */
+	if (smp_load_acquire(&io->io_acct_time))
+		return;
+
+	bio_init_clone(io->orig_bio->bi_bdev,
+		       &io_acct_clone, bio, GFP_NOIO);
+	__start_io_acct(io, &io_acct_clone);
+}
+
 static void end_io_acct(struct mapped_device *md, struct bio *bio,
 			unsigned long start_time, struct dm_stats_aux *stats_aux)
 {
+	if (!start_time)
+		return;
+
 	bio_end_io_acct(bio, start_time);
 
 	if (unlikely(dm_stats_used(&md->stats)))
@@ -529,6 +562,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
 	spin_lock_init(&io->lock);
 
 	io->start_time = jiffies;
+	io->io_acct_time = 0;
 
 	return io;
 }
@@ -818,7 +852,8 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 		}
 
 		io_error = io->status;
-		start_time = io->start_time;
+		if (io->io_acct_time)
+			start_time = io->start_time;
 		stats_aux = io->stats_aux;
 		free_io(io);
 		end_io_acct(md, bio, start_time, &stats_aux);
@@ -1099,6 +1134,43 @@ void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
 }
 EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
 
+/*
+ * @clone: clone bio that DM core passed to target's .map function
+ * @tgt_clone: bio that target needs to submit (after DM_MAPIO_SUBMITTED)
+ *
+ * Targets should use this interface to submit bios they take
+ * ownership of when returning DM_MAPIO_SUBMITTED.
+ *
+ * Target should also enable ti->accounts_remapped_io
+ */
+void dm_submit_bio_remap(struct bio *clone, struct bio *tgt_clone)
+{
+	struct dm_target_io *tio = clone_to_tio(clone);
+	struct dm_io *io = tio->io;
+	struct block_device *clone_bdev = clone->bi_bdev;
+
+	/* establish bio that will get submitted */
+	if (!tgt_clone)
+		tgt_clone = clone;
+
+	/*
+	 * account IO to DM device in terms of clone's
+	 * payload to avoid concern about late bio splitting.
+	 * - clone will reflect any dm_accept_partial_bio()
+	 * - any bio splitting is ultimately reflected in
+	 *   io->orig_bio so there is no IO imbalance in
+	 *   end_io_acct().
+	 */
+	clone->bi_bdev = io->orig_bio->bi_bdev;
+	start_io_acct(io, clone);
+	clone->bi_bdev = clone_bdev;
+
+	trace_block_bio_remap(tgt_clone, bio_dev(io->orig_bio),
+			      tio->old_sector);
+	submit_bio_noacct(tgt_clone);
+}
+EXPORT_SYMBOL_GPL(dm_submit_bio_remap);
+
 static noinline void __set_swap_bios_limit(struct mapped_device *md, int latch)
 {
 	mutex_lock(&md->swap_bios_lock);
@@ -1151,12 +1223,18 @@ static void __map_bio(struct bio *clone)
 	switch (r) {
 	case DM_MAPIO_SUBMITTED:
 		/* target has assumed ownership of this io */
+		if (!ti->accounts_remapped_io) {
+			/*
+			 * Any split isn't reflected in io->orig_bio yet. And bio
+			 * cannot be modified because target is submitting it.
+			 * Clone bio and account IO to DM device.
+			 */
+			clone_and_start_io_acct(io, clone);
+		}
 		break;
 	case DM_MAPIO_REMAPPED:
 		/* the bio has been remapped so dispatch it */
-		trace_block_bio_remap(clone, bio_dev(io->orig_bio),
-				      tio->old_sector);
-		submit_bio_noacct(clone);
+		dm_submit_bio_remap(clone, NULL);
 		break;
 	case DM_MAPIO_KILL:
 	case DM_MAPIO_REQUEUE:
@@ -1403,7 +1481,6 @@ static void dm_split_and_process_bio(struct mapped_device *md,
 		submit_bio_noacct(bio);
 	}
 out:
-	start_io_acct(ci.io);
 	/* drop the extra reference count */
 	dm_io_dec_pending(ci.io, errno_to_blk_status(error));
 }
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index b26fecf6c8e8..a3e397155bc9 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -362,6 +362,12 @@ struct dm_target {
 	 * zone append operations using regular writes.
 	 */
 	bool emulate_zone_append:1;
+
+	/*
+	 * Set if the target will submit IO using dm_submit_bio_remap()
+	 * after returning DM_MAPIO_SUBMITTED from its map function.
+	 */
+	bool accounts_remapped_io:1;
 };
 
 void *dm_per_bio_data(struct bio *bio, size_t data_size);
@@ -465,6 +471,7 @@ int dm_suspended(struct dm_target *ti);
 int dm_post_suspending(struct dm_target *ti);
 int dm_noflush_suspending(struct dm_target *ti);
 void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors);
+void dm_submit_bio_remap(struct bio *clone, struct bio *tgt_clone);
 union map_info *dm_get_rq_mapinfo(struct request *rq);
 
 #ifdef CONFIG_BLK_DEV_ZONED
-- 
2.15.0

[PATCH 11/14] dm crypt: use dm_submit_bio_remap

[Mike Snitzer]

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-crypt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a5006cb6ee8a..9ea197de08c2 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1855,7 +1855,7 @@ static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
 		return 1;
 	}
 
-	submit_bio_noacct(clone);
+	dm_submit_bio_remap(io->base_bio, clone);
 	return 0;
 }
 
@@ -1881,7 +1881,7 @@ static void kcryptd_io_write(struct dm_crypt_io *io)
 {
 	struct bio *clone = io->ctx.bio_out;
 
-	submit_bio_noacct(clone);
+	dm_submit_bio_remap(io->base_bio, clone);
 }
 
 #define crypt_io_from_node(node) rb_entry((node), struct dm_crypt_io, rb_node)
@@ -1960,7 +1960,7 @@ static void kcryptd_crypt_write_io_submit(struct dm_crypt_io *io, int async)
 
 	if ((likely(!async) && test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags)) ||
 	    test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)) {
-		submit_bio_noacct(clone);
+		dm_submit_bio_remap(io->base_bio, clone);
 		return;
 	}
 
@@ -3363,6 +3363,7 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_bios = 1;
 	ti->limit_swap_bios = true;
+	ti->accounts_remapped_io = true;
 
 	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
 	return 0;
-- 
2.15.0

[PATCH 12/14] dm delay: dm_submit_bio_remap

[Mike Snitzer]

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-delay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index 59e51d285b0e..8235927a3912 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -72,7 +72,7 @@ static void flush_bios(struct bio *bio)
 	while (bio) {
 		n = bio->bi_next;
 		bio->bi_next = NULL;
-		submit_bio_noacct(bio);
+		dm_submit_bio_remap(bio, NULL);
 		bio = n;
 	}
 }
@@ -232,6 +232,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_bios = 1;
 	ti->num_discard_bios = 1;
+	ti->accounts_remapped_io = true;
 	ti->per_io_data_size = sizeof(struct dm_delay_info);
 	return 0;
 
-- 
2.15.0

[PATCH 13/14] dm: improve correctness and efficiency of bio-based IO accounting

[Mike Snitzer]

Don't use jiffies as a glorified bool because jiffies can/will
rollover to 0.

Also use xchg(), instead of spin_lock_irq{save,restore} and
smp_load_acquire/smp_store_release, to avoid performance impact of
disabling and enabling interrupts.

Suggested-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-core.h |  2 +-
 drivers/md/dm.c      | 34 ++++++++++++----------------------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 3ecd6f294f53..d3c116866fd7 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -230,7 +230,7 @@ struct dm_io {
 	atomic_t io_count;
 	struct bio *orig_bio;
 	unsigned long start_time;
-	unsigned long io_acct_time;
+	int was_accounted;
 	spinlock_t lock;
 	struct dm_stats_aux stats_aux;
 	/* last member of dm_target_io is 'struct bio' */
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ad512f40716e..329f0be64523 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -487,17 +487,6 @@ EXPORT_SYMBOL_GPL(dm_start_time_ns_from_clone);
 
 static void __start_io_acct(struct dm_io *io, struct bio *bio)
 {
-	unsigned long flags;
-
-	/* Ensure IO accounting is only ever started once */
-	spin_lock_irqsave(&io->lock, flags);
-	if (smp_load_acquire(&io->io_acct_time)) {
-		spin_unlock_irqrestore(&io->lock, flags);
-		return;
-	}
-	smp_store_release(&io->io_acct_time, jiffies);
-	spin_unlock_irqrestore(&io->lock, flags);
-
 	bio_start_io_acct_time(bio, io->start_time);
 	if (unlikely(dm_stats_used(&io->md->stats)))
 		dm_stats_account_io(&io->md->stats, bio_data_dir(bio),
@@ -507,8 +496,8 @@ static void __start_io_acct(struct dm_io *io, struct bio *bio)
 
 static void start_io_acct(struct dm_io *io, struct bio *bio)
 {
-	/* Only start_io_acct() once for this IO */
-	if (smp_load_acquire(&io->io_acct_time))
+	/* Ensure IO accounting is only ever started once */
+	if (xchg(&io->was_accounted, 1) == 1)
 		return;
 
 	__start_io_acct(io, bio);
@@ -518,8 +507,8 @@ static void clone_and_start_io_acct(struct dm_io *io, struct bio *bio)
 {
 	struct bio io_acct_clone;
 
-	/* Only clone_and_start_io_acct() once for this IO */
-	if (smp_load_acquire(&io->io_acct_time))
+	/* Ensure IO accounting is only ever started once */
+	if (xchg(&io->was_accounted, 1) == 1)
 		return;
 
 	bio_init_clone(io->orig_bio->bi_bdev,
@@ -530,9 +519,6 @@ static void clone_and_start_io_acct(struct dm_io *io, struct bio *bio)
 static void end_io_acct(struct mapped_device *md, struct bio *bio,
 			unsigned long start_time, struct dm_stats_aux *stats_aux)
 {
-	if (!start_time)
-		return;
-
 	bio_end_io_acct(bio, start_time);
 
 	if (unlikely(dm_stats_used(&md->stats)))
@@ -562,7 +548,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
 	spin_lock_init(&io->lock);
 
 	io->start_time = jiffies;
-	io->io_acct_time = 0;
+	io->was_accounted = 0;
 
 	return io;
 }
@@ -819,6 +805,7 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 	blk_status_t io_error;
 	struct bio *bio;
 	struct mapped_device *md = io->md;
+	bool was_accounted = false;
 	unsigned long start_time = 0;
 	struct dm_stats_aux stats_aux;
 
@@ -852,11 +839,14 @@ void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
 		}
 
 		io_error = io->status;
-		if (io->io_acct_time)
+		if (io->was_accounted) {
+			was_accounted = true;
 			start_time = io->start_time;
-		stats_aux = io->stats_aux;
+			stats_aux = io->stats_aux;
+		}
 		free_io(io);
-		end_io_acct(md, bio, start_time, &stats_aux);
+		if (was_accounted)
+			end_io_acct(md, bio, start_time, &stats_aux);
 
 		/* nudge anyone waiting on suspend queue */
 		if (unlikely(wq_has_sleeper(&md->wait)))
-- 
2.15.0

[PATCH 14/14] block: add bio_start_io_acct_remapped for the benefit of DM

[Mike Snitzer]

DM needs the ability to account a clone bio's IO to the original
block_device. So add @orig_bdev argument to bio_start_io_acct_time.
Rename bio_start_io_acct_time to bio_start_io_acct_remapped.
Also, follow bio_end_io_acct and bio_end_io_acct_remapped pattern by
moving bio_start_io_acct to blkdev.h and have it call
bio_start_io_acct_remapped.

Improve DM to no longer need to play games with swizzling a clone
bio's bi_bdev (in dm_submit_bio_remap) and remove DM's
clone_and_start_io_acct() interface.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 block/blk-core.c       | 24 ++++++++----------------
 drivers/md/dm.c        | 41 ++++++++---------------------------------
 include/linux/blkdev.h | 16 ++++++++++++++--
 3 files changed, 30 insertions(+), 51 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index be8812f5489d..8f23be96c737 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1077,29 +1077,21 @@ static unsigned long __part_start_io_acct(struct block_device *part,
 }
 
 /**
- * bio_start_io_acct_time - start I/O accounting for bio based drivers
+ * bio_start_io_acct_remapped - start I/O accounting for bio based drivers
  * @bio:	bio to start account for
  * @start_time:	start time that should be passed back to bio_end_io_acct().
- */
-void bio_start_io_acct_time(struct bio *bio, unsigned long start_time)
-{
-	__part_start_io_acct(bio->bi_bdev, bio_sectors(bio),
-			     bio_op(bio), start_time);
-}
-EXPORT_SYMBOL_GPL(bio_start_io_acct_time);
-
-/**
- * bio_start_io_acct - start I/O accounting for bio based drivers
- * @bio:	bio to start account for
+ * @orig_bdev:	block device that I/O must be accounted to.
  *
  * Returns the start time that should be passed back to bio_end_io_acct().
  */
-unsigned long bio_start_io_acct(struct bio *bio)
+unsigned long bio_start_io_acct_remapped(struct bio *bio,
+				unsigned long start_time,
+				struct block_device *orig_bdev)
 {
-	return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio),
-				    bio_op(bio), jiffies);
+	return __part_start_io_acct(orig_bdev, bio_sectors(bio),
+				    bio_op(bio), start_time);
 }
-EXPORT_SYMBOL_GPL(bio_start_io_acct);
+EXPORT_SYMBOL_GPL(bio_start_io_acct_remapped);
 
 unsigned long disk_start_io_acct(struct gendisk *disk, unsigned int sectors,
 				 unsigned int op)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 329f0be64523..e020f505e243 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -485,35 +485,19 @@ u64 dm_start_time_ns_from_clone(struct bio *bio)
 }
 EXPORT_SYMBOL_GPL(dm_start_time_ns_from_clone);
 
-static void __start_io_acct(struct dm_io *io, struct bio *bio)
-{
-	bio_start_io_acct_time(bio, io->start_time);
-	if (unlikely(dm_stats_used(&io->md->stats)))
-		dm_stats_account_io(&io->md->stats, bio_data_dir(bio),
-				    bio->bi_iter.bi_sector, bio_sectors(bio),
-				    false, 0, &io->stats_aux);
-}
-
 static void start_io_acct(struct dm_io *io, struct bio *bio)
 {
 	/* Ensure IO accounting is only ever started once */
 	if (xchg(&io->was_accounted, 1) == 1)
 		return;
 
-	__start_io_acct(io, bio);
-}
+	bio_start_io_acct_remapped(bio, io->start_time,
+				   io->orig_bio->bi_bdev);
 
-static void clone_and_start_io_acct(struct dm_io *io, struct bio *bio)
-{
-	struct bio io_acct_clone;
-
-	/* Ensure IO accounting is only ever started once */
-	if (xchg(&io->was_accounted, 1) == 1)
-		return;
-
-	bio_init_clone(io->orig_bio->bi_bdev,
-		       &io_acct_clone, bio, GFP_NOIO);
-	__start_io_acct(io, &io_acct_clone);
+	if (unlikely(dm_stats_used(&io->md->stats)))
+		dm_stats_account_io(&io->md->stats, bio_data_dir(bio),
+				    bio->bi_iter.bi_sector, bio_sectors(bio),
+				    false, 0, &io->stats_aux);
 }
 
 static void end_io_acct(struct mapped_device *md, struct bio *bio,
@@ -1137,7 +1121,6 @@ void dm_submit_bio_remap(struct bio *clone, struct bio *tgt_clone)
 {
 	struct dm_target_io *tio = clone_to_tio(clone);
 	struct dm_io *io = tio->io;
-	struct block_device *clone_bdev = clone->bi_bdev;
 
 	/* establish bio that will get submitted */
 	if (!tgt_clone)
@@ -1151,9 +1134,7 @@ void dm_submit_bio_remap(struct bio *clone, struct bio *tgt_clone)
 	 *   io->orig_bio so there is no IO imbalance in
 	 *   end_io_acct().
 	 */
-	clone->bi_bdev = io->orig_bio->bi_bdev;
 	start_io_acct(io, clone);
-	clone->bi_bdev = clone_bdev;
 
 	trace_block_bio_remap(tgt_clone, bio_dev(io->orig_bio),
 			      tio->old_sector);
@@ -1213,14 +1194,8 @@ static void __map_bio(struct bio *clone)
 	switch (r) {
 	case DM_MAPIO_SUBMITTED:
 		/* target has assumed ownership of this io */
-		if (!ti->accounts_remapped_io) {
-			/*
-			 * Any split isn't reflected in io->orig_bio yet. And bio
-			 * cannot be modified because target is submitting it.
-			 * Clone bio and account IO to DM device.
-			 */
-			clone_and_start_io_acct(io, clone);
-		}
+		if (!ti->accounts_remapped_io)
+			start_io_acct(io, clone);
 		break;
 	case DM_MAPIO_REMAPPED:
 		/* the bio has been remapped so dispatch it */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3bfc75a2a450..31d055d4a17e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1512,11 +1512,23 @@ unsigned long disk_start_io_acct(struct gendisk *disk, unsigned int sectors,
 void disk_end_io_acct(struct gendisk *disk, unsigned int op,
 		unsigned long start_time);
 
-void bio_start_io_acct_time(struct bio *bio, unsigned long start_time);
-unsigned long bio_start_io_acct(struct bio *bio);
+unsigned long bio_start_io_acct_remapped(struct bio *bio,
+				unsigned long start_time,
+				struct block_device *orig_bdev);
 void bio_end_io_acct_remapped(struct bio *bio, unsigned long start_time,
 		struct block_device *orig_bdev);
 
+/**
+ * bio_start_io_acct - start I/O accounting for bio based drivers
+ * @bio:	bio to start account for
+ *
+ * Returns the start time that should be passed back to bio_end_io_acct().
+ */
+static inline unsigned long bio_start_io_acct(struct bio *bio)
+{
+	return bio_start_io_acct_remapped(bio, jiffies, bio->bi_bdev);
+}
+
 /**
  * bio_end_io_acct - end I/O accounting for bio based drivers
  * @bio:	bio to end account for
-- 
2.15.0

Re: [PATCH 14/14] block: add bio_start_io_acct_remapped for the benefit of DM

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 05:38:32PM -0500, Mike Snitzer wrote:
> DM needs the ability to account a clone bio's IO to the original
> block_device. So add @orig_bdev argument to bio_start_io_acct_time.
> Rename bio_start_io_acct_time to bio_start_io_acct_remapped.
> Also, follow bio_end_io_acct and bio_end_io_acct_remapped pattern by
> moving bio_start_io_acct to blkdev.h and have it call
> bio_start_io_acct_remapped.
> 
> Improve DM to no longer need to play games with swizzling a clone
> bio's bi_bdev (in dm_submit_bio_remap) and remove DM's
> clone_and_start_io_acct() interface.

Please split the core block layer part of this out, reorder it before
the current patch 10 and then do the right thing in that patch from the
start.