On Fri, Feb 11 2022 at 10:03P -0500,
Mikulas Patocka <mpato...@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 <mpato...@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)))
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Reply via email to