Re: [PATCH v2 1/3] btrfs: pre-flush reflink source before taking inode locks

[Boris Burkov <boris@bur.io>]

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
> >
> >