On Tue, Sep 17, 2024 at 04:37:04PM +0200, Amir Goldstein wrote:
> On Wed, Sep 4, 2024 at 1:25 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxxx> 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@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.
> > >
> > > 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.
> >
>
> Hi Zorro,
>
> I thought that you agreed to include the overlay specific test overlay/087,
> but I still do not see it in for-next.
>
> Did I misunderstand you, or was it accidently left out of for-next?
Sorry, I thought you'd like to send another patch with this patch in your
next patchset. OK, I'll check this one singly, and try to merge it this
week.
Thanks,
Zorro
>
> Regarding moving xfs/546 to generic/, I had sent a patch to ext4 [1],
> but no comment from Ted yet.
>
> [1] https://lore.kernel.org/linux-ext4/20240904084657.1062243-1-amir73il@xxxxxxxxx/
>
> > >
> > > >
> > > > 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.
>
> Correct, this is why the dance with -c open -c close is needed when the test is
> overlayfs specific and/or generic.
>
> Thanks,
> Amir.
>
