| Message ID | 20240303065251.111868-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: btrfs/121: allow snapshot with invalid qgroup to return error | expand |
On Sun, Mar 03, 2024 at 05:22:51PM +1030, Qu Wenruo wrote: > [BUG] > After incoming kernel commit "btrfs: qgroup: verify btrfs_qgroup_inherit > parameter", test case btrfs/121 would fail like this: > > btrfs/121 1s ... [failed, exit status 1]- output mismatch (see /xfstests/results//btrfs/121.out.bad) > --- tests/btrfs/121.out 2022-05-11 09:55:30.739999997 +0800 > +++ /xfstests/results//btrfs/121.out.bad 2024-03-03 13:33:38.076666665 +0800 > @@ -1,2 +1,3 @@ > QA output created by 121 > -Silence is golden > +failed: '/usr/bin/btrfs subvolume snapshot -i 1/10 /mnt/scratch /mnt/scratch/snap1' > +(see /xfstests/results//btrfs/121.full for details) > ... > (Run 'diff -u /xfstests/tests/btrfs/121.out /xfstests/results//btrfs/121.out.bad' to see the entire diff) > > [CAUSE] > The incoming kernel commit would do early qgroups validation before > subvolume/snapshot creation, and reject invalid qgroups immediately. > > Meanwhile that test case itself still assume the ioctl would go on > without any error, thus the new behavior would break the test case. > > [FIX] > Instead of relying on the snapshot creation ioctl return value, we just > completely ignore the output of that snapshot creation. > Then manually check if the fs is still read-write. > > For different kernels (3 cases), they would lead to the following > results: > > - Older unpatched kernel > The filesystem would trigger a transaction abort (would be caught by > dmesg filter), and also fail the "touch" command. > > - Older but patched kernel > The filesystem continues to create the snapshot, while still keeps the > fs read-write. > > - Latest kernel with qgroup validation > The filesystem refuses to create the snapshot, while still keeps the > fs read-write. > > Both "older but patched" and "latest" kernels would still pass the test > case, even with different behaviors. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > tests/btrfs/121 | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/tests/btrfs/121 b/tests/btrfs/121 > index f4d54962..15a54274 100755 > --- a/tests/btrfs/121 > +++ b/tests/btrfs/121 > @@ -24,8 +24,14 @@ _require_scratch > _scratch_mkfs >/dev/null > _scratch_mount > _run_btrfs_util_prog quota enable $SCRATCH_MNT > -# The qgroup '1/10' does not exist and should be silently ignored > -_run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 > +# The qgroup '1/10' does not exist. The kernel should either gives an error > +# (newer kernel with invalid qgroup detection) or ignore it (older kernel with > +# above fix). > +# Either way, we just ignore the output completely, and we will check if the fs > +# is still RW later. The explanation makes sense to me, just ask if you might want to output to .full file, to save some information for debug if the test fails? I can help to change the "&> /dev/null" to "&> $seqres.full" if you only need to change. Reviewed-by: Zorro Lang <zlang@redhat.com> Thanks, Zorro > +$BTRFS_UTIL_PROG subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 &> /dev/null > + > +touch $SCRATCH_MNT/foobar > > echo "Silence is golden" > > -- > 2.42.0 > >
在 2024/3/4 14:48, Zorro Lang 写道: > On Sun, Mar 03, 2024 at 05:22:51PM +1030, Qu Wenruo wrote: >> [BUG] >> After incoming kernel commit "btrfs: qgroup: verify btrfs_qgroup_inherit >> parameter", test case btrfs/121 would fail like this: >> >> btrfs/121 1s ... [failed, exit status 1]- output mismatch (see /xfstests/results//btrfs/121.out.bad) >> --- tests/btrfs/121.out 2022-05-11 09:55:30.739999997 +0800 >> +++ /xfstests/results//btrfs/121.out.bad 2024-03-03 13:33:38.076666665 +0800 >> @@ -1,2 +1,3 @@ >> QA output created by 121 >> -Silence is golden >> +failed: '/usr/bin/btrfs subvolume snapshot -i 1/10 /mnt/scratch /mnt/scratch/snap1' >> +(see /xfstests/results//btrfs/121.full for details) >> ... >> (Run 'diff -u /xfstests/tests/btrfs/121.out /xfstests/results//btrfs/121.out.bad' to see the entire diff) >> >> [CAUSE] >> The incoming kernel commit would do early qgroups validation before >> subvolume/snapshot creation, and reject invalid qgroups immediately. >> >> Meanwhile that test case itself still assume the ioctl would go on >> without any error, thus the new behavior would break the test case. >> >> [FIX] >> Instead of relying on the snapshot creation ioctl return value, we just >> completely ignore the output of that snapshot creation. >> Then manually check if the fs is still read-write. >> >> For different kernels (3 cases), they would lead to the following >> results: >> >> - Older unpatched kernel >> The filesystem would trigger a transaction abort (would be caught by >> dmesg filter), and also fail the "touch" command. >> >> - Older but patched kernel >> The filesystem continues to create the snapshot, while still keeps the >> fs read-write. >> >> - Latest kernel with qgroup validation >> The filesystem refuses to create the snapshot, while still keeps the >> fs read-write. >> >> Both "older but patched" and "latest" kernels would still pass the test >> case, even with different behaviors. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> tests/btrfs/121 | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/tests/btrfs/121 b/tests/btrfs/121 >> index f4d54962..15a54274 100755 >> --- a/tests/btrfs/121 >> +++ b/tests/btrfs/121 >> @@ -24,8 +24,14 @@ _require_scratch >> _scratch_mkfs >/dev/null >> _scratch_mount >> _run_btrfs_util_prog quota enable $SCRATCH_MNT >> -# The qgroup '1/10' does not exist and should be silently ignored >> -_run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 >> +# The qgroup '1/10' does not exist. The kernel should either gives an error >> +# (newer kernel with invalid qgroup detection) or ignore it (older kernel with >> +# above fix). >> +# Either way, we just ignore the output completely, and we will check if the fs >> +# is still RW later. > > The explanation makes sense to me, just ask if you might want to output to .full > file, to save some information for debug if the test fails? I can help to change > the "&> /dev/null" to "&> $seqres.full" if you only need to change. Oh, that's very kind of you. Although in that case "&>" would overwrite the .full file, ">> $seqres.full 2>&1" would be better IHMO. Thanks, Qu > > Reviewed-by: Zorro Lang <zlang@redhat.com> > > Thanks, > Zorro > >> +$BTRFS_UTIL_PROG subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 &> /dev/null >> + >> +touch $SCRATCH_MNT/foobar >> >> echo "Silence is golden" >> >> -- >> 2.42.0 >> >> >
On Mon, Mar 04, 2024 at 02:57:45PM +1030, Qu Wenruo wrote: > > > 在 2024/3/4 14:48, Zorro Lang 写道: > > On Sun, Mar 03, 2024 at 05:22:51PM +1030, Qu Wenruo wrote: > > > [BUG] > > > After incoming kernel commit "btrfs: qgroup: verify btrfs_qgroup_inherit > > > parameter", test case btrfs/121 would fail like this: > > > > > > btrfs/121 1s ... [failed, exit status 1]- output mismatch (see /xfstests/results//btrfs/121.out.bad) > > > --- tests/btrfs/121.out 2022-05-11 09:55:30.739999997 +0800 > > > +++ /xfstests/results//btrfs/121.out.bad 2024-03-03 13:33:38.076666665 +0800 > > > @@ -1,2 +1,3 @@ > > > QA output created by 121 > > > -Silence is golden > > > +failed: '/usr/bin/btrfs subvolume snapshot -i 1/10 /mnt/scratch /mnt/scratch/snap1' > > > +(see /xfstests/results//btrfs/121.full for details) > > > ... > > > (Run 'diff -u /xfstests/tests/btrfs/121.out /xfstests/results//btrfs/121.out.bad' to see the entire diff) > > > > > > [CAUSE] > > > The incoming kernel commit would do early qgroups validation before > > > subvolume/snapshot creation, and reject invalid qgroups immediately. > > > > > > Meanwhile that test case itself still assume the ioctl would go on > > > without any error, thus the new behavior would break the test case. > > > > > > [FIX] > > > Instead of relying on the snapshot creation ioctl return value, we just > > > completely ignore the output of that snapshot creation. > > > Then manually check if the fs is still read-write. > > > > > > For different kernels (3 cases), they would lead to the following > > > results: > > > > > > - Older unpatched kernel > > > The filesystem would trigger a transaction abort (would be caught by > > > dmesg filter), and also fail the "touch" command. > > > > > > - Older but patched kernel > > > The filesystem continues to create the snapshot, while still keeps the > > > fs read-write. > > > > > > - Latest kernel with qgroup validation > > > The filesystem refuses to create the snapshot, while still keeps the > > > fs read-write. > > > > > > Both "older but patched" and "latest" kernels would still pass the test > > > case, even with different behaviors. > > > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > > --- > > > tests/btrfs/121 | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/btrfs/121 b/tests/btrfs/121 > > > index f4d54962..15a54274 100755 > > > --- a/tests/btrfs/121 > > > +++ b/tests/btrfs/121 > > > @@ -24,8 +24,14 @@ _require_scratch > > > _scratch_mkfs >/dev/null > > > _scratch_mount > > > _run_btrfs_util_prog quota enable $SCRATCH_MNT > > > -# The qgroup '1/10' does not exist and should be silently ignored > > > -_run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 > > > +# The qgroup '1/10' does not exist. The kernel should either gives an error > > > +# (newer kernel with invalid qgroup detection) or ignore it (older kernel with > > > +# above fix). > > > +# Either way, we just ignore the output completely, and we will check if the fs > > > +# is still RW later. > > > > The explanation makes sense to me, just ask if you might want to output to .full > > file, to save some information for debug if the test fails? I can help to change > > the "&> /dev/null" to "&> $seqres.full" if you only need to change. > > Oh, that's very kind of you. > > Although in that case "&>" would overwrite the .full file, > ">> $seqres.full 2>&1" would be better IHMO. Oh, you're right, thanks for point out that! It's in "patches-in-queue" branch now, and will be in next release if no more review points from others. Thanks, Zorro > > Thanks, > Qu > > > > > Reviewed-by: Zorro Lang <zlang@redhat.com> > > > > Thanks, > > Zorro > > > > > +$BTRFS_UTIL_PROG subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 &> /dev/null > > > + > > > +touch $SCRATCH_MNT/foobar > > > echo "Silence is golden" > > > -- > > > 2.42.0 > > > > > > > > >
diff --git a/tests/btrfs/121 b/tests/btrfs/121 index f4d54962..15a54274 100755 --- a/tests/btrfs/121 +++ b/tests/btrfs/121 @@ -24,8 +24,14 @@ _require_scratch _scratch_mkfs >/dev/null _scratch_mount _run_btrfs_util_prog quota enable $SCRATCH_MNT -# The qgroup '1/10' does not exist and should be silently ignored -_run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 +# The qgroup '1/10' does not exist. The kernel should either gives an error +# (newer kernel with invalid qgroup detection) or ignore it (older kernel with +# above fix). +# Either way, we just ignore the output completely, and we will check if the fs +# is still RW later. +$BTRFS_UTIL_PROG subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1 &> /dev/null + +touch $SCRATCH_MNT/foobar echo "Silence is golden"
[BUG] After incoming kernel commit "btrfs: qgroup: verify btrfs_qgroup_inherit parameter", test case btrfs/121 would fail like this: btrfs/121 1s ... [failed, exit status 1]- output mismatch (see /xfstests/results//btrfs/121.out.bad) --- tests/btrfs/121.out 2022-05-11 09:55:30.739999997 +0800 +++ /xfstests/results//btrfs/121.out.bad 2024-03-03 13:33:38.076666665 +0800 @@ -1,2 +1,3 @@ QA output created by 121 -Silence is golden +failed: '/usr/bin/btrfs subvolume snapshot -i 1/10 /mnt/scratch /mnt/scratch/snap1' +(see /xfstests/results//btrfs/121.full for details) ... (Run 'diff -u /xfstests/tests/btrfs/121.out /xfstests/results//btrfs/121.out.bad' to see the entire diff) [CAUSE] The incoming kernel commit would do early qgroups validation before subvolume/snapshot creation, and reject invalid qgroups immediately. Meanwhile that test case itself still assume the ioctl would go on without any error, thus the new behavior would break the test case. [FIX] Instead of relying on the snapshot creation ioctl return value, we just completely ignore the output of that snapshot creation. Then manually check if the fs is still read-write. For different kernels (3 cases), they would lead to the following results: - Older unpatched kernel The filesystem would trigger a transaction abort (would be caught by dmesg filter), and also fail the "touch" command. - Older but patched kernel The filesystem continues to create the snapshot, while still keeps the fs read-write. - Latest kernel with qgroup validation The filesystem refuses to create the snapshot, while still keeps the fs read-write. Both "older but patched" and "latest" kernels would still pass the test case, even with different behaviors. Signed-off-by: Qu Wenruo <wqu@suse.com> --- tests/btrfs/121 | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)