[PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[Qu Wenruo]

For "btrfs subvolume snapshot -i <qgroupid>", we only expect the target
qgroup to be a higher level one.

Assigning a 0 level qgroup to another 0 level qgroup is only going to
cause confusion, and I'm planning to do extra sanity checks both in
kernel and btrfs-progs to reject such behavior.

So change the test case to do regular higher level qgroup assignment
only.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/224 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/btrfs/224 b/tests/btrfs/224
index de10942f..611df3ab 100755
--- a/tests/btrfs/224
+++ b/tests/btrfs/224
@@ -67,7 +67,7 @@ assign_no_shared_test()
 	_check_scratch_fs
 }
 
-# Test snapshot with assigning qgroup for submodule
+# Test snapshot with assigning qgroup for higher level qgroup
 snapshot_test()
 {
 	_scratch_mkfs > /dev/null 2>&1
@@ -78,9 +78,9 @@ snapshot_test()
 	_qgroup_rescan $SCRATCH_MNT >> $seqres.full
 
 	$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a >> $seqres.full
+	$BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT >> $seqres.full
 	_ddt of="$SCRATCH_MNT"/a/file1 bs=1M count=1 >> $seqres.full 2>&1
-	subvolid=$(_btrfs_get_subvolid $SCRATCH_MNT a)
-	$BTRFS_UTIL_PROG subvolume snapshot -i 0/$subvolid $SCRATCH_MNT/a $SCRATCH_MNT/b >> $seqres.full
+	$BTRFS_UTIL_PROG subvolume snapshot -i 1/0 $SCRATCH_MNT/a $SCRATCH_MNT/b >> $seqres.full
 
 	_scratch_unmount
 	_check_scratch_fs
-- 
2.42.0

Re: [PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[Anand Jain]

On 2/26/24 09:32, Qu Wenruo wrote:
> For "btrfs subvolume snapshot -i <qgroupid>", we only expect the target
> qgroup to be a higher level one.
> 
> Assigning a 0 level qgroup to another 0 level qgroup is only going to
> cause confusion, and I'm planning to do extra sanity checks both in
> kernel and btrfs-progs to reject such behavior.
> 
> So change the test case to do regular higher level qgroup assignment
> only.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

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

Thanks, Anand

> ---
>   tests/btrfs/224 | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/btrfs/224 b/tests/btrfs/224
> index de10942f..611df3ab 100755
> --- a/tests/btrfs/224
> +++ b/tests/btrfs/224
> @@ -67,7 +67,7 @@ assign_no_shared_test()
>   	_check_scratch_fs
>   }
>   
> -# Test snapshot with assigning qgroup for submodule
> +# Test snapshot with assigning qgroup for higher level qgroup
>   snapshot_test()
>   {
>   	_scratch_mkfs > /dev/null 2>&1
> @@ -78,9 +78,9 @@ snapshot_test()
>   	_qgroup_rescan $SCRATCH_MNT >> $seqres.full
>   
>   	$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a >> $seqres.full
> +	$BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT >> $seqres.full
>   	_ddt of="$SCRATCH_MNT"/a/file1 bs=1M count=1 >> $seqres.full 2>&1
> -	subvolid=$(_btrfs_get_subvolid $SCRATCH_MNT a)
> -	$BTRFS_UTIL_PROG subvolume snapshot -i 0/$subvolid $SCRATCH_MNT/a $SCRATCH_MNT/b >> $seqres.full
> +	$BTRFS_UTIL_PROG subvolume snapshot -i 1/0 $SCRATCH_MNT/a $SCRATCH_MNT/b >> $seqres.full
>   
>   	_scratch_unmount
>   	_check_scratch_fs

Re: [PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[Qu Wenruo]

在 2024/2/26 16:58, Anand Jain 写道:
> On 2/26/24 09:32, Qu Wenruo wrote:
>> For "btrfs subvolume snapshot -i <qgroupid>", we only expect the target
>> qgroup to be a higher level one.
>>
>> Assigning a 0 level qgroup to another 0 level qgroup is only going to
>> cause confusion, and I'm planning to do extra sanity checks both in
>> kernel and btrfs-progs to reject such behavior.
>>
>> So change the test case to do regular higher level qgroup assignment
>> only.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> looks good.
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> 
>   Applied to
>     https://github.com/asj/fstests.git for-next

Thanks for the review and merge, although I'd also like to get some 
feedback from the original author, to make sure there are not some weird 
use case.

Thanks,
Qu
> 
> Thanks, Anand
> 
>> ---
>>   tests/btrfs/224 | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/btrfs/224 b/tests/btrfs/224
>> index de10942f..611df3ab 100755
>> --- a/tests/btrfs/224
>> +++ b/tests/btrfs/224
>> @@ -67,7 +67,7 @@ assign_no_shared_test()
>>       _check_scratch_fs
>>   }
>> -# Test snapshot with assigning qgroup for submodule
>> +# Test snapshot with assigning qgroup for higher level qgroup
>>   snapshot_test()
>>   {
>>       _scratch_mkfs > /dev/null 2>&1
>> @@ -78,9 +78,9 @@ snapshot_test()
>>       _qgroup_rescan $SCRATCH_MNT >> $seqres.full
>>       $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a >> $seqres.full
>> +    $BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT >> $seqres.full
>>       _ddt of="$SCRATCH_MNT"/a/file1 bs=1M count=1 >> $seqres.full 2>&1
>> -    subvolid=$(_btrfs_get_subvolid $SCRATCH_MNT a)
>> -    $BTRFS_UTIL_PROG subvolume snapshot -i 0/$subvolid $SCRATCH_MNT/a 
>> $SCRATCH_MNT/b >> $seqres.full
>> +    $BTRFS_UTIL_PROG subvolume snapshot -i 1/0 $SCRATCH_MNT/a 
>> $SCRATCH_MNT/b >> $seqres.full
>>       _scratch_unmount
>>       _check_scratch_fs
> 

Re: [PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[Sidong Yang]

On Mon, Feb 26, 2024 at 06:31:31PM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/2/26 16:58, Anand Jain 写道:
> > On 2/26/24 09:32, Qu Wenruo wrote:
> > > For "btrfs subvolume snapshot -i <qgroupid>", we only expect the target
> > > qgroup to be a higher level one.
> > > 
> > > Assigning a 0 level qgroup to another 0 level qgroup is only going to
> > > cause confusion, and I'm planning to do extra sanity checks both in
> > > kernel and btrfs-progs to reject such behavior.
> > > 
> > > So change the test case to do regular higher level qgroup assignment
> > > only.
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > 
> > looks good.
> > 
> > Reviewed-by: Anand Jain <anand.jain@oracle.com>
> > 
> >   Applied to
> >     https://github.com/asj/fstests.git for-next
> 
> Thanks for the review and merge, although I'd also like to get some feedback
> from the original author, to make sure there are not some weird use case.

It seems that this line intented to assign a qgroup for new snapshot same as
subvolume. But I agree that 0 level qgroup doesn't make sense. This patch
Looks good to me. Thanks for asking to me. 

Thanks,
Sidong

> 
> Thanks,
> Qu
> > 
> > Thanks, Anand
> > 
> > > ---
> > >   tests/btrfs/224 | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/btrfs/224 b/tests/btrfs/224
> > > index de10942f..611df3ab 100755
> > > --- a/tests/btrfs/224
> > > +++ b/tests/btrfs/224
> > > @@ -67,7 +67,7 @@ assign_no_shared_test()
> > >       _check_scratch_fs
> > >   }
> > > -# Test snapshot with assigning qgroup for submodule
> > > +# Test snapshot with assigning qgroup for higher level qgroup
> > >   snapshot_test()
> > >   {
> > >       _scratch_mkfs > /dev/null 2>&1
> > > @@ -78,9 +78,9 @@ snapshot_test()
> > >       _qgroup_rescan $SCRATCH_MNT >> $seqres.full
> > >       $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a >> $seqres.full
> > > +    $BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT >> $seqres.full
> > >       _ddt of="$SCRATCH_MNT"/a/file1 bs=1M count=1 >> $seqres.full 2>&1
> > > -    subvolid=$(_btrfs_get_subvolid $SCRATCH_MNT a)
> > > -    $BTRFS_UTIL_PROG subvolume snapshot -i 0/$subvolid
> > > $SCRATCH_MNT/a $SCRATCH_MNT/b >> $seqres.full
> > > +    $BTRFS_UTIL_PROG subvolume snapshot -i 1/0 $SCRATCH_MNT/a
> > > $SCRATCH_MNT/b >> $seqres.full
> > >       _scratch_unmount
> > >       _check_scratch_fs
> > 

Re: [PATCH] fstests: btrfs/224: do not assign snapshot to a subvolume qgroup

[David Sterba]

On Mon, Feb 26, 2024 at 02:32:34PM +1030, Qu Wenruo wrote:
> For "btrfs subvolume snapshot -i <qgroupid>", we only expect the target
> qgroup to be a higher level one.
> 
> Assigning a 0 level qgroup to another 0 level qgroup is only going to
> cause confusion, and I'm planning to do extra sanity checks both in
> kernel and btrfs-progs to reject such behavior.

I think this was never intended, the higher level were meant to group
the leaf subvolumes. But it's possible that somebody is using it like
is in the test. In that case we'd have to define the semantics or at
least start warning about that and then remove it completely.