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

[Sidong Yang <realwakka@gmail.com>]

On Mon, Sep 21, 2020 at 01:27:40AM +0800, Eryu Guan wrote:
> On Sun, Sep 20, 2020 at 08:57:53AM +0000, 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.
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> 

Hi Eryu.
Thanks for review!

> We usually don't add new test case to existing tests, as that may make a
> PASSed test starting to FAIL, which looks like a regression.

Okay, If then, is it okay for writing a new script for this?
> 
> > ---
> >  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()
> 
> Local function names don't need to be prefixed with "_".
okay, thanks.
> 
> > +{
> > +	echo "=== parent assign test ===" >> $seqres.full
> > +	_run_btrfs_util_prog subvolume create $SCRATCH_MNT/a
> 
> The helpers based on run_check are not recommended, please just
> open-coded btrfs subvolume command and filter the output when necessary.
> j
> > +	_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
> > +
> > +	_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
> 
> Just fsync the individule files if possible.

Thanks. I'll fix it like this.
sync $SCRATCH_MNT/b/file

> 
> 
> > +
> > +	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')
> 
> $AWK_PROG
Okay! awk -> $AWK_PROG
> 
> > +
> > +	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"
> 
> Print the actual number and expected number as well?
Yes, you mean writing like this?

echo "a_shared = $a_shared" >> $seqres.full
echo "b_shared = $b_shared" >> $seqres.full
echo "sum = $sum" >> $seqres.full
echo "q_shared = $q_shared" >> $seqres.full

and I'm just curious that we don't need to cleanup qgroup environment after testing finished?

Thanks,
Sidong

> 
> Thanks,
> Eryu
> 
> > +}
> > +
> >  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"
> > -- 
> > 2.25.1