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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jun 23, 2020 at 08:10:31AM -0400, Brian Foster wrote:
> On Mon, Jun 22, 2020 at 08:50:10PM -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>
> > ---
> > v2: explain why it's ok to keep going even if writeback fails
> > ---
> >  fs/xfs/scrub/bmap.c |   19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > index 7badd6dfe544..0d7062b7068b 100644
> > --- a/fs/xfs/scrub/bmap.c
> > +++ b/fs/xfs/scrub/bmap.c
> > @@ -47,7 +47,24 @@ 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.
> > +			 *
> > +			 * 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.  We properly
> > +			 * account for delalloc extents, so leaving them in
> > +			 * memory is fine.
> > +			 */
> > +			mapping_set_error(VFS_I(sc->ip)->i_mapping, error);
> 
> I think the more appropriate thing to do is open code the data write and
> wait and use the variants of the latter that don't consume address space
> errors in the first place (i.e. filemap_fdatawait_keep_errors()). Then
> we wouldn't need the special error handling branch or perhaps the first
> part of the comment. Hm?

Yes, it's certainly possible.  I don't want to go opencoding more vfs
methods (like some e4 filesystems do) so I'll propose that as a second
patch for 5.9.

On second thought, I wonder if I should just drop the flush entirely?
It's not a huge burden to skip past the delalloc reservations.

Hmmm.  Any preferences?

--D

> Brian
> 
> > +		} else if (error)
> >  			goto out;
> >  	}
> >  
> > 
>