On Tue, Feb 18, 2025 at 11:10 PM Qu Wenruo wqu@suse.com wrote:

[BUG] When running generic/417 with a btrfs whose block size < page size

generic/417 -> generic/418

(subpage cases), it always fails.

And the following minimal reproducer is more than enough to trigger it reliably:

workload() { mkfs.btrfs -s 4k -f $dev > /dev/null dmesg -C mount $dev $mnt $fsstree_dir/src/dio-invalidate-cache -r -b 4096 -n 3 -i 1 -f $mnt/diotest ret=$? umount $mnt stop_trace if [ $ret -ne 0 ]; then fail fi }

for (( i = 0; i < 1024; i++)); do echo "=== $i/$runtime ===" workload done

[CAUSE] With extra trace printk added to the following functions:

• btrfs_buffered_write()

  • Which folio is touched
  • The file offset (start) where the buffered write is at
  • How many bytes are copied
  • The content of the write (the first 2 bytes)

• submit_one_sector()

  • Which folio is touched
  • The position inside the folio

• pagecache_isize_extended()

  • The parameters of the function itself
  • The parameters of the folio_zero_range()

Which are enough to show the problem:

22.158114: btrfs_buffered_write: folio pos=0 start=0 copied=4096 content=0x0101 22.158161: submit_one_sector: r/i=5/257 folio=0 pos=0 content=0x0101 22.158609: btrfs_buffered_write: folio pos=0 start=4096 copied=4096 content=0x0101 22.158634: btrfs_buffered_write: folio pos=0 start=8192 copied=4096 content=0x0101 22.158650: pagecache_isize_extended: folio=0 from=4096 to=8192 bsize=4096 zero off=4096 len=8192 22.158682: submit_one_sector: r/i=5/257 folio=0 pos=4096 content=0x0000 22.158686: submit_one_sector: r/i=5/257 folio=0 pos=8192 content=0x0101

The tool dio-invalidate-cache will start 3 threads, each doing a buffered write with 0x01 at 4096 * i (i is 0, 1 ,2), do a fsync, then do a direct read, and compare the read buffer with the write buffer.

Note that all 3 btrfs_buffered_write() are writing the correct 0x01 into the page cache.

But at submit_one_sector(), at file offset 4096, the content is zeroed out, mostly by pagecache_isize_extended().

The race happens like this: Thread A is writing into range [4K, 8K). Thread B is writing into range [8K, 12k).

           Thread A              |         Thread B

-------------------------------------+------------------------------------ btrfs_buffered_write() | btrfs_buffered_write() |- old_isize = 4K; | |- old_isize = 4096; |- btrfs_inode_lock() | | |- write into folio range [4K, 8K) | | |- pagecache_isize_extended() | | | extend isize from 4096 to 8192 | | | no folio_zero_range() called | | |- btrfs_inode_lock() | | | |- btrfs_inode_lock() | |- write into folio range [8K, 12K) | |- pagecache_isize_extended() | | calling folio_zero_range(4K, 8K) | | This is caused by the old_isize is | | grabbed too early, without any | | inode lock. | |- btrfs_inode_unlock()

The @old_isize is grabbed without inode lock, causing race between two buffered write threads and making pagecache_isize_extended() to zero range which is still containing cached data.

And this is only affecting subpage btrfs, because for regular blocksize == page size case, the function pagecache_isize_extended() will do nothing if the block size >= page size.

[FIX] Grab the old isize with inode lock hold.

hold -> held

Or easier to understand:

Grab the old i_size while holding the inode lock.

This means each buffered write thread will have a stable view of the old inode size, thus avoid the above race.

Cc: stable@vger.kernel.org Signed-off-by: Qu Wenruo wqu@suse.com

Fixes: 5e8b9ef30392 ("btrfs: move pos increment and pagecache extension to btrfs_buffered_write")

That is the commit that introduced the race, before it we were grabbing i_size and doing the pagecache_isize_extended() call while holding the inode lock.

---

fs/btrfs/file.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index fd90855fe717..896dc03689d6 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1090,7 +1090,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i) u64 lockend; size_t num_written = 0; ssize_t ret;

•   loff_t old_isize = i_size_read(inode);
  

•   loff_t old_isize;
    unsigned int ilock_flags = 0;
    const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
    unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
  

@@ -1103,6 +1103,13 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i) if (ret < 0) return ret;

•   /*
  

•    * We can only trust the isize with inode lock hold, or it can race with
  

Same here, hold -> held

With those things addressed:

Reviewed-by: Filipe Manana fdmanana@suse.com

Thanks.

•    * other buffered writes and cause incorrect call of
  

•    * pagecache_isize_extended() to overwrite existing data.
  

•    */
  

•   old_isize = i_size_read(inode);
  

•   ret = generic_write_checks(iocb, i);
    if (ret <= 0)
            goto out;
  

-- 2.48.1