[PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Qu Wenruo]

There is a bug that, when btrfs is beyond qgroup limit, touching a file
could crash btrfs.

Such beyond limit situation needs to be intentionally created, e.g.
writing 1GiB file, then limit the subvolume to 512 MiB.
As current qgroup works pretty well at preventing us from reaching the
limit.

This makes existing qgroup test cases unable to detect it.

The regression is introduced by commit c53e9653605d ("btrfs: qgroup: try
to flush qgroup space when we get -EDQUOT"), and the fix is titled
"btrfs: qgroup: don't commit transaction when we have already
 hold a transaction handler"

Link: https://bugzilla.suse.com/show_bug.cgi?id=1178634
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/154     | 62 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/154.out |  2 ++
 tests/btrfs/group   |  1 +
 3 files changed, 65 insertions(+)
 create mode 100755 tests/btrfs/154
 create mode 100644 tests/btrfs/154.out

diff --git a/tests/btrfs/154 b/tests/btrfs/154
new file mode 100755
index 00000000..2a65d182
--- /dev/null
+++ b/tests/btrfs/154
@@ -0,0 +1,62 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 154
+#
+# Test if btrfs qgroup would crash if we're modifying the fs
+# after exceeding the limit
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+
+# Need at least 2GiB
+_require_scratch_size $((2 * 1024 * 1024))
+_scratch_mkfs > /dev/null 2>&1
+
+_scratch_mount
+
+_pwrite_byte 0xcd 0 1G $SCRATCH_MNT/file >> $seqres.full
+
+# Make sure the data reach disk so later qgroup scan can see it
+sync
+
+$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
+$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
+
+# Set the limit to just 512MiB, which is way below the existing usage
+$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
+
+# Touch above file, if kernel not patched, it will trigger an ASSERT()
+#
+# Even for patched kernel, we will still get EDQUOT error, but that
+# is expected behavior.
+touch $SCRATCH_MNT/file 2>&1 | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
new file mode 100644
index 00000000..b526c3f3
--- /dev/null
+++ b/tests/btrfs/154.out
@@ -0,0 +1,2 @@
+QA output created by 154
+touch: setting times of 'SCRATCH_MNT/file': Disk quota exceeded
diff --git a/tests/btrfs/group b/tests/btrfs/group
index d18450c7..c491e339 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -156,6 +156,7 @@
 151 auto quick volume
 152 auto quick metadata qgroup send
 153 auto quick qgroup limit
+154 auto quick qgroup limit
 155 auto quick send
 156 auto quick trim balance
 157 auto quick raid
-- 
2.28.0

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Filipe Manana]

On Wed, Nov 11, 2020 at 11:32 AM Qu Wenruo <wqu@suse.com> wrote:
>
> There is a bug that, when btrfs is beyond qgroup limit, touching a file
> could crash btrfs.
>
> Such beyond limit situation needs to be intentionally created, e.g.
> writing 1GiB file, then limit the subvolume to 512 MiB.
> As current qgroup works pretty well at preventing us from reaching the
> limit.
>
> This makes existing qgroup test cases unable to detect it.
>
> The regression is introduced by commit c53e9653605d ("btrfs: qgroup: try
> to flush qgroup space when we get -EDQUOT"), and the fix is titled
> "btrfs: qgroup: don't commit transaction when we have already
>  hold a transaction handler"
>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1178634
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

Looks good, just one comment below.

> ---
>  tests/btrfs/154     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/154.out |  2 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/btrfs/154
>  create mode 100644 tests/btrfs/154.out
>
> diff --git a/tests/btrfs/154 b/tests/btrfs/154
> new file mode 100755
> index 00000000..2a65d182
> --- /dev/null
> +++ b/tests/btrfs/154
> @@ -0,0 +1,62 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 154
> +#
> +# Test if btrfs qgroup would crash if we're modifying the fs
> +# after exceeding the limit
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +
> +# Need at least 2GiB
> +_require_scratch_size $((2 * 1024 * 1024))
> +_scratch_mkfs > /dev/null 2>&1
> +
> +_scratch_mount
> +
> +_pwrite_byte 0xcd 0 1G $SCRATCH_MNT/file >> $seqres.full
> +
> +# Make sure the data reach disk so later qgroup scan can see it
> +sync
> +
> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
> +
> +# Set the limit to just 512MiB, which is way below the existing usage
> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT

$SCRATCH_MNT twice by mistake, though the command still works and the
test still reproduces the issue.

Eryu can probably remove one occurrence when picking this patch.

Thanks.

> +
> +# Touch above file, if kernel not patched, it will trigger an ASSERT()
> +#
> +# Even for patched kernel, we will still get EDQUOT error, but that
> +# is expected behavior.
> +touch $SCRATCH_MNT/file 2>&1 | _filter_scratch
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
> new file mode 100644
> index 00000000..b526c3f3
> --- /dev/null
> +++ b/tests/btrfs/154.out
> @@ -0,0 +1,2 @@
> +QA output created by 154
> +touch: setting times of 'SCRATCH_MNT/file': Disk quota exceeded
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index d18450c7..c491e339 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -156,6 +156,7 @@
>  151 auto quick volume
>  152 auto quick metadata qgroup send
>  153 auto quick qgroup limit
> +154 auto quick qgroup limit
>  155 auto quick send
>  156 auto quick trim balance
>  157 auto quick raid
> --
> 2.28.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4358 bytes --]



On 2020/11/11 下午10:38, Filipe Manana wrote:
> On Wed, Nov 11, 2020 at 11:32 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> There is a bug that, when btrfs is beyond qgroup limit, touching a file
>> could crash btrfs.
>>
>> Such beyond limit situation needs to be intentionally created, e.g.
>> writing 1GiB file, then limit the subvolume to 512 MiB.
>> As current qgroup works pretty well at preventing us from reaching the
>> limit.
>>
>> This makes existing qgroup test cases unable to detect it.
>>
>> The regression is introduced by commit c53e9653605d ("btrfs: qgroup: try
>> to flush qgroup space when we get -EDQUOT"), and the fix is titled
>> "btrfs: qgroup: don't commit transaction when we have already
>>  hold a transaction handler"
>>
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1178634
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Looks good, just one comment below.
> 
>> ---
>>  tests/btrfs/154     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/btrfs/154.out |  2 ++
>>  tests/btrfs/group   |  1 +
>>  3 files changed, 65 insertions(+)
>>  create mode 100755 tests/btrfs/154
>>  create mode 100644 tests/btrfs/154.out
>>
>> diff --git a/tests/btrfs/154 b/tests/btrfs/154
>> new file mode 100755
>> index 00000000..2a65d182
>> --- /dev/null
>> +++ b/tests/btrfs/154
>> @@ -0,0 +1,62 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved.
>> +#
>> +# FS QA Test 154
>> +#
>> +# Test if btrfs qgroup would crash if we're modifying the fs
>> +# after exceeding the limit
>> +#
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1       # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +       cd /
>> +       rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs btrfs
>> +
>> +# Need at least 2GiB
>> +_require_scratch_size $((2 * 1024 * 1024))
>> +_scratch_mkfs > /dev/null 2>&1
>> +
>> +_scratch_mount
>> +
>> +_pwrite_byte 0xcd 0 1G $SCRATCH_MNT/file >> $seqres.full
>> +
>> +# Make sure the data reach disk so later qgroup scan can see it
>> +sync
>> +
>> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
>> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
>> +
>> +# Set the limit to just 512MiB, which is way below the existing usage
>> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
> 
> $SCRATCH_MNT twice by mistake, though the command still works and the
> test still reproduces the issue.


Nope, that's the expected behavior.

Btrfs qgroup limit <size> <path>|<qgroupid> <path>

The first path is to determine qgroupid, while the last path is to
determine the fs.

In this particular case, since we're limit the 0/5 qgroup, it's also the
as the mount point, thus we specific it twice.

Thanks,
Qu
> 
> Eryu can probably remove one occurrence when picking this patch.
> 
> Thanks.
> 
>> +
>> +# Touch above file, if kernel not patched, it will trigger an ASSERT()
>> +#
>> +# Even for patched kernel, we will still get EDQUOT error, but that
>> +# is expected behavior.
>> +touch $SCRATCH_MNT/file 2>&1 | _filter_scratch
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
>> new file mode 100644
>> index 00000000..b526c3f3
>> --- /dev/null
>> +++ b/tests/btrfs/154.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 154
>> +touch: setting times of 'SCRATCH_MNT/file': Disk quota exceeded
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index d18450c7..c491e339 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -156,6 +156,7 @@
>>  151 auto quick volume
>>  152 auto quick metadata qgroup send
>>  153 auto quick qgroup limit
>> +154 auto quick qgroup limit
>>  155 auto quick send
>>  156 auto quick trim balance
>>  157 auto quick raid
>> --
>> 2.28.0
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[David Sterba]

On Thu, Nov 12, 2020 at 07:50:22AM +0800, Qu Wenruo wrote:
> >> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
> >> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
> >> +
> >> +# Set the limit to just 512MiB, which is way below the existing usage
> >> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
> > 
> > $SCRATCH_MNT twice by mistake, though the command still works and the
> > test still reproduces the issue.
> 
> Nope, that's the expected behavior.
> 
> Btrfs qgroup limit <size> <path>|<qgroupid> <path>
> 
> The first path is to determine qgroupid, while the last path is to
> determine the fs.
> 
> In this particular case, since we're limit the 0/5 qgroup, it's also the
> as the mount point, thus we specific it twice.

So why didn't you specify 0/5 so it's clear?

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1015 bytes --]



On 2020/11/13 下午11:19, David Sterba wrote:
> On Thu, Nov 12, 2020 at 07:50:22AM +0800, Qu Wenruo wrote:
>>>> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
>>>> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
>>>> +
>>>> +# Set the limit to just 512MiB, which is way below the existing usage
>>>> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
>>>
>>> $SCRATCH_MNT twice by mistake, though the command still works and the
>>> test still reproduces the issue.
>>
>> Nope, that's the expected behavior.
>>
>> Btrfs qgroup limit <size> <path>|<qgroupid> <path>
>>
>> The first path is to determine qgroupid, while the last path is to
>> determine the fs.
>>
>> In this particular case, since we're limit the 0/5 qgroup, it's also the
>> as the mount point, thus we specific it twice.
> 
> So why didn't you specify 0/5 so it's clear?
> 
Oh no, my brain just shorted, and forgot that it's 0/5 fixed for fs tree.

0/5 is indeed much better.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Filipe Manana]

On Sat, Nov 14, 2020 at 12:09 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2020/11/13 下午11:19, David Sterba wrote:
> > On Thu, Nov 12, 2020 at 07:50:22AM +0800, Qu Wenruo wrote:
> >>>> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
> >>>> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
> >>>> +
> >>>> +# Set the limit to just 512MiB, which is way below the existing usage
> >>>> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
> >>>
> >>> $SCRATCH_MNT twice by mistake, though the command still works and the
> >>> test still reproduces the issue.
> >>
> >> Nope, that's the expected behavior.
> >>
> >> Btrfs qgroup limit <size> <path>|<qgroupid> <path>
> >>
> >> The first path is to determine qgroupid, while the last path is to
> >> determine the fs.
> >>
> >> In this particular case, since we're limit the 0/5 qgroup, it's also the
> >> as the mount point, thus we specific it twice.
> >
> > So why didn't you specify 0/5 so it's clear?
> >
> Oh no, my brain just shorted, and forgot that it's 0/5 fixed for fs tree.
>
> 0/5 is indeed much better.

Any reason this wasn't merged? What happened?

Thanks.

>
> Thanks,
> Qu
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] fstests: btrfs: check qgroup doesn't crash when beyond limit

[Qu Wenruo]

On 2021/1/11 下午8:15, Filipe Manana wrote:
> On Sat, Nov 14, 2020 at 12:09 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2020/11/13 下午11:19, David Sterba wrote:
>>> On Thu, Nov 12, 2020 at 07:50:22AM +0800, Qu Wenruo wrote:
>>>>>> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
>>>>>> +$BTRFS_UTIL_PROG quota rescan -w $SCRATCH_MNT >> $seqres.full
>>>>>> +
>>>>>> +# Set the limit to just 512MiB, which is way below the existing usage
>>>>>> +$BTRFS_UTIL_PROG qgroup limit  512M $SCRATCH_MNT $SCRATCH_MNT
>>>>>
>>>>> $SCRATCH_MNT twice by mistake, though the command still works and the
>>>>> test still reproduces the issue.
>>>>
>>>> Nope, that's the expected behavior.
>>>>
>>>> Btrfs qgroup limit <size> <path>|<qgroupid> <path>
>>>>
>>>> The first path is to determine qgroupid, while the last path is to
>>>> determine the fs.
>>>>
>>>> In this particular case, since we're limit the 0/5 qgroup, it's also the
>>>> as the mount point, thus we specific it twice.
>>>
>>> So why didn't you specify 0/5 so it's clear?
>>>
>> Oh no, my brain just shorted, and forgot that it's 0/5 fixed for fs tree.
>>
>> 0/5 is indeed much better.
>
> Any reason this wasn't merged? What happened?

Thanks for the reminder.

Let me just resend with 0/5 to replace the $SCRATCH_MNT.

Thanks,
Qu
>
> Thanks.
>
>>
>> Thanks,
>> Qu
>>
>
>