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

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

[Zhang Yi]

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;
 	} while ((bytes = iomap_length(iter)) > 0);
 
-	if (did_zero)
+	if (did_zero && zeroed)
 		*did_zero = true;
 	return status;
 }
-- 
2.52.0

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

[Darrick J. Wong]

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
> 
> 

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

[Zhang Yi]

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.

Thanks,
Yi.

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

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

[Darrick J. Wong]

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
> >>
> >>
> 
>