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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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.
*/
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
