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

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

On Wed, Mar 11, 2026 at 10:17:25AM +0800, Zhang Yi wrote:
> On 3/11/2026 6:13 AM, Darrick J. Wong wrote:
> > 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?
> > 
> 
> In the event of an error return, no caller will be concerned with the
> did_zero state. It is only relevant when the operation is successful.
> So I think we can just return if some error happens, consistent with
> other error-handling practices in this loop.
> 
> >>  	} 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;
> > 
> > ?
> > 
> 
> If we proceed with this approach, when there are mixed extents and the
> final iteration of the iomap_zero_range() loop does not zero out any
> data, it will clear the flags that were set in previous loop when the
> data was zeroed.

Ah, got it.  This makes sense enough to me to warrant wider testing so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> Thanks,
> Yi.
> 
> > --D
> > 
> >>  	return status;
> >>  }
> >> -- 
> >> 2.52.0
> >>
> >>
> 
>