generic/322: remove bad xfs_io sync_range
diff mbox series

Message ID 20190805222738.21422-1-agruenba@redhat.com
State New
Headers show
Series
  • generic/322: remove bad xfs_io sync_range
Related show

Commit Message

Andreas Gruenbacher Aug. 5, 2019, 10:27 p.m. UTC
The xfs_io sync_range command requires offset and length arguments.  Those are
missing here, so the command fails with:

  bad argument count 1 to sync_range, expected at least 2 arguments

This went unnoticed because xfs_io still exits with status 0 in such cases,
which looks like a separate bug.

I'm assuming that the test did catch regressions as is and that the sync_range
command isn't needed.  If this isn't the case, please fix the test.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 tests/generic/322 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Gruenbacher Aug. 6, 2019, 1:48 p.m. UTC | #1
On Tue, 6 Aug 2019 at 00:27, Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> The xfs_io sync_range command requires offset and length arguments.  Those are
> missing here, so the command fails with:
>
>   bad argument count 1 to sync_range, expected at least 2 arguments
>
> This went unnoticed because xfs_io still exits with status 0 in such cases,
> which looks like a separate bug.
>
> I'm assuming that the test did catch regressions as is and that the sync_range
> command isn't needed.  If this isn't the case, please fix the test.

Copying Josef who seems to be the author of this test case.

> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  tests/generic/322 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/generic/322 b/tests/generic/322
> index 2afd7127..22797c04 100755
> --- a/tests/generic/322
> +++ b/tests/generic/322
> @@ -65,7 +65,7 @@ _write_after_fsync_rename_test()
>         echo "fsync rename test"
>         _mount_flakey
>         $XFS_IO_PROG -f -c "pwrite 0 1M" -c "fsync" -c "pwrite 2M 1M" \
> -               -c "sync_range -b" $SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"
> +               $SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"
>         mv $SCRATCH_MNT/foo $SCRATCH_MNT/bar
>         $XFS_IO_PROG -c "fsync" $SCRATCH_MNT/bar
>         md5sum $SCRATCH_MNT/bar | _filter_scratch
> --
> 2.20.1

Thanks,
Andreas
Eryu Guan Aug. 9, 2019, 9:47 a.m. UTC | #2
On Tue, Aug 06, 2019 at 12:27:38AM +0200, Andreas Gruenbacher wrote:
> The xfs_io sync_range command requires offset and length arguments.  Those are
> missing here, so the command fails with:
> 
>   bad argument count 1 to sync_range, expected at least 2 arguments
> 
> This went unnoticed because xfs_io still exits with status 0 in such cases,
> which looks like a separate bug.

Yes, fstests should not rely on return status of xfs_io.

> 
> I'm assuming that the test did catch regressions as is and that the sync_range
> command isn't needed.  If this isn't the case, please fix the test.

Josef, would you please confirm?

> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  tests/generic/322 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/322 b/tests/generic/322
> index 2afd7127..22797c04 100755
> --- a/tests/generic/322
> +++ b/tests/generic/322
> @@ -65,7 +65,7 @@ _write_after_fsync_rename_test()
>  	echo "fsync rename test"
>  	_mount_flakey
>  	$XFS_IO_PROG -f -c "pwrite 0 1M" -c "fsync" -c "pwrite 2M 1M" \
> -		-c "sync_range -b" $SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"
> +		$SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"

We don't need the "_fail" part then, and appending xfs_io log to
$seqres.full, not overwriting. The same is true for _rename_test().

Thanks,
Eryu

>  	mv $SCRATCH_MNT/foo $SCRATCH_MNT/bar
>  	$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/bar
>  	md5sum $SCRATCH_MNT/bar | _filter_scratch
> -- 
> 2.20.1
Josef Bacik Aug. 15, 2019, 2:49 p.m. UTC | #3
On Tue, Aug 06, 2019 at 03:48:32PM +0200, Andreas Gruenbacher wrote:
> On Tue, 6 Aug 2019 at 00:27, Andreas Gruenbacher <agruenba@redhat.com> wrote:
> >
> > The xfs_io sync_range command requires offset and length arguments.  Those are
> > missing here, so the command fails with:
> >
> >   bad argument count 1 to sync_range, expected at least 2 arguments
> >
> > This went unnoticed because xfs_io still exits with status 0 in such cases,
> > which looks like a separate bug.
> >
> > I'm assuming that the test did catch regressions as is and that the sync_range
> > command isn't needed.  If this isn't the case, please fix the test.
> 
> Copying Josef who seems to be the author of this test case.

Looking back at it I think I added the sync_range because it was a little racey
wether it would trip the problem or not.  The rename in btrfs would do the dirty
writeout IIRC so that's probably why the problem still reproduced even though
the sync file range was wrong.

That being said the sync_range needs to be there, so instead of deleting it we
should have

sync_range -b 2M 1M

Thanks,

Josef
Eryu Guan Aug. 18, 2019, 3:22 p.m. UTC | #4
On Thu, Aug 15, 2019 at 10:49:58AM -0400, Josef Bacik wrote:
> On Tue, Aug 06, 2019 at 03:48:32PM +0200, Andreas Gruenbacher wrote:
> > On Tue, 6 Aug 2019 at 00:27, Andreas Gruenbacher <agruenba@redhat.com> wrote:
> > >
> > > The xfs_io sync_range command requires offset and length arguments.  Those are
> > > missing here, so the command fails with:
> > >
> > >   bad argument count 1 to sync_range, expected at least 2 arguments
> > >
> > > This went unnoticed because xfs_io still exits with status 0 in such cases,
> > > which looks like a separate bug.
> > >
> > > I'm assuming that the test did catch regressions as is and that the sync_range
> > > command isn't needed.  If this isn't the case, please fix the test.
> > 
> > Copying Josef who seems to be the author of this test case.
> 
> Looking back at it I think I added the sync_range because it was a little racey
> wether it would trip the problem or not.  The rename in btrfs would do the dirty
> writeout IIRC so that's probably why the problem still reproduced even though
> the sync file range was wrong.
> 
> That being said the sync_range needs to be there, so instead of deleting it we
> should have
> 
> sync_range -b 2M 1M

Hi Andreas, would you like to update the patch with the sync_range
fixed?

Thanks,
Eryu

Patch
diff mbox series

diff --git a/tests/generic/322 b/tests/generic/322
index 2afd7127..22797c04 100755
--- a/tests/generic/322
+++ b/tests/generic/322
@@ -65,7 +65,7 @@  _write_after_fsync_rename_test()
 	echo "fsync rename test"
 	_mount_flakey
 	$XFS_IO_PROG -f -c "pwrite 0 1M" -c "fsync" -c "pwrite 2M 1M" \
-		-c "sync_range -b" $SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"
+		$SCRATCH_MNT/foo > $seqres.full 2>&1 || _fail "xfs_io failed"
 	mv $SCRATCH_MNT/foo $SCRATCH_MNT/bar
 	$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/bar
 	md5sum $SCRATCH_MNT/bar | _filter_scratch