Re: [PATCH 0/8] ext4: fix endio zeroout corruption by tracking to-be-converted unwritten IOs

[Zhang Yi]

On 11/12/2026 3:12 PM, Ojaswin Mujoo wrote:
> On Thu, Nov 12, 2026 at 11:33:35AM +0800, Zhang Yi wrote:
>> On 11/3/2026 2:26 AM, Ojaswin Mujoo wrote:
>>> There is a data corruption in the zeroout fallback
>>> ext4_split_extent_zeroout() when we encounter an error like ENOSPC
>>> while trying to perform endio extent splitting.
>>>
>>> The issue was discovered by Fedora coreos community while using 1kb
>>> blocks size on a 64kb pagesize arch like powerpc or aarch64 and the
>>> discussion can be found at [1].
>>>
>>> ** The Issue **
>>>
>>> The issue can be encountered as follows:
>>>
>>> 1. Make an FS with no reserved to root reserved blocks and fill it
>>>    completely.
>>> 2. Delete a file so that ext4_sb_info thinks there is free space but
>>>    mballoc doesnt have any (until txn is committed).
>>> 3. Now when endio kicks in it eventually reaches:
>>>
>>> ext4_handle_unwritten_extents
>>>   ext4_split_convert_extents
>>>     ext4_split_extent
>>>       ext4_split_extent_at
>>>         ext4_ext_insert_extent
>>>           ext4_new_meta_blocks  // extent tree split/grow needs a block
>>>             ext4_mb_new_blocks
>>>               ext4_claim_free_clusters  // thinks we have free space but mballoc
>>>                                            has none until txn commit frees it
>>>               ext4_mb_regular_allocator
>>>                 // return -ENOSPC
>>>     // ext4_split_extent fallsback to zeroout
>>>     ext4_split_extent_zeroout
>>>       // We try to zeroout surrounding unwritten ranges. This code has a bug
>>>          where we zero an unwritten extent which could have valid data on disk 
>>>          whose endio has not yet converted it
>>>
>>> The main issue is that zeroout assumes any unwritten extent is valid to
>>> zeroout but it could actually be a soon-to-be-converted extent holding
>>> valid data.
>>>
>>
>> Hi Ojaswin,
>>
>> Thank you for your analysis! This makes sense to me. I apologize for not
>> having considered this concurrent scenario earlier and for amplifying
>> the data loss issue.
>>
>>> ** The fix **
>>>
>>> The way this series fixes it is by per-inode tracking the no. of
>>> unwritten IOs that are submitted but not yet converted
>>> (ei->i_conv_pending_ios). If we end up in the zeroout fallback and see
>>> there are such IOs existing in the inode, then we don't zeroout and just
>>> return back the original error. More design notes can be found in
>>> subsequent patches.
>>>
>>
>> Regarding the solution, I agree that your approach looks like it can fix
>> this issue, and the idea is fairly straightforward. When zeroing out, if
>> there is other concurrent I/O, just fall back and return an error.
>> However, I've been thinking about it over the past few days, hoping to
>> find another simpler and more elegant solution, but that seems rather
>> difficult.
>>
>> First, due to the map range caching mechanism in the iomap writeback
>> path and for performance reasons, we probably can't go back to splitting
>> extents when issuing I/O as before. Moreover, the previous approach also
>> couldn't completely avoid zeroing the entire unwritten extent when the
>> I/O completes, so the problem already existed before. It just required
>> concurrency and had a very low probability. Therefore, I agree that we
>> need to address this problem head-on.
> 
> Hi Zhang, thank you so much for looking into this.
> 
> So I agree that it is a tricky issue to solve since we don't want to
> add too much complexity in the fast path. Actually the first prototype I
> did for this (back in sep before we started seeing this in distros) was
> becuase I was working on moving zeroout to the fast path rather than
> just a fallback and had hit this fairly easily there.
> 
> In the first prototype, I took the exact approach you discussed here. A
> new CONV_PENDING type for extent cache with all the plumbing to ensure
> we don't lose the status untill the conversion happens. What I reliased
> was that although this was a much more accurate fix, it needed lots of
> core changes to es layer, and I ran into several bugs where data got
> corrupted cause the status was getting lost or not in sync. I did manage
> to fix most of them but it somehow makes me a bit uncomfortable from the
> POV of a backport.
> 
> The patch for the inital version (v v rough and not cleaned up since
> Sep) can be found here [1]. 

Ha, it looks like we're thinking along the same lines, the
implementation of this POC is largely similar to what I had in mind.
Thank you for working on this. Feel free to reach out anytime to
discuss.  :)

> 
> I'll address your below ideas on how/if i implemented them in [1]
> 
>>
>>
>> Here's a rough idea I'd like to share:
>>
>> 1. Add an in-core extent status flag, something like
>>    EXTENT_STATUS_PENDING_WRITTEN.  Like EXTENT_STATUS_DELAYED, these
>>    entries would be must-keep.  The idea is to mark ranges for which
>>    we've submitted I/O over an unwritten extent but haven't done the
>>    conversion yet.  It would just be an extra flag on top of
>>    EXTENT_STATUS_UNWRITTEN; the zeroout paths would be the only thing
> 
> I added a CONV_PENDING es type rather than flag. I did start with the
> flag over UNWRITTEN approach but most places like merging logic care
> that the type rather than flag and which was causing us to merge
> UNWRITTEN and CONV_PENDING which we can't allow. So instead of modiying
> code around merging and other areas I just made CONV_PENDING a separate
> type.
> 

Yeah, it's also fine with me to make it a separate type, whichever is
clearer.

[...]

> I agree that in the long term es cache approach is definitely the way to
> move forward. I'm just wondering if it would be feasible to backport
> something like that. Then again if we want to backport only till 7.0,
> maybe it wont be that big of a deal. 
> 
> There are 2 approaches that I can see here:
> 1. USe this patch set's counter apprach as a stop gap, easy to backport
> solution which works but is a bit clumsy. With the immediate fix in
> place, we can take time to test and design the extent status approach
> correctly and land it as a feature rather than something that needs
> backport.
> 
> 2. Ditch the counter approach and just fix it in one shot with extent
> status approach like in [1].
> 
> I'm happy to work on either of the two, what do you think shall be the
> right way forward?
> 

Personally, although I still think this counting approach is a bit
hacky, I'm not opposed to using it to fix the problem first.

Best Regards,
Yi.