From cf02c310d018c7e5363b80dfd68ffdf159f3c914 Mon Sep 17 00:00:00 2001 From: Boris Burkov Date: Tue, 15 Sep 2026 10:58:33 -0700 Subject: [PATCH] btrfs: downgrade the reflink source inode lock for tree walking When doing a reflink, we are not writing to the src inode or modifying it in any way, so it is counter-intuitive to have to lock it exclusively. This has the cost of blocking other shared lock users of the src file for the duration of any new writeback since the optimistic pre-flush and all the actual reflinking. So we would like to downgrade the lock to shared as much as possible. There are two important gotchas to do with direct io that must be addressed to make this idea work. The first gotcha is that dio writes that come in under EOF try to take inode->i_rwsem shared, which would result in messed up views of the extents in the reflinked file. Therefore, add "being used as a reflink src" as another inode flag exception for the dio write shared tests that already exist to check for other dangerous conditions. In order to ensure that this flag is visible to the dio writer, reflink must first take i_rwsem exclusive, then set the flag, then downgrade to shared. The second is that dio reads increment inode->i_dio_count which blocks inode_dio_wait() in __generic_remap_file_range_prep(). Therefore, we must remain exclusive to dio reads through finishing the prep or risk starving the clone on a stream of dio readers. There is still O(extents) work in btrfs_clone() which can be done without holding the lock exclusive, so we should downgrade to shared and admit dio readers (and other shared lock users like lseek) after we are finished with inode_dio_wait(). By running concurrent clones and lseeks or dio reads on a src file with 32k extents on a vm, I can demonstrate 100ms+ stalls on the readers without this patch and they go away entirely with the patch. Reviewed-by: Filipe Manana Signed-off-by: Boris Burkov Signed-off-by: David Sterba --- fs/btrfs/btrfs_inode.h | 6 ++++++ fs/btrfs/direct-io.c | 7 +++++++ fs/btrfs/reflink.c | 17 +++++++++++++++-- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index 46c62f980c24..03c00c99099d 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -99,6 +99,12 @@ enum { * range). */ BTRFS_INODE_COW_WRITE_ERROR, + /* + * Set by reflink while it holds the source's VFS inode lock shared. + * Direct IO writes also take that lock shared; one that finds this bit + * set after acquiring it must retry with the exclusive lock. + */ + BTRFS_INODE_REFLINK_SRC, /* * Indicate this is a directory that points to a subvolume for which * there is no root reference item. That's a case like the following: diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c index 3075d7992713..ed3914257d39 100644 --- a/fs/btrfs/direct-io.c +++ b/fs/btrfs/direct-io.c @@ -913,6 +913,13 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) goto relock; } + if ((ilock_flags & BTRFS_ILOCK_SHARED) && + test_bit(BTRFS_INODE_REFLINK_SRC, &BTRFS_I(inode)->runtime_flags)) { + btrfs_inode_unlock(BTRFS_I(inode), ilock_flags); + ilock_flags &= ~BTRFS_ILOCK_SHARED; + goto relock; + } + ret = generic_write_checks(iocb, from); if (ret <= 0) { btrfs_inode_unlock(BTRFS_I(inode), ilock_flags); diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c index 8e37706b98c3..b5d2c094941d 100644 --- a/fs/btrfs/reflink.c +++ b/fs/btrfs/reflink.c @@ -939,6 +939,7 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off, struct btrfs_inode *dst_inode = BTRFS_I(file_inode(dst_file)); bool same_inode = dst_inode == src_inode; u64 wb_start, wb_len; + bool src_downgraded = false; int ret; if (btrfs_is_shutdown(src_inode->root->fs_info)) @@ -976,6 +977,12 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off, if (ret < 0 || len == 0) goto out_unlock; + if (!same_inode) { + set_bit(BTRFS_INODE_REFLINK_SRC, &src_inode->runtime_flags); + downgrade_write(&src_inode->vfs_inode.i_rwsem); + src_downgraded = true; + } + if (remap_flags & REMAP_FILE_DEDUP) ret = btrfs_extent_same(src_inode, off, len, dst_inode, destoff); else @@ -986,8 +993,14 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off, btrfs_inode_unlock(src_inode, BTRFS_ILOCK_MMAP); } else { btrfs_double_mmap_unlock(src_inode, dst_inode); - unlock_two_nondirectories(&src_inode->vfs_inode, - &dst_inode->vfs_inode); + if (src_downgraded) { + clear_bit(BTRFS_INODE_REFLINK_SRC, &src_inode->runtime_flags); + inode_unlock_shared(&src_inode->vfs_inode); + inode_unlock(&dst_inode->vfs_inode); + } else { + unlock_two_nondirectories(&src_inode->vfs_inode, + &dst_inode->vfs_inode); + } } /*