Re: [PATCH] btrfs/022: Add qgroup assign test

[Sidong Yang <realwakka@gmail.com>]

On Tue, Sep 22, 2020 at 07:49:19AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/9/21 下午11:16, Sidong Yang wrote:
> > On Mon, Sep 21, 2020 at 09:21:33AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2020/9/20 下午4:57, Sidong Yang wrote:
> >>> The btrfs/022 test is basic test about qgroup. but it doesn't have
> >>> test with qgroup assign function. This patch adds parent assign
> >>> test. parent assign test make two subvolumes and a qgroup for assign.
> >>> Assign two subvolumes with a qgroup and check that quota of group
> >>> has same value with sum of two subvolumes.
> > 
> > Hi Qu!
> > Thanks for review!
> > 
> >>
> >> A little surprised that I haven't submitted such test case, especially
> >> we had a fix in kernel.
> >>
> >> cbab8ade585a ("btrfs: qgroup: mark qgroup inconsistent if we're
> >> inherting snapshot to a new qgroup")
> > 
> > Yeah, there was no test code for qgroup.
> > 
> >>
> >> Despite the comment from Eryu, some btrfs specific comment inlined below.
> >>
> >>>
> >>> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> >>> ---
> >>>  tests/btrfs/022 | 40 ++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 40 insertions(+)
> >>>
> >>> diff --git a/tests/btrfs/022 b/tests/btrfs/022
> >>> index aaa27aaa..cafaa8b2 100755
> >>> --- a/tests/btrfs/022
> >>> +++ b/tests/btrfs/022
> >>> @@ -110,6 +110,40 @@ _limit_test_noexceed()
> >>>  	[ $? -eq 0 ] || _fail "should have been allowed to write"
> >>>  }
> >>>  
> >>> +#basic assign testing
> >>> +_parent_assign_test()
> >>> +{
> >>> +	echo "=== parent assign test ===" >> $seqres.full
> >>> +	_run_btrfs_util_prog subvolume create $SCRATCH_MNT/a
> >>> +	_run_btrfs_util_prog quota enable $SCRATCH_MNT
> >>> +	subvolid_a=$(_btrfs_get_subvolid $SCRATCH_MNT a)
> >>> +
> >>> +	_run_btrfs_util_prog subvolume create $SCRATCH_MNT/b
> >>> +	_run_btrfs_util_prog quota enable $SCRATCH_MNT
> >>> +	subvolid_b=$(_btrfs_get_subvolid $SCRATCH_MNT b)
> >>> +
> >>> +	_run_btrfs_util_prog qgroup create 1/100 $SCRATCH_MNT
> >>> +
> >>> +	_run_btrfs_util_prog qgroup assign 0/$subvolid_a 1/100 $SCRATCH_MNT
> >>> +	_run_btrfs_util_prog qgroup assign 0/$subvolid_b 1/100 $SCRATCH_MNT
> >>
> >> The coverage is not good enough.
> >>
> >> Qgroup assign (or inherit) happens not only during "btrfs qgroup assign"
> >> but also "btrfs subvolume snapshot -i".
> >>
> >> And we also need to consider cases like shared extents between two
> >> subvolumes (either caused by snapshot or reflink).
> >>
> >> That means we have two factors, assign or snapshot -i, subvolumes with
> >> shared extents or not.
> >> That means we need at least 3 combinations:
> >>
> >> - assign, no shared extents
> >> - assign, shared extents
> >> - snapshot -i, shared extents
> >>
> >> (snapshot -i, no shared extents is invalid, as snapshot will always
> >> cause shared extents)
> > 
> > Thanks for good example!
> > but there is a question. How can I make shared extents in test code?
> 
> Reflink is the most simple solution.
> 
> > It's okay that write some file and make a snapshot from it?
> 
> If you only want some shared extents, reflink is easier than snapshot.

I've written a new patch v2 for this. Could you review it?

Thanks,
Sidong

> 
> Thanks,
> Qu
> 
> > 
> >>
> >>> +
> >>> +	_ddt of=$SCRATCH_MNT/a/file bs=4M count=1 >> $seqres.full 2>&1
> >>> +	_ddt of=$SCRATCH_MNT/b/file bs=4M count=1 >> $seqres.full 2>&1
> >>> +	sync
> >>> +
> >>> +	a_shared=$($BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | grep "0/$subvolid_a")
> >>> +	a_shared=$(echo $a_shared | awk '{ print $2 }' | tr -dc '0-9')
> >>> +
> >>> +	b_shared=$($BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | grep "0/$subvolid_b")
> >>> +	b_shared=$(echo $b_shared | awk '{ print $2 }' | tr -dc '0-9')
> >>> +	sum=$(expr $b_shared  + $a_shared)
> >>> +
> >>> +	q_shared=$($BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | grep "1/100")
> >>> +	q_shared=$(echo $q_shared | awk '{ print $2 }' | tr -dc '0-9')
> >>> +
> >>> +	[ $sum -eq $q_shared ] || _fail "shared values don't match"
> >>
> >> Nope, we don't need to do such complex checking all by ourselves.
> >>
> >> Just let "btrfs check" to handle it, as it will also check the qgroup
> >> numbers.
> > 
> > It's very easy way to check! Thanks.
> > 
> > Taken together, I'll work for new test case that tests qgroup cases.
> > 
> > Sincerely,
> > Sidong
> > 
> >>
> >> Thanks,
> >> Qu
> >>
> >>> +}
> >>> +
> >>>  units=`_btrfs_qgroup_units`
> >>>  
> >>>  _scratch_mkfs > /dev/null 2>&1
> >>> @@ -133,6 +167,12 @@ _check_scratch_fs
> >>>  _scratch_mkfs > /dev/null 2>&1
> >>>  _scratch_mount
> >>>  _limit_test_noexceed
> >>> +_scratch_unmount
> >>> +_check_scratch_fs
> >>> +
> >>> +_scratch_mkfs > /dev/null 2>&1
> >>> +_scratch_mount
> >>> +_parent_assign_test
> >>>  
> >>>  # success, all done
> >>>  echo "Silence is golden"
> >>>
> >>
>