[PATCH] generic/322: remove bad xfs_io sync_range

[PATCH] generic/322: remove bad xfs_io sync_range

[Andreas Gruenbacher]

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(-)

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

Re: [PATCH] generic/322: remove bad xfs_io sync_range

[Andreas Gruenbacher]

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

Re: [PATCH] generic/322: remove bad xfs_io sync_range

[Eryu Guan]

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

Re: [PATCH] generic/322: remove bad xfs_io sync_range

[Josef Bacik]

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

Re: [PATCH] generic/322: remove bad xfs_io sync_range

[Eryu Guan]

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

Re: [PATCH] generic/322: remove bad xfs_io sync_range

[Andreas Gruenbacher]

On Sun, Aug 18, 2019 at 5:22 PM Eryu Guan <guaneryu@gmail.com> wrote:
> 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?

Done, if that helps.

Andreas