Re: [PATCH 1/2] btrfs/254: don't leave mount on test fs in case of failure/interruption

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Feb 21, 2025 at 02:02:43PM +0800, Zorro Lang wrote:
> On Thu, Feb 20, 2025 at 08:18:19PM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 20, 2025 at 06:22:57PM +0000, Filipe Manana wrote:
> > > On Thu, Feb 20, 2025 at 5:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > On Thu, Feb 20, 2025 at 01:27:32PM +0800, Anand Jain wrote:
> > > > > On 20/2/25 02:19, fdmanana@kernel.org wrote:
> > > > > > From: Filipe Manana <fdmanana@suse.com>
> > > > > >
> > > > > > If the test fails or is interrupted after mounting $scratch_dev3 inside
> > > > > > the test filesystem and before unmounting at test_add_device(), we leave
> > > > > > without being unable to unmount the test filesystem since it has a mount
> > > > > > inside it. This results in the need to manually unmount $scratch_dev3,
> > > > > > otherwise a subsequent run of fstests fails since the unmount of the
> > > > > > test device fails with -EBUSY.
> > > > > >
> > > > > > Fix this by unmounting $scratch_dev3 ($seq_mnt) in the _cleanup()
> > > > > > function.
> > > > > >
> > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > > > > ---
> > > > > >   tests/btrfs/254 | 1 +
> > > > > >   1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/tests/btrfs/254 b/tests/btrfs/254
> > > > > > index d9c9eea9..6523389b 100755
> > > > > > --- a/tests/btrfs/254
> > > > > > +++ b/tests/btrfs/254
> > > > > > @@ -21,6 +21,7 @@ _cleanup()
> > > > > >   {
> > > > > >     cd /
> > > > > >     rm -f $tmp.*
> > > > > > +   $UMOUNT_PROG $seq_mnt > /dev/null 2>&1
> > > >
> > > > This should use the _unmount helper that's in for-next.
> > > 
> > > Sure, it does the same, except that it redirects stdout and stderr to
> > > $seqres.full.
> > > 
> > > Some tests are still calling  $UMOUNT_PROG directly. And that's often
> > > what we want, so that if umount fails we get a mismatch with the
> > > golden output instead of ignoring the failure.
> > > But in this case it's fine.
> > 
> > <groan> You're right, I'd repressed that Chinner decided to introduce
> > _unmount so that he could improve logging of unmount failures but then
> > he only bothered converting tests/{generic,xfs} because he didn't give
> > a damn about anyone else.
> > 
> > Now fstests is stuck with a half finished conversion and no clarity
> > about whether the rest of the $UMOUNT_PROG invocations should be
> > converted to _umount or if those are somehow intentional.
> > 
> > Hey Zorro, do you have any opinion on this?  Should someone just finish
> > the $UMOUNT_PROG -> _unmount conversion next week?
> 
> The release of this week will have your big randome-fixes. Next release will
> deal with your 10 PRs mainly :) So I'll deal with the "$UMOUNT_PROG ->
> _unmount conversion" after that. Anyway, we still can review the "conversion"
> patch at first.

Ok.  Let's do that next week.

--D

> Thanks,
> Zorro
> 
> > 
> > --D
> > 
> > > Anand, since you've already merged this patch into your repo, can you
> > > please replace that line with the following?
> > > 
> > > _unmount $seq_mnt
> > > 
> > > Thanks.
> > > 
> > > >
> > > > --D
> > > >
> > > > > >     rm -rf $seq_mnt > /dev/null 2>&1
> > > > > >     cleanup_dmdev
> > > > > >   }
> > > > >
> > > > >
> > > > > Reviewed-by: Anand Jain <anand.jain@oracle.com>
> > > > >
> > > > >
> > > > >
> > > 
> > 
> 
>