| Message ID | 20240226040234.102767-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: btrfs/224: do not assign snapshot to a subvolume qgroup | expand |
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
在 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 >
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.
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 > >
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
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(-)