On Thu, Apr 14, 2022 at 10:43:30PM +0800, Zorro Lang wrote:
> On Tue, Apr 12, 2022 at 10:28:53AM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 12, 2022 at 05:37:27PM +0800, Zorro Lang wrote:
> > > On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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
> > >
> > > This case looks good to me. Just one question, is it possible to be a generic
> > > case? From the code logic, it doesn't use xfs specified operations, but I'm
> > > not sure if other filesystems would like to treat sync_fs return value as XFS.
> >
> > Other filesystems (ext4 in particular) haven't been fixed to make
> > ->sync_fs return error codes when the fs has been shut down via
> > FS_IOC_SHUTDOWN. We'll get there eventually, but for now I'd like to
> > get this under test for XFS since we've applied those fixes.
>
> If other filesystems intend to do that, how about using a generic case failure to
> remind them they haven't done that :) If they don't intend that, keep this case
> under xfs is good to me.
<shrug> I don't know if they do or not; I've been so strapped for time
trying to get all these fixes out that I haven't had time to ask the
ext4 or btrfs communities, let alone propose patches.
At the moment I'd really like to get as many patches out of djwong-dev
as I can without people asking for more side projects. As it stands
today, landing the online fsck patchset is going to involve getting 185
kernel patches, 95 xfsprogs patches, and 87 fstests patches all through
review.
--D
> >
> > > > 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")
> > >
> > > BTW, after this change, now can I assume that sync(2) flushes all data and metadata
> > > to underlying disk, if it returns 0.
> >
> > Yes.
> >
> > > Sorry, really confused on what these sync things
> > > really guarantee :)
> >
> > No worries -- the history of the sync variants has been very messy and
> > confusing even to people on fsdevel.
> >
> > --D
> >
> > >
> > > Thanks,
> > > Zorro
> > >
> > > > +#
> > > > +# 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
> > > >
> > >
> >
>
