Re: [RFC PATCH 08/11] xfs: have sync_fs op report writeback errors when passed a since pointer

[Jeff Layton <jlayton@kernel.org>]

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!
-- 
Jeff Layton <jlayton@kernel.org>