| Message ID | 20240830180844.857283-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | overlay: create a variant to syncfs error test xfs/546 | expand |
On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > Test overlayfs over xfs with and without "volatile" mount option. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Zorro, > > I was going to make a generic test from xfs/546, so that overlayfs could > also run it, but then I realized that ext4 does not behave as xfs in > that case (it returns success on syncfs post shutdown). > > Unless and until this behavior is made a standard, I made an overlayfs > specialized test instead, which checks for underlying fs xfs. > While at it, I also added test coverage for the "volatile" mount options > that is expected to return succuss in that case regardles of the > behavior of the underlying fs. As I said elsewhere in the thread, I think that's a bug in ext4 that needs fixing, not a divergence of a testcase. Perhaps we ought to promote xfs/546 to generic/ and (if Ted disagrees with me about the EIO) add a _notrun for the overlayfs-on-ext4 case? --D > Thanks, > Amir. > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > tests/overlay/087.out | 4 +++ > 2 files changed, 66 insertions(+) > create mode 100755 tests/overlay/087 > create mode 100644 tests/overlay/087.out > > diff --git a/tests/overlay/087 b/tests/overlay/087 > new file mode 100755 > index 00000000..a5afb0d5 > --- /dev/null > +++ b/tests/overlay/087 > @@ -0,0 +1,62 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2022 Oracle. All Rights Reserved. > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > +# > +# FS QA Test No. 087 > +# > +# This is a variant of test xfs/546 for overlayfs > +# with and without the "volatile" mount option. > +# It only works over xfs underlying fs. > +# > +# 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 mount shutdown > + > + > +# Modify as appropriate. > +_require_xfs_io_command syncfs > +_require_scratch_nocheck > +_require_scratch_shutdown > + > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > +echo "=== syncfs after shutdown" > +_scratch_mount > +# This command is complicated a bit because in the case of overlayfs the > +# syncfs fd needs to be opened before shutdown and it is different from the > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > +# Filter out xfs_io output of active fds. > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > + grep -vF '[00' > + > +# Now repeat the same test with a volatile overlayfs mount and expect no error > +_scratch_unmount > +echo "=== syncfs after shutdown (volatile)" > +_scratch_mount -o volatile > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > + grep -vF '[00' > + > +# success, all done > +status=0 > +exit > diff --git a/tests/overlay/087.out b/tests/overlay/087.out > new file mode 100644 > index 00000000..44b538d8 > --- /dev/null > +++ b/tests/overlay/087.out > @@ -0,0 +1,4 @@ > +QA output created by 087 > +=== syncfs after shutdown > +syncfs: Input/output error > +=== syncfs after shutdown (volatile) > -- > 2.34.1 > >
On Mon, Sep 2, 2024 at 9:07 PM Darrick J. Wong <djwong@kernel.org> wrote: > > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > Test overlayfs over xfs with and without "volatile" mount option. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Zorro, > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > also run it, but then I realized that ext4 does not behave as xfs in > > that case (it returns success on syncfs post shutdown). > > > > Unless and until this behavior is made a standard, I made an overlayfs > > specialized test instead, which checks for underlying fs xfs. > > While at it, I also added test coverage for the "volatile" mount options > > that is expected to return succuss in that case regardles of the > > behavior of the underlying fs. > > As I said elsewhere in the thread, I think that's a bug in ext4 that > needs fixing, not a divergence of a testcase. Perhaps we ought to > promote xfs/546 to generic/ and (if Ted disagrees with me about the EIO) > add a _notrun for the overlayfs-on-ext4 case? > Well, I have the generic test ready, but it would also need to notrun on ext4 I would rather wait to see what Ted says. I don't mind promoting xfs/546 to generic/ if there is an agreement, but since I need test coverage for the overlayfs "volatile" mount option, I think I will have added this overlayfs test variant anyway. If ext4 shutdown code changes and there is a general agreement that the behavior in xfs/546 is expected from all filesystems, then I will be able to remove the requirement of "only over xfs" from this overlayfs test variant. Thanks, Amir.
On Mon, Sep 02, 2024 at 12:07:26PM -0700, Darrick J. Wong wrote: > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > Test overlayfs over xfs with and without "volatile" mount option. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Zorro, > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > also run it, but then I realized that ext4 does not behave as xfs in > > that case (it returns success on syncfs post shutdown). > > > > Unless and until this behavior is made a standard, I made an overlayfs > > specialized test instead, which checks for underlying fs xfs. > > While at it, I also added test coverage for the "volatile" mount options > > that is expected to return succuss in that case regardles of the > > behavior of the underlying fs. > > As I said elsewhere in the thread, I think that's a bug in ext4 that > needs fixing, not a divergence of a testcase. Perhaps we ought to > promote xfs/546 to generic/ and (if Ted disagrees with me about the EIO) > add a _notrun for the overlayfs-on-ext4 case? Agree, move xfs/546 to generic/, and help it to work with overlay. If ext4 won't change its behavior, then add comment and notrun. Thanks, Zorro > > --D > > > Thanks, > > Amir. > > > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > > tests/overlay/087.out | 4 +++ > > 2 files changed, 66 insertions(+) > > create mode 100755 tests/overlay/087 > > create mode 100644 tests/overlay/087.out > > > > diff --git a/tests/overlay/087 b/tests/overlay/087 > > new file mode 100755 > > index 00000000..a5afb0d5 > > --- /dev/null > > +++ b/tests/overlay/087 > > @@ -0,0 +1,62 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > > +# > > +# FS QA Test No. 087 > > +# > > +# This is a variant of test xfs/546 for overlayfs > > +# with and without the "volatile" mount option. > > +# It only works over xfs underlying fs. > > +# > > +# 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 mount shutdown > > + > > + > > +# Modify as appropriate. > > +_require_xfs_io_command syncfs > > +_require_scratch_nocheck > > +_require_scratch_shutdown > > + > > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > > +echo "=== syncfs after shutdown" > > +_scratch_mount > > +# This command is complicated a bit because in the case of overlayfs the > > +# syncfs fd needs to be opened before shutdown and it is different from the > > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > > +# Filter out xfs_io output of active fds. > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > + grep -vF '[00' > > + > > +# Now repeat the same test with a volatile overlayfs mount and expect no error > > +_scratch_unmount > > +echo "=== syncfs after shutdown (volatile)" > > +_scratch_mount -o volatile > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > + grep -vF '[00' > > + > > +# success, all done > > +status=0 > > +exit > > diff --git a/tests/overlay/087.out b/tests/overlay/087.out > > new file mode 100644 > > index 00000000..44b538d8 > > --- /dev/null > > +++ b/tests/overlay/087.out > > @@ -0,0 +1,4 @@ > > +QA output created by 087 > > +=== syncfs after shutdown > > +syncfs: Input/output error > > +=== syncfs after shutdown (volatile) > > -- > > 2.34.1 > > > > >
On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > Test overlayfs over xfs with and without "volatile" mount option. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Zorro, > > I was going to make a generic test from xfs/546, so that overlayfs could > also run it, but then I realized that ext4 does not behave as xfs in > that case (it returns success on syncfs post shutdown). > > Unless and until this behavior is made a standard, I made an overlayfs > specialized test instead, which checks for underlying fs xfs. > While at it, I also added test coverage for the "volatile" mount options > that is expected to return succuss in that case regardles of the > behavior of the underlying fs. > > Thanks, > Amir. > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > tests/overlay/087.out | 4 +++ > 2 files changed, 66 insertions(+) > create mode 100755 tests/overlay/087 > create mode 100644 tests/overlay/087.out > > diff --git a/tests/overlay/087 b/tests/overlay/087 > new file mode 100755 > index 00000000..a5afb0d5 > --- /dev/null > +++ b/tests/overlay/087 > @@ -0,0 +1,62 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2022 Oracle. All Rights Reserved. > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > +# > +# FS QA Test No. 087 > +# > +# This is a variant of test xfs/546 for overlayfs > +# with and without the "volatile" mount option. > +# It only works over xfs underlying fs. > +# > +# 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 mount shutdown > + > + > +# Modify as appropriate. > +_require_xfs_io_command syncfs > +_require_scratch_nocheck > +_require_scratch_shutdown > + > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > +echo "=== syncfs after shutdown" > +_scratch_mount > +# This command is complicated a bit because in the case of overlayfs the > +# syncfs fd needs to be opened before shutdown and it is different from the > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > +# Filter out xfs_io output of active fds. > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > + grep -vF '[00' > + > +# Now repeat the same test with a volatile overlayfs mount and expect no error > +_scratch_unmount > +echo "=== syncfs after shutdown (volatile)" > +_scratch_mount -o volatile > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > + grep -vF '[00' Oh, the test steps are much different from xfs/546. If we move x/546 to generic/, can overlay reproduce this bug by that? If not, I think we can have this overlay specific test case at first, and "move x/546 to generic" can be another job. Thanks, Zorro > + > +# success, all done > +status=0 > +exit > diff --git a/tests/overlay/087.out b/tests/overlay/087.out > new file mode 100644 > index 00000000..44b538d8 > --- /dev/null > +++ b/tests/overlay/087.out > @@ -0,0 +1,4 @@ > +QA output created by 087 > +=== syncfs after shutdown > +syncfs: Input/output error > +=== syncfs after shutdown (volatile) > -- > 2.34.1 >
On Tue, Sep 3, 2024 at 6:21 AM Zorro Lang <zlang@redhat.com> wrote: > > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > Test overlayfs over xfs with and without "volatile" mount option. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Zorro, > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > also run it, but then I realized that ext4 does not behave as xfs in > > that case (it returns success on syncfs post shutdown). > > > > Unless and until this behavior is made a standard, I made an overlayfs > > specialized test instead, which checks for underlying fs xfs. > > While at it, I also added test coverage for the "volatile" mount options > > that is expected to return succuss in that case regardles of the > > behavior of the underlying fs. > > > > Thanks, > > Amir. > > > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > > tests/overlay/087.out | 4 +++ > > 2 files changed, 66 insertions(+) > > create mode 100755 tests/overlay/087 > > create mode 100644 tests/overlay/087.out > > > > diff --git a/tests/overlay/087 b/tests/overlay/087 > > new file mode 100755 > > index 00000000..a5afb0d5 > > --- /dev/null > > +++ b/tests/overlay/087 > > @@ -0,0 +1,62 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > > +# > > +# FS QA Test No. 087 > > +# > > +# This is a variant of test xfs/546 for overlayfs > > +# with and without the "volatile" mount option. > > +# It only works over xfs underlying fs. > > +# > > +# 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 mount shutdown > > + > > + > > +# Modify as appropriate. > > +_require_xfs_io_command syncfs > > +_require_scratch_nocheck > > +_require_scratch_shutdown > > + > > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > > +echo "=== syncfs after shutdown" > > +_scratch_mount > > +# This command is complicated a bit because in the case of overlayfs the > > +# syncfs fd needs to be opened before shutdown and it is different from the > > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > > +# Filter out xfs_io output of active fds. > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > + grep -vF '[00' > > + > > +# Now repeat the same test with a volatile overlayfs mount and expect no error > > +_scratch_unmount > > +echo "=== syncfs after shutdown (volatile)" > > +_scratch_mount -o volatile > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > + grep -vF '[00' > > Oh, the test steps are much different from xfs/546. If we move x/546 to generic/, > can overlay reproduce this bug by that? Yes and no. For overlayfs to support this as a generic test, the helper _scratch_shutdown_handle must be used and the shutdown+syncfs command must be complicated to something like this: $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ grep -vF '[00' This is because overlayfs itself does not support the shutdown ioctl. If the test is moved to generic as it is we get an error when running overlayfs: XFS_IOC_GOINGDOWN: Inappropriate ioctl for device because _require_scratch_shutdown is "supported" by overlayfs but only when the _scratch_shutdown helpers are used. If the test is to be moved as is, it will need to opt-out of overlayfs explicitly. > If not, I think we can have this overlay > specific test case at first, and "move x/546 to generic" can be another job. > The reason I posted the overlayfs test is because for a while I noticed that we do not have test coverage for "volatile" mount option and adding this test coverage to this test was a very low hanging fruit. So I would like to keep the overlayfs test regardless of the decision about moving x/546 to generic. Thanks, Amir.
On Tue, Sep 03, 2024 at 08:41:28AM +0200, Amir Goldstein wrote: > On Tue, Sep 3, 2024 at 6:21 AM Zorro Lang <zlang@redhat.com> wrote: > > > > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > > Test overlayfs over xfs with and without "volatile" mount option. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > Zorro, > > > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > > also run it, but then I realized that ext4 does not behave as xfs in > > > that case (it returns success on syncfs post shutdown). > > > > > > Unless and until this behavior is made a standard, I made an overlayfs > > > specialized test instead, which checks for underlying fs xfs. > > > While at it, I also added test coverage for the "volatile" mount options > > > that is expected to return succuss in that case regardles of the > > > behavior of the underlying fs. > > > > > > Thanks, > > > Amir. > > > > > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > > > tests/overlay/087.out | 4 +++ > > > 2 files changed, 66 insertions(+) > > > create mode 100755 tests/overlay/087 > > > create mode 100644 tests/overlay/087.out > > > > > > diff --git a/tests/overlay/087 b/tests/overlay/087 > > > new file mode 100755 > > > index 00000000..a5afb0d5 > > > --- /dev/null > > > +++ b/tests/overlay/087 > > > @@ -0,0 +1,62 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > > > +# > > > +# FS QA Test No. 087 > > > +# > > > +# This is a variant of test xfs/546 for overlayfs > > > +# with and without the "volatile" mount option. > > > +# It only works over xfs underlying fs. > > > +# > > > +# 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 mount shutdown > > > + > > > + > > > +# Modify as appropriate. > > > +_require_xfs_io_command syncfs > > > +_require_scratch_nocheck > > > +_require_scratch_shutdown > > > + > > > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > > > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > > > +echo "=== syncfs after shutdown" > > > +_scratch_mount > > > +# This command is complicated a bit because in the case of overlayfs the > > > +# syncfs fd needs to be opened before shutdown and it is different from the > > > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > > > +# Filter out xfs_io output of active fds. > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > + grep -vF '[00' > > > + > > > +# Now repeat the same test with a volatile overlayfs mount and expect no error > > > +_scratch_unmount > > > +echo "=== syncfs after shutdown (volatile)" > > > +_scratch_mount -o volatile > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > + grep -vF '[00' > > > > Oh, the test steps are much different from xfs/546. If we move x/546 to generic/, > > can overlay reproduce this bug by that? > > Yes and no. > > For overlayfs to support this as a generic test, the helper > _scratch_shutdown_handle must be used and the shutdown+syncfs > command must be complicated to something like this: > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f > ' -c close -c syncfs $SCRATCH_MNT | \ > grep -vF '[00' > > This is because overlayfs itself does not support the shutdown ioctl. > If the test is moved to generic as it is we get an error when running > overlayfs: > > XFS_IOC_GOINGDOWN: Inappropriate ioctl for device > > because _require_scratch_shutdown is "supported" by overlayfs > but only when the _scratch_shutdown helpers are used. Yeah, I know this. > > If the test is to be moved as is, it will need to opt-out of overlayfs > explicitly. I mean you have a "-o volatile" option test, that's an overlayfs specific mount option. If you need that test, that's an overlay specific test, that part can be an overlay specific test case. If not, we can use a generic case (from xfs/546) to cover overlay and other fs. BTW, we actually we have a common _scratch_shutdown helper, I'm wondering if it works for this test? Likes: _scratch_shutdown -f $XFS_IO_PROG -c syncfs $SCRATCH_MNT Can this work? Thanks, Zorro > > > If not, I think we can have this overlay > > specific test case at first, and "move x/546 to generic" can be another job. > > > > The reason I posted the overlayfs test is because for a while I noticed > that we do not have test coverage for "volatile" mount option > and adding this test coverage to this test was a very low hanging fruit. > So I would like to keep the overlayfs test regardless of the decision > about moving x/546 to generic. > > Thanks, > Amir. >
On Wed, Sep 4, 2024 at 4:58 AM Zorro Lang <zlang@redhat.com> wrote: > > On Tue, Sep 03, 2024 at 08:41:28AM +0200, Amir Goldstein wrote: > > On Tue, Sep 3, 2024 at 6:21 AM Zorro Lang <zlang@redhat.com> wrote: > > > > > > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > > > Test overlayfs over xfs with and without "volatile" mount option. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > > > > > Zorro, > > > > > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > > > also run it, but then I realized that ext4 does not behave as xfs in > > > > that case (it returns success on syncfs post shutdown). > > > > > > > > Unless and until this behavior is made a standard, I made an overlayfs > > > > specialized test instead, which checks for underlying fs xfs. > > > > While at it, I also added test coverage for the "volatile" mount options > > > > that is expected to return succuss in that case regardles of the > > > > behavior of the underlying fs. > > > > > > > > Thanks, > > > > Amir. > > > > > > > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > > > > tests/overlay/087.out | 4 +++ > > > > 2 files changed, 66 insertions(+) > > > > create mode 100755 tests/overlay/087 > > > > create mode 100644 tests/overlay/087.out > > > > > > > > diff --git a/tests/overlay/087 b/tests/overlay/087 > > > > new file mode 100755 > > > > index 00000000..a5afb0d5 > > > > --- /dev/null > > > > +++ b/tests/overlay/087 > > > > @@ -0,0 +1,62 @@ > > > > +#! /bin/bash > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > > > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > > > > +# > > > > +# FS QA Test No. 087 > > > > +# > > > > +# This is a variant of test xfs/546 for overlayfs > > > > +# with and without the "volatile" mount option. > > > > +# It only works over xfs underlying fs. > > > > +# > > > > +# 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 mount shutdown > > > > + > > > > + > > > > +# Modify as appropriate. > > > > +_require_xfs_io_command syncfs > > > > +_require_scratch_nocheck > > > > +_require_scratch_shutdown > > > > + > > > > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > > > > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > > > > +echo "=== syncfs after shutdown" > > > > +_scratch_mount > > > > +# This command is complicated a bit because in the case of overlayfs the > > > > +# syncfs fd needs to be opened before shutdown and it is different from the > > > > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > > > > +# Filter out xfs_io output of active fds. > > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > > + grep -vF '[00' > > > > + > > > > +# Now repeat the same test with a volatile overlayfs mount and expect no error > > > > +_scratch_unmount > > > > +echo "=== syncfs after shutdown (volatile)" > > > > +_scratch_mount -o volatile > > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > > + grep -vF '[00' > > > > > > Oh, the test steps are much different from xfs/546. If we move x/546 to generic/, > > > can overlay reproduce this bug by that? > > > > Yes and no. > > > > For overlayfs to support this as a generic test, the helper > > _scratch_shutdown_handle must be used and the shutdown+syncfs > > command must be complicated to something like this: > > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f > > ' -c close -c syncfs $SCRATCH_MNT | \ > > grep -vF '[00' > > > > This is because overlayfs itself does not support the shutdown ioctl. > > If the test is moved to generic as it is we get an error when running > > overlayfs: > > > > XFS_IOC_GOINGDOWN: Inappropriate ioctl for device > > > > because _require_scratch_shutdown is "supported" by overlayfs > > but only when the _scratch_shutdown helpers are used. > > Yeah, I know this. > > > > > If the test is to be moved as is, it will need to opt-out of overlayfs > > explicitly. > > I mean you have a "-o volatile" option test, that's an overlayfs specific > mount option. If you need that test, that's an overlay specific test, that > part can be an overlay specific test case. If not, we can use a generic > case (from xfs/546) to cover overlay and other fs. I need the -o volatile test regardless of moving xfs/546 to generic. That's why I posted this patch. > > BTW, we actually we have a common _scratch_shutdown helper, I'm wondering > if it works for this test? Likes: > > _scratch_shutdown -f > $XFS_IO_PROG -c syncfs $SCRATCH_MNT > > Can this work? Nope, because $XFS_IO_PROG -c syncfs $SCRATCH_MNT will fail (EIO) to open the directory after shutdown. That's why the dance with -c open -c close is needed. Thanks, Amir.
On Wed, Sep 04, 2024 at 10:23:16AM +0200, Amir Goldstein wrote: > On Wed, Sep 4, 2024 at 4:58 AM Zorro Lang <zlang@redhat.com> wrote: > > > > On Tue, Sep 03, 2024 at 08:41:28AM +0200, Amir Goldstein wrote: > > > On Tue, Sep 3, 2024 at 6:21 AM Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > On Fri, Aug 30, 2024 at 08:08:44PM +0200, Amir Goldstein wrote: > > > > > Test overlayfs over xfs with and without "volatile" mount option. > > > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > --- > > > > > > > > > > Zorro, > > > > > > > > > > I was going to make a generic test from xfs/546, so that overlayfs could > > > > > also run it, but then I realized that ext4 does not behave as xfs in > > > > > that case (it returns success on syncfs post shutdown). > > > > > > > > > > Unless and until this behavior is made a standard, I made an overlayfs > > > > > specialized test instead, which checks for underlying fs xfs. > > > > > While at it, I also added test coverage for the "volatile" mount options > > > > > that is expected to return succuss in that case regardles of the > > > > > behavior of the underlying fs. > > > > > > > > > > Thanks, > > > > > Amir. > > > > > > > > > > tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/overlay/087.out | 4 +++ > > > > > 2 files changed, 66 insertions(+) > > > > > create mode 100755 tests/overlay/087 > > > > > create mode 100644 tests/overlay/087.out > > > > > > > > > > diff --git a/tests/overlay/087 b/tests/overlay/087 > > > > > new file mode 100755 > > > > > index 00000000..a5afb0d5 > > > > > --- /dev/null > > > > > +++ b/tests/overlay/087 > > > > > @@ -0,0 +1,62 @@ > > > > > +#! /bin/bash > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > +# Copyright (c) 2022 Oracle. All Rights Reserved. > > > > > +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. > > > > > +# > > > > > +# FS QA Test No. 087 > > > > > +# > > > > > +# This is a variant of test xfs/546 for overlayfs > > > > > +# with and without the "volatile" mount option. > > > > > +# It only works over xfs underlying fs. > > > > > +# > > > > > +# 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 mount shutdown > > > > > + > > > > > + > > > > > +# Modify as appropriate. > > > > > +_require_xfs_io_command syncfs > > > > > +_require_scratch_nocheck > > > > > +_require_scratch_shutdown > > > > > + > > > > > +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ > > > > > + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. > > > > > +echo "=== syncfs after shutdown" > > > > > +_scratch_mount > > > > > +# This command is complicated a bit because in the case of overlayfs the > > > > > +# syncfs fd needs to be opened before shutdown and it is different from the > > > > > +# shutdown fd, so we cannot use the _scratch_shutdown() helper. > > > > > +# Filter out xfs_io output of active fds. > > > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > > > + grep -vF '[00' > > > > > + > > > > > +# Now repeat the same test with a volatile overlayfs mount and expect no error > > > > > +_scratch_unmount > > > > > +echo "=== syncfs after shutdown (volatile)" > > > > > +_scratch_mount -o volatile > > > > > +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ > > > > > + grep -vF '[00' > > > > > > > > Oh, the test steps are much different from xfs/546. If we move x/546 to generic/, > > > > can overlay reproduce this bug by that? > > > > > > Yes and no. > > > > > > For overlayfs to support this as a generic test, the helper > > > _scratch_shutdown_handle must be used and the shutdown+syncfs > > > command must be complicated to something like this: > > > > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f > > > ' -c close -c syncfs $SCRATCH_MNT | \ > > > grep -vF '[00' > > > > > > This is because overlayfs itself does not support the shutdown ioctl. > > > If the test is moved to generic as it is we get an error when running > > > overlayfs: > > > > > > XFS_IOC_GOINGDOWN: Inappropriate ioctl for device > > > > > > because _require_scratch_shutdown is "supported" by overlayfs > > > but only when the _scratch_shutdown helpers are used. > > > > Yeah, I know this. > > > > > > > > If the test is to be moved as is, it will need to opt-out of overlayfs > > > explicitly. > > > > I mean you have a "-o volatile" option test, that's an overlayfs specific > > mount option. If you need that test, that's an overlay specific test, that > > part can be an overlay specific test case. If not, we can use a generic > > case (from xfs/546) to cover overlay and other fs. > > I need the -o volatile test regardless of moving xfs/546 to generic. > That's why I posted this patch. OK, let's have two patches, one moves xfs/546 to generic/, the other is this overlay specific test case. > > > > > BTW, we actually we have a common _scratch_shutdown helper, I'm wondering > > if it works for this test? Likes: > > > > _scratch_shutdown -f > > $XFS_IO_PROG -c syncfs $SCRATCH_MNT > > > > Can this work? > > Nope, because $XFS_IO_PROG -c syncfs $SCRATCH_MNT > will fail (EIO) to open the directory after shutdown. > That's why the dance with -c open -c close is needed. Oh, although below line: $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT trys to shudown on $(_scratch_shutdown_handle), then syncfs on $SCRATCH_MNT too. But the difference is it opens $SCRATCH_MNT before shutdown, so that's what you need. Thanks, Zorro > > Thanks, > Amir. >
diff --git a/tests/overlay/087 b/tests/overlay/087 new file mode 100755 index 00000000..a5afb0d5 --- /dev/null +++ b/tests/overlay/087 @@ -0,0 +1,62 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 Oracle. All Rights Reserved. +# Copyright (c) 2024 CTERA Networks. All Rights Reserved. +# +# FS QA Test No. 087 +# +# This is a variant of test xfs/546 for overlayfs +# with and without the "volatile" mount option. +# It only works over xfs underlying fs. +# +# 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 mount shutdown + + +# Modify as appropriate. +_require_xfs_io_command syncfs +_require_scratch_nocheck +_require_scratch_shutdown + +[ "$OVL_BASE_FSTYP" == "xfs" ] || \ + _notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after 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. +echo "=== syncfs after shutdown" +_scratch_mount +# This command is complicated a bit because in the case of overlayfs the +# syncfs fd needs to be opened before shutdown and it is different from the +# shutdown fd, so we cannot use the _scratch_shutdown() helper. +# Filter out xfs_io output of active fds. +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ + grep -vF '[00' + +# Now repeat the same test with a volatile overlayfs mount and expect no error +_scratch_unmount +echo "=== syncfs after shutdown (volatile)" +_scratch_mount -o volatile +$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \ + grep -vF '[00' + +# success, all done +status=0 +exit diff --git a/tests/overlay/087.out b/tests/overlay/087.out new file mode 100644 index 00000000..44b538d8 --- /dev/null +++ b/tests/overlay/087.out @@ -0,0 +1,4 @@ +QA output created by 087 +=== syncfs after shutdown +syncfs: Input/output error +=== syncfs after shutdown (volatile)
Test overlayfs over xfs with and without "volatile" mount option. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Zorro, I was going to make a generic test from xfs/546, so that overlayfs could also run it, but then I realized that ext4 does not behave as xfs in that case (it returns success on syncfs post shutdown). Unless and until this behavior is made a standard, I made an overlayfs specialized test instead, which checks for underlying fs xfs. While at it, I also added test coverage for the "volatile" mount options that is expected to return succuss in that case regardles of the behavior of the underlying fs. Thanks, Amir. tests/overlay/087 | 62 +++++++++++++++++++++++++++++++++++++++++++ tests/overlay/087.out | 4 +++ 2 files changed, 66 insertions(+) create mode 100755 tests/overlay/087 create mode 100644 tests/overlay/087.out