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

[Filipe Manana <fdmanana@kernel.org>]

On Wed, Mar 02, 2022 at 01:43:16PM +0000, Sidong Yang wrote:
> 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.

Well, in the kernel patch we have:

  Fixes: e804861bd4e6 ("btrfs: fix deadlock between quota disable and qgroup rescan worker")

And that commit was introduced in 5.17-rc3. Maybe it deadlocked in a different
way before that commit, perhaps in the way that e804861bd4e6 describes. However
I haven't checked how it behaves on a kernel without that commit. But at least we
know that currently it deadlocks at 5.17-rc3+.

> > 
> > > 
> > > 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
> > >