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

Commit Message

Darrick J. Wong April 11, 2022, 10:54 p.m. UTC

From: Darrick J. Wong <[e-mail]>

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 <[e-mail]>
---
 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

Comments

Zorro Lang April 12, 2022, 9:37 a.m. UTC | #1

On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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.

> 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. Sorry, really confused on what these sync things
really guarantee :)

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
>

Darrick J. Wong April 12, 2022, 5:28 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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.

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

Zorro Lang April 14, 2022, 2:43 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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.

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

Darrick J. Wong April 14, 2022, 3:42 p.m. UTC | #4

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 <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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
> > > > 
> > > 
> > 
>

Zorro Lang April 14, 2022, 6:58 p.m. UTC | #5

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 <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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 <[e-mail]>

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

Patch

diff --git a/tests/xfs/839 b/tests/xfs/839
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")
+#
+# 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