| Message ID | 094a1bdc304b236ba41aff4da454abe3c93a355c.1710871719.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs fstests fixups and updates | expand |
On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote: > This tests if a clone source can be read but in btrfs there's an > exclusive lock and the test always fails. The functionality might be > implemented in btrfs in the future but for now disable the test. Well, this sounds like btrf sis broken and it should fail? > # real QA test starts here > -_supported_fs generic > +_supported_fs ^btrfs and just throwing random not supported in generic testes is just a mine field. You;d better explain very well why something is not supported, and do that by adding a _require_. If you can't explain the _require this probably actually is a bug and not just an exclude..
On Tue, Mar 19, 2024 at 02:01:00PM -0700, Christoph Hellwig wrote: > On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote: > > This tests if a clone source can be read but in btrfs there's an > > exclusive lock and the test always fails. The functionality might be > > implemented in btrfs in the future but for now disable the test. > > Well, this sounds like btrf sis broken and it should fail? > > > # real QA test starts here > > -_supported_fs generic > > +_supported_fs ^btrfs > > > and just throwing random not supported in generic testes is just a mine > field. You;d better explain very well why something is not supported, > and do that by adding a _require_. If you can't explain the > _require this probably actually is a bug and not just an exclude.. I don't think there's a good way to _require-test for holding i_rwsem in exclusive or shared mode during an FICLONE operation. But that ought to be a comment somewhere at least. --D
On Tue, Mar 19, 2024 at 02:10:50PM -0700, Darrick J. Wong wrote: > I don't think there's a good way to _require-test for holding i_rwsem in > exclusive or shared mode during an FICLONE operation. But that ought to > be a comment somewhere at least. Throwing random per-fs hacks into generic tests simply does not scale. We'll need a way to describe and opt into these behaviors in common helpers.
On Tue, Mar 19, 2024 at 02:01:00PM -0700, Christoph Hellwig wrote: > On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote: > > This tests if a clone source can be read but in btrfs there's an > > exclusive lock and the test always fails. The functionality might be > > implemented in btrfs in the future but for now disable the test. > > Well, this sounds like btrf sis broken and it should fail? It does not sound like that to me, rather that the test was tailored for xfs and assumes too much about the behaviour that does not apply to btrfs. If you look at src/t_reflink_read_race.c it's full of magic constants used for timing, this itself points to an unreliable test depending on hardware and other system load. > > > # real QA test starts here > > -_supported_fs generic > > +_supported_fs ^btrfs > > > and just throwing random not supported in generic testes is just a mine > field. You;d better explain very well why something is not supported, > and do that by adding a _require_. If you can't explain the > _require this probably actually is a bug and not just an exclude.. 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.
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.
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
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, Josef
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 >
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.
diff --git a/tests/generic/733 b/tests/generic/733 index d88d92a4705add..adb141e190b7b0 100755 --- a/tests/generic/733 +++ b/tests/generic/733 @@ -18,7 +18,7 @@ _begin_fstest auto clone punch . ./common/reflink # real QA test starts here -_supported_fs generic +_supported_fs ^btrfs _require_scratch_reflink _require_cp_reflink _require_xfs_io_command "fpunch"