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

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

[Ojaswin Mujoo]

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>

[PATCH 1/8] ext4: add extra_flags param to ext4_convert_unwritten_extents()

[Ojaswin Mujoo]

Let callers pass in additional flags for ext4_map_blocks(). This is in
preparation of the upcoming changes.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/ext4.h    | 3 ++-
 fs/ext4/extents.c | 8 ++++----
 fs/ext4/file.c    | 2 +-
 fs/ext4/page-io.c | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d3af062a830..b72f90b6375b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3971,7 +3971,8 @@ extern long ext4_fallocate(struct file *file, int mode, loff_t offset,
 			  loff_t len);
 extern int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
 					  loff_t offset, ssize_t len,
-					  ext4_lblk_t *converted);
+					  ext4_lblk_t *converted,
+					  int extra_flags);
 extern int ext4_convert_unwritten_extents_atomic(handle_t *handle,
 			struct inode *inode, loff_t offset, ssize_t len);
 extern int ext4_convert_unwritten_io_end_vec(handle_t *handle,
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 7193429a05e1..a229983f91dd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4704,7 +4704,7 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
 			ret = ext4_convert_unwritten_extents(handle,
 					inode, (loff_t)map.m_lblk << blkbits,
 					(loff_t)map.m_len << blkbits,
-					&converted);
+					&converted, 0);
 			if (ret)
 				map.m_len = converted;
 
@@ -5152,7 +5152,7 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
  */
 int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
 				   loff_t offset, ssize_t len,
-				   ext4_lblk_t *converted)
+				   ext4_lblk_t *converted, int extra_flags)
 {
 	ext4_lblk_t max_blocks, conv_blocks = 0;
 	int ret = 0, ret2 = 0, ret3 = 0;
@@ -5195,7 +5195,7 @@ int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
 		 */
 		ret = ext4_map_blocks(handle, inode, &map,
 				      EXT4_GET_BLOCKS_IO_CONVERT_EXT |
-				      EXT4_EX_NOCACHE);
+				      EXT4_EX_NOCACHE | extra_flags);
 		if (ret <= 0) {
 			ext4_warning(inode->i_sb,
 				     "inode #%llu: block %u: len %u: ext4_map_blocks returned %d",
@@ -5248,7 +5248,7 @@ int ext4_convert_unwritten_io_end_vec(handle_t *handle, ext4_io_end_t *io_end)
 	list_for_each_entry(io_end_vec, &io_end->list_vec, list) {
 		ret = ext4_convert_unwritten_extents(handle, io_end->inode,
 						     io_end_vec->offset,
-						     io_end_vec->size, NULL);
+						     io_end_vec->size, NULL, 0);
 		if (ret)
 			break;
 	}
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 50d3c92709c8..06ef7a9b9f83 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -457,7 +457,7 @@ static int ext4_dio_write_end_io(struct kiocb *iocb, ssize_t size,
 							      size);
 	else if (!error && size && flags & IOMAP_DIO_UNWRITTEN)
 		error = ext4_convert_unwritten_extents(NULL, inode, pos, size,
-						       NULL);
+						       NULL, 0);
 	if (error)
 		return error;
 	/*
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 734d856a6d47..4c5c77da2a5e 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -685,7 +685,7 @@ static void ext4_iomap_finish_ioend(struct iomap_ioend *ioend)
 
 	if (ioend->io_flags & IOMAP_IOEND_UNWRITTEN)
 		ret = ext4_convert_unwritten_extents(handle, inode, pos,
-						     size, NULL);
+						     size, NULL, 0);
 
 out_journal:
 	err = ext4_journal_stop(handle);

-- 
2.55.0

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

[Ojaswin Mujoo]

In case endio conversion fails due to factors like ENOSPC or ENOMEM, ext4
uses a fallback where we zeroout the unwritten extents instead of
splitting. However, not all unwritten extents can be zeroed out
unconditionally as they might have data underneath which is pending endio
conversion. Zeroing out such an extent can cause silent corruption.

To fix this we track the IOs which are about to undergo conversion in
ei->i_conv_pending_ios. The zeroout code then uses this counter to
determine if it's safe to zeroout.

Design Notes:

**Locking**

The only mutual exclusion we need is between an IO that is about to be
submitted over an unwritten extent and zeroout. This is done using a
dedicated rwsem, ei->i_conv_pending_sem with the following rules:

1. Incrementing the counter must down_read(i_conv_pending_sem).
2. A zeroout must check the counter and zero data under
   down_write(i_conv_pending_sem)
3. Decrementing the counter needs no lock. At worst, this makes zeroout
   bail out due to a stale, elevated counter value, which is acceptable
4. Changes to i_conv_pending_ios should use atomic helpers.

This design provides us the required exclusion while ensuring that fast
paths don't see any heavy lock contention. Atomic counters help parallel
modifications to be serialized.

** Tolerance for false positives **

We trade false positives for simplicity of implementation because having a
false positive here will just error the endio which is still better than
silently corrupting the data. We can always optimize our tracking to make
it more accurate in the future.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/ext4.h              | 19 ++++++++++
 fs/ext4/extents-test.c      |  2 +
 fs/ext4/extents.c           | 78 ++++++++++++++++++++++++++++++++------
 fs/ext4/inode.c             | 92 +++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/super.c             | 12 ++++++
 include/trace/events/ext4.h |  1 +
 6 files changed, 192 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b72f90b6375b..1a01b1f809f7 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -736,6 +736,11 @@ enum {
  * Look EXT4_MAP_QUERY_LAST_IN_LEAF.
  */
 #define EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF	0x1000
+/*
+ * Caller may convert an unwritten extent (possibly zeroing parts of it)
+ * without elevating the i_conv_pending_ios count to account for itself.
+ */
+#define EXT4_GET_BLOCKS_NO_CONV_ACC		0x2000
 
 /*
  * The bit position of these flags must not overlap with any of the
@@ -1235,6 +1240,15 @@ struct ext4_inode_info {
 #ifdef CONFIG_FS_ENCRYPTION
 	struct fscrypt_inode_info *i_crypt_info;
 #endif
+	/*
+	 * Number of in-flight IOs pending unwritten->written conversion. Used
+	 * by the zeroout fallback to avoid zeroing out data of such IOs.
+	 * Incrementing needs down_read(i_conv_pending_sem), zeroout needs
+	 * down_write(i_conv_pending_sem), decrementing needs no lock. See
+	 * comment above ext4_conv_pending_inc().
+	 */
+	atomic_t i_conv_pending_ios;
+	struct rw_semaphore i_conv_pending_sem;
 };
 
 /*
@@ -3983,6 +3997,11 @@ extern int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 				  struct ext4_map_blocks *map, int flags);
 extern int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
 				  struct ext4_map_blocks *map, int flags);
+extern void ext4_conv_pending_inc(struct inode *inode);
+extern void ext4_conv_pending_sum(struct inode *inode, int n);
+extern int ext4_conv_pending_inc_trylock(struct inode *inode);
+extern void ext4_conv_pending_dec(struct inode *inode);
+extern void ext4_conv_pending_sub(struct inode *inode, int n);
 extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
 						   int num,
 						   struct ext4_ext_path *path);
diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
index c3836ecb89f9..4d63c2050135 100644
--- a/fs/ext4/extents-test.c
+++ b/fs/ext4/extents-test.c
@@ -271,6 +271,8 @@ static int extents_kunit_init(struct kunit *test)
 
 	ext4_es_init_tree(&ei->i_es_tree);
 	rwlock_init(&ei->i_es_lock);
+	atomic_set(&ei->i_conv_pending_ios, 0);
+	init_rwsem(&ei->i_conv_pending_sem);
 	INIT_LIST_HEAD(&ei->i_es_list);
 	ei->i_es_all_nr = 0;
 	ei->i_es_shk_nr = 0;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a229983f91dd..d255884cf075 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3311,6 +3311,40 @@ static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
 	return path;
 }
 
+/*
+ * Returns true with i_conv_pending_sem held for write if it's safe to zeroout,
+ * else drops the lock and returns false. The caller must call
+ * ext4_conv_pending_zeroout_unlock() once the zeroout IO is complete.
+ *
+ * Callers are expected to have elevated i_conv_pending_ios to account for
+ * themselves, so a count of 1 is their own. Callers that haven't must pass
+ * EXT4_GET_BLOCKS_NO_CONV_ACC in flags, in which case any count is another IO.
+ *
+ * Refer to comment above ext4_conv_pending_inc() for details.
+ */
+static bool ext4_conv_pending_zeroout_lock(struct inode *inode, int flags)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	int pending, thresh;
+
+	thresh = (flags & EXT4_GET_BLOCKS_NO_CONV_ACC) ? 0 : 1;
+
+	down_write(&ei->i_conv_pending_sem);
+	pending = atomic_read(&ei->i_conv_pending_ios);
+
+	WARN_ON_ONCE(pending < 0);
+	if (pending > thresh || pending < 0) {
+		up_write(&ei->i_conv_pending_sem);
+		return false;
+	}
+	return true;
+}
+
+static void ext4_conv_pending_zeroout_unlock(struct inode *inode)
+{
+	up_write(&EXT4_I(inode)->i_conv_pending_sem);
+}
+
 static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 				     struct ext4_ext_path *path,
 				     struct ext4_map_blocks *map, int flags)
@@ -3328,6 +3362,9 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 	ee_len = ext4_ext_get_actual_len(ex);
 	is_unwrit = ext4_ext_is_unwritten(ex);
 
+	if (!ext4_conv_pending_zeroout_lock(inode, flags))
+		return -EINVAL;
+
 	if (flags & EXT4_GET_BLOCKS_CONVERT) {
 		/*
 		 * EXT4_GET_BLOCKS_CONVERT: Caller wants the range specified by
@@ -3338,9 +3375,11 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 		loff_t map_end = (loff_t) map->m_lblk + map->m_len;
 		loff_t ex_end = (loff_t) ee_block + ee_len;
 
-		if (!is_unwrit)
+		if (!is_unwrit) {
 			/* Shouldn't happen. Just exit */
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_unlock;
+		}
 
 		/* zeroout left */
 		if (map->m_lblk > ee_block) {
@@ -3350,7 +3389,7 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 			err = ext4_issue_zeroout(inode, lblk, pblk, len);
 			if (err)
 				/* ZEROOUT failed, just return original error */
-				return err;
+				goto out_unlock;
 		}
 
 		/* zeroout right */
@@ -3361,7 +3400,7 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 			err = ext4_issue_zeroout(inode, lblk, pblk, len);
 			if (err)
 				/* ZEROOUT failed, just return original error */
-				return err;
+				goto out_unlock;
 		}
 	} else if (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
 		/*
@@ -3370,9 +3409,11 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 		 * Zeroout the map range leaving rest as it is.
 		 */
 
-		if (is_unwrit)
+		if (is_unwrit) {
 			/* Shouldn't happen. Just exit */
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_unlock;
+		}
 
 		lblk = map->m_lblk;
 		len = map->m_len;
@@ -3380,16 +3421,23 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 		err = ext4_issue_zeroout(inode, lblk, pblk, len);
 		if (err)
 			/* ZEROOUT failed, just return original error */
-			return err;
+			goto out_unlock;
 	} else {
 		/*
 		 * We no longer perform unwritten to unwritten splits in IO paths.
 		 * Hence this should not happen.
 		 */
 		WARN_ON_ONCE(true);
-		return -EINVAL;
+		err = -EINVAL;
+		goto out_unlock;
 	}
 
+	/*
+	 * The zeroout IO is done, no need to hold off new unwritten IOs while we
+	 * update the extent tree.
+	 */
+	ext4_conv_pending_zeroout_unlock(inode);
+
 	err = ext4_ext_get_access(handle, inode, path + depth);
 	if (err)
 		return err;
@@ -3401,6 +3449,10 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
 		return err;
 
 	return 0;
+
+out_unlock:
+	ext4_conv_pending_zeroout_unlock(inode);
+	return err;
 }
 
 /*
@@ -3743,7 +3795,8 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
 	split_map.m_lblk = map->m_lblk;
 	split_map.m_len = map->m_len;
 
-	if (max_zeroout && (*allocated > split_map.m_len)) {
+	if (max_zeroout && (*allocated > split_map.m_len) &&
+	    ext4_conv_pending_zeroout_lock(inode, flags)) {
 		if (*allocated <= max_zeroout) {
 			/* case 3 or 5 */
 			zero_ex1.ee_block =
@@ -3756,7 +3809,7 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
 				split_map.m_len - ee_block);
 			err = ext4_ext_zeroout(inode, &zero_ex1);
 			if (err)
-				goto fallback;
+				goto zeroout_unlock;
 			split_map.m_len = *allocated;
 		}
 		if (split_map.m_lblk - ee_block + split_map.m_len <
@@ -3770,16 +3823,17 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
 						      ext4_ext_pblock(ex));
 				err = ext4_ext_zeroout(inode, &zero_ex2);
 				if (err)
-					goto fallback;
+					goto zeroout_unlock;
 			}
 
 			split_map.m_len += split_map.m_lblk - ee_block;
 			split_map.m_lblk = ee_block;
 			*allocated = map->m_len;
 		}
+zeroout_unlock:
+		ext4_conv_pending_zeroout_unlock(inode);
 	}
 
-fallback:
 	path = ext4_split_convert_extents(handle, inode, &split_map, path,
 					  flags | EXT4_GET_BLOCKS_CONVERT, NULL);
 	if (IS_ERR(path))
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 6ede3644a738..280ba3d359e3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -669,6 +669,98 @@ int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 	return retval;
 }
 
+static void _ext4_conv_pending_mod(struct inode *inode, int n)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	atomic_add(n, &ei->i_conv_pending_ios);
+}
+
+/*
+ * In case endio conversion fails due to factors like ENOSPC or ENOMEM, ext4
+ * uses a fallback where we zeroout the unwritten extents instead of splitting.
+ * However, not all unwritten extents can be zeroed out unconditionally as
+ * they might have data underneath which is pending endio conversion. Zeroing
+ * out such an extent can cause silent corruption.
+ *
+ * To fix this we track the IOs which are about to undergo conversion in
+ * ei->i_conv_pending_ios. The zeroout code then uses this counter to
+ * determine if it's safe to zeroout.
+ *
+ * Design Notes:
+ *
+ * **Locking**
+ *
+ * The only mutual exclusion we need is between an IO that is about to be
+ * submitted over an unwritten extent and zeroout. This is done using a
+ * dedicated rwsem, ei->i_conv_pending_sem with the following rules:
+ *
+ * 1. Incrementing the counter must down_read(i_conv_pending_sem).
+ * 2. A zeroout must check the counter and zero data under
+ *    down_write(i_conv_pending_sem)
+ * 3. Decrementing the counter needs no lock. At worst, this makes zeroout bail
+ *    out due to a stale, elevated counter value, which is acceptable
+ * 4. Changes to i_conv_pending_ios should use atomic helpers.
+ *
+ * This design provides us the required exclusion while ensuring that fast paths
+ * don't see any heavy lock contention. Atomic counters help parallel
+ * modifications to be serialized.
+ *
+ * ** Tolerance for false positives **
+ *
+ * We trade false positives for simplicity of implementation because having a
+ * false positive here will just error the endio which is still better than
+ * silently corrupting the data. We can always optimize our tracking to make it
+ * more accurate in the future.
+ */
+void ext4_conv_pending_inc(struct inode *inode)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	down_read(&ei->i_conv_pending_sem);
+	_ext4_conv_pending_mod(inode, 1);
+	up_read(&ei->i_conv_pending_sem);
+}
+
+void ext4_conv_pending_sum(struct inode *inode, int n)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	down_read(&ei->i_conv_pending_sem);
+	_ext4_conv_pending_mod(inode, n);
+	up_read(&ei->i_conv_pending_sem);
+}
+
+/*
+ * Same as ext4_conv_pending_inc() but doesn't sleep if a zeroout is in
+ * progress. Returns -EAGAIN and leaves the counter untouched in that case.
+ */
+int ext4_conv_pending_inc_trylock(struct inode *inode)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	if (!down_read_trylock(&ei->i_conv_pending_sem))
+		return -EAGAIN;
+
+	_ext4_conv_pending_mod(inode, 1);
+	up_read(&ei->i_conv_pending_sem);
+	return 0;
+}
+
+void ext4_conv_pending_dec(struct inode *inode)
+{
+	_ext4_conv_pending_mod(inode, -1);
+}
+
+void ext4_conv_pending_sub(struct inode *inode, int n)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	down_read(&ei->i_conv_pending_sem);
+	_ext4_conv_pending_mod(inode, -n);
+	up_read(&ei->i_conv_pending_sem);
+}
+
 int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
 			   struct ext4_map_blocks *map, int flags)
 {
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7d8aedfbbb87..866a6a70535e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1429,6 +1429,8 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 	ei->i_es_shrink_lblk = 0;
 	ei->i_es_seq = 0;
 	ei->i_reserved_data_blocks = 0;
+	atomic_set(&ei->i_conv_pending_ios, 0);
+	init_rwsem(&ei->i_conv_pending_sem);
 	spin_lock_init(&(ei->i_block_reservation_lock));
 	ext4_init_pending_tree(&ei->i_pending_tree);
 #ifdef CONFIG_QUOTA
@@ -1505,6 +1507,16 @@ static void ext4_destroy_inode(struct inode *inode)
 		ext4_msg(inode->i_sb, KERN_ERR,
 			 "Inode %llu (%p): EXT4_STATE_DISKSIZE_GROW_PENDING not cleared!",
 			 inode->i_ino, EXT4_I(inode));
+
+	if (!(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ERROR_FS) &&
+	    !ext4_emergency_state(inode->i_sb)) {
+		int pending = atomic_read(&EXT4_I(inode)->i_conv_pending_ios);
+
+		if (WARN_ON_ONCE(pending))
+			ext4_msg(inode->i_sb, KERN_ERR,
+				 "Inode %llu (%p): i_conv_pending_ios (%d) not cleared!",
+				 inode->i_ino, EXT4_I(inode), pending);
+	}
 }
 
 static void ext4_shutdown(struct super_block *sb)
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 92048da9dae4..7d255c47c446 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -43,6 +43,7 @@ struct partial_cluster;
 	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
 	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
 	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\
+	{ EXT4_GET_BLOCKS_NO_CONV_ACC,		"NO_CONV_ACC" },	\
 	{ EXT4_GET_BLOCKS_CONVERT_UNWRITTEN,	"CONVERT_UNWRITTEN" },  \
 	{ EXT4_GET_BLOCKS_ZERO,			"ZERO" },		\
 	{ EXT4_GET_BLOCKS_IO_SUBMIT,		"IO_SUBMIT" },		\

-- 
2.55.0

[PATCH 3/8] ext4: add tracing for i_conv_pending_ios tracking

[Ojaswin Mujoo]

Add tracing to track changes to i_conv_pending_ios count.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/inode.c             | 11 ++++++++++-
 include/trace/events/ext4.h | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 280ba3d359e3..8b5cb37cdd6d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -673,7 +673,16 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
 
-	atomic_add(n, &ei->i_conv_pending_ios);
+	/*
+	 * Only use the full ordering variant when tracing is needed.
+	 */
+	if (trace_ext4_conv_pending_ios_enabled()) {
+		int new_ios = atomic_add_return(n, &ei->i_conv_pending_ios);
+
+		trace_ext4_conv_pending_ios(inode, n > 0, new_ios - n, new_ios);
+	} else {
+		atomic_add(n, &ei->i_conv_pending_ios);
+	}
 }
 
 /*
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 7d255c47c446..f1754b2e836c 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -3367,6 +3367,30 @@ TRACE_EVENT(ext4_block_zero_eof,
 		  __entry->i_disksize, __entry->did_zero, __entry->zero_written)
 );
 
+/* i_conv_pending_ios accounting tracepoint */
+TRACE_EVENT(ext4_conv_pending_ios,
+	TP_PROTO(struct inode *inode, bool add, int old_ios, int new_ios),
+	TP_ARGS(inode, add, old_ios, new_ios),
+	TP_STRUCT__entry(
+		__field(dev_t, dev)
+		__field(u64, ino)
+		__field(bool, add)
+		__field(int, old_ios)
+		__field(int, new_ios)
+	),
+	TP_fast_assign(
+		__entry->dev = inode->i_sb->s_dev;
+		__entry->ino = inode->i_ino;
+		__entry->add = add;
+		__entry->old_ios = old_ios;
+		__entry->new_ios = new_ios;
+	),
+	TP_printk("dev %d:%d ino %llu %s i_conv_pending_ios %d -> %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino,
+		  __entry->add ? "inc" : "dec",
+		  __entry->old_ios, __entry->new_ios)
+);
+
 #endif /* _TRACE_EXT4_H */
 
 /* This part must be outside protection */

-- 
2.55.0

[PATCH 4/8] ext4: don't zeroout if we have inflight unwritten IOs in non buffered-iomap path

[Ojaswin Mujoo]

When endio conversion of an unwritten extent fails (e.g. ENOSPC while
trying to split the extent), ext4 falls back to zeroing out the unwritten
portions of the extent and marking it as written. This is safe only when no
other in-flight writes have valid data underneath other parts of the same
unwritten extent, otherwise the zeroout would silently corrupt that data.

Commit ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
changed the dioread_nolock writeback (non buffered iomap) and 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.

Fix this for dioread_nolock non buffered iomap writeback by making sure the
writeback path increments the i_conv_pending_ios counter when it encounters
an unwritten io_end, and decrements it after all the io_end_vecs have been
converted. The existing checks in zeroout paths then use this counter to
suppress the zeroout fallback whenever there are other unwritten IOs
pending conversion, preventing silent data corruption.

Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/ext4.h    |  7 ++++---
 fs/ext4/extents.c | 29 +++++++++++++++++++++++------
 fs/ext4/page-io.c |  6 ++++++
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1a01b1f809f7..44cd9f12d350 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -4113,14 +4113,15 @@ extern int ext4_resize_end(struct super_block *sb, bool update_backups);
 
 static inline void ext4_set_io_unwritten_flag(struct ext4_io_end *io_end)
 {
-	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN))
+	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN)) {
 		io_end->flag |= EXT4_IO_END_UNWRITTEN;
+		ext4_conv_pending_inc(io_end->inode);
+	}
 }
 
 static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end)
 {
-	if (io_end->flag & EXT4_IO_END_UNWRITTEN)
-		io_end->flag &= ~EXT4_IO_END_UNWRITTEN;
+	io_end->flag &= ~EXT4_IO_END_UNWRITTEN;
 }
 
 extern const struct iomap_ops ext4_iomap_ops;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d255884cf075..96d8ad012b2a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5286,6 +5286,19 @@ int ext4_convert_unwritten_io_end_vec(handle_t *handle, ext4_io_end_t *io_end)
 {
 	int ret = 0, err = 0;
 	struct ext4_io_end_vec *io_end_vec;
+	int n = 0;
+
+	/*
+	 * We take a single i_conv_pending_ios count for every unwritten io_end
+	 * however each io_vec also needs its count to prevent a neighboring
+	 * io_end_vec zeroing its data. This can't be accurately done during
+	 * submission so do it here.
+	 */
+	list_for_each_entry(io_end_vec, &io_end->list_vec, list)
+		n++;
+
+	if (n > 1)
+		ext4_conv_pending_sum(io_end->inode, n - 1);
 
 	/*
 	 * This is somewhat ugly but the idea is clear: When transaction is
@@ -5295,16 +5308,20 @@ int ext4_convert_unwritten_io_end_vec(handle_t *handle, ext4_io_end_t *io_end)
 	if (handle) {
 		handle = ext4_journal_start_reserved(handle,
 						     EXT4_HT_EXT_CONVERT);
-		if (IS_ERR(handle))
+		if (IS_ERR(handle)) {
+			ext4_conv_pending_sub(io_end->inode, n);
 			return PTR_ERR(handle);
+		}
 	}
 
 	list_for_each_entry(io_end_vec, &io_end->list_vec, list) {
-		ret = ext4_convert_unwritten_extents(handle, io_end->inode,
-						     io_end_vec->offset,
-						     io_end_vec->size, NULL, 0);
-		if (ret)
-			break;
+		if (!ret)
+			ret = ext4_convert_unwritten_extents(handle,
+							     io_end->inode,
+							     io_end_vec->offset,
+							     io_end_vec->size,
+							     NULL, 0);
+		ext4_conv_pending_dec(io_end->inode);
 	}
 
 	if (handle)
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 4c5c77da2a5e..00abef904702 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -190,6 +190,12 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 
 		if (test_opt(sb, DATA_ERR_ABORT))
 			jbd2_journal_abort(EXT4_SB(sb)->s_journal, ret);
+
+		/*
+		 * Conversion was skipped so drop the count that submission took
+		 */
+		if (io_end->flag & EXT4_IO_END_UNWRITTEN)
+			ext4_conv_pending_dec(inode);
 	} else {
 		ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
 	}

-- 
2.55.0

[PATCH 5/8] ext4: don't zeroout if we have inflight direct IOs

[Ojaswin Mujoo]

When endio conversion of an unwritten extent fails (e.g. ENOSPC while
trying to split the extent), ext4 falls back to zeroing out the unwritten
portions of the extent and marking it as written. This is safe only when no
other in-flight writes have valid data underneath other parts of the same
unwritten extent, otherwise the zeroout would silently corrupt that data.

Commit ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
changed the 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.

Buffered IO fixes this by counting such IOs on unwritten extents in
i_conv_pending_ios and decrementing them after conversion. However, due to
the way dio iomap is designed, it is not very straightforward for the FS
layer to increment once per unwritten IO.

Hence, to keep it simple use the existing inode->i_dio_count which is
incremented/decremented once per dio. zeroout code can now use
i_dio_count with i_conv_pending_ios to decide if zeroout is safe to do.

The only thing missing is the mutual exclusion of i_dio_count increment vs
the zeroout decision. To fix that, add ext4_conv_pending_barrier() in
ext4_iomap_begin() which just rotates i_conv_pending_sem before returning
an unwritten mapping for a write. This ensures that either a racing zeroout
has finished before we issue an IO over unwritten extent, or zeroout starts
after our dio is accounted for.

This design is overly cautious in disabling zeroout (eg, we will disable it
for all inflight dio, not just dio over unwritten extents) but it keeps the
code simple.

Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/ext4.h    |  2 ++
 fs/ext4/extents.c |  9 ++++++++-
 fs/ext4/inode.c   | 43 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 44cd9f12d350..bf390b3b5606 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -4002,6 +4002,8 @@ extern void ext4_conv_pending_sum(struct inode *inode, int n);
 extern int ext4_conv_pending_inc_trylock(struct inode *inode);
 extern void ext4_conv_pending_dec(struct inode *inode);
 extern void ext4_conv_pending_sub(struct inode *inode, int n);
+extern void ext4_conv_pending_barrier(struct inode *inode);
+extern int ext4_conv_pending_barrier_trylock(struct inode *inode);
 extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
 						   int num,
 						   struct ext4_ext_path *path);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 96d8ad012b2a..e4e65f3f576d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3333,10 +3333,17 @@ static bool ext4_conv_pending_zeroout_lock(struct inode *inode, int flags)
 	pending = atomic_read(&ei->i_conv_pending_ios);
 
 	WARN_ON_ONCE(pending < 0);
-	if (pending > thresh || pending < 0) {
+	if (pending < 0) {
 		up_write(&ei->i_conv_pending_sem);
 		return false;
 	}
+
+	pending += atomic_read(&inode->i_dio_count);
+	if (pending > thresh) {
+		up_write(&ei->i_conv_pending_sem);
+		return false;
+	}
+
 	return true;
 }
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 8b5cb37cdd6d..9d7d4da0071f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -692,9 +692,9 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n)
  * they might have data underneath which is pending endio conversion. Zeroing
  * out such an extent can cause silent corruption.
  *
- * To fix this we track the IOs which are about to undergo conversion in
- * ei->i_conv_pending_ios. The zeroout code then uses this counter to
- * determine if it's safe to zeroout.
+ * To fix this we track the no. of buffered IOs which are about to undergo
+ * conversion in ei->i_conv_pending_ios. The zeroout code then uses this counter
+ * to determine if it's safe to zeroout.
  *
  * Design Notes:
  *
@@ -715,6 +715,9 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n)
  * don't see any heavy lock contention. Atomic counters help parallel
  * modifications to be serialized.
  *
+ * Note that dio does not use i_conv_pending_ios; instead we use i_dio_count to
+ * disable zeroout if there are any other inflight dios.
+ *
  * ** Tolerance for false positives **
  *
  * We trade false positives for simplicity of implementation because having a
@@ -756,6 +759,30 @@ int ext4_conv_pending_inc_trylock(struct inode *inode)
 	return 0;
 }
 
+/*
+ * Rotate i_conv_pending_sem. This guarantees mutual exclusion against a
+ * concurrent endio zeroout, which holds i_conv_pending_sem exclusive. This
+ * barrier ensures either the zeroout has completed before we return, or it will
+ * start after we have accounted for the io and will see it while zeroing out.
+ * This can be used for paths that might not be changing ei->i_conv_pending_ios
+ */
+void ext4_conv_pending_barrier(struct inode *inode)
+{
+	down_read(&EXT4_I(inode)->i_conv_pending_sem);
+	up_read(&EXT4_I(inode)->i_conv_pending_sem);
+}
+
+/*
+ * Same as ext4_conv_pending_barrier() but doesn't sleep on i_conv_pending_sem.
+ */
+int ext4_conv_pending_barrier_trylock(struct inode *inode)
+{
+	if (!down_read_trylock(&EXT4_I(inode)->i_conv_pending_sem))
+		return -EAGAIN;
+	up_read(&EXT4_I(inode)->i_conv_pending_sem);
+	return 0;
+}
+
 void ext4_conv_pending_dec(struct inode *inode)
 {
 	_ext4_conv_pending_mod(inode, -1);
@@ -4073,6 +4100,16 @@ int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	if (ret < 0)
 		return ret;
 out:
+	if ((flags & IOMAP_WRITE) && (map.m_flags & EXT4_MAP_UNWRITTEN)) {
+		if (flags & IOMAP_NOWAIT) {
+			ret = ext4_conv_pending_barrier_trylock(inode);
+			if (ret)
+				return ret;
+		} else {
+			ext4_conv_pending_barrier(inode);
+		}
+	}
+
 	/*
 	 * Sometimes I/O to an encrypted file has to be broken up to guarantee
 	 * DUN contiguity.  Handle this by limiting the length of the mapping

-- 
2.55.0

[PATCH 6/8] ext4: make sure zero range WRITE_ZEROES uses NO_CONV_ACC flag

[Ojaswin Mujoo]

A zero range WRITE_ZEROES splits and converts an unwrit -> written extent
but it doesn't need any protection against the zeroout fallback in
ext4_split_extent_zeroout() because it's data is just zeroes anyways.

Hence, indicate to the zeroout code that the caller doesn't account for
itself in ei->i_conv_pending_ios.

Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e4e65f3f576d..d546001fedc6 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4765,7 +4765,7 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
 			ret = ext4_convert_unwritten_extents(handle,
 					inode, (loff_t)map.m_lblk << blkbits,
 					(loff_t)map.m_len << blkbits,
-					&converted, 0);
+					&converted, EXT4_GET_BLOCKS_NO_CONV_ACC);
 			if (ret)
 				map.m_len = converted;
 

-- 
2.55.0

[PATCH 7/8] ext4: don't zeroout if we have inflight unwritten IOs in buffered iomap path

[Ojaswin Mujoo]

The i_conv_pending_ios counter tracks the number of unwritten IOs that
are currently undergoing endio conversion, and is used to prevent
ext4_can_zeroout() from zeroing out unwritten extents that have valid
data pending conversion.

Enable the tracking for the buffered iomap writeback path by incrementing
i_conv_pending_ios once per submitted unwritten ioend in
ext4_iomap_writeback_submit() and decrement it after extent conversion.

Fixes: 8c4f6e337634 ("ext4: introduce a mount option for iomap buffered I/O path")
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/inode.c   |  3 +++
 fs/ext4/page-io.c | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9d7d4da0071f..0c924ad8210b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4585,6 +4585,9 @@ static int ext4_iomap_writeback_submit(struct iomap_writepage_ctx *wpc,
 	if (ioend->io_bio.bi_end_io)
 		bio_clear_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
 
+	if (ioend->io_flags & IOMAP_IOEND_UNWRITTEN)
+		ext4_conv_pending_inc(inode);
+
 	return iomap_ioend_writeback_submit(wpc, error);
 }
 
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 00abef904702..e99816017a18 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -652,6 +652,21 @@ static void ext4_iomap_finish_ioend(struct iomap_ioend *ioend)
 	int credits;
 	int ret, err;
 
+	/*
+	 * We increment conv_pending for each IO we submit but endio might have
+	 * merged them into a single ioend. The data of all merged sub-ioends is
+	 * covered by a single conversion, so drop their counts keeping just
+	 * the one count for the single ioend we will try to convert.
+	 */
+	if (ioend->io_flags & IOMAP_IOEND_UNWRITTEN) {
+		struct iomap_ioend *sub;
+
+		list_for_each_entry(sub, &ioend->io_list, io_list) {
+			if (sub->io_flags & IOMAP_IOEND_UNWRITTEN)
+				ext4_conv_pending_dec(inode);
+		}
+	}
+
 	ret = blk_status_to_errno(ioend->io_bio.bi_status);
 	if (is_disksize_grow)
 		trace_ext4_iomap_wb_disksize_pending_complete(ioend->io_inode,
@@ -704,6 +719,10 @@ static void ext4_iomap_finish_ioend(struct iomap_ioend *ioend)
 			 inode->i_ino, ret);
 	}
 out:
+	/* Drop the count of the head ioend now that conversion is done. */
+	if (ioend->io_flags & IOMAP_IOEND_UNWRITTEN)
+		ext4_conv_pending_dec(inode);
+
 	iomap_finish_ioends(ioend, ret);
 }
 

-- 
2.55.0

[PATCH 8/8] ext4: optimize inflight dio tracking for zeroout

[Ojaswin Mujoo]

Earlier zeroout used inode->i_dio_count to detect in-flight DIOs
that might have valid data underneath an unwritten extent. However,
i_dio_count accounts for every DIO on the inode, including reads and writes
that never touch an unwritten extent, which makes it prone to false
positives and needlessly suppresses the zeroout fallback.

Optimize this by moving DIO accounting to i_conv_pending_ios, which is only
incremented for DIOs that actually encounter an unwritten extent, and
decremented once at endio after the conversion is done.

The main issue is that ext4_iomap_begin() doesn't have any way to track
something only once per dio, hence, we use a custom ext4_iomap_next() that
utilizes iter->private to ensure we increment the i_conv_pending_ios only
once per a dio that encounters unwritten extents.

Now that dio has moved to i_conv_pending_ios, we need to pass NO_CONV_ACC
get blocks flag to ext4_ext_convert_to_initialized(). This path is
reachable from dio but doesn't need to account in i_conv_pending_ios
because we don't issue any IO before conversion, hence the flag helps
zeroout to make the right call.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/ext4.h    |  2 --
 fs/ext4/extents.c | 11 ++-----
 fs/ext4/file.c    | 12 +++++++-
 fs/ext4/inode.c   | 85 ++++++++++++++++++++++++++++---------------------------
 4 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bf390b3b5606..44cd9f12d350 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -4002,8 +4002,6 @@ extern void ext4_conv_pending_sum(struct inode *inode, int n);
 extern int ext4_conv_pending_inc_trylock(struct inode *inode);
 extern void ext4_conv_pending_dec(struct inode *inode);
 extern void ext4_conv_pending_sub(struct inode *inode, int n);
-extern void ext4_conv_pending_barrier(struct inode *inode);
-extern int ext4_conv_pending_barrier_trylock(struct inode *inode);
 extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
 						   int num,
 						   struct ext4_ext_path *path);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d546001fedc6..3150754b9766 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3333,13 +3333,7 @@ static bool ext4_conv_pending_zeroout_lock(struct inode *inode, int flags)
 	pending = atomic_read(&ei->i_conv_pending_ios);
 
 	WARN_ON_ONCE(pending < 0);
-	if (pending < 0) {
-		up_write(&ei->i_conv_pending_sem);
-		return false;
-	}
-
-	pending += atomic_read(&inode->i_dio_count);
-	if (pending > thresh) {
+	if (pending > thresh || pending < 0) {
 		up_write(&ei->i_conv_pending_sem);
 		return false;
 	}
@@ -4117,7 +4111,8 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	 * discovered unwritten extent to written.
 	 */
 	path = ext4_ext_convert_to_initialized(handle, inode, map, path,
-					       flags, allocated);
+					       flags | EXT4_GET_BLOCKS_NO_CONV_ACC,
+					       allocated);
 	if (IS_ERR(path))
 		return path;
 	ext4_update_inode_fsync_trans(handle, inode, 1);
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 06ef7a9b9f83..52b1fb99484d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -458,6 +458,15 @@ static int ext4_dio_write_end_io(struct kiocb *iocb, ssize_t size,
 	else if (!error && size && flags & IOMAP_DIO_UNWRITTEN)
 		error = ext4_convert_unwritten_extents(NULL, inode, pos, size,
 						       NULL, 0);
+
+	/*
+	 * If this dio covered an unwritten extent, we would have incremented
+	 * i_conv_pending_ios in ext4_iomap_next(). Decrement it now that
+	 * the conversion has completed (or failed).
+	 */
+	if (flags & IOMAP_DIO_UNWRITTEN)
+		ext4_conv_pending_dec(inode);
+
 	if (error)
 		return error;
 	/*
@@ -599,6 +608,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	size_t count = iov_iter_count(from);
 	bool extend = false;
 	bool ilock_shared = true;
+	bool conv_pending_inc = false;
 	int dio_flags = 0;
 
 	/*
@@ -664,7 +674,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	}
 
 	ret = iomap_dio_rw(iocb, from, &ext4_iomap_ops, &ext4_dio_write_ops,
-			   dio_flags, NULL, 0);
+			   dio_flags, &conv_pending_inc, 0);
 	if (ret == -ENOTBLK)
 		ret = 0;
 	if (extend) {
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 0c924ad8210b..46f91d6d8ed5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -692,9 +692,9 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n)
  * they might have data underneath which is pending endio conversion. Zeroing
  * out such an extent can cause silent corruption.
  *
- * To fix this we track the no. of buffered IOs which are about to undergo
- * conversion in ei->i_conv_pending_ios. The zeroout code then uses this counter
- * to determine if it's safe to zeroout.
+ * To fix this we track the IOs which are about to undergo conversion in
+ * ei->i_conv_pending_ios. The zeroout code then uses this counter to
+ * determine if it's safe to zeroout.
  *
  * Design Notes:
  *
@@ -715,9 +715,6 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n)
  * don't see any heavy lock contention. Atomic counters help parallel
  * modifications to be serialized.
  *
- * Note that dio does not use i_conv_pending_ios; instead we use i_dio_count to
- * disable zeroout if there are any other inflight dios.
- *
  * ** Tolerance for false positives **
  *
  * We trade false positives for simplicity of implementation because having a
@@ -759,30 +756,6 @@ int ext4_conv_pending_inc_trylock(struct inode *inode)
 	return 0;
 }
 
-/*
- * Rotate i_conv_pending_sem. This guarantees mutual exclusion against a
- * concurrent endio zeroout, which holds i_conv_pending_sem exclusive. This
- * barrier ensures either the zeroout has completed before we return, or it will
- * start after we have accounted for the io and will see it while zeroing out.
- * This can be used for paths that might not be changing ei->i_conv_pending_ios
- */
-void ext4_conv_pending_barrier(struct inode *inode)
-{
-	down_read(&EXT4_I(inode)->i_conv_pending_sem);
-	up_read(&EXT4_I(inode)->i_conv_pending_sem);
-}
-
-/*
- * Same as ext4_conv_pending_barrier() but doesn't sleep on i_conv_pending_sem.
- */
-int ext4_conv_pending_barrier_trylock(struct inode *inode)
-{
-	if (!down_read_trylock(&EXT4_I(inode)->i_conv_pending_sem))
-		return -EAGAIN;
-	up_read(&EXT4_I(inode)->i_conv_pending_sem);
-	return 0;
-}
-
 void ext4_conv_pending_dec(struct inode *inode)
 {
 	_ext4_conv_pending_mod(inode, -1);
@@ -4100,16 +4073,6 @@ int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	if (ret < 0)
 		return ret;
 out:
-	if ((flags & IOMAP_WRITE) && (map.m_flags & EXT4_MAP_UNWRITTEN)) {
-		if (flags & IOMAP_NOWAIT) {
-			ret = ext4_conv_pending_barrier_trylock(inode);
-			if (ret)
-				return ret;
-		} else {
-			ext4_conv_pending_barrier(inode);
-		}
-	}
-
 	/*
 	 * Sometimes I/O to an encrypted file has to be broken up to guarantee
 	 * DUN contiguity.  Handle this by limiting the length of the mapping
@@ -4132,7 +4095,47 @@ int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	return 0;
 }
 
-static DEFINE_IOMAP_ITER_NEXT(ext4_iomap_next, ext4_iomap_begin);
+static int ext4_iomap_next(const struct iomap_iter *iter, struct iomap *iomap,
+			   struct iomap *srcmap)
+{
+	int ret = iomap_iter_next(iter, iomap, srcmap, ext4_iomap_begin, NULL);
+
+	/*
+	 * For dio, increment i_conv_pending_ios the first time we see an
+	 * unwritten mapping for this IO, so that it cleanly balances with the
+	 * single endio call. We use iter->private to track state to ensure we
+	 * only increment once per dio regardless of how many extents it spans.
+	 * The decrement happens in ext4_dio_write_end_io() if
+	 * IOMAP_DIO_UNWRITTEN is set.
+	 *
+	 * Hack alert: iomap_dio_bio_iter() fails a misaligned mapping with
+	 * -EINVAL before it sets IOMAP_DIO_UNWRITTEN, so endio would skip the
+	 * decrement. Don't increment for such a mapping. The alignment check
+	 * mirrors the one in iomap_dio_bio_iter()
+	 */
+	if (ret > 0 && iter->private &&
+	    (iter->flags & IOMAP_WRITE) && (iomap->type == IOMAP_UNWRITTEN) &&
+	    IS_ALIGNED(iter->pos | iomap_length(iter),
+		       bdev_logical_block_size(iomap->bdev))) {
+		bool *inc_done = (bool *)iter->private;
+
+		if (!*inc_done) {
+			/* NOWAIT dio must not sleep on i_conv_pending_sem */
+			if (iter->flags & IOMAP_NOWAIT) {
+				int err;
+
+				err = ext4_conv_pending_inc_trylock(iter->inode);
+				if (err)
+					return err;
+			} else {
+				ext4_conv_pending_inc(iter->inode);
+			}
+			*inc_done = true;
+		}
+	}
+
+	return ret;
+}
 
 const struct iomap_ops ext4_iomap_ops = {
 	.iomap_next		= ext4_iomap_next,

-- 
2.55.0

Re: [PATCH 1/8] ext4: add extra_flags param to ext4_convert_unwritten_extents()

[sashiko-bot]

> Let callers pass in additional flags for ext4_map_blocks(). This is in
> preparation of the upcoming changes.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=1

Re: [PATCH 3/8] ext4: add tracing for i_conv_pending_ios tracking

[sashiko-bot]

> Add tracing to track changes to i_conv_pending_ios count.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=3

Re: [PATCH 7/8] ext4: don't zeroout if we have inflight unwritten IOs in buffered iomap path

[sashiko-bot]

> The i_conv_pending_ios counter tracks the number of unwritten IOs that
> are currently undergoing endio conversion, and is used to prevent
> ext4_can_zeroout() from zeroing out unwritten extents that have valid
> data pending conversion.
> 
> Enable the tracking for the buffered iomap writeback path by incrementing
> i_conv_pending_ios once per submitted unwritten ioend in
> ext4_iomap_writeback_submit() and decrement it after extent conversion.
> 
> Fixes: 8c4f6e337634 ("ext4: introduce a mount option for iomap buffered I/O path")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=7

Re: [PATCH 4/8] ext4: don't zeroout if we have inflight unwritten IOs in non buffered-iomap path

[sashiko-bot]

> When endio conversion of an unwritten extent fails (e.g. ENOSPC while
> trying to split the extent), ext4 falls back to zeroing out the unwritten
> portions of the extent and marking it as written. This is safe only when no
> other in-flight writes have valid data underneath other parts of the same
> unwritten extent, otherwise the zeroout would silently corrupt that data.
> 
> Commit ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
> changed the dioread_nolock writeback (non buffered iomap) and 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.
> 
> Fix this for dioread_nolock non buffered iomap writeback by making sure the
> writeback path increments the i_conv_pending_ios counter when it encounters
> an unwritten io_end, and decrements it after all the io_end_vecs have been
> converted. The existing checks in zeroout paths then use this counter to
> suppress the zeroout fallback whenever there are other unwritten IOs
> pending conversion, preventing silent data corruption.
> 
> Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=4

Re: [PATCH 8/8] ext4: optimize inflight dio tracking for zeroout

[sashiko-bot]

> Earlier zeroout used inode->i_dio_count to detect in-flight DIOs
> that might have valid data underneath an unwritten extent. However,
> i_dio_count accounts for every DIO on the inode, including reads and writes
> that never touch an unwritten extent, which makes it prone to false
> positives and needlessly suppresses the zeroout fallback.
> 
> Optimize this by moving DIO accounting to i_conv_pending_ios, which is only
> incremented for DIOs that actually encounter an unwritten extent, and
> decremented once at endio after the conversion is done.
> 
> The main issue is that ext4_iomap_begin() doesn't have any way to track
> something only once per dio, hence, we use a custom ext4_iomap_next() that
> utilizes iter->private to ensure we increment the i_conv_pending_ios only
> once per a dio that encounters unwritten extents.
> 
> Now that dio has moved to i_conv_pending_ios, we need to pass NO_CONV_ACC
> get blocks flag to ext4_ext_convert_to_initialized(). This path is
> reachable from dio but doesn't need to account in i_conv_pending_ios
> because we don't issue any IO before conversion, hence the flag helps
> zeroout to make the right call.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=8

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ext4: ext4_conv_pending_sub() takes i_conv_pending_sem on decrement
- [Medium] ext4: unnecessary zeroout write lock in ext4_ext_convert_to_initialized()
- [Medium] ext4: unnecessary conv_pending lock in ext4_split_extent_zeroout()
--

commit 93e49ed931682b3fa02d1b8dadca8a78a2dd31b3
Author: Ojaswin Mujoo <ojaswin@linux.ibm.com>

ext4: fix endio zeroout corruption by tracking unwritten to-be-converted IOs

This commit introduces ei->i_conv_pending_ios and ei->i_conv_pending_sem to
track in-flight IOs pending unwritten-to-written extent conversion so that
extent zeroout paths can avoid zeroing out blocks with pending IO data.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a229983f91dd8..d255884cf0750 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c

[ ... ]

> @@ -3328,6 +3362,9 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
>  	ee_len = ext4_ext_get_actual_len(ex);
>  	is_unwrit = ext4_ext_is_unwritten(ex);
>  
> +	if (!ext4_conv_pending_zeroout_lock(inode, flags))
> +		return -EINVAL;
> +
>  	if (flags & EXT4_GET_BLOCKS_CONVERT) {

[Severity: Medium]
Should ext4_conv_pending_zeroout_lock() only be acquired for the
EXT4_GET_BLOCKS_CONVERT branch in ext4_split_extent_zeroout() rather than
unconditionally at function entry?

When FALLOC_FL_ZERO_RANGE splits an initialized extent via:

  ext4_fallocate()
    -> ext4_zero_range()
      -> ext4_alloc_file_blocks()
        -> ext4_map_blocks()
          -> ext4_ext_map_blocks()
            -> convert_initialized_extent()
              -> ext4_split_convert_extents()
                -> ext4_split_extent()
                  -> ext4_split_extent_zeroout()

it enters the EXT4_GET_BLOCKS_CONVERT_UNWRITTEN branch:

	} else if (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
		if (is_unwrit) {
			/* Shouldn't happen. Just exit */
			err = -EINVAL;
			goto out_unlock;
		}

		lblk = map->m_lblk;
		len = map->m_len;
		pblk = ext4_ext_pblock(ex) + (map->m_lblk - ee_block);
		err = ext4_issue_zeroout(inode, lblk, pblk, len);

That branch only operates on an already-initialized (!is_unwrit) extent and
only zeroes the requested range [map->m_lblk, map->m_lblk + map->m_len),
where ext4_fallocate() and ext4_zero_range() have already waited for DIO and
truncated the page cache for that range.

Acquiring ext4_conv_pending_zeroout_lock() unconditionally holds
i_conv_pending_sem exclusively across ext4_issue_zeroout() on a written
extent and causes the zeroout fallback to fail with -EINVAL whenever
unrelated unwritten writeback IOs are in flight elsewhere on the inode.

[ ... ]

> @@ -3743,7 +3795,8 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
>  	split_map.m_lblk = map->m_lblk;
>  	split_map.m_len = map->m_len;
>  
> -	if (max_zeroout && (*allocated > split_map.m_len)) {
> +	if (max_zeroout && (*allocated > split_map.m_len) &&
> +	    ext4_conv_pending_zeroout_lock(inode, flags)) {
>  		if (*allocated <= max_zeroout) {
>  			/* case 3 or 5 */
>  			zero_ex1.ee_block =
> @@ -3756,7 +3809,7 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
>  				split_map.m_len - ee_block);
>  			err = ext4_ext_zeroout(inode, &zero_ex1);
>  			if (err)
> -				goto fallback;
> +				goto zeroout_unlock;
>  			split_map.m_len = *allocated;
>  		}
>  		if (split_map.m_lblk - ee_block + split_map.m_len <
> @@ -3770,16 +3823,17 @@ ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
>  						      ext4_ext_pblock(ex));
>  				err = ext4_ext_zeroout(inode, &zero_ex2);
>  				if (err)
> -					goto fallback;
> +					goto zeroout_unlock;
>  			}
>  
>  			split_map.m_len += split_map.m_lblk - ee_block;
>  			split_map.m_lblk = ee_block;
>  			*allocated = map->m_len;
>  		}
> +zeroout_unlock:
> +		ext4_conv_pending_zeroout_unlock(inode);
>  	}

[Severity: Medium]
In ext4_ext_convert_to_initialized(), does calling
ext4_conv_pending_zeroout_lock() in the outer condition take
down_write(&ei->i_conv_pending_sem) even when neither the tail nor the head
is small enough to qualify for zeroout?

When max_zeroout is non-zero and *allocated > split_map.m_len, a partial
conversion of an unwritten extent larger than max_zeroout where neither
*allocated <= max_zeroout nor split_map.m_lblk - ee_block + split_map.m_len <
max_zeroout holds will acquire i_conv_pending_sem for write and immediately
unlock it at zeroout_unlock without issuing any zeroout.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738b..280ba3d359e3b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -669,6 +669,98 @@ int ext4_map_query_blocks(handle_t *handle, struct inode *inode,

[ ... ]

> + * 1. Incrementing the counter must down_read(i_conv_pending_sem).
> + * 2. A zeroout must check the counter and zero data under
> + *    down_write(i_conv_pending_sem)
> + * 3. Decrementing the counter needs no lock. At worst, this makes zeroout bail
> + *    out due to a stale, elevated counter value, which is acceptable
> + * 4. Changes to i_conv_pending_ios should use atomic helpers.

[ ... ]

> +void ext4_conv_pending_dec(struct inode *inode)
> +{
> +	_ext4_conv_pending_mod(inode, -1);
> +}
> +
> +void ext4_conv_pending_sub(struct inode *inode, int n)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +
> +	down_read(&ei->i_conv_pending_sem);
> +	_ext4_conv_pending_mod(inode, -n);
> +	up_read(&ei->i_conv_pending_sem);
> +}

[Severity: Medium]
Why does ext4_conv_pending_sub() acquire down_read(&ei->i_conv_pending_sem)
when subtracting from i_conv_pending_ios?

The locking rules in struct ext4_inode_info and above ext4_conv_pending_inc()
state that decrementing the counter needs no lock, and ext4_conv_pending_dec()
calls _ext4_conv_pending_mod(inode, -1) directly without taking
i_conv_pending_sem.

Taking down_read(&ei->i_conv_pending_sem) in ext4_conv_pending_sub() causes
callers decrementing the counter to sleep behind any writer holding or waiting
for down_write(&ei->i_conv_pending_sem) during synchronous zeroout IO, and
prevents ext4_conv_pending_sub() from being used in non-sleeping completion
contexts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=2

Re: [PATCH 5/8] ext4: don't zeroout if we have inflight direct IOs

[sashiko-bot]

> When endio conversion of an unwritten extent fails (e.g. ENOSPC while
> trying to split the extent), ext4 falls back to zeroing out the unwritten
> portions of the extent and marking it as written. This is safe only when no
> other in-flight writes have valid data underneath other parts of the same
> unwritten extent, otherwise the zeroout would silently corrupt that data.
> 
> Commit ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
> changed the 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.
> 
> Buffered IO fixes this by counting such IOs on unwritten extents in
> i_conv_pending_ios and decrementing them after conversion. However, due to
> the way dio iomap is designed, it is not very straightforward for the FS
> [ ... ]
> for all inflight dio, not just dio over unwritten extents) but it keeps the
> code simple.
> 
> Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=5

Re: [PATCH 6/8] ext4: make sure zero range WRITE_ZEROES uses NO_CONV_ACC flag

[sashiko-bot]

> A zero range WRITE_ZEROES splits and converts an unwrit -> written extent
> but it doesn't need any protection against the zeroout fallback in
> ext4_split_extent_zeroout() because it's data is just zeroes anyways.
> 
> Hence, indicate to the zeroout code that the caller doesn't account for
> itself in ei->i_conv_pending_ios.
> 
> Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ext4-endio-zero-bug-v1-0-266d2bb9e6dc@linux.ibm.com?part=6