| Message ID | 20180518123415.28181-9-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 18, 2018 at 08:34:12AM -0400, Jeff Layton wrote: > From: Jeff Layton <jlayton@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/xfs/xfs_super.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 9255de2767b4..7dc847f48f9f 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1092,6 +1092,7 @@ xfs_fs_sync_fs( > int wait, > errseq_t *since) > { > + int ret; > struct xfs_mount *mp = XFS_M(sb); > > /* > @@ -1110,7 +1111,13 @@ xfs_fs_sync_fs( > flush_delayed_work(&mp->m_log->l_work); > } > out: > - return __sync_blockdev(sb->s_bdev, wait); Where did this come from? XFS doesn't use the underlying blockdev address space, so this does nothing at all and should not be here. > + ret = __sync_blockdev(sb->s_bdev, wait); > + if (since) { > + int ret2 = errseq_check_and_advance(&sb->s_wb_err, since); > + if (ret == 0) > + ret = ret2; > + } > + return ret; > } So to return errors correctly, xfs_fs_sync_fs() needs to capture errors from the log force (i.e. metadata errors such as filesystem shutdowns, journal IO errors, etc), then check for pending data IO errors. i.e: STATIC int xfs_fs_sync_fs( struct super_block *sb, int wait) { struct xfs_mount *mp = XFS_M(sb); + int err; /* * Doing anything during the async pass would be counterproductive. */ if (!wait) return 0; - xfs_log_force(mp, XFS_LOG_SYNC); + err = xfs_log_force(mp, XFS_LOG_SYNC); + if (err) + return err; + if (laptop_mode) { /* * The disk must be active because we're syncing. * We schedule log work now (now that the disk is * active) instead of later (when it might not be). */ flush_delayed_work(&mp->m_log->l_work); } - return 0 + return errseq_check_and_advance(&sb->s_wb_err, since); } Cheers, Dave.
On Tue, 2018-05-22 at 09:01 +1000, Dave Chinner wrote: > On Fri, May 18, 2018 at 08:34:12AM -0400, Jeff Layton wrote: > > From: Jeff Layton <jlayton@redhat.com> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/xfs/xfs_super.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index 9255de2767b4..7dc847f48f9f 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -1092,6 +1092,7 @@ xfs_fs_sync_fs( > > int wait, > > errseq_t *since) > > { > > + int ret; > > struct xfs_mount *mp = XFS_M(sb); > > > > /* > > @@ -1110,7 +1111,13 @@ xfs_fs_sync_fs( > > flush_delayed_work(&mp->m_log->l_work); > > } > > out: > > - return __sync_blockdev(sb->s_bdev, wait); > > Where did this come from? XFS doesn't use the underlying blockdev > address space, so this does nothing at all and should not be here. > An earlier patch that pushed this down into the sync_fs routines. We call this today for all filesystems, and I wasn't sure about xfs. Christoph already pointed out that it's not needed so it's removed from my current branch. > > + ret = __sync_blockdev(sb->s_bdev, wait); > > + if (since) { > > + int ret2 = errseq_check_and_advance(&sb->s_wb_err, since); > > + if (ret == 0) > > + ret = ret2; > > + } > > + return ret; > > } > > So to return errors correctly, xfs_fs_sync_fs() needs to capture > errors from the log force (i.e. metadata errors such as filesystem > shutdowns, journal IO errors, etc), then check for pending data IO > errors. i.e: > > > STATIC int > xfs_fs_sync_fs( > struct super_block *sb, > int wait) > { > struct xfs_mount *mp = XFS_M(sb); > + int err; > > /* > * Doing anything during the async pass would be counterproductive. > */ > if (!wait) > return 0; > > - xfs_log_force(mp, XFS_LOG_SYNC); > + err = xfs_log_force(mp, XFS_LOG_SYNC); > + if (err) > + return err; > + > if (laptop_mode) { > /* > * The disk must be active because we're syncing. > * We schedule log work now (now that the disk is > * active) instead of later (when it might not be). > */ > flush_delayed_work(&mp->m_log->l_work); > } > > - return 0 > + return errseq_check_and_advance(&sb->s_wb_err, since); > } > Ok, sounds good. I'll fix that too. FWIW, we'll actually want to advance the cursor even if xfs_log_force returns an error to ensure that we don't end up reporting errors twice, but that's simple enough to do. Thanks!
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 9255de2767b4..7dc847f48f9f 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1092,6 +1092,7 @@ xfs_fs_sync_fs( int wait, errseq_t *since) { + int ret; struct xfs_mount *mp = XFS_M(sb); /* @@ -1110,7 +1111,13 @@ xfs_fs_sync_fs( flush_delayed_work(&mp->m_log->l_work); } out: - return __sync_blockdev(sb->s_bdev, wait); + ret = __sync_blockdev(sb->s_bdev, wait); + if (since) { + int ret2 = errseq_check_and_advance(&sb->s_wb_err, since); + if (ret == 0) + ret = ret2; + } + return ret; } STATIC int