* [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