diff mbox series

[v2] btrfs: add test for btrfstune squota enable/disable

Message ID 7339416633376271b21b1be844e1a34f8656f780.1720799383.git.boris@bur.io (mailing list archive)
State New, archived
Headers show
Series [v2] btrfs: add test for btrfstune squota enable/disable | expand

Commit Message

Boris Burkov July 12, 2024, 3:51 p.m. UTC 
btrfstune supports enabling simple quotas on a fleshed out filesystem
(without adding owner refs) and clearing squotas entirely from a
filesystem that ran under squotas (clearing the incompat bit)

Test that these operations work on a relatively complicated filesystem
populated by fsstress while preserving fssum.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/332.out |  2 ++
 2 files changed, 71 insertions(+)
 create mode 100755 tests/btrfs/332
 create mode 100644 tests/btrfs/332.out

Comments

Filipe Manana July 12, 2024, 4:56 p.m. UTC | #1 
On Fri, Jul 12, 2024 at 4:53 PM Boris Burkov <boris@bur.io> wrote:
>
> btrfstune supports enabling simple quotas on a fleshed out filesystem
> (without adding owner refs) and clearing squotas entirely from a
> filesystem that ran under squotas (clearing the incompat bit)
>
> Test that these operations work on a relatively complicated filesystem
> populated by fsstress while preserving fssum.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/332.out |  2 ++
>  2 files changed, 71 insertions(+)
>  create mode 100755 tests/btrfs/332
>  create mode 100644 tests/btrfs/332.out
>
> diff --git a/tests/btrfs/332 b/tests/btrfs/332
> new file mode 100755
> index 000000000..d5cf32664
> --- /dev/null
> +++ b/tests/btrfs/332
> @@ -0,0 +1,69 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
> +#
> +# FS QA Test No. btrfs/332
> +#
> +# Test tune enabling and removing squotas on a live filesystem
> +#
> +. ./common/preamble
> +_begin_fstest auto quick qgroup
> +
> +# Import common functions.
> +. ./common/filter.btrfs
> +
> +# real QA test starts here
> +_supported_fs btrfs
> +_require_scratch_enable_simple_quota
> +_require_no_compress
> +_require_command "$BTRFS_TUNE_PROG" btrfstune
> +_require_fssum
> +_require_btrfs_dump_super
> +_require_btrfs_command inspect-internal dump-tree
> +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
> +       _notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
> +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
> +       _notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
> +
> +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> +_scratch_mount
> +
> +# do some stuff
> +d1=$SCRATCH_MNT/d1
> +d2=$SCRATCH_MNT/d2
> +mkdir $d1
> +mkdir $d2
> +run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
> +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> +
> +# enable squotas
> +_scratch_unmount
> +$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> +       _fail "enable simple quota failed"

Instead of doing a "|| _fail ..." everywhere, can't we simply not
redirect stderr to the .full file and instead have a golden output
mismatch in case an error happens?
Not only that makes the test shorter and easier to read, it goes along
with the most common practice in fstests.

> +_check_btrfs_filesystem $SCRATCH_DEV
> +_scratch_mount
> +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> +[ "$fssum_pre" == "$fssum_post" ] \
> +       || echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
> +
> +# do some more stuff
> +run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
> +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> +_scratch_unmount
> +_check_btrfs_filesystem $SCRATCH_DEV
> +
> +$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> +       _fail "remove simple quota failed"

Same here.

With that fixed (if it can be done):

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +_check_btrfs_filesystem $SCRATCH_DEV
> +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
> +$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
> +
> +_scratch_mount
> +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> +_scratch_unmount
> +[ "$fssum_pre" == "$fssum_post" ] \
> +       || echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
> +
> +echo Silence is golden
> +status=0
> +exit
> diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
> new file mode 100644
> index 000000000..adb316136
> --- /dev/null
> +++ b/tests/btrfs/332.out
> @@ -0,0 +1,2 @@
> +QA output created by 332
> +Silence is golden
> --
> 2.45.2
>
>
Boris Burkov July 12, 2024, 5:25 p.m. UTC | #2 
On Fri, Jul 12, 2024 at 05:56:15PM +0100, Filipe Manana wrote:
> On Fri, Jul 12, 2024 at 4:53 PM Boris Burkov <boris@bur.io> wrote:
> >
> > btrfstune supports enabling simple quotas on a fleshed out filesystem
> > (without adding owner refs) and clearing squotas entirely from a
> > filesystem that ran under squotas (clearing the incompat bit)
> >
> > Test that these operations work on a relatively complicated filesystem
> > populated by fsstress while preserving fssum.
> >
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >  tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/332.out |  2 ++
> >  2 files changed, 71 insertions(+)
> >  create mode 100755 tests/btrfs/332
> >  create mode 100644 tests/btrfs/332.out
> >
> > diff --git a/tests/btrfs/332 b/tests/btrfs/332
> > new file mode 100755
> > index 000000000..d5cf32664
> > --- /dev/null
> > +++ b/tests/btrfs/332
> > @@ -0,0 +1,69 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test No. btrfs/332
> > +#
> > +# Test tune enabling and removing squotas on a live filesystem
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick qgroup
> > +
> > +# Import common functions.
> > +. ./common/filter.btrfs
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
> > +_require_scratch_enable_simple_quota
> > +_require_no_compress
> > +_require_command "$BTRFS_TUNE_PROG" btrfstune
> > +_require_fssum
> > +_require_btrfs_dump_super
> > +_require_btrfs_command inspect-internal dump-tree
> > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
> > +       _notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
> > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
> > +       _notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
> > +
> > +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> > +_scratch_mount
> > +
> > +# do some stuff
> > +d1=$SCRATCH_MNT/d1
> > +d2=$SCRATCH_MNT/d2
> > +mkdir $d1
> > +mkdir $d2
> > +run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
> > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > +
> > +# enable squotas
> > +_scratch_unmount
> > +$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > +       _fail "enable simple quota failed"
> 
> Instead of doing a "|| _fail ..." everywhere, can't we simply not
> redirect stderr to the .full file and instead have a golden output
> mismatch in case an error happens?
> Not only that makes the test shorter and easier to read, it goes along
> with the most common practice in fstests.
> 

That's what I wanted to do, since you have given me that feedback in the
past, but unfortunately --enable-simple-quota currently spits out a line
for each qgroup it creates, which we can't predict in the output, since
I don't think the fsstress run is deterministic?

Options I considered where:
- grep -v or otherwise filter out those lines
- check the failure

I am happy to switch to the grep.

> > +_check_btrfs_filesystem $SCRATCH_DEV
> > +_scratch_mount
> > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > +[ "$fssum_pre" == "$fssum_post" ] \
> > +       || echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
> > +
> > +# do some more stuff
> > +run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
> > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > +_scratch_unmount
> > +_check_btrfs_filesystem $SCRATCH_DEV
> > +
> > +$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > +       _fail "remove simple quota failed"
> 
> Same here.
> 
> With that fixed (if it can be done):

I think here, the command does generally work how we want: silent on
success, spews on failure, but I wanted it to be consistent with the
enable, not have to look in different files (or stick in a tee) etc..

I'll play around with it and re-send if the filtered version looks
better.

> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
> > +_check_btrfs_filesystem $SCRATCH_DEV
> > +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
> > +$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
> > +
> > +_scratch_mount
> > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > +_scratch_unmount
> > +[ "$fssum_pre" == "$fssum_post" ] \
> > +       || echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
> > +
> > +echo Silence is golden
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
> > new file mode 100644
> > index 000000000..adb316136
> > --- /dev/null
> > +++ b/tests/btrfs/332.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 332
> > +Silence is golden
> > --
> > 2.45.2
> >
> >
Filipe Manana July 12, 2024, 5:33 p.m. UTC | #3 
On Fri, Jul 12, 2024 at 6:26 PM Boris Burkov <boris@bur.io> wrote:
>
> On Fri, Jul 12, 2024 at 05:56:15PM +0100, Filipe Manana wrote:
> > On Fri, Jul 12, 2024 at 4:53 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > btrfstune supports enabling simple quotas on a fleshed out filesystem
> > > (without adding owner refs) and clearing squotas entirely from a
> > > filesystem that ran under squotas (clearing the incompat bit)
> > >
> > > Test that these operations work on a relatively complicated filesystem
> > > populated by fsstress while preserving fssum.
> > >
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > ---
> > >  tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/332.out |  2 ++
> > >  2 files changed, 71 insertions(+)
> > >  create mode 100755 tests/btrfs/332
> > >  create mode 100644 tests/btrfs/332.out
> > >
> > > diff --git a/tests/btrfs/332 b/tests/btrfs/332
> > > new file mode 100755
> > > index 000000000..d5cf32664
> > > --- /dev/null
> > > +++ b/tests/btrfs/332
> > > @@ -0,0 +1,69 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. btrfs/332
> > > +#
> > > +# Test tune enabling and removing squotas on a live filesystem
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick qgroup
> > > +
> > > +# Import common functions.
> > > +. ./common/filter.btrfs
> > > +
> > > +# real QA test starts here
> > > +_supported_fs btrfs
> > > +_require_scratch_enable_simple_quota
> > > +_require_no_compress
> > > +_require_command "$BTRFS_TUNE_PROG" btrfstune
> > > +_require_fssum
> > > +_require_btrfs_dump_super
> > > +_require_btrfs_command inspect-internal dump-tree
> > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
> > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
> > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
> > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
> > > +
> > > +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> > > +_scratch_mount
> > > +
> > > +# do some stuff
> > > +d1=$SCRATCH_MNT/d1
> > > +d2=$SCRATCH_MNT/d2
> > > +mkdir $d1
> > > +mkdir $d2
> > > +run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
> > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > +
> > > +# enable squotas
> > > +_scratch_unmount
> > > +$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > +       _fail "enable simple quota failed"
> >
> > Instead of doing a "|| _fail ..." everywhere, can't we simply not
> > redirect stderr to the .full file and instead have a golden output
> > mismatch in case an error happens?
> > Not only that makes the test shorter and easier to read, it goes along
> > with the most common practice in fstests.
> >
>
> That's what I wanted to do, since you have given me that feedback in the
> past, but unfortunately --enable-simple-quota currently spits out a line
> for each qgroup it creates, which we can't predict in the output, since
> I don't think the fsstress run is deterministic?

But are those messages printed to stderr?
As they aren't errors, I would expect them to be sent to stdout only.

>
> Options I considered where:
> - grep -v or otherwise filter out those lines
> - check the failure
>
> I am happy to switch to the grep.

If those non-error messages are sent to stderr, then I would say just
leave the test as it is.

Thanks.

>
> > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > +_scratch_mount
> > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > +       || echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
> > > +
> > > +# do some more stuff
> > > +run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
> > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > +_scratch_unmount
> > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > +
> > > +$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > +       _fail "remove simple quota failed"
> >
> > Same here.
> >
> > With that fixed (if it can be done):
>
> I think here, the command does generally work how we want: silent on
> success, spews on failure, but I wanted it to be consistent with the
> enable, not have to look in different files (or stick in a tee) etc..
>
> I'll play around with it and re-send if the filtered version looks
> better.
>
> >
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >
> > Thanks.
> >
> > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
> > > +$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
> > > +
> > > +_scratch_mount
> > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > +_scratch_unmount
> > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > +       || echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
> > > +
> > > +echo Silence is golden
> > > +status=0
> > > +exit
> > > diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
> > > new file mode 100644
> > > index 000000000..adb316136
> > > --- /dev/null
> > > +++ b/tests/btrfs/332.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 332
> > > +Silence is golden
> > > --
> > > 2.45.2
> > >
> > >
Boris Burkov July 12, 2024, 6:01 p.m. UTC | #4 
On Fri, Jul 12, 2024 at 06:33:59PM +0100, Filipe Manana wrote:
> On Fri, Jul 12, 2024 at 6:26 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Fri, Jul 12, 2024 at 05:56:15PM +0100, Filipe Manana wrote:
> > > On Fri, Jul 12, 2024 at 4:53 PM Boris Burkov <boris@bur.io> wrote:
> > > >
> > > > btrfstune supports enabling simple quotas on a fleshed out filesystem
> > > > (without adding owner refs) and clearing squotas entirely from a
> > > > filesystem that ran under squotas (clearing the incompat bit)
> > > >
> > > > Test that these operations work on a relatively complicated filesystem
> > > > populated by fsstress while preserving fssum.
> > > >
> > > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > > ---
> > > >  tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/btrfs/332.out |  2 ++
> > > >  2 files changed, 71 insertions(+)
> > > >  create mode 100755 tests/btrfs/332
> > > >  create mode 100644 tests/btrfs/332.out
> > > >
> > > > diff --git a/tests/btrfs/332 b/tests/btrfs/332
> > > > new file mode 100755
> > > > index 000000000..d5cf32664
> > > > --- /dev/null
> > > > +++ b/tests/btrfs/332
> > > > @@ -0,0 +1,69 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test No. btrfs/332
> > > > +#
> > > > +# Test tune enabling and removing squotas on a live filesystem
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick qgroup
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/filter.btrfs
> > > > +
> > > > +# real QA test starts here
> > > > +_supported_fs btrfs
> > > > +_require_scratch_enable_simple_quota
> > > > +_require_no_compress
> > > > +_require_command "$BTRFS_TUNE_PROG" btrfstune
> > > > +_require_fssum
> > > > +_require_btrfs_dump_super
> > > > +_require_btrfs_command inspect-internal dump-tree
> > > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
> > > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
> > > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
> > > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
> > > > +
> > > > +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> > > > +_scratch_mount
> > > > +
> > > > +# do some stuff
> > > > +d1=$SCRATCH_MNT/d1
> > > > +d2=$SCRATCH_MNT/d2
> > > > +mkdir $d1
> > > > +mkdir $d2
> > > > +run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
> > > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > +
> > > > +# enable squotas
> > > > +_scratch_unmount
> > > > +$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > > +       _fail "enable simple quota failed"
> > >
> > > Instead of doing a "|| _fail ..." everywhere, can't we simply not
> > > redirect stderr to the .full file and instead have a golden output
> > > mismatch in case an error happens?
> > > Not only that makes the test shorter and easier to read, it goes along
> > > with the most common practice in fstests.
> > >
> >
> > That's what I wanted to do, since you have given me that feedback in the
> > past, but unfortunately --enable-simple-quota currently spits out a line
> > for each qgroup it creates, which we can't predict in the output, since
> > I don't think the fsstress run is deterministic?
> 
> But are those messages printed to stderr?
> As they aren't errors, I would expect them to be sent to stdout only.
> 
> >
> > Options I considered where:
> > - grep -v or otherwise filter out those lines
> > - check the failure
> >
> > I am happy to switch to the grep.
> 
> If those non-error messages are sent to stderr, then I would say just
> leave the test as it is.

They get sent to stdout, and adjusting the code to just grep them out
came out well enough, so I resent that as v3.

We crossed streams a little here, so hopefully I did a reasonable enough
thing and the v3 looks good to you :)

Thanks for your time, and sorry for the churn on this simple stuff.

> 
> Thanks.
> 
> >
> > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > +_scratch_mount
> > > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > > +       || echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
> > > > +
> > > > +# do some more stuff
> > > > +run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
> > > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > +_scratch_unmount
> > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > +
> > > > +$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > > +       _fail "remove simple quota failed"
> > >
> > > Same here.
> > >
> > > With that fixed (if it can be done):
> >
> > I think here, the command does generally work how we want: silent on
> > success, spews on failure, but I wanted it to be consistent with the
> > enable, not have to look in different files (or stick in a tee) etc..
> >
> > I'll play around with it and re-send if the filtered version looks
> > better.
> >
> > >
> > > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> > >
> > > Thanks.
> > >
> > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
> > > > +$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
> > > > +
> > > > +_scratch_mount
> > > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > +_scratch_unmount
> > > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > > +       || echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
> > > > +
> > > > +echo Silence is golden
> > > > +status=0
> > > > +exit
> > > > diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
> > > > new file mode 100644
> > > > index 000000000..adb316136
> > > > --- /dev/null
> > > > +++ b/tests/btrfs/332.out
> > > > @@ -0,0 +1,2 @@
> > > > +QA output created by 332
> > > > +Silence is golden
> > > > --
> > > > 2.45.2
> > > >
> > > >
Filipe Manana July 12, 2024, 6:13 p.m. UTC | #5 
On Fri, Jul 12, 2024 at 7:02 PM Boris Burkov <boris@bur.io> wrote:
>
> On Fri, Jul 12, 2024 at 06:33:59PM +0100, Filipe Manana wrote:
> > On Fri, Jul 12, 2024 at 6:26 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > On Fri, Jul 12, 2024 at 05:56:15PM +0100, Filipe Manana wrote:
> > > > On Fri, Jul 12, 2024 at 4:53 PM Boris Burkov <boris@bur.io> wrote:
> > > > >
> > > > > btrfstune supports enabling simple quotas on a fleshed out filesystem
> > > > > (without adding owner refs) and clearing squotas entirely from a
> > > > > filesystem that ran under squotas (clearing the incompat bit)
> > > > >
> > > > > Test that these operations work on a relatively complicated filesystem
> > > > > populated by fsstress while preserving fssum.
> > > > >
> > > > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > > > ---
> > > > >  tests/btrfs/332     | 69 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/btrfs/332.out |  2 ++
> > > > >  2 files changed, 71 insertions(+)
> > > > >  create mode 100755 tests/btrfs/332
> > > > >  create mode 100644 tests/btrfs/332.out
> > > > >
> > > > > diff --git a/tests/btrfs/332 b/tests/btrfs/332
> > > > > new file mode 100755
> > > > > index 000000000..d5cf32664
> > > > > --- /dev/null
> > > > > +++ b/tests/btrfs/332
> > > > > @@ -0,0 +1,69 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test No. btrfs/332
> > > > > +#
> > > > > +# Test tune enabling and removing squotas on a live filesystem
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto quick qgroup
> > > > > +
> > > > > +# Import common functions.
> > > > > +. ./common/filter.btrfs
> > > > > +
> > > > > +# real QA test starts here
> > > > > +_supported_fs btrfs
> > > > > +_require_scratch_enable_simple_quota
> > > > > +_require_no_compress
> > > > > +_require_command "$BTRFS_TUNE_PROG" btrfstune
> > > > > +_require_fssum
> > > > > +_require_btrfs_dump_super
> > > > > +_require_btrfs_command inspect-internal dump-tree
> > > > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
> > > > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
> > > > > +$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
> > > > > +       _notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
> > > > > +
> > > > > +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> > > > > +_scratch_mount
> > > > > +
> > > > > +# do some stuff
> > > > > +d1=$SCRATCH_MNT/d1
> > > > > +d2=$SCRATCH_MNT/d2
> > > > > +mkdir $d1
> > > > > +mkdir $d2
> > > > > +run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
> > > > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > > +
> > > > > +# enable squotas
> > > > > +_scratch_unmount
> > > > > +$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > > > +       _fail "enable simple quota failed"
> > > >
> > > > Instead of doing a "|| _fail ..." everywhere, can't we simply not
> > > > redirect stderr to the .full file and instead have a golden output
> > > > mismatch in case an error happens?
> > > > Not only that makes the test shorter and easier to read, it goes along
> > > > with the most common practice in fstests.
> > > >
> > >
> > > That's what I wanted to do, since you have given me that feedback in the
> > > past, but unfortunately --enable-simple-quota currently spits out a line
> > > for each qgroup it creates, which we can't predict in the output, since
> > > I don't think the fsstress run is deterministic?
> >
> > But are those messages printed to stderr?
> > As they aren't errors, I would expect them to be sent to stdout only.
> >
> > >
> > > Options I considered where:
> > > - grep -v or otherwise filter out those lines
> > > - check the failure
> > >
> > > I am happy to switch to the grep.
> >
> > If those non-error messages are sent to stderr, then I would say just
> > leave the test as it is.
>
> They get sent to stdout, and adjusting the code to just grep them out
> came out well enough, so I resent that as v3.

I see, my idea was simple, no need to grep or do anything.

Instead of :

$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full
2>&1 || _fail "enable simple quota failed"

Just doing:

$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full

So those informative messages would be ignored, and in case an error
happens, anything btrfstune prints to stderr makes the test
automatically fail due to mismatch with the golden output.

v3 does instead:

$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV | grep -v 'created
qgroup for'


>
> We crossed streams a little here, so hopefully I did a reasonable enough
> thing and the v3 looks good to you :)
>
> Thanks for your time, and sorry for the churn on this simple stuff.

No worries, thank you.

>
> >
> > Thanks.
> >
> > >
> > > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > > +_scratch_mount
> > > > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > > > +       || echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
> > > > > +
> > > > > +# do some more stuff
> > > > > +run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
> > > > > +fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > > +_scratch_unmount
> > > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > > +
> > > > > +$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
> > > > > +       _fail "remove simple quota failed"
> > > >
> > > > Same here.
> > > >
> > > > With that fixed (if it can be done):
> > >
> > > I think here, the command does generally work how we want: silent on
> > > success, spews on failure, but I wanted it to be consistent with the
> > > enable, not have to look in different files (or stick in a tee) etc..
> > >
> > > I'll play around with it and re-send if the filtered version looks
> > > better.
> > >
> > > >
> > > > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > Thanks.
> > > >
> > > > > +_check_btrfs_filesystem $SCRATCH_DEV
> > > > > +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
> > > > > +$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
> > > > > +
> > > > > +_scratch_mount
> > > > > +fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
> > > > > +_scratch_unmount
> > > > > +[ "$fssum_pre" == "$fssum_post" ] \
> > > > > +       || echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
> > > > > +
> > > > > +echo Silence is golden
> > > > > +status=0
> > > > > +exit
> > > > > diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
> > > > > new file mode 100644
> > > > > index 000000000..adb316136
> > > > > --- /dev/null
> > > > > +++ b/tests/btrfs/332.out
> > > > > @@ -0,0 +1,2 @@
> > > > > +QA output created by 332
> > > > > +Silence is golden
> > > > > --
> > > > > 2.45.2
> > > > >
> > > > >
diff mbox series

Patch

diff --git a/tests/btrfs/332 b/tests/btrfs/332
new file mode 100755
index 000000000..d5cf32664
--- /dev/null
+++ b/tests/btrfs/332
@@ -0,0 +1,69 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Meta Platforms, Inc.  All Rights Reserved.
+#
+# FS QA Test No. btrfs/332
+#
+# Test tune enabling and removing squotas on a live filesystem
+#
+. ./common/preamble
+_begin_fstest auto quick qgroup
+
+# Import common functions.
+. ./common/filter.btrfs
+
+# real QA test starts here
+_supported_fs btrfs
+_require_scratch_enable_simple_quota
+_require_no_compress
+_require_command "$BTRFS_TUNE_PROG" btrfstune
+_require_fssum
+_require_btrfs_dump_super
+_require_btrfs_command inspect-internal dump-tree
+$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--enable-simple-quota' || \
+	_notrun "$BTRFS_TUNE_PROG too old (must support --enable-simple-quota)"
+$BTRFS_TUNE_PROG --help 2>&1 | grep -wq -- '--remove-simple-quota' || \
+	_notrun "$BTRFS_TUNE_PROG too old (must support --remove-simple-quota)"
+
+_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
+_scratch_mount
+
+# do some stuff
+d1=$SCRATCH_MNT/d1
+d2=$SCRATCH_MNT/d2
+mkdir $d1
+mkdir $d2
+run_check $FSSTRESS_PROG -d $d1 -w -n 2000 $FSSTRESS_AVOID
+fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
+
+# enable squotas
+_scratch_unmount
+$BTRFS_TUNE_PROG --enable-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
+	_fail "enable simple quota failed"
+_check_btrfs_filesystem $SCRATCH_DEV
+_scratch_mount
+fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
+[ "$fssum_pre" == "$fssum_post" ] \
+	|| echo "fssum $fssum_pre does not match $fssum_post after enabling squota"
+
+# do some more stuff
+run_check $FSSTRESS_PROG -d $d2 -w -n 2000 $FSSTRESS_AVOID
+fssum_pre=$($FSSUM_PROG -A $SCRATCH_MNT)
+_scratch_unmount
+_check_btrfs_filesystem $SCRATCH_DEV
+
+$BTRFS_TUNE_PROG --remove-simple-quota $SCRATCH_DEV >> $seqres.full 2>&1 || \
+	_fail "remove simple quota failed"
+_check_btrfs_filesystem $SCRATCH_DEV
+$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep 'SIMPLE_QUOTA'
+$BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV  | grep -e 'QUOTA' -e 'QGROUP'
+
+_scratch_mount
+fssum_post=$($FSSUM_PROG -A $SCRATCH_MNT)
+_scratch_unmount
+[ "$fssum_pre" == "$fssum_post" ] \
+	|| echo "fssum $fssum_pre does not match $fssum_post after disabling squota"
+
+echo Silence is golden
+status=0
+exit
diff --git a/tests/btrfs/332.out b/tests/btrfs/332.out
new file mode 100644
index 000000000..adb316136
--- /dev/null
+++ b/tests/btrfs/332.out
@@ -0,0 +1,2 @@ 
+QA output created by 332
+Silence is golden