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 12:10:08PM +0000, Filipe Manana wrote:

Hi, Filipe!
Thanks for review.
> On Tue, Mar 01, 2022 at 03:19:30PM +0000, Sidong Yang wrote:
> > Test enabling/disable quota and creating/destroying qgroup repeatedly
> > in asynchronous and confirm it does not cause kernel hang. This is a
> 
> in asynchronous -> in parallel

Sure, Thanks!
> 
> > regression test for the problem reported to linux-btrfs list [1].
> 
> It's worth mentioning the deadlock only happens starting with kernel 5.17-rc3.

It only happens in 5.17-rc3 version? I didn't know about it. I'll add
mention about this.
> 
> > 
> > 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>
> 
> In addition to Shinichiro's comments...
> 
> > ---
> >  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.
> > +#
> > +# FS QA Test 262
> > +#
> > +# Test the deadlock between qgroup and quota commands
> 
> The test description should be a lot more clear.
> 
> "the deadlock" is vague a gives the wrong idea we only ever had a single
> deadlock related to qgroups. "qgroup and quota commands" is confusing,
> and "qgroup" and "quota" are pretty much synonyms, and it should mention
> which commands.
> 
> Plus what we want to test is that we can run some qgroup operations in
> parallel without triggering a deadlock, crash, etc.
> 
> Perhaps something like:
> 
> """
> Test that running qgroup enable, create, destroy and disable commands in
> parallel does not result in a deadlock, a crash or any filesystem
> inconsistency.
> """
> 
Yeah, It was not clear. I found that this test is not only for checking
deadlock. But it checks that test runs without any problem.

> 
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto qgroup
> 
> Can also be added to the "quick" group. It takes 1 second in my slowest vm.

Okay, Thanks!
> 
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs btrfs
> > +
> > +_require_scratch
> > +
> > +# Run command that enable/disable quota and create/destroy qgroup asynchronously
> 
> With the more clear test description above, this can go away.

Sure!
> 
> > +qgroup_deadlock_test()
> > +{
> > +	_scratch_mkfs > /dev/null 2>&1
> > +	_scratch_mount
> > +	echo "=== qgroup deadlock test ===" >> $seqres.full
> 
> There's no point in echoing this message to the .full file, it provides no
> value at all, as testing that is all that this testcase does.

I agree. This is pointless because it's simple test.
> 
> > +
> > +	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+=($!)		
> > +	done
> > +
> > +	for pid in "${pids[@]}"; do
> > +		wait $pid
> > +	done
> 
> As pointed before by Shinichiro, a simple 'wait' here is enough, no need to
> keep track of the PIDs.

Yeah, I don't have to go hard way.
> 
> > +
> > +	_scratch_unmount
> > +	_check_scratch_fs
> 
> Not needed, the fstests framework automatically runs 'btrfs check' when a test
> finishes. Doing this explicitly is only necessary when we need to do several
> mount/unmount operations and want to check the fs is fine after each unmount
> and before the next mount.
> 

I didn't know about that. I don't need to check manually.
> > +}
> > +
> > +qgroup_deadlock_test
> 
> There's no point in putting all the test code in a function, as the function
> is only called once.

Of course!
> 
> Otherwise it looks good, and the test works as advertised, it triggers a
> deadlock on 5.17-rc3+ kernel and passes on a patched kernel.
> 
> Thanks for converting the reproducer into a test case.
> 

Thanks for detailed review. I'll back soon with v2.

Thanks,
Sidong
> > +
> > +# 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
> >