| Message ID | 164740143497.3371809.2959237196772812909.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: new tests for kernel 5.17 | expand |
On Tue, Mar 15, 2022 at 08:30:35PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > This is a regression test to make sure that nonzero error returns from > a filesystem's ->sync_fs implementation are actually passed back to > userspace when the call stack involves syncfs(2). > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > tests/xfs/839 | 42 ++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/839.out | 2 ++ > 2 files changed, 44 insertions(+) > create mode 100755 tests/xfs/839 > create mode 100644 tests/xfs/839.out > > > diff --git a/tests/xfs/839 b/tests/xfs/839 > new file mode 100755 > index 00000000..9bfe93ef > --- /dev/null > +++ b/tests/xfs/839 > @@ -0,0 +1,42 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2022 Oracle. All Rights Reserved. > +# > +# FS QA Test No. 839 > +# > +# Regression test for kernel commits: > +# > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs") > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs") > +# > +# During a code inspection, I noticed that sync_filesystem ignores the return > +# value of the ->sync_fs calls that it makes. sync_filesystem, in turn is used > +# by the syncfs(2) syscall to persist filesystem changes to disk. This means > +# that syncfs(2) does not capture internal filesystem errors that are neither > +# visible from the block device (e.g. media error) nor recorded in s_wb_err. > +# XFS historically returned 0 from ->sync_fs even if there were log failures, > +# so that had to be corrected as well. > +# > +# The kernel commits above fix this problem, so this test tries to trigger the > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the > +# hope that the EIO generated as a result of the filesystem being shut down is > +# only visible via ->sync_fs. > +# > +. ./common/preamble > +_begin_fstest auto quick shutdown > + > +# real QA test starts here > + > +# Modify as appropriate. > +_require_xfs_io_command syncfs > +_require_scratch_nocheck > +_require_scratch_shutdown Can this case be a generic case, with the help of _require_scratch_shutdown and _require_xfs_io_command? Thanks, Zorro > + > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't > +# bother checking the filesystem afterwards since we never wrote anything. > +_scratch_mount > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT > + > +# success, all done > +status=0 > +exit > diff --git a/tests/xfs/839.out b/tests/xfs/839.out > new file mode 100644 > index 00000000..f275cdcc > --- /dev/null > +++ b/tests/xfs/839.out > @@ -0,0 +1,2 @@ > +QA output created by 839 > +syncfs: Input/output error >
[adding tytso for the ext4 question] On Wed, Mar 23, 2022 at 10:44:32AM +0800, Zorro Lang wrote: > On Tue, Mar 15, 2022 at 08:30:35PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > This is a regression test to make sure that nonzero error returns from > > a filesystem's ->sync_fs implementation are actually passed back to > > userspace when the call stack involves syncfs(2). > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > tests/xfs/839 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > tests/xfs/839.out | 2 ++ > > 2 files changed, 44 insertions(+) > > create mode 100755 tests/xfs/839 > > create mode 100644 tests/xfs/839.out > > > > > > diff --git a/tests/xfs/839 b/tests/xfs/839 > > new file mode 100755 > > index 00000000..9bfe93ef > > --- /dev/null > > +++ b/tests/xfs/839 > > @@ -0,0 +1,42 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > +# > > +# FS QA Test No. 839 > > +# > > +# Regression test for kernel commits: > > +# > > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs") > > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs") > > +# > > +# During a code inspection, I noticed that sync_filesystem ignores the return > > +# value of the ->sync_fs calls that it makes. sync_filesystem, in turn is used > > +# by the syncfs(2) syscall to persist filesystem changes to disk. This means > > +# that syncfs(2) does not capture internal filesystem errors that are neither > > +# visible from the block device (e.g. media error) nor recorded in s_wb_err. > > +# XFS historically returned 0 from ->sync_fs even if there were log failures, > > +# so that had to be corrected as well. > > +# > > +# The kernel commits above fix this problem, so this test tries to trigger the > > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the > > +# hope that the EIO generated as a result of the filesystem being shut down is > > +# only visible via ->sync_fs. > > +# > > +. ./common/preamble > > +_begin_fstest auto quick shutdown > > + > > +# real QA test starts here > > + > > +# Modify as appropriate. > > +_require_xfs_io_command syncfs > > +_require_scratch_nocheck > > +_require_scratch_shutdown > > Can this case be a generic case, with the help of _require_scratch_shutdown > and _require_xfs_io_command? I'm not sure. Of the three filesystems that both have a ->sync_fs function and implement FS_IOC_SHUTDOWN, xfs and f2fs look like they passes errors like they should. ext4 is another story -- curiously, if the fs is shut down, it'll return 0 and it doesn't check the return value of dquot_writeback_dquots. I don't remember enough of ext to know if that's deliberate or merely an age-old artifact of the Bad Old Days when that whole code path didn't care about errors. --D > Thanks, > Zorro > > > + > > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't > > +# bother checking the filesystem afterwards since we never wrote anything. > > +_scratch_mount > > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT > > + > > +# success, all done > > +status=0 > > +exit > > diff --git a/tests/xfs/839.out b/tests/xfs/839.out > > new file mode 100644 > > index 00000000..f275cdcc > > --- /dev/null > > +++ b/tests/xfs/839.out > > @@ -0,0 +1,2 @@ > > +QA output created by 839 > > +syncfs: Input/output error > > >
diff --git a/tests/xfs/839 b/tests/xfs/839 new file mode 100755 index 00000000..9bfe93ef --- /dev/null +++ b/tests/xfs/839 @@ -0,0 +1,42 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 Oracle. All Rights Reserved. +# +# FS QA Test No. 839 +# +# Regression test for kernel commits: +# +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs") +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs") +# +# During a code inspection, I noticed that sync_filesystem ignores the return +# value of the ->sync_fs calls that it makes. sync_filesystem, in turn is used +# by the syncfs(2) syscall to persist filesystem changes to disk. This means +# that syncfs(2) does not capture internal filesystem errors that are neither +# visible from the block device (e.g. media error) nor recorded in s_wb_err. +# XFS historically returned 0 from ->sync_fs even if there were log failures, +# so that had to be corrected as well. +# +# The kernel commits above fix this problem, so this test tries to trigger the +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the +# hope that the EIO generated as a result of the filesystem being shut down is +# only visible via ->sync_fs. +# +. ./common/preamble +_begin_fstest auto quick shutdown + +# real QA test starts here + +# Modify as appropriate. +_require_xfs_io_command syncfs +_require_scratch_nocheck +_require_scratch_shutdown + +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't +# bother checking the filesystem afterwards since we never wrote anything. +_scratch_mount +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT + +# success, all done +status=0 +exit diff --git a/tests/xfs/839.out b/tests/xfs/839.out new file mode 100644 index 00000000..f275cdcc --- /dev/null +++ b/tests/xfs/839.out @@ -0,0 +1,2 @@ +QA output created by 839 +syncfs: Input/output error