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

[Ojaswin Mujoo <ojaswin@linux.ibm.com>]

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.

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

For direct IO, ext4_iomap_begin() is called once per mapping and has no
way to account something only once per dio. So we first fix it in a
simple way by relying on inode->i_dio_count which is already
incremented/decremented once per dio. The last patch optimizes this by
moving dio accounting to i_conv_pending_ios as well, by using a custom
ext4_iomap_next() (needed because we want to increment once per
dio). Since iomap_next() infra was added in 7.2+ this can't be
backported so kept this approach separate.

There are other approaches considered like completely disabling zeroout
(not ideal as it's a useful fallback) or tracking the conv_pending ranges
in extent cache (this is an intrusive change) however the current
approach was chosen as a middle ground to balance complexity vs
accuracy of accounting.

** Backport Notes **

This issue seems to have existed ever since we had the zeroout logic
however this was never encountered until we moved extent splitting from
writeback time to endio time in commit:

ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O") (merged
in 7.0)

which changed the dioread_nolock writeback/dio path to submit I/O
without pre-splitting extents, deferring the split to endio. This means
multiple concurrent writes can share a single unwritten extent, and a
failed split on one of them could zeroout data belonging to another,
which exposes this issue in the zeroout logic. 

Note that the issue in zeroout was always present but the above patch
just uncovered it. However, for the purpose of backports, I've used this
patch. Seeing how we never saw this issue before this, hopefully just
backporting it till 7.0 should be fine.

** Testing **

I've tested -g quick for various configurations to catch any obvious
issues or counter leaks, saw no regressions. I'll continue to test for
auto group. I've also added some new tests to hit this regression which
I'll post soon.

[1] https://github.com/coreos/fedora-coreos-tracker/issues/2234

Thoughts and suggestions welcome.

Regards,
ojaswin

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
Ojaswin Mujoo (8):
      ext4: add extra_flags param to ext4_convert_unwritten_extents()
      ext4: fix endio zeroout corruption by tracking unwritten to-be-converted IOs
      ext4: add tracing for i_conv_pending_ios tracking
      ext4: don't zeroout if we have inflight unwritten IOs in non buffered-iomap path
      ext4: don't zeroout if we have inflight direct IOs
      ext4: make sure zero range WRITE_ZEROES uses NO_CONV_ACC flag
      ext4: don't zeroout if we have inflight unwritten IOs in buffered iomap path
      ext4: optimize inflight dio tracking for zeroout

 fs/ext4/ext4.h              |  29 +++++++--
 fs/ext4/extents-test.c      |   2 +
 fs/ext4/extents.c           | 117 ++++++++++++++++++++++++++++-------
 fs/ext4/file.c              |  14 ++++-
 fs/ext4/inode.c             | 146 +++++++++++++++++++++++++++++++++++++++++++-
 fs/ext4/page-io.c           |  27 +++++++-
 fs/ext4/super.c             |  12 ++++
 include/trace/events/ext4.h |  25 ++++++++
 8 files changed, 342 insertions(+), 30 deletions(-)
---
base-commit: 8c4f6e3376348fc3162eed86a05b933921c8f9ed
change-id: 20261023-ext4-endio-zero-bug-ec627ae61f39

Best regards,
-- 
Ojaswin Mujoo <ojaswin@linux.ibm.com>