[dm-devel] dm: account statistics correctly in case of bio split

[Mike Snitzer]

On Fri, Feb 11 2022 at 10:03P -0500,
Mikulas Patocka <mpatocka at redhat.com> wrote:

> If a bio was split to multiple targets, only one target's sub-range was
> counted. This patch changes it so that all the targets' ranges are
> counted.
> 
> Note that calls to bio_start_io_acct_remapped and bio_end_io_acct must
> match, so we maintain a counter how many times we have called
> bio_start_io_acct_remapped. When the io finishes, we call end_io_acct
> repeatedly.
> 
> Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>


Sorry but I'm not understanding, because any split that is needed due
to target boundaries will occur and the remainder (destined for other
targets) gets recursively submit_bio_noacct()'d back to the DM device.

So why do you think this wasn't happening properly?

Mike


> 
> ---
>  drivers/md/dm-core.h |    2 +-
>  drivers/md/dm.c      |   39 ++++++++++++++++++++-------------------
>  2 files changed, 21 insertions(+), 20 deletions(-)
> 
> Index: linux-dm/drivers/md/dm-core.h
> ===================================================================
> --- linux-dm.orig/drivers/md/dm-core.h	2022-02-11 15:44:33.000000000 +0100
> +++ linux-dm/drivers/md/dm-core.h	2022-02-11 15:44:33.000000000 +0100
> @@ -230,7 +230,7 @@ struct dm_io {
>  	atomic_t io_count;
>  	struct bio *orig_bio;
>  	unsigned long start_time;
> -	int was_accounted;
> +	atomic_t n_accounted;
>  	spinlock_t lock;
>  	struct dm_stats_aux stats_aux;
>  	/* last member of dm_target_io is 'struct bio' */
> Index: linux-dm/drivers/md/dm.c
> ===================================================================
> --- linux-dm.orig/drivers/md/dm.c	2022-02-11 15:44:33.000000000 +0100
> +++ linux-dm/drivers/md/dm.c	2022-02-11 15:52:04.000000000 +0100
> @@ -487,27 +487,30 @@ EXPORT_SYMBOL_GPL(dm_start_time_ns_from_
>  
>  static void start_io_acct(struct dm_io *io, struct bio *bio)
>  {
> -	struct bio *orig_bio;
> -
> -	/* Ensure IO accounting is only ever started once */
> -	if (xchg(&io->was_accounted, 1) == 1)
> -		return;
> -
> -	orig_bio = io->orig_bio;
> +	struct bio *orig_bio = io->orig_bio;
>  
>  	bio_start_io_acct_remapped(bio, io->start_time,
>  				   orig_bio->bi_bdev);
>  
> -	if (unlikely(dm_stats_used(&io->md->stats)))
> +	if (unlikely(dm_stats_used(&io->md->stats))) {
> +		if (unlikely(atomic_inc_return(&io->n_accounted) != 1))
> +			return;
>  		dm_stats_account_io(&io->md->stats, bio_data_dir(orig_bio),
>  				    orig_bio->bi_iter.bi_sector, bio_sectors(orig_bio),
>  				    false, 0, &io->stats_aux);
> +	} else {
> +		atomic_inc(&io->n_accounted);
> +	}
>  }
>  
> -static void end_io_acct(struct mapped_device *md, struct bio *bio,
> +static void end_io_acct(struct mapped_device *md, unsigned n_accounted, struct bio *bio,
>  			unsigned long start_time, struct dm_stats_aux *stats_aux)
>  {
> -	bio_end_io_acct(bio, start_time);
> +	if (unlikely(!n_accounted))
> +		return;
> +	do {
> +		bio_end_io_acct(bio, start_time);
> +	} while (unlikely(--n_accounted));
>  
>  	if (unlikely(dm_stats_used(&md->stats)))
>  		dm_stats_account_io(&md->stats, bio_data_dir(bio),
> @@ -536,7 +539,7 @@ static struct dm_io *alloc_io(struct map
>  	spin_lock_init(&io->lock);
>  
>  	io->start_time = jiffies;
> -	io->was_accounted = 0;
> +	io->n_accounted = (atomic_t)ATOMIC_INIT(0);
>  
>  	return io;
>  }
> @@ -793,7 +796,7 @@ void dm_io_dec_pending(struct dm_io *io,
>  	blk_status_t io_error;
>  	struct bio *bio;
>  	struct mapped_device *md = io->md;
> -	bool was_accounted = false;
> +	int n_accounted;
>  	unsigned long start_time = 0;
>  	struct dm_stats_aux stats_aux;
>  
> @@ -827,14 +830,12 @@ void dm_io_dec_pending(struct dm_io *io,
>  		}
>  
>  		io_error = io->status;
> -		if (io->was_accounted) {
> -			was_accounted = true;
> -			start_time = io->start_time;
> -			stats_aux = io->stats_aux;
> -		}
> +		n_accounted = atomic_read(&io->n_accounted);
> +		start_time = io->start_time;
> +		stats_aux = io->stats_aux;
> +
>  		free_io(io);
> -		if (was_accounted)
> -			end_io_acct(md, bio, start_time, &stats_aux);
> +		end_io_acct(md, n_accounted, bio, start_time, &stats_aux);
>  
>  		/* nudge anyone waiting on suspend queue */
>  		if (unlikely(wq_has_sleeper(&md->wait)))
>