| Message ID | 9aa6c8318d11b2fd1c2e208d85b2f83ea81ff88d.1739989076.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fstests: a couple fixes for btrfs/254 | expand |
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 > rm -rf $seq_mnt > /dev/null 2>&1 > cleanup_dmdev > } Reviewed-by: Anand Jain <anand.jain@oracle.com>
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. --D > > rm -rf $seq_mnt > /dev/null 2>&1 > > cleanup_dmdev > > } > > > Reviewed-by: Anand Jain <anand.jain@oracle.com> > > >
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. 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> > > > > > >
On 21/2/25 02:22, 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. > > Anand, since you've already merged this patch into your repo, can you > please replace that line with the following? > > _unmount $seq_mnt > Applied. Thanks. Zorro, Just checked, and it looks like you haven’t pulled in patches from my for-next yet. I went ahead and force-updated it to keep unnecessary noise off the ML. If you're pulling this weekend, there are three patches in for-next ready to merge. ( https://github.com/asj/fstests.git for-next.) Thanks! Anand > Thanks. > >> >> --D >> >>>> rm -rf $seq_mnt > /dev/null 2>&1 >>>> cleanup_dmdev >>>> } >>> >>> >>> Reviewed-by: Anand Jain <anand.jain@oracle.com> >>> >>> >>>
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? --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> > > > > > > > > > >
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. 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> > > > > > > > > > > > > > > >
On Fri, Feb 21, 2025 at 09:48:10AM +0800, Anand Jain wrote: > On 21/2/25 02:22, 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. > > > > Anand, since you've already merged this patch into your repo, can you > > please replace that line with the following? > > > > _unmount $seq_mnt > > > > Applied. > Thanks. > > Zorro, > > Just checked, and it looks like you haven’t pulled in patches > from my for-next yet. > I went ahead and force-updated it to keep unnecessary noise > off the ML. > > If you're pulling this weekend, there are three patches in > for-next ready to merge. Actually I've pulled, just haven't pushed :) Then I saw this conversation, so I'm going to reset and re-pull. The next release is nearly done, I'll pick up more simple patches with RVB today, then prepare to release it. Thanks! Thanks, Zorro > > ( https://github.com/asj/fstests.git for-next.) > > > Thanks! > Anand > > > Thanks. > > > > > > > > --D > > > > > > > > rm -rf $seq_mnt > /dev/null 2>&1 > > > > > cleanup_dmdev > > > > > } > > > > > > > > > > > > Reviewed-by: Anand Jain <anand.jain@oracle.com> > > > > > > > > > > > > >
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 rm -rf $seq_mnt > /dev/null 2>&1 cleanup_dmdev }