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 08:24:48AM +0000, Shinichiro Kawasaki wrote:
> 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
> 
Thanks for a good example. I understand that there is no help in
userspace for the kernel deadlocked. In this case, Just "wait" is
enough.

Thanks,
Sidong
> -- 
> Best Regards,
> Shin'ichiro Kawasaki