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

[PATCH v4] 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>
---
v4: remove if block that only had a gigantic comment
v3: don't play this game where we clear the mapping error only to re-set it
v2: explain why it's ok to keep going even if writeback fails
---
 fs/xfs/scrub/bmap.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 7badd6dfe544..955302e7cdde 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -45,9 +45,27 @@ xchk_setup_inode_bmap(
 	 */
 	if (S_ISREG(VFS_I(sc->ip)->i_mode) &&
 	    sc->sm->sm_type == XFS_SCRUB_TYPE_BMBTD) {
+		struct address_space	*mapping = VFS_I(sc->ip)->i_mapping;
+
 		inode_dio_wait(VFS_I(sc->ip));
-		error = filemap_write_and_wait(VFS_I(sc->ip)->i_mapping);
-		if (error)
+
+		/*
+		 * Try to flush all incore state to disk before we examine the
+		 * space mappings for the data fork.  Leave accumulated errors
+		 * in the mapping for the writer threads to consume.
+		 *
+		 * On ENOSPC or EIO writeback errors, 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 can handle delalloc
+		 * extents in the scrubber, so leaving them in memory is fine.
+		 */
+		error = filemap_fdatawrite(mapping);
+		if (!error)
+			error = filemap_fdatawait_keep_errors(mapping);
+		if (error && (error != -ENOSPC && error != -EIO))
 			goto out;
 	}
 

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

[Brian Foster]

On Wed, Jun 24, 2020 at 06:16:43PM -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>
> ---
> v4: remove if block that only had a gigantic comment
> v3: don't play this game where we clear the mapping error only to re-set it
> v2: explain why it's ok to keep going even if writeback fails
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/scrub/bmap.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 7badd6dfe544..955302e7cdde 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -45,9 +45,27 @@ xchk_setup_inode_bmap(
>  	 */
>  	if (S_ISREG(VFS_I(sc->ip)->i_mode) &&
>  	    sc->sm->sm_type == XFS_SCRUB_TYPE_BMBTD) {
> +		struct address_space	*mapping = VFS_I(sc->ip)->i_mapping;
> +
>  		inode_dio_wait(VFS_I(sc->ip));
> -		error = filemap_write_and_wait(VFS_I(sc->ip)->i_mapping);
> -		if (error)
> +
> +		/*
> +		 * Try to flush all incore state to disk before we examine the
> +		 * space mappings for the data fork.  Leave accumulated errors
> +		 * in the mapping for the writer threads to consume.
> +		 *
> +		 * On ENOSPC or EIO writeback errors, 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 can handle delalloc
> +		 * extents in the scrubber, so leaving them in memory is fine.
> +		 */
> +		error = filemap_fdatawrite(mapping);
> +		if (!error)
> +			error = filemap_fdatawait_keep_errors(mapping);
> +		if (error && (error != -ENOSPC && error != -EIO))
>  			goto out;
>  	}
>  
> 

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

[Dave Chinner]

On Wed, Jun 24, 2020 at 06:16:43PM -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>
> ---
> v4: remove if block that only had a gigantic comment
> v3: don't play this game where we clear the mapping error only to re-set it
> v2: explain why it's ok to keep going even if writeback fails
> ---
>  fs/xfs/scrub/bmap.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 7badd6dfe544..955302e7cdde 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -45,9 +45,27 @@ xchk_setup_inode_bmap(
>  	 */
>  	if (S_ISREG(VFS_I(sc->ip)->i_mode) &&
>  	    sc->sm->sm_type == XFS_SCRUB_TYPE_BMBTD) {
> +		struct address_space	*mapping = VFS_I(sc->ip)->i_mapping;
> +
>  		inode_dio_wait(VFS_I(sc->ip));
> -		error = filemap_write_and_wait(VFS_I(sc->ip)->i_mapping);
> -		if (error)
> +
> +		/*
> +		 * Try to flush all incore state to disk before we examine the
> +		 * space mappings for the data fork.  Leave accumulated errors
> +		 * in the mapping for the writer threads to consume.
> +		 *
> +		 * On ENOSPC or EIO writeback errors, 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 can handle delalloc
> +		 * extents in the scrubber, so leaving them in memory is fine.
> +		 */
> +		error = filemap_fdatawrite(mapping);
> +		if (!error)
> +			error = filemap_fdatawait_keep_errors(mapping);
> +		if (error && (error != -ENOSPC && error != -EIO))
>  			goto out;
>  	}

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com