[dm-devel] [PATCH 1/3] dm: don't grab target io reference in dm_zone_map_bio

[Damien Le Moal]

On 4/9/22 02:12, Ming Lei wrote:
> dm_zone_map_bio() is only called from __map_bio in which the io's
> reference is grabbed already, and the reference won't be released
> until the bio is submitted, so no necessary to do it dm_zone_map_bio
> any more.

I do not think that this patch is correct. Removing the extra reference on
the io can lead to problems if the io is completed in the target
->map(ti, clone) call or before dm_zone_map_bio_end() is called for the
DM_MAPIO_SUBMITTED or DM_MAPIO_REMAPPED cases. dm_zone_map_bio_end() needs
to reference the original bio, so we must ensure that it is still around,
which means keeping an extra IO reference to avoid dm_io_dec_pending()
calling bio_endio() on the orgi bio before dm_zone_map_bio_end() has a
chance to run.

> 
> Cc: Damien Le Moal <damien.lemoal at wdc.com>
> Signed-off-by: Ming Lei <ming.lei at redhat.com>
> ---
>  drivers/md/dm-core.h |  7 -------
>  drivers/md/dm-zone.c | 10 ----------
>  drivers/md/dm.c      |  7 ++++++-
>  3 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 4277853c7535..13136bd47cb3 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -277,13 +277,6 @@ static inline void dm_io_set_flag(struct dm_io *io, unsigned int bit)
>  	io->flags |= (1U << bit);
>  }
>  
> -static inline void dm_io_inc_pending(struct dm_io *io)
> -{
> -	atomic_inc(&io->io_count);
> -}
> -
> -void dm_io_dec_pending(struct dm_io *io, blk_status_t error);
> -
>  static inline struct completion *dm_get_completion_from_kobject(struct kobject *kobj)
>  {
>  	return &container_of(kobj, struct dm_kobject_holder, kobj)->completion;
> diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
> index c1ca9be4b79e..85d3c158719f 100644
> --- a/drivers/md/dm-zone.c
> +++ b/drivers/md/dm-zone.c
> @@ -545,13 +545,6 @@ int dm_zone_map_bio(struct dm_target_io *tio)
>  		return DM_MAPIO_KILL;
>  	}
>  
> -	/*
> -	 * The target map function may issue and complete the IO quickly.
> -	 * Take an extra reference on the IO to make sure it does disappear
> -	 * until we run dm_zone_map_bio_end().
> -	 */
> -	dm_io_inc_pending(io);
> -
>  	/* Let the target do its work */
>  	r = ti->type->map(ti, clone);
>  	switch (r) {
> @@ -580,9 +573,6 @@ int dm_zone_map_bio(struct dm_target_io *tio)
>  		break;
>  	}
>  
> -	/* Drop the extra reference on the IO */
> -	dm_io_dec_pending(io, sts);
> -
>  	if (sts != BLK_STS_OK)
>  		return DM_MAPIO_KILL;
>  
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 3c5fad7c4ee6..b8424a4b4725 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -929,11 +929,16 @@ static inline bool dm_tio_is_normal(struct dm_target_io *tio)
>  		!dm_tio_flagged(tio, DM_TIO_IS_DUPLICATE_BIO));
>  }
>  
> +static void dm_io_inc_pending(struct dm_io *io)
> +{
> +	atomic_inc(&io->io_count);
> +}
> +
>  /*
>   * Decrements the number of outstanding ios that a bio has been
>   * cloned into, completing the original io if necc.
>   */
> -void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
> +static void dm_io_dec_pending(struct dm_io *io, blk_status_t error)
>  {
>  	/* Push-back supersedes any I/O errors */
>  	if (unlikely(error)) {


-- 
Damien Le Moal
Western Digital Research