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

[Sidong Yang <realwakka@gmail.com>]

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?
It's okay that write some file and make a snapshot from it?

> 
> > +
> > +	_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"
> > 
>