On Sat, Feb 24, 2024 at 06:21:44AM +1030, Qu Wenruo wrote:
>
>
> 在 2024/2/24 03:37, Anand Jain 写道:
> > On 2/23/24 15:05, Qu Wenruo wrote:
> > > There is a kernel regression caused by commit e15e9f43c7ca ("btrfs:
> > > introduce BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup
> > > accounting"), where if qgroup is inconsistent (not that hard to trigger)
> > > btrfs would leak its qgroup data reserved space, and cause a warning at
> > > unmount time.
> > >
> > > The test case would verify the behavior by:
> > >
> > > - Enable qgroup first
> > >
> > > - Intentionally mark qgroup inconsistent
> > > This is done by taking a snapshot and assign it to a higher level
> > > qgroup, meanwhile the source has no higher level qgroup.
> > >
> > > - Trigger a large enough write to cause qgroup data space leak
> > >
> > > - Unmount and check the dmesg for the qgroup rsv leak warning
> > >
> > > Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> > > Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > looks good.
> >
> > Reviewed-by: Anand Jain <anand.jain@xxxxxxxxxx>
>
> Thanks for the review.
>
> >
> >
> > Queued for the upcoming pull request.
>
> So for the btrfs fstests part, you'll do the pull request to upstream
> fstests, right?
Anand helps to merge some complicated btrfs patches, he'll help to give
it more reviewing and testing on btrfs side. Thanks for his help. Some
small changes for btrfs might be merged by me directly for quick
response/fix.
>
> In that case, if we have some conflicting test case number, how do we
> resolve it?
>
> It would be resolved by you or the author would be notified and need a
> resend?
Don't worry, that would be resolved by maintainer, to avoid you always need
to resend a patch only for changing a case number :)
>
> And do we have a branch to base our new test cases upon? (Mostly to avoid
> the number conflicting)
Sometimes it's hard to make sure the final case number before release.
If you need, I can provide a branch for "patches wating for pushing", but
that branch might be unstable, and might be removed & recreated sometimes.
Thanks,
Zorro
>
> Thanks,
> Qu
>
> >
> > Thanks, Anand
> >
> > > ---
> > > tests/btrfs/303 | 59 +++++++++++++++++++++++++++++++++++++++++++++
> > > tests/btrfs/303.out | 2 ++
> > > 2 files changed, 61 insertions(+)
> > > create mode 100755 tests/btrfs/303
> > > create mode 100644 tests/btrfs/303.out
> > > ---
> > > Changelog:
> > > v2:
> > > - Fix various spelling errors
> > >
> > > - Remove a copied _fixed_by_kernel_commit line
> > > Which was used to align the number of 'x', but forgot to remove
> > >
> > > diff --git a/tests/btrfs/303 b/tests/btrfs/303
> > > new file mode 100755
> > > index 00000000..9f7605ab
> > > --- /dev/null
> > > +++ b/tests/btrfs/303
> > > @@ -0,0 +1,59 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2024 SUSE Linux Products GmbH. All Rights Reserved.
> > > +#
> > > +# FS QA Test 303
> > > +#
> > > +# Make sure btrfs qgroup won't leak its reserved data space if qgroup is
> > > +# marked inconsistent.
> > > +#
> > > +# This exercises a regression introduced in v6.1 kernel by the
> > > following commit:
> > > +#
> > > +# e15e9f43c7ca ("btrfs: introduce
> > > BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting")
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick qgroup
> > > +
> > > +_supported_fs btrfs
> > > +_require_scratch
> > > +
> > > +_fixed_by_kernel_commit xxxxxxxxxxxx \
> > > + "btrfs: qgroup: always free reserved space for extent records"
> > > +
> > > +_scratch_mkfs >> $seqres.full
> > > +_scratch_mount
> > > +
> > > +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
> > > +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
> > > +
> > > +$BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT >> $seqres.full
> > > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subv1 >> $seqres.full
> > > +
> > > +# This would mark qgroup inconsistent, as the snapshot belongs to a
> > > different
> > > +# higher level qgroup, we have to do full rescan on both source and
> > > snapshot.
> > > +# This can be very slow for large subvolumes, so btrfs only marks qgroup
> > > +# inconsistent and let users to determine when to do a full rescan
> > > +$BTRFS_UTIL_PROG subvolume snapshot -i 1/0 $SCRATCH_MNT/subv1
> > > $SCRATCH_MNT/snap1 >> $seqres.full
> > > +
> > > +# This write would lead to a qgroup extent record holding the
> > > reserved 128K.
> > > +# And for unpatched kernels, the reserved space would not be freed
> > > properly
> > > +# due to qgroup is inconsistent.
> > > +_pwrite_byte 0xcd 0 128K $SCRATCH_MNT/foobar >> $seqres.full
> > > +
> > > +# The qgroup leak detection is only triggered at unmount time.
> > > +_scratch_unmount
> > > +
> > > +# Check the dmesg warning for data rsv leak.
> > > +#
> > > +# If CONFIG_BTRFS_DEBUG is enabled, we would have a kernel warning with
> > > +# backtrace, but for release builds, it's just a warning line.
> > > +# So here we manually check the warning message.
> > > +if _dmesg_since_test_start | grep -q "leak"; then
> > > + _fail "qgroup data reserved space leaked"
> > > +fi
> > > +
> > > +echo "Silence is golden"
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/btrfs/303.out b/tests/btrfs/303.out
> > > new file mode 100644
> > > index 00000000..d48808e6
> > > --- /dev/null
> > > +++ b/tests/btrfs/303.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 303
> > > +Silence is golden
> >
>
