Re: [PATCH] btrfs: add test for enable/disable quota and create/destroy qgroup repeatedly

[Sidong Yang <realwakka@gmail.com>]

On Wed, Mar 02, 2022 at 04:43:35AM +0000, Shinichiro Kawasaki wrote:

Hi, Shinichiro.

Thanks for reply!

> Hi Sidong,
> 
> I tried this patch and observed that it recreates the hang and confirms the fix.
> Thanks. Here's my comments for improvements.
> 
> On Mar 01, 2022 / 15:19, Sidong Yang wrote:
> > Test enabling/disable quota and creating/destroying qgroup repeatedly
> 
> nit: gerund (...ing) and base form are mixed. Base form would be the better to
> be same as the code comment.

Yeah, 'disable' should be disabling.
> 
> > in asynchronous and confirm it does not cause kernel hang. This is a
> > regression test for the problem reported to linux-btrfs list [1].
> > 
> > The hang was recreated using the test case and fixed by kernel patch
> > titled
> > 
> >   btrfs: qgroup: fix deadlock between rescan worker and remove qgroup
> > 
> > [1] https://lore.kernel.org/linux-btrfs/20220228014340.21309-1-realwakka@gmail.com/
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  tests/btrfs/262     | 54 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/262.out |  2 ++
> >  2 files changed, 56 insertions(+)
> >  create mode 100755 tests/btrfs/262
> >  create mode 100644 tests/btrfs/262.out
> > 
> > diff --git a/tests/btrfs/262 b/tests/btrfs/262
> > new file mode 100755
> > index 00000000..9be380f9
> > --- /dev/null
> > +++ b/tests/btrfs/262
> > @@ -0,0 +1,54 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 YOUR NAME HERE.  All Rights Reserved.
> 
> It's the better to put your name there :)

Oops, 
> 
> > +#
> > +# FS QA Test 262
> > +#
> > +# Test the deadlock between qgroup and quota commands
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto qgroup
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> 
> Can we remove this comment?
Sure!
> 
> > +_supported_fs btrfs
> > +
> > +_require_scratch
> > +
> > +# Run command that enable/disable quota and create/destroy qgroup asynchronously
> > +qgroup_deadlock_test()
> > +{
> > +	_scratch_mkfs > /dev/null 2>&1
> > +	_scratch_mount
> > +	echo "=== qgroup deadlock test ===" >> $seqres.full
> > +
> > +	pids=()
> > +	for ((i = 0; i < 200; i++)); do
> > +		$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT 2>> $seqres.full &
> > +		pids+=($!)
> > +		$BTRFS_UTIL_PROG qgroup create 1/0 $SCRATCH_MNT 2>> $seqres.full &
> > +		pids+=($!)
> > +		$BTRFS_UTIL_PROG qgroup destroy 1/0 $SCRATCH_MNT 2>> $seqres.full &
> > +		pids+=($!)
> > +		$BTRFS_UTIL_PROG quota disable $SCRATCH_MNT 2>> $seqres.full &
> > +		pids+=($!)		
> 
> Trailing tabs in the line above.
Oops..
> 
> > +	done
> > +
> > +	for pid in "${pids[@]}"; do
> > +		wait $pid
> > +	done
> 
> I think simple "wait" command does what the for loop does.

I didn't know that "wait" command with no parameter waits all background
processes to finish. So it seems that we don't need pids it can be
deleted. Thanks.

Actually I've been agony about this. Does it needs timeout? When I tried
to command like this "timeout 10s wait", This command couldn't be
executed becase "wait" command is not binary. How can I insert timeout?

> 
> > +
> > +	_scratch_unmount
> > +	_check_scratch_fs
> 
> I think _scratch_unmount and _check_scratch_fs are required only when
> the test case repeats scratch mount and unmount. This test case looks
> simple for me and the qgroup_deadlock_test function may not be necessary.

I agree. This case is very simple. We don't need to use function. It
would be better that it's written in simple.

> 
> > +}
> > +
> > +qgroup_deadlock_test
> > +
> > +# success, all done
> > +echo "Silence is golden"
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/262.out b/tests/btrfs/262.out
> > new file mode 100644
> > index 00000000..404badc3
> > --- /dev/null
> > +++ b/tests/btrfs/262.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 262
> > +Silence is golden
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Best Regards,
> Shin'ichiro Kawasaki