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@xxxxxxxxx/ > > > > Signed-off-by: Sidong Yang <realwakka@xxxxxxxxx> > > 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 > >
