Re: [PATCH v5 10/10] iomap: advance the iter directly on zero range

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

On Wed, Feb 05, 2025 at 08:58:21AM -0500, Brian Foster wrote:
> Modify zero range to advance the iter directly. Replace the local pos
> and length calculations with direct advances and loop based on iter
> state instead.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/buffered-io.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index f953bf66beb1..ec227b45f3aa 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1345,17 +1345,16 @@ static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i)
>  
>  static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  {
> -	loff_t pos = iter->pos;
> -	loff_t length = iomap_length(iter);
> -	loff_t written = 0;
> +	u64 bytes = iomap_length(iter);
> +	int status;
>  
>  	do {
>  		struct folio *folio;
> -		int status;
>  		size_t offset;
> -		size_t bytes = min_t(u64, SIZE_MAX, length);
> +		loff_t pos = iter->pos;

I wonder, is there any need for the local variable here?

Looks fine to me though.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


>  		bool ret;
>  
> +		bytes = min_t(u64, SIZE_MAX, bytes);
>  		status = iomap_write_begin(iter, pos, bytes, &folio);
>  		if (status)
>  			return status;
> @@ -1376,14 +1375,14 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
>  
> -		pos += bytes;
> -		length -= bytes;
> -		written += bytes;
> -	} while (length > 0);
> +		status = iomap_iter_advance(iter, &bytes);
> +		if (status)
> +			break;
> +	} while (bytes > 0);
>  
>  	if (did_zero)
>  		*did_zero = true;
> -	return written;
> +	return status;
>  }
>  
>  int
> @@ -1436,11 +1435,14 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,
>  
>  		if (srcmap->type == IOMAP_HOLE ||
>  		    srcmap->type == IOMAP_UNWRITTEN) {
> -			loff_t proc = iomap_length(&iter);
> +			s64 proc;
>  
>  			if (range_dirty) {
>  				range_dirty = false;
>  				proc = iomap_zero_iter_flush_and_stale(&iter);
> +			} else {
> +				u64 length = iomap_length(&iter);
> +				proc = iomap_iter_advance(&iter, &length);
>  			}
>  			iter.processed = proc;
>  			continue;
> -- 
> 2.48.1
> 
>