Re: [PATCH v3 3/3] Btrfs: implement full reflink support for inline extents

Filipe Manana
2020-03-05T11:58:06Z

On Tue, Mar 3, 2020 at 9:19 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On 2/24/20 12:13 PM, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > There are a few cases where we don't allow cloning an inline extent into
> > the destination inode, returning -EOPNOTSUPP to user space. This was done
> > to prevent several types of file corruption and because it's not very
> > straightforward to deal with these cases, as they can't rely on simply
> > copying the inline extent between leaves. Such cases require copying the
> > inline extent's data into the respective page of the destination inode.
> >
> > Not supporting these cases makes it harder and more cumbersome to write
> > applications/libraries that work on any filesystem with reflink support,
> > since all these cases for which btrfs fails with -EOPNOTSUPP work just
> > fine on xfs for example. These unsupported cases are also not documented
> > anywhere and explaining which exact cases fail require a bit of too
> > technical understanding of btrfs's internal (inline extents and when and
> > where can they exist in a file), so it's not really user friendly.
> >
> > Also some test cases from fstests that use fsx, such as generic/522 for
> > example, can sporadically fail because they trigger one of these cases,
> > and fsx expects all operations to succeed.
> >
> > This change adds supports for cloning all these cases by copying the
> > inline extent's data into the respective page of the destination inode.
> >
> > With this change test case btrfs/112 from fstests fails because it
> > expects some clone operations to fail, so it will be updated. Also a
> > new test case that exercises all these previously unsupported cases
> > will be added to fstests.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

So this actually isn't safe.

It can bring back the race that leads to file extent items with
overlapping ranges. Not because of the hole detection part but because
of the part where we copy extent items from the fs/subvolume tree into
the log tree using btrfs_search_forward(), as we copy all extent
items, including the ones outside the fsync range - so we could race
in the same way as we did during hole detection with ordered extent
completion for ordered extents outside the range.

I'll have to rework this a bit.

thanks

>
> Thanks,
>
> Josef