* [PATCH v2 0/3] reduce reflink src inode lock contention
@ 2026-09-11 0:26 Boris Burkov
2026-09-11 0:26 ` [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks Boris Burkov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Boris Burkov @ 2026-09-11 0:26 UTC (permalink / raw)
To: linux-btrfs, kernel-team
We currently take the src and dst inode i_rwsem exclusive for the
entirety of a reflink which includes writing back all of both files and
walking all the src extents and duplicating them to dst. This results in
contention on those locks which I think can be reduced.
The basic strategy is:
1. try to do obvious easy src writeback outside the lock.
2. downgrade the src lock to shared where it is safe.
Boris Burkov (3):
btrfs: pre-flush reflink source before taking inode locks
btrfs: skip unlocked reflink source flush if the inode has writers
btrfs: downgrade the reflink source inode lock for tree walking
fs/btrfs/btrfs_inode.h | 6 ++++
fs/btrfs/direct-io.c | 7 ++++
fs/btrfs/reflink.c | 72 ++++++++++++++++++++++++++++++++++++++++--
3 files changed, 83 insertions(+), 2 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks
2026-09-11 0:26 [PATCH v2 0/3] reduce reflink src inode lock contention Boris Burkov
@ 2026-09-11 0:26 ` Boris Burkov
2026-09-11 9:17 ` Filipe Manana
2026-09-11 0:26 ` [PATCH v2 2/3] btrfs: skip unlocked reflink source flush if the inode has writers Boris Burkov
2026-09-11 0:26 ` [PATCH v2 3/3] btrfs: downgrade the reflink source inode lock for tree walking Boris Burkov
2 siblings, 1 reply; 6+ messages in thread
From: Boris Burkov @ 2026-09-11 0:26 UTC (permalink / raw)
To: linux-btrfs, kernel-team
Consider the following sketch of a shell script:
dd if=/dev/urandom of=/mnt/src bs=1M count=4096
cp --reflink=always /mnt/src /mnt/dst &
sleep 0.1 # let the clone reach its flush
time dd if=/mnt/src of=/dev/null bs=4K count=1 iflag=direct
The current logic in reflink ensures the existence and stability of both
the src and destination inode by locking them both and then flushing all
dirty pages / ordered_extents under the inode lock. This blocks
concurrent usage by even readers of the inode locks, like direct reads
or seeks for the duration of writeback on the entirety of the two files.
We observe this particular contention frequently in the Meta fleet.
It is also a relatively common pattern to write the src file, then reflink
it while it is still dirty, so that typical path naturally hits this
contention. This workload motivates a relatively simple optimization:
trigger the unavoidable src flushing outside the locked region.
It is tempting to try to move the writeback out of the locks entirely
but this is fraught with all kinds of consistency errors under various
patterns of concurrent writes. fsync is able to manage such a pattern,
but with significant infrastructure investment I don't think is
justified for reflink.
Therefore, do an optimistic single backwards pass which completes latent
OEs while relying on the calls to btrfs_wait_ordered_range() under the
locks in btrfs_remap_file_range_prep() ensure correctness. In the worst
case with concurrent writes while unlocked, we can end up doing the
writeback twice, which I think is a reasonable price to pay to avoid
victimizing innocent readers in a common case.
With and without this patch, the above reproducer runs the reflink in
the same ~0.5s on my system. Without the patch, the direct read blocks
for basically the full duration of the reflink while with the patch it
returns in a few milliseconds.
Signed-off-by: Boris Burkov <boris@bur.io>
---
fs/btrfs/reflink.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index d2a4101912bd..7bab391f1be4 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -923,6 +923,46 @@ static bool file_sync_write(const struct file *file)
return false;
}
+/*
+ * Do a single backwards pass waiting for ordered extents on the inode.
+ *
+ * Backwards is helpful because it avoids picking up concurrent appended OEs
+ * and the intent of the function is to wait on pre-existing OEs without going
+ * to the extreme of locking the ordered_tree and snapshotting its contents.
+ *
+ * This is only useful as an optimization for waiting for ordered extents outside
+ * locks. It DOES NOT ensure that the range is free of ordered extents.
+ *
+ * Returns -EIO if any waited on OE had the ORDERED_IOERR bit set and 0 otherwise.
+ */
+static int wait_existing_ordered_extents(struct btrfs_inode *inode)
+{
+ u64 orig_end = i_size_read(&inode->vfs_inode);
+ u64 end = orig_end;
+ int ret = 0;
+
+ while (true) {
+ struct btrfs_ordered_extent *ordered;
+
+ ordered = btrfs_lookup_first_ordered_extent(inode, end);
+ if (!ordered)
+ break;
+ if (ordered->file_offset > end) {
+ btrfs_put_ordered_extent(ordered);
+ break;
+ }
+ btrfs_start_ordered_extent(ordered);
+ end = ordered->file_offset;
+ if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags))
+ ret = -EIO;
+ btrfs_put_ordered_extent(ordered);
+ if (!end)
+ break;
+ end--;
+ }
+ return ret;
+}
+
loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
struct file *dst_file, loff_t destoff, loff_t len,
unsigned int remap_flags)
@@ -938,6 +978,14 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
return -EINVAL;
+ ret = filemap_flush(src_inode->vfs_inode.i_mapping);
+ if (ret < 0)
+ return ret;
+
+ ret = wait_existing_ordered_extents(src_inode);
+ if (ret < 0)
+ return ret;
+
if (same_inode) {
btrfs_inode_lock(src_inode, BTRFS_ILOCK_MMAP);
} else {
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] btrfs: skip unlocked reflink source flush if the inode has writers
2026-09-11 0:26 [PATCH v2 0/3] reduce reflink src inode lock contention Boris Burkov
2026-09-11 0:26 ` [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks Boris Burkov
@ 2026-09-11 0:26 ` Boris Burkov
2026-09-11 0:26 ` [PATCH v2 3/3] btrfs: downgrade the reflink source inode lock for tree walking Boris Burkov
2 siblings, 0 replies; 6+ messages in thread
From: Boris Burkov @ 2026-09-11 0:26 UTC (permalink / raw)
To: linux-btrfs, kernel-team
The optimistic unlocked flushing is counter-productive if there is an
active writer dirtying pages, as it results in doubly writing back any
pages that get dirtied again before the locked flush. Therefore,
pessimize it slightly and don't do the unlocked flushing if we have
outstanding writable file descriptors or memory mappings. Since the
inode isn't locked, this is only advisory, any new mapping or fd that
sneaks in after the check but before the locking can still cause extra
writeback. But it trivially helps quite a bit in simple cases with a
long open file descriptor or mapping.
Signed-off-by: Boris Burkov <boris@bur.io>
---
fs/btrfs/reflink.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index 7bab391f1be4..abe44769179a 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -978,13 +978,20 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
return -EINVAL;
- ret = filemap_flush(src_inode->vfs_inode.i_mapping);
- if (ret < 0)
- return ret;
+ /*
+ * If there are no outstanding writers, it is likely to be helpful to
+ * flush any latent dirty pages before locking the inodes.
+ */
+ if (atomic_read(&src_inode->vfs_inode.i_writecount) <= 0 &&
+ !mapping_writably_mapped(src_inode->vfs_inode.i_mapping)) {
+ ret = filemap_flush(src_inode->vfs_inode.i_mapping);
+ if (ret < 0)
+ return ret;
- ret = wait_existing_ordered_extents(src_inode);
- if (ret < 0)
- return ret;
+ ret = wait_existing_ordered_extents(src_inode);
+ if (ret < 0)
+ return ret;
+ }
if (same_inode) {
btrfs_inode_lock(src_inode, BTRFS_ILOCK_MMAP);
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] btrfs: downgrade the reflink source inode lock for tree walking
2026-09-11 0:26 [PATCH v2 0/3] reduce reflink src inode lock contention Boris Burkov
2026-09-11 0:26 ` [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks Boris Burkov
2026-09-11 0:26 ` [PATCH v2 2/3] btrfs: skip unlocked reflink source flush if the inode has writers Boris Burkov
@ 2026-09-11 0:26 ` Boris Burkov
2 siblings, 0 replies; 6+ messages in thread
From: Boris Burkov @ 2026-09-11 0:26 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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 all the writeback and reflinking.
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().
Signed-off-by: Boris Burkov <boris@bur.io>
---
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 abe44769179a..22c55633cfb4 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -970,6 +970,7 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
struct btrfs_inode *src_inode = BTRFS_I(file_inode(src_file));
struct btrfs_inode *dst_inode = BTRFS_I(file_inode(dst_file));
bool same_inode = dst_inode == src_inode;
+ bool src_downgraded = false;
int ret;
if (btrfs_is_shutdown(src_inode->root->fs_info))
@@ -1005,6 +1006,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
@@ -1015,8 +1022,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);
+ }
}
/*
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks
2026-09-11 0:26 ` [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks Boris Burkov
@ 2026-09-11 9:17 ` Filipe Manana
2026-09-11 21:51 ` Boris Burkov
0 siblings, 1 reply; 6+ messages in thread
From: Filipe Manana @ 2026-09-11 9:17 UTC (permalink / raw)
To: Boris Burkov; +Cc: linux-btrfs, kernel-team
On Fri, Sep 11, 2026 at 1:39 AM Boris Burkov <boris@bur.io> wrote:
>
> Consider the following sketch of a shell script:
> dd if=/dev/urandom of=/mnt/src bs=1M count=4096
> cp --reflink=always /mnt/src /mnt/dst &
> sleep 0.1 # let the clone reach its flush
> time dd if=/mnt/src of=/dev/null bs=4K count=1 iflag=direct
>
> The current logic in reflink ensures the existence and stability of both
> the src and destination inode by locking them both and then flushing all
> dirty pages / ordered_extents under the inode lock. This blocks
> concurrent usage by even readers of the inode locks, like direct reads
> or seeks for the duration of writeback on the entirety of the two files.
>
> We observe this particular contention frequently in the Meta fleet.
>
> It is also a relatively common pattern to write the src file, then reflink
> it while it is still dirty, so that typical path naturally hits this
> contention. This workload motivates a relatively simple optimization:
> trigger the unavoidable src flushing outside the locked region.
>
> It is tempting to try to move the writeback out of the locks entirely
> but this is fraught with all kinds of consistency errors under various
> patterns of concurrent writes. fsync is able to manage such a pattern,
> but with significant infrastructure investment I don't think is
> justified for reflink.
>
> Therefore, do an optimistic single backwards pass which completes latent
> OEs while relying on the calls to btrfs_wait_ordered_range() under the
> locks in btrfs_remap_file_range_prep() ensure correctness. In the worst
> case with concurrent writes while unlocked, we can end up doing the
> writeback twice, which I think is a reasonable price to pay to avoid
> victimizing innocent readers in a common case.
>
> With and without this patch, the above reproducer runs the reflink in
> the same ~0.5s on my system. Without the patch, the direct read blocks
> for basically the full duration of the reflink while with the patch it
> returns in a few milliseconds.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/reflink.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> index d2a4101912bd..7bab391f1be4 100644
> --- a/fs/btrfs/reflink.c
> +++ b/fs/btrfs/reflink.c
> @@ -923,6 +923,46 @@ static bool file_sync_write(const struct file *file)
> return false;
> }
>
> +/*
> + * Do a single backwards pass waiting for ordered extents on the inode.
> + *
> + * Backwards is helpful because it avoids picking up concurrent appended OEs
> + * and the intent of the function is to wait on pre-existing OEs without going
> + * to the extreme of locking the ordered_tree and snapshotting its contents.
> + *
> + * This is only useful as an optimization for waiting for ordered extents outside
> + * locks. It DOES NOT ensure that the range is free of ordered extents.
> + *
> + * Returns -EIO if any waited on OE had the ORDERED_IOERR bit set and 0 otherwise.
> + */
> +static int wait_existing_ordered_extents(struct btrfs_inode *inode)
> +{
> + u64 orig_end = i_size_read(&inode->vfs_inode);
> + u64 end = orig_end;
What's the point of orig_end if it's not used elsehwere?
Just this:
u64 end = i_size_read(&inode->vfs_inode);
> + int ret = 0;
> +
> + while (true) {
> + struct btrfs_ordered_extent *ordered;
> +
> + ordered = btrfs_lookup_first_ordered_extent(inode, end);
> + if (!ordered)
> + break;
> + if (ordered->file_offset > end) {
> + btrfs_put_ordered_extent(ordered);
> + break;
> + }
> + btrfs_start_ordered_extent(ordered);
> + end = ordered->file_offset;
> + if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags))
> + ret = -EIO;
> + btrfs_put_ordered_extent(ordered);
> + if (!end)
> + break;
> + end--;
> + }
> + return ret;
Why do we need another function to wait for ordered extents just for reflinks?
We have btrfs_wait_ordered_range() that does exactly the same...
Besides that, this waits for all ordered extents. If a reflink
operates on a small range, we end up waiting for any ordered extents,
which slows down such ranged reflinks.
btrfs_wait_ordered_range() allows to pass a range.
> +}
> +
> loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> struct file *dst_file, loff_t destoff, loff_t len,
> unsigned int remap_flags)
> @@ -938,6 +978,14 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> return -EINVAL;
>
> + ret = filemap_flush(src_inode->vfs_inode.i_mapping);
So again, this flushes the entire file, which adds overhead for
reflinks operating on a small range and causes unnecessary IO.
Further, using filemap_flush() is not enough in case we have compression.
The flush call only starts the compression work in an async worker, we
need a second flush call to wait for the compression to finish and for
writeback to start (creating ordered extents).
That's why we have btrfs_fdatawrite_range(), which does the double
flush in case we have compression, and also supports specifying a
range.
Thanks.
> + if (ret < 0)
> + return ret;
> +
> + ret = wait_existing_ordered_extents(src_inode);
> + if (ret < 0)
> + return ret;
> +
> if (same_inode) {
> btrfs_inode_lock(src_inode, BTRFS_ILOCK_MMAP);
> } else {
> --
> 2.55.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks
2026-09-11 9:17 ` Filipe Manana
@ 2026-09-11 21:51 ` Boris Burkov
0 siblings, 0 replies; 6+ messages in thread
From: Boris Burkov @ 2026-09-11 21:51 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs, kernel-team
On Fri, Sep 11, 2026 at 10:17:27AM +0100, Filipe Manana wrote:
> On Fri, Sep 11, 2026 at 1:39 AM Boris Burkov <boris@bur.io> wrote:
> >
> > Consider the following sketch of a shell script:
> > dd if=/dev/urandom of=/mnt/src bs=1M count=4096
> > cp --reflink=always /mnt/src /mnt/dst &
> > sleep 0.1 # let the clone reach its flush
> > time dd if=/mnt/src of=/dev/null bs=4K count=1 iflag=direct
> >
> > The current logic in reflink ensures the existence and stability of both
> > the src and destination inode by locking them both and then flushing all
> > dirty pages / ordered_extents under the inode lock. This blocks
> > concurrent usage by even readers of the inode locks, like direct reads
> > or seeks for the duration of writeback on the entirety of the two files.
> >
> > We observe this particular contention frequently in the Meta fleet.
> >
> > It is also a relatively common pattern to write the src file, then reflink
> > it while it is still dirty, so that typical path naturally hits this
> > contention. This workload motivates a relatively simple optimization:
> > trigger the unavoidable src flushing outside the locked region.
> >
> > It is tempting to try to move the writeback out of the locks entirely
> > but this is fraught with all kinds of consistency errors under various
> > patterns of concurrent writes. fsync is able to manage such a pattern,
> > but with significant infrastructure investment I don't think is
> > justified for reflink.
> >
> > Therefore, do an optimistic single backwards pass which completes latent
> > OEs while relying on the calls to btrfs_wait_ordered_range() under the
> > locks in btrfs_remap_file_range_prep() ensure correctness. In the worst
> > case with concurrent writes while unlocked, we can end up doing the
> > writeback twice, which I think is a reasonable price to pay to avoid
> > victimizing innocent readers in a common case.
> >
> > With and without this patch, the above reproducer runs the reflink in
> > the same ~0.5s on my system. Without the patch, the direct read blocks
> > for basically the full duration of the reflink while with the patch it
> > returns in a few milliseconds.
> >
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> > fs/btrfs/reflink.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> > index d2a4101912bd..7bab391f1be4 100644
> > --- a/fs/btrfs/reflink.c
> > +++ b/fs/btrfs/reflink.c
> > @@ -923,6 +923,46 @@ static bool file_sync_write(const struct file *file)
> > return false;
> > }
> >
> > +/*
> > + * Do a single backwards pass waiting for ordered extents on the inode.
> > + *
> > + * Backwards is helpful because it avoids picking up concurrent appended OEs
> > + * and the intent of the function is to wait on pre-existing OEs without going
> > + * to the extreme of locking the ordered_tree and snapshotting its contents.
> > + *
> > + * This is only useful as an optimization for waiting for ordered extents outside
> > + * locks. It DOES NOT ensure that the range is free of ordered extents.
> > + *
> > + * Returns -EIO if any waited on OE had the ORDERED_IOERR bit set and 0 otherwise.
> > + */
> > +static int wait_existing_ordered_extents(struct btrfs_inode *inode)
> > +{
> > + u64 orig_end = i_size_read(&inode->vfs_inode);
> > + u64 end = orig_end;
>
> What's the point of orig_end if it's not used elsehwere?
> Just this:
>
> u64 end = i_size_read(&inode->vfs_inode);
>
>
> > + int ret = 0;
> > +
> > + while (true) {
> > + struct btrfs_ordered_extent *ordered;
> > +
> > + ordered = btrfs_lookup_first_ordered_extent(inode, end);
> > + if (!ordered)
> > + break;
> > + if (ordered->file_offset > end) {
> > + btrfs_put_ordered_extent(ordered);
> > + break;
> > + }
> > + btrfs_start_ordered_extent(ordered);
> > + end = ordered->file_offset;
> > + if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags))
> > + ret = -EIO;
> > + btrfs_put_ordered_extent(ordered);
> > + if (!end)
> > + break;
> > + end--;
> > + }
> > + return ret;
>
> Why do we need another function to wait for ordered extents just for reflinks?
>
> We have btrfs_wait_ordered_range() that does exactly the same...
>
> Besides that, this waits for all ordered extents. If a reflink
> operates on a small range, we end up waiting for any ordered extents,
> which slows down such ranged reflinks.
> btrfs_wait_ordered_range() allows to pass a range.
>
TL;DR you are right
I started with btrfs_wait_ordered_range() then got turned around while
exploring the space and ended here which turns out to not be any better.
Thanks for pushing back.
Basically I was battling against cases related to the one helped by the
second patch (concurrent writers producing dirtying which causes write
amplification with the duplicate flush outside the locks) and tried to
create a "lightest weight good enough" flush for this application.
However, I don't think I succeeded and can't think of anything actually
better than btrfs_wait_ordered_range(). (and it's worse for using
SYNC_NONE and doing the whole file...)
> > +}
> > +
> > loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> > struct file *dst_file, loff_t destoff, loff_t len,
> > unsigned int remap_flags)
> > @@ -938,6 +978,14 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> > if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> > return -EINVAL;
> >
> > + ret = filemap_flush(src_inode->vfs_inode.i_mapping);
>
> So again, this flushes the entire file, which adds overhead for
> reflinks operating on a small range and causes unnecessary IO.
>
> Further, using filemap_flush() is not enough in case we have compression.
> The flush call only starts the compression work in an async worker, we
> need a second flush call to wait for the compression to finish and for
> writeback to start (creating ordered extents).
> That's why we have btrfs_fdatawrite_range(), which does the double
> flush in case we have compression, and also supports specifying a
> range.
Interestingly, this turns out to not really matter due to how the folio
locking works today (and that we don't care about 100% perfect
submission anyway). Basically the second folio gets blocked right away
in filemap_flush and waits until the first folio's async compression
finishes and unlocks it. This is the same pattern that Qu is contending
with in his parent-child OE redesign to try to make compressed writeback
look "normal".
There is 0 benefit I can demonstrate objectively from relying on this so
I am going to switch back to plain btrfs_wait_ordered_range() for v3.
>
> Thanks.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = wait_existing_ordered_extents(src_inode);
> > + if (ret < 0)
> > + return ret;
> > +
> > if (same_inode) {
> > btrfs_inode_lock(src_inode, BTRFS_ILOCK_MMAP);
> > } else {
> > --
> > 2.55.0
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-11 21:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-11 0:26 [PATCH v2 0/3] reduce reflink src inode lock contention Boris Burkov
2026-09-11 0:26 ` [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks Boris Burkov
2026-09-11 9:17 ` Filipe Manana
2026-09-11 21:51 ` Boris Burkov
2026-09-11 0:26 ` [PATCH v2 2/3] btrfs: skip unlocked reflink source flush if the inode has writers Boris Burkov
2026-09-11 0:26 ` [PATCH v2 3/3] btrfs: downgrade the reflink source inode lock for tree walking Boris Burkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.