Skip to content

btrfs: fix generic/362 failures with nodatasum

Qu Wenruo linux-btrfs stable 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest3 patches 1 replyIn review
  2. v23 patches 4 repliesSuperseded
  3. v12 patches 5 repliesSuperseded

Changes in v3 from the cover letter

- Change the timing of isize revert to avoid race with buffered read Normally we need to take extent lock to update isize, but in this particular case, the ordered extent we're holding acts like an extent lock, thus we are safe to modify the isize as long as the OE is not finished.

- Add a comment on why it's safe to revert the isize

- Explain that the isize revert is not perfect
We should hold the extent lock until endio/iomap_end, and only update the isize for a successful submission.

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/3btrfs: fix false IO failure after falling back to buffered writefs/btrfs/direct-io.c fs/btrfs/inode.c fs/btrfs/ordered-data.c +1 more+29 -80 replies
2/3btrfs: fix incorrect buffered IO fallback for append direct writesfs/btrfs/direct-io.c+42 -11 reply
3/3btrfs: retry faulting in the pages after a zero sized short direct writefs/btrfs/direct-io.c+1 -10 replies

Reviews trailers per patch

All 3 patches have a Reviewed-by or Acked-by.

PatchBoris Burkov
1/3R-b
2/3R-b
3/3R-b

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v3 thread

Cover letter [PATCH v3 0/3] btrfs: fix generic/362 failures with nodatasum

Changelog
v3:
- Change the timing of isize revert to avoid race with buffered read Normally we need to take extent lock to update isize, but in this particular case, the ordered extent we're holding acts like an extent lock, thus we are safe to modify the isize as long as the OE is not finished.

- Add a comment on why it's safe to revert the isize

- Explain that the isize revert is not perfect
We should hold the extent lock until endio/iomap_end, and only update the isize for a successful submission.

But that will require quite a lot of extra work, and should be done later by a dedicated series other than a short fix.

v2:
- Add a new patch to allow retry after no byte is submitted for direct write

- Add a more clean callchain to explain why __iomap_dio_rw() doesn't return -EFAULT/-ENOTBLK directly.

- Add a code comment explaining the OE split sitation and why we can truncate all the remaining OE after a short write

- Fix the isize revert when part of the direct write succeeded

- Dig deeper into the original cause
Which is very old, dates back to v5.15 LTS at least, and add a reason why no specific fixes tag is provided.

The test case generic/362 is pretty well hidden by several factors:

- "nodatasum" will not take effect due to the bad design of the test case
If the target file already exists, the test case will reuse it, meanwhile "nodatasum" mount option only affects new files, meaning if the test case is executed before with data checksum, it will never go through the nodatasum path.

There is already an update on the test case to make the failure reliably reproducible:
https://lore.kernel.org/linux-btrfs/[email protected]/

- Btrfs always falls back to buffered IO if the inode has csum Thus we do not exercise the zero-copy path, hide the failure for the default mount option.

With that said, the test case failure needs to be fixed, and there are several bugs in the direct write path:

- Treat any short write as an error
This is especially common as we have disabled page faulting for reading from the @from iov_iter.

This is fixed by the first patch.

- No isize rollback after a short write
The isize is increased at btrfs_get_blocks_direct_write() but when a short write happened, the isize is not propoerly rolledback, causing later buffered fallback to write at the new isize.

This is fixed by the second patch.

- No page fault-in retry after a zero-submitted short write Unlike the previous two, this is only a minor problem which reduces the chance to do zero-copy direct IO.

This is fixed by the third patch.

Qu Wenruo (3):
btrfs: fix false IO failure after falling back to buffered write
btrfs: fix incorrect buffered IO fallback for append direct writes
btrfs: retry faulting in the pages after a zero sized short direct
write

 fs/btrfs/direct-io.c    | 62 +++++++++++++++++++++++++++++++++++++----
 fs/btrfs/inode.c        |  6 +---
 fs/btrfs/ordered-data.c | 12 ++++++++
 fs/btrfs/ordered-data.h |  2 ++
 4 files changed, 72 insertions(+), 10 deletions(-)
Signature · 1 line
2.54.0