| Message ID | 20190805222738.21422-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/322: remove bad xfs_io sync_range | expand |
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
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
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
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
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
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
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(-)