Re: [PATCH] xfs: don't eat an EIO/ENOSPC writeback error when scrubbing data fork

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 22, 2020 at 06:02:40PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 23, 2020 at 09:58:00AM +1000, Dave Chinner wrote:
> > On Mon, Jun 22, 2020 at 04:28:43PM -0700, Darrick J. Wong wrote:
> > > On Tue, Jun 23, 2020 at 08:08:39AM +1000, Dave Chinner wrote:
> > > > On Mon, Jun 22, 2020 at 10:17:13AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > The data fork scrubber calls filemap_write_and_wait to flush dirty pages
> > > > > and delalloc reservations out to disk prior to checking the data fork's
> > > > > extent mappings.  Unfortunately, this means that scrub can consume the
> > > > > EIO/ENOSPC errors that would otherwise have stayed around in the address
> > > > > space until (we hope) the writer application calls fsync to persist data
> > > > > and collect errors.  The end result is that programs that wrote to a
> > > > > file might never see the error code and proceed as if nothing were
> > > > > wrong.
> > > > > 
> > > > > xfs_scrub is not in a position to notify file writers about the
> > > > > writeback failure, and it's only here to check metadata, not file
> > > > > contents.  Therefore, if writeback fails, we should stuff the error code
> > > > > back into the address space so that an fsync by the writer application
> > > > > can pick that up.
> > > > > 
> > > > > Fixes: 99d9d8d05da2 ("xfs: scrub inode block mappings")
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > >  fs/xfs/scrub/bmap.c |   10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > > > > index 7badd6dfe544..03be7cf3fe5a 100644
> > > > > --- a/fs/xfs/scrub/bmap.c
> > > > > +++ b/fs/xfs/scrub/bmap.c
> > > > > @@ -47,7 +47,15 @@ xchk_setup_inode_bmap(
> > > > >  	    sc->sm->sm_type == XFS_SCRUB_TYPE_BMBTD) {
> > > > >  		inode_dio_wait(VFS_I(sc->ip));
> > > > >  		error = filemap_write_and_wait(VFS_I(sc->ip)->i_mapping);
> > > > > -		if (error)
> > > > > +		if (error == -ENOSPC || error == -EIO) {
> > > > > +			/*
> > > > > +			 * If writeback hits EIO or ENOSPC, reflect it back
> > > > > +			 * into the address space mapping so that a writer
> > > > > +			 * program calling fsync to look for errors will still
> > > > > +			 * capture the error.
> > > > > +			 */
> > > > > +			mapping_set_error(VFS_I(sc->ip)->i_mapping, error);
> > > > > +		} else if (error)
> > > > >  			goto out;
> > > > 
> > > > calling mapping_set_error() seems reasonable here and you've
> > > > explained that well, but shouldn't the error then be processed the
> > > > same way as all other errors? i.e. by jumping to out?
> > > > 
> > > > If we are now continuing to scrub the bmap after ENOSPC/EIO occur,
> > > > why?
> > > 
> > > Heh, ok, more explanation is needed.  How about this?
> > > 
> > > 	/*
> > > 	 * If writeback hits EIO or ENOSPC, reflect it back into the
> > > 	 * address space mapping so that a writer program calling fsync
> > > 	 * to look for errors will still capture the error.
> > > 	 *
> > > 	 * However, we continue into the extent mapping checks because
> > > 	 * write failures do not necessarily imply anything about the
> > > 	 * correctness of the file metadata.  The metadata and the file
> > > 	 * data could be on completely separate devices; a media failure
> > > 	 * might only affect a subset of the disk, etc.
> > > 	 */
> > 
> > Ok. Does scrub deal with left-over delalloc extents correctly in
> > this case?
> 
> It ignores the ones in the incore extent tree and flags them if they
> show up in the ondisk metadata.

OK. Perhaps add this to the comment?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com