Re: [PATCH v2 12/12] iomap: introduce a full map advance helper

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 19, 2025 at 12:50:50PM -0500, Brian Foster wrote:
> Various iomap_iter_advance() calls advance by the full mapping
> length and thus have no need for the current length input or
> post-advance remaining length output from the standard advance
> function. Add an iomap_iter_advance_full() helper to clean up these
> cases.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/dax.c               | 10 ++++------
>  fs/iomap/buffered-io.c |  3 +--
>  fs/iomap/fiemap.c      |  3 +--
>  fs/iomap/swapfile.c    |  4 +---
>  include/linux/iomap.h  |  9 +++++++++
>  5 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index cab3c5abe5cb..7fd4cd9a51f2 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1266,11 +1266,11 @@ static int dax_unshare_iter(struct iomap_iter *iter)
>  	u64 copy_len = iomap_length(iter);
>  	u32 mod;
>  	int id = 0;
> -	s64 ret = iomap_length(iter);
> +	s64 ret;
>  	void *daddr = NULL, *saddr = NULL;
>  
>  	if (!iomap_want_unshare_iter(iter))
> -		return iomap_iter_advance(iter, &ret);
> +		return iomap_iter_advance_full(iter);
>  
>  	/*
>  	 * Extend the file range to be aligned to fsblock/pagesize, because
> @@ -1300,16 +1300,14 @@ static int dax_unshare_iter(struct iomap_iter *iter)
>  	if (ret < 0)
>  		goto out_unlock;
>  
> -	if (copy_mc_to_kernel(daddr, saddr, copy_len) == 0)
> -		ret = iomap_length(iter);
> -	else
> +	if (copy_mc_to_kernel(daddr, saddr, copy_len) != 0)
>  		ret = -EIO;
>  
>  out_unlock:
>  	dax_read_unlock(id);
>  	if (ret < 0)
>  		return dax_mem2blk_err(ret);
> -	return iomap_iter_advance(iter, &ret);
> +	return iomap_iter_advance_full(iter);
>  }
>  
>  int dax_file_unshare(struct inode *inode, loff_t pos, loff_t len,
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 2b86978010bb..e53ac635e47c 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1436,8 +1436,7 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,
>  				range_dirty = false;
>  				status = iomap_zero_iter_flush_and_stale(&iter);
>  			} else {
> -				u64 length = iomap_length(&iter);
> -				status = iomap_iter_advance(&iter, &length);
> +				status = iomap_iter_advance_full(&iter);
>  			}
>  			iter.status = status;
>  			continue;
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index 6776b800bde7..80675c42e94e 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -42,7 +42,6 @@ static int iomap_to_fiemap(struct fiemap_extent_info *fi,
>  static int iomap_fiemap_iter(struct iomap_iter *iter,
>  		struct fiemap_extent_info *fi, struct iomap *prev)
>  {
> -	u64 length = iomap_length(iter);
>  	int ret;
>  
>  	if (iter->iomap.type == IOMAP_HOLE)
> @@ -56,7 +55,7 @@ static int iomap_fiemap_iter(struct iomap_iter *iter,
>  		return 0;
>  
>  advance:
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance_full(iter);
>  }
>  
>  int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> diff --git a/fs/iomap/swapfile.c b/fs/iomap/swapfile.c
> index 9ea185e58ca7..c1a762c10ce4 100644
> --- a/fs/iomap/swapfile.c
> +++ b/fs/iomap/swapfile.c
> @@ -97,8 +97,6 @@ static int iomap_swapfile_fail(struct iomap_swapfile_info *isi, const char *str)
>  static int iomap_swapfile_iter(struct iomap_iter *iter,
>  		struct iomap *iomap, struct iomap_swapfile_info *isi)
>  {
> -	u64 length = iomap_length(iter);
> -
>  	switch (iomap->type) {
>  	case IOMAP_MAPPED:
>  	case IOMAP_UNWRITTEN:
> @@ -135,7 +133,7 @@ static int iomap_swapfile_iter(struct iomap_iter *iter,
>  		memcpy(&isi->iomap, iomap, sizeof(isi->iomap));
>  	}
>  
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance_full(iter);
>  }
>  
>  /*
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 29b72a671104..4c7e9fe32117 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -264,6 +264,15 @@ static inline u64 iomap_length(const struct iomap_iter *iter)
>  	return iomap_length_trim(iter, iter->pos, iter->len);
>  }
>  
> +/**
> + * iomap_iter_advance_full - advance by the full length of current map
> + */
> +static inline int iomap_iter_advance_full(struct iomap_iter *iter)
> +{
> +	u64 length = iomap_length(iter);
> +	return iomap_iter_advance(iter, &length);

Dumb nit: blank line between the variable definition and the return
statement.

With that fixed
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +}
> +
>  /**
>   * iomap_iter_srcmap - return the source map for the current iomap iteration
>   * @i: iteration structure
> -- 
> 2.48.1
> 
>