[PATCH v2 0/2] btrfs: drain in-flight direct IO before direct writes dirty the page cache

[PATCH v2 0/2] btrfs: drain in-flight direct IO before direct writes dirty the page cache

[Breno Leitao]

Atish reported that arm64 hosts with 64K pages and 4K btrfs blocks,
which keep VM disk images as NOCOW files opened with O_DIRECT, log

  Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!

and the next fsync() on the image fails with -EIO. Every write the VM
issues is O_DIRECT, the buffered writes come from btrfs itself.

Both patches close the same race: a direct write dirties a folio while
another task's direct write to a different block of that folio is in
flight, and the completion of the latter fails to invalidate it.

Patch 1 waits before the buffered fallback for unaligned writes. Patch 2
waits before btrfs_write_check() zeroes the block at an unaligned EOF,
which btrfs_direct_write() does before it decides to fall back.

Tested on a 64K page arm64 kernel, on a NOCOW file of a 4K block btrfs.
Async 4K O_DIRECT writes race 512 byte O_DIRECT writes inside EOF
(fallback), and O_DIRECT writes past an unaligned EOF, of 4K (eof-dio)
and of 512 bytes, which fall back (eof-fb). fdatasync() EIOs in 20
seconds per test:

              base   patch 1   patch 1+2
  fallback       6         0           0
  eof-dio       24        20           0
  eof-fb        34        14           0

60 second runs got 56 and 38 EIOs in the eof tests with patch 1, and
none in any test with both patches, in two runs. The reproducer for
patch 1 is at [1].

[1] https://github.com/leitao/btrfs-dio-fallback

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changes in v2:
- New patch 2: drain in-flight direct IO before btrfs_write_check()
  zeroes the block at an unaligned EOF (Qu, from a Sashiko review).
- Patch 1: no code change. Picked up Boris' Reviewed-by, and added a
  stable Cc.
- Both patches: the Fixes: tags now name 95ea0486b20e, which first
  allowed writes with more than one block per folio, 4K blocks on 64K
  pages.
- Link to v1: https://lore.kernel.org/all/auxv7Tg0xSq5yHmb@gmail.com/

---
Breno Leitao (2):
      btrfs: drain in-flight direct IO before buffered write fallback
      btrfs: drain in-flight direct IO before zeroing an unaligned EOF block

 fs/btrfs/file.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
---
base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
change-id: 20261106-btrfs_page_cache-2ab11049c913

Best regards,
--  
Breno Leitao <leitao@debian.org>

[PATCH v2 1/2] btrfs: drain in-flight direct IO before buffered write fallback

[Breno Leitao]

When btrfs_direct_write() cannot do a write as direct IO, because the
offset, the length or the user buffer is not aligned to the sectorsize,
or the range has compressed extents or page cache, it falls back to
btrfs_buffered_write(). That dirties the page cache without waiting for
direct IO still in flight from other tasks:

1) Task A submits an async direct write to block 1 of a folio. iomap
   invalidates the page cache in the range and submits the bio, and the
   inode lock is dropped.
2) Task B issues a 512 byte O_DIRECT write to block 0 of the same folio.
   btrfs_direct_write() falls back to btrfs_buffered_write(), which
   dirties the folio.
3) Task A's write completes. kiocb_invalidate_post_direct_write() finds
   the folio dirty, invalidate_inode_pages2_range() fails with -EBUSY,
   and dio_warn_stale_pagecache() logs:

   [    2.048568] Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!
   [    2.048574] File: /tmp/diorepro.JTCKpJ/block.img PID: 260 Comm: kworker/11:1

   It also records -EIO in the mapping, so the next fsync() fails.

The two writes do not touch the same bytes, they only share a folio.
With 4K blocks on 64K pages a folio holds 16 blocks, so this is easy to
hit there. Meta's arm64 hosts with 64K pages hit it with VM disk images
that are NOCOW files on btrfs, opened with O_DIRECT. In a test, XFS on a
loop device with direct IO on such a file gets EIO on a log write and
shuts down within seconds.

Wait for in-flight direct IO before the fallback dirties the page cache,
as ext4 does since commit 15cdefd0c052 ("ext4: drain in-flight DIO
before buffered write fallback"). btrfs_buffered_write() holds the inode
lock exclusively there, so no new direct IO can start until the write is
done. NOWAIT writes never get there, as btrfs_direct_write() returns
-EAGAIN for them before falling back.

A reproducer that keeps 4K libaio O_DIRECT writes in flight while other
threads issue 512 byte O_DIRECT writes to the same folios of a NOCOW
file got 1123 fdatasync() EIOs in 10 seconds on a 64K page VM, and none
with this patch.

Fixes: 95ea0486b20e ("btrfs: allow read-write for 4K sectorsize on 64K page size systems")
Cc: stable@vger.kernel.org
Reported-by: Atish Patra <atish.patra@linux.dev>
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 fs/btrfs/file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f978c6524aa03f..2017a96814d857 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1334,6 +1334,14 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *iter)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * A direct write that fell back to buffered IO must not dirty folios
+	 * while direct IO is in flight: its completion fails to invalidate
+	 * them and records -EIO in the mapping.
+	 */
+	if (iocb->ki_flags & IOCB_DIRECT)
+		inode_dio_wait(inode);
+
 	/*
 	 * We can only trust the isize with inode lock held, or it can race with
 	 * other buffered writes and cause incorrect call of

-- 
2.53.0-Meta

[PATCH v2 2/2] btrfs: drain in-flight direct IO before zeroing an unaligned EOF block

[Breno Leitao]

A write that starts past an unaligned i_size first zeroes the rest of
the block at i_size: btrfs_write_check() calls btrfs_cont_expand(),
whose btrfs_truncate_block() reads the folio, zeroes it past i_size and
marks that block dirty.

Reading the folio waits for the ordered extents of direct writes to the
other blocks of the folio, but not for their iomap completion, which
runs separately from the ordered extent completion and is where the
page cache gets invalidated:

1) Task A has an async direct write in flight to block 0 of a folio.
2) A fallen back write or a truncate left i_size in the middle of block
   1 of the same folio. Task B issues an O_DIRECT write past i_size, and
   btrfs_truncate_block() waits for the ordered extent of Task A.
3) Task A's write completes and its ordered extent finishes, so Task B
   zeroes the rest of block 1 and dirties the folio. When iomap then
   completes Task A's write, kiocb_invalidate_post_direct_write() finds
   the folio dirty and records -EIO in the mapping, so the next fsync()
   fails.

btrfs_direct_write() calls btrfs_write_check() before it decides to fall
back to buffered IO, so this hits direct writes past EOF as well as the
unaligned ones that fall back: for those, the inode_dio_wait() in
btrfs_buffered_write() runs after the folio is dirty.

Wait for in-flight direct IO before zeroing, as XFS does in
xfs_file_write_zero_eof(). Unlike waiting for the ordered extents,
inode_dio_wait() covers the invalidation, as iomap_dio_complete() calls
inode_dio_end() after it.

Only an unaligned i_size needs the wait, as btrfs_truncate_block()
dirties nothing otherwise. Buffered and encoded writes past such an
i_size wait as well, since they share btrfs_write_check().

A reproducer keeps 4K AIO O_DIRECT writes in flight to the first 15
blocks of a folio of a NOCOW file. Another task truncates the file to
end inside the last block of that folio, then writes past EOF.

On a 64K page VM it got 56 fdatasync() EIOs in 60 seconds before this
patch, 38 when the write past EOF is 512 bytes and falls back, and none
in two runs with this patch.

Qu Wenruo pointed this path out, from a Sashiko review.

Fixes: 95ea0486b20e ("btrfs: allow read-write for 4K sectorsize on 64K page size systems")
Cc: stable@vger.kernel.org
Reported-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 fs/btrfs/file.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 2017a96814d857..15802db7b664b8 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1063,6 +1063,13 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
 		/* Expand hole size to cover write data, preventing empty gap */
 		loff_t end_pos = round_up(pos + count, fs_info->sectorsize);
 
+		/*
+		 * Zeroing the block at an unaligned EOF dirties its folio, so
+		 * drain in-flight direct IO first, see btrfs_buffered_write().
+		 */
+		if (!IS_ALIGNED(oldsize, fs_info->sectorsize))
+			inode_dio_wait(inode);
+
 		ret = btrfs_cont_expand(BTRFS_I(inode), oldsize, end_pos);
 		if (ret)
 			return ret;

-- 
2.53.0-Meta