Re: [PATCH v2] fstests: btrfs: add a test case to make sure inconsitent qgroup won't leak reserved data space

[Anand Jain <anand.jain@oracle.com>]

On 2/24/24 01:21, 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@suse.com>
>>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>>
>> looks good.
>>
>> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> 
> 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?
> 
> 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?
> 
Conflicting test case numbers are resolved during integration,
as I've done for this new testcase. You can find it in the
branch mentioned below.

> And do we have a branch to base our new test cases upon? (Mostly to 
> avoid the number conflicting)
> 

Yes, you can view the upcoming test cases here:

     https://github.com/asj/fstests.git for-next

By basing the newer test cases on top of this, we can minimize
the chances of conflict.

Thanks, Anand


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