| Message ID | bc7149309a8eca5999f22477a838602023094cb8.1662048451.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove 'seek' group from btrfs/007 | expand |
On Thu, Sep 01, 2022 at 05:12:25PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > btrfs/007 does not test lseek, it tests send/receive and lseek is not > exercised anywhere in this test. So just remove it from the 'seek' group. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- This 'seek' group is brought in by below commit: commit 6fd9210bc97710f81e5a7646a2abfd11af0f0c28 Author: Christoph Hellwig <hch@lst.de> Date: Mon Feb 18 10:05:03 2019 +0100 fstests: add a seek group So I'd like to let Christoph help to double check it. Thanks, Zorro > tests/btrfs/007 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/btrfs/007 b/tests/btrfs/007 > index ed7d143a..c4a8d9d0 100755 > --- a/tests/btrfs/007 > +++ b/tests/btrfs/007 > @@ -13,7 +13,7 @@ > owner=list.btrfs@jan-o-sch.net > > . ./common/preamble > -_begin_fstest auto quick rw metadata send seek > +_begin_fstest auto quick rw metadata send > > # Override the default cleanup function. > _cleanup() > -- > 2.35.1 >
On Fri, Sep 02, 2022 at 10:00:30AM +0800, Zorro Lang wrote: > On Thu, Sep 01, 2022 at 05:12:25PM +0100, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > btrfs/007 does not test lseek, it tests send/receive and lseek is not > > exercised anywhere in this test. So just remove it from the 'seek' group. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > This 'seek' group is brought in by below commit: > > commit 6fd9210bc97710f81e5a7646a2abfd11af0f0c28 > Author: Christoph Hellwig <hch@lst.de> > Date: Mon Feb 18 10:05:03 2019 +0100 > > fstests: add a seek group > > So I'd like to let Christoph help to double check it. It's quite obvious from the test itself that it tests only send/receive, which is mentioned in the changelog. The commit adding the seek group does not provide any rationale so it's hard to argue but as it stands now the 'seek' group should not be there.
On Fri, Sep 02, 2022 at 11:44:24AM +0200, David Sterba wrote: > > commit 6fd9210bc97710f81e5a7646a2abfd11af0f0c28 > > Author: Christoph Hellwig <hch@lst.de> > > Date: Mon Feb 18 10:05:03 2019 +0100 > > > > fstests: add a seek group > > > > So I'd like to let Christoph help to double check it. > > It's quite obvious from the test itself that it tests only send/receive, > which is mentioned in the changelog. The commit adding the seek group > does not provide any rationale so it's hard to argue but as it stands > now the 'seek' group should not be there. Probably. Unless it somehow exercised seeks through the userspace seek code I can't see any good rationale for this addition, and the patch was far too long ago for me to remember.
On Mon, Sep 05, 2022 at 08:35:39AM +0200, Christoph Hellwig wrote: > On Fri, Sep 02, 2022 at 11:44:24AM +0200, David Sterba wrote: > > > commit 6fd9210bc97710f81e5a7646a2abfd11af0f0c28 > > > Author: Christoph Hellwig <hch@lst.de> > > > Date: Mon Feb 18 10:05:03 2019 +0100 > > > > > > fstests: add a seek group > > > > > > So I'd like to let Christoph help to double check it. > > > > It's quite obvious from the test itself that it tests only send/receive, > > which is mentioned in the changelog. The commit adding the seek group > > does not provide any rationale so it's hard to argue but as it stands > > now the 'seek' group should not be there. > > Probably. Unless it somehow exercised seeks through the userspace > seek code I can't see any good rationale for this addition, and the > patch was far too long ago for me to remember. Hi, I just tried to learn about the history of this *problem*: At first, Jan Schmidt added src/fssum.c into fstests by df0fd18101b6 ("xfstests: add fssum tool"). In this original version, fssum does SEEK_DATA in both sum_file_data_permissive() and sum_file_data_strict(), that means it always does SEEK_DATA. So all cases run fssum, need SEEK_DATA/HOLE support. Then 5 years later, Filipe removed SEEK_DATA operations from the sum_file_data_permissive(), by 1deed13f69b2 ("fstests: fix fssum to actually ignore file holes when supposed to"). And fssum run sum_file_data_permissive() by default. So that cause fssum don't need SEEK_DATA support by default (except you use "-s" option). Then 1 year later, Christoph added btrfs/007 into seek group, I think that might because btrfs/007 still keeps the *_require_seek_data_hole*, which runs the src/seek_sanity_test. So, now, if we all agree that btrfs/007 isn't a seek related test, we can remove the seek group and the *_require_seek_data_hole*. Thanks, Zorro >
On Mon, Sep 5, 2022 at 10:08 AM Zorro Lang <zlang@redhat.com> wrote: > > On Mon, Sep 05, 2022 at 08:35:39AM +0200, Christoph Hellwig wrote: > > On Fri, Sep 02, 2022 at 11:44:24AM +0200, David Sterba wrote: > > > > commit 6fd9210bc97710f81e5a7646a2abfd11af0f0c28 > > > > Author: Christoph Hellwig <hch@lst.de> > > > > Date: Mon Feb 18 10:05:03 2019 +0100 > > > > > > > > fstests: add a seek group > > > > > > > > So I'd like to let Christoph help to double check it. > > > > > > It's quite obvious from the test itself that it tests only send/receive, > > > which is mentioned in the changelog. The commit adding the seek group > > > does not provide any rationale so it's hard to argue but as it stands > > > now the 'seek' group should not be there. > > > > Probably. Unless it somehow exercised seeks through the userspace > > seek code I can't see any good rationale for this addition, and the > > patch was far too long ago for me to remember. > > Hi, > > I just tried to learn about the history of this *problem*: > > At first, Jan Schmidt added src/fssum.c into fstests by df0fd18101b6 ("xfstests: > add fssum tool"). In this original version, fssum does SEEK_DATA in both > sum_file_data_permissive() and sum_file_data_strict(), that means it always > does SEEK_DATA. So all cases run fssum, need SEEK_DATA/HOLE support. > > Then 5 years later, Filipe removed SEEK_DATA operations from the > sum_file_data_permissive(), by 1deed13f69b2 ("fstests: fix fssum to actually > ignore file holes when supposed to"). And fssum run sum_file_data_permissive() > by default. So that cause fssum don't need SEEK_DATA support by default (except > you use "-s" option). > > Then 1 year later, Christoph added btrfs/007 into seek group, I think that might > because btrfs/007 still keeps the *_require_seek_data_hole*, which runs the > src/seek_sanity_test. > > So, now, if we all agree that btrfs/007 isn't a seek related test, we can remove > the seek group and the *_require_seek_data_hole*. fssum exercises lseek (SEEK_DATA) only if we pass the -s option to it, which is not the case for btrfs/007 (as well as for all other btrfs tests that exercise send/receive and use fssum). And that is because send/receive does not always preserve holes and prealloc (specially on incremental send/receive). That's a short version of the changelog from 1deed13f69b2, hopefully clear enough. And yes, the _require_seek_data_hole can go away from btrfs/007 too. Thanks. > > Thanks, > Zorro > > > >
diff --git a/tests/btrfs/007 b/tests/btrfs/007 index ed7d143a..c4a8d9d0 100755 --- a/tests/btrfs/007 +++ b/tests/btrfs/007 @@ -13,7 +13,7 @@ owner=list.btrfs@jan-o-sch.net . ./common/preamble -_begin_fstest auto quick rw metadata send seek +_begin_fstest auto quick rw metadata send # Override the default cleanup function. _cleanup()