Re: [PATCH] overlay: create a variant to syncfs error test xfs/546

["Darrick J. Wong" <djwong@kernel.org>]

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