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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >
> > > 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.
>
