Skip to content

Commit ff66fe6

Browse files
adam900710morbidrsa
authored andcommitted
btrfs: fix incorrect buffered IO fallback for append direct writes
[BUG] With the previous bug of short direct writes fixed, test case generic/362 (*) still fails with the following error with nodatasum mount option: generic/362 0s ... - output mismatch (see /home/adam/xfstests/results//generic/362.out.bad) - output mismatch (see /home/adam/xfstests/results//generic/362.out.bad) --- tests/generic/362.out 2024-08-24 15:31:37.200000000 +0930 +++ /home/adam/xfstests/results//generic/362.out.bad 2026-05-27 10:13:09.072485767 +0930 @@ -1,2 +1,3 @@ QA output created by 362 +Wrong file size after first write, got 8192 expected 4096 Silence is golden ... *: If the test case has been executed before with default data checksum, the failure will not reproduce. Need the following fix to make it reliably reproducible: https://lore.kernel.org/linux-btrfs/20260528111659.87113-1-wqu@suse.com/ [CAUSE] Inside btrfs_dio_iomap_begin() for a direct write, we increase the isize if it's beyond the current isize. But if the direct io finished short, we do not revert the isize to the previous value nor to the short write end. Then if we need to fall back to buffered writes, and the write has IOCB_APPEND flag, then the buffered write will be positioned at the incorrect isize. The call chain looks like this: btrfs_direct_write(pos=0, length=4K) |- __iomap_dio_rw() | |- iomap_iter() | | |- btrfs_dio_iomap_begin() | | |- btrfs_get_blocks_direct_write() | | |- i_size_write() | | Which updates the isize to the write end (4K). | | | |- iomap_dio_iter() | | Failed with -EFAULT on the first page. | | | |- iomap_iter() | | |- btrfs_dio_iomap_end() | | Detects a short write, return -ENOTBLK | |- if (ret == -ENOTBLK) { ret = 0;} | Which resets the return value. | |- ret = iomap_dio_complet() | Which returns 0. | |- btrfs_buffered_write(iocb, from); |- generic_write_checks() |- iocb->ki_pos = i_size_read() Which is still the new size (4K), other than the original isize 0. [FIX] Introduce the following btrfs_dio_data members: - old_isize - updated_isize If the direct write has enlarged the isize. Then if we got a short write, and btrfs_dio_data::updated_isize is set, revert to the correct isize based on old_isize and current file position. And here we call i_size_write() without holding an extent lock, which is a very special case that we're safe to do: - Only a single writer can be enlarging isize Enlarging isize will take the exclusive inode lock. - Buffered readers need to wait for the OE we're holding Buffered readers will lock extent and wait for OE of the folio range. Sometimes we can skip the OE wait, but since all page cache is invalidated, the OE wait can not be skipped. But I do not think this is the most elegant solution, nor covers all cases. E.g. if the bio is submitted but IO failed, we are unable to do the revert. I believe the more elegant one would be extend the EXTENT_DIO_LOCKED lifespan for direct writes, so that we can update the isize when a write beyond EOF finished successfully. However that change is too huge for a small bug fix. So only implement the minimal partial fix for now. [REASON FOR NO FIXES TAG] The bug is again very old, before commit f85781f ("btrfs: switch to iomap for direct IO") we are already increasing isize without a proper rollback for short writes. Thus only a CC to stable. CC: stable@vger.kernel.org # 5.15+ Reviewed-by: Boris Burkov <boris@bur.io> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
1 parent 66ff4d3 commit ff66fe6

1 file changed

Lines changed: 42 additions & 1 deletion

File tree

fs/btrfs/direct-io.c

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,12 @@
1515

1616
struct btrfs_dio_data {
1717
ssize_t submitted;
18+
loff_t old_isize;
1819
struct extent_changeset *data_reserved;
1920
struct btrfs_ordered_extent *ordered;
2021
bool data_space_reserved;
2122
bool nocow_done;
23+
bool updated_isize;
2224
};
2325

2426
struct btrfs_dio_private {
@@ -228,6 +230,7 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
228230
bool space_reserved = false;
229231
u64 len = *lenp;
230232
u64 prev_len;
233+
loff_t old_isize;
231234
int ret = 0;
232235

233236
/*
@@ -341,8 +344,14 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
341344
* Need to update the i_size under the extent lock so buffered
342345
* readers will get the updated i_size when we unlock.
343346
*/
344-
if (start + len > i_size_read(inode))
347+
old_isize = i_size_read(inode);
348+
if (start + len > old_isize) {
349+
if (!dio_data->updated_isize) {
350+
dio_data->old_isize = old_isize;
351+
dio_data->updated_isize = true;
352+
}
345353
i_size_write(inode, start + len);
354+
}
346355
out:
347356
if (ret && space_reserved) {
348357
btrfs_delalloc_release_extents(BTRFS_I(inode), len);
@@ -625,6 +634,38 @@ static int btrfs_dio_iomap_end(struct inode *inode, loff_t pos, loff_t length,
625634
pos += submitted;
626635
length -= submitted;
627636
if (write) {
637+
/*
638+
* Got a short write and have updated the isize, need to
639+
* revert the isize change.
640+
*
641+
* Normally we need to update isize with extent lock hold,
642+
* but we're safe due to the following factors:
643+
*
644+
* - Only a single writer can be enlarging isize
645+
* Enlarging isize will take the exclusive inode lock.
646+
*
647+
* - Buffered readers need to wait for the OE we're holding
648+
* Buffered readers will lock extent and wait for OE
649+
* of the folio range, and since page cache is invalidated
650+
* the OE wait can not be skipped.
651+
*
652+
* So here we are safe to revert the isize before
653+
* finishing the OE, and no reader of the remaining range
654+
* can see the enlarged size.
655+
*
656+
* TODO: Extend the DIO_LOCKED lifespan for direct writes,
657+
* and only enlarge isize after a successful write.
658+
*/
659+
if (dio_data->updated_isize) {
660+
u64 new_isize;
661+
662+
if (submitted == 0)
663+
new_isize = dio_data->old_isize;
664+
else
665+
new_isize = max(dio_data->old_isize, pos);
666+
i_size_write(inode, new_isize);
667+
dio_data->updated_isize = false;
668+
}
628669
/*
629670
* We have a short write, if there is any range
630671
* that is submitted properly, that part will have

0 commit comments

Comments
 (0)