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, Well, if you really don't intend to change btrfs that's fine; if it was me I consider performance bugs to be bugs - not cases where we're off by some small ratio, but if it's significant I'd want to fix it. But if it's just that you're doing range locks (not locking the whole inode, as was my previous impression) - that makes sense, there's a read vs. write atomicity tradeoff there.
