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

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

[Darrick J. Wong]

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;
 	}
 

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

[Dave Chinner]

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?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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.
	 */

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

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?

Cheers,
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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.

--D

> Cheers,
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

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

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

[Darrick J. Wong]

On Tue, Jun 23, 2020 at 12:37:10PM +1000, Dave Chinner wrote:
> 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?

Will do.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Darrick J. Wong]

On Tue, Jun 23, 2020 at 12:37:10PM +1000, Dave Chinner wrote:
> 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?

ok, will do

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com