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

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Mar 02, 2022 / 06:26, Sidong Yang wrote:
> 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>
> > > ---

(snip)

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

I think recent discussion on the list is a good reference [1]. A patch was
posted to add timeout to btrfs/255.

More importantly, it was discussed that such timeout of user space program will
not help. Eryu pointed out that once "the kernel already deadlocked, and
filesystem and/or device can't be used by next test either". IMHO, your new case
will not require timeout either with same reasoning.

[1] https://lore.kernel.org/fstests/20220223171126.GQ12643@twin.jikos.cz/T/#me349d62ff367a0a6a28076bdd5b89263fc8109c0

-- 
Best Regards,
Shin'ichiro Kawasaki