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

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

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