Re: [PATCH] iomap: fix incorrect did_zero setting in iomap_zero_iter()

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

On Tue, Mar 10, 2026 at 04:22:50PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> The did_zero output parameter was unconditionally set after the loop,
> which is incorrect. It should only be set when the zeroing operation
> actually happens, not when IOMAP_F_STALE is set or when
> IOMAP_F_FOLIO_BATCH is set but !folio causes the loop to break early,
> or when iomap_iter_advance() returns an error.
> 
> This causes did_zero to be incorrectly set when zeroing a clean
> unwritten extent because the loop exits early without actually zeroing
> any data.
> 
> Fix it by using a local variable to track whether any folio was actually
> zeroed, and only set did_zero after the loop if zeroing happened.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  fs/iomap/buffered-io.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 5297491d5e3e..7a3780242cde 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1537,6 +1537,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
>  		const struct iomap_write_ops *write_ops)
>  {
>  	u64 bytes = iomap_length(iter);
> +	bool zeroed = false;
>  	int status;
>  
>  	do {
> @@ -1555,6 +1556,8 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
>  		/* a NULL folio means we're done with a folio batch */
>  		if (!folio) {
>  			status = iomap_iter_advance_full(iter);
> +			if (status)
> +				return status;
>  			break;
>  		}
>  
> @@ -1565,6 +1568,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
>  				bytes);
>  
>  		folio_zero_range(folio, offset, bytes);
> +		zeroed = true;
>  		folio_mark_accessed(folio);
>  
>  		ret = iomap_write_end(iter, bytes, bytes, folio);
> @@ -1574,10 +1578,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
>  
>  		status = iomap_iter_advance(iter, bytes);
>  		if (status)
> -			break;
> +			return status;

I think this seems like an unrelated change?  Do any of the callers
behave differently if iomap_zero_range() returns an error but also sets
did_zero to true?

>  	} while ((bytes = iomap_length(iter)) > 0);
>  
> -	if (did_zero)
> +	if (did_zero && zeroed)
>  		*did_zero = true;

Why not just do:

	if (did_zero)
		*did_zero = zeroed;

?

--D

>  	return status;
>  }
> -- 
> 2.52.0
> 
>