Re: [PATCH 5/5] generic/733: disable for btrfs

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 22, 2024 at 11:08:00AM -0400, Josef Bacik wrote:
> On Thu, Mar 21, 2024 at 05:52:33PM -0400, Kent Overstreet wrote:
> > On Thu, Mar 21, 2024 at 02:36:47PM -0700, Christoph Hellwig wrote:
> > > On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> > > > It is a bug in the test, it should have been xfs specific and never
> > > > promoted to generic/ and not affect btrfs unless explained how
> > > > in the first place.
> > > 
> > > Well, the concept that reflinks are reasonably fast and are not live
> > > locked by ongoing I/O seems pretty natural.  But I guess we can't
> > > just asusme quality of implementation everywhere, and do an opt-in
> > > like _require_non_sucky_reflink.  Either way we need to document
> > > the assumptions and not add a magic exclude for a single fs.
> > 
> > generic/733 runs just fine on bcachefs, sounds like btrfs folks just
> > need to fix their filesystem
> 
> Neither of these comments are particularly helpful or relevant to the
> conversation.
> 
> Btrfs range locks the extent during the clone operation, and also range locks
> the area that it reads.  This doesn't make it "sucky" or "worse", simply
> different.  I'm not entirely sure why protecting a range of extents that's
> currently being modified is considered "bad" or "broken".
> 
> In any case, I can accept that we need to have a different option for skipping
> this test, but this is sort of an argument for the shared/ directory or some
> other mechanism, or at the very least validating the test passes on all the
> major file systems before including it as a generic test.
> 
> Adding an opt in _require feels like the best option, or we can simply keep
> excluding it in our own fstests branch and ignore the upstream branch.  I'm good
> with either solution.  Thanks,

Frankly I'd have been ok with a per-test exclusion with some
documentation:

test $FSTYP = btrfs && \
	_notrun 'FIXME: btrfs does not support concurrent read+ficlone'

There's nothing in the FICLONE spec about implementations needing to
support concurrent reads and clones on the source file.

--D

> Josef
>