Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 14, 2022 at 08:42:34AM -0700, Darrick J. Wong wrote:
> 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.

Sure, this case can be a xfs specified case at first. We'll see if need to
change it to be generic case later.

Reviewed-by: Zorro Lang <zlang@xxxxxxxxxx>

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




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux