* [PATCH] btrfs: remove 'seek' group from btrfs/007 @ 2022-09-01 16:12 fdmanana 2022-09-02 2:00 ` Zorro Lang 0 siblings, 1 reply; 6+ messages in thread From: fdmanana @ 2022-09-01 16:12 UTC (permalink / raw) To: fstests; +Cc: linux-btrfs, Filipe Manana 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> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: remove 'seek' group from btrfs/007 2022-09-01 16:12 [PATCH] btrfs: remove 'seek' group from btrfs/007 fdmanana @ 2022-09-02 2:00 ` Zorro Lang 2022-09-02 9:44 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Zorro Lang @ 2022-09-02 2:00 UTC (permalink / raw) To: fdmanana, Christoph Hellwig; +Cc: fstests, linux-btrfs, Filipe Manana 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: remove 'seek' group from btrfs/007 2022-09-02 2:00 ` Zorro Lang @ 2022-09-02 9:44 ` David Sterba 2022-09-05 6:35 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2022-09-02 9:44 UTC (permalink / raw) To: Zorro Lang Cc: fdmanana, Christoph Hellwig, fstests, linux-btrfs, Filipe Manana 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: remove 'seek' group from btrfs/007 2022-09-02 9:44 ` David Sterba @ 2022-09-05 6:35 ` Christoph Hellwig 2022-09-05 9:08 ` Zorro Lang 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2022-09-05 6:35 UTC (permalink / raw) To: dsterba, Zorro Lang, fdmanana, Christoph Hellwig, fstests, linux-btrfs, Filipe Manana 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: remove 'seek' group from btrfs/007 2022-09-05 6:35 ` Christoph Hellwig @ 2022-09-05 9:08 ` Zorro Lang 2022-09-05 9:20 ` Filipe Manana 0 siblings, 1 reply; 6+ messages in thread From: Zorro Lang @ 2022-09-05 9:08 UTC (permalink / raw) To: Christoph Hellwig, dsterba, fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: remove 'seek' group from btrfs/007 2022-09-05 9:08 ` Zorro Lang @ 2022-09-05 9:20 ` Filipe Manana 0 siblings, 0 replies; 6+ messages in thread From: Filipe Manana @ 2022-09-05 9:20 UTC (permalink / raw) To: Zorro Lang Cc: Christoph Hellwig, dsterba, fstests, linux-btrfs, Filipe Manana 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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-05 9:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-01 16:12 [PATCH] btrfs: remove 'seek' group from btrfs/007 fdmanana 2022-09-02 2:00 ` Zorro Lang 2022-09-02 9:44 ` David Sterba 2022-09-05 6:35 ` Christoph Hellwig 2022-09-05 9:08 ` Zorro Lang 2022-09-05 9:20 ` Filipe Manana
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox