On Tue, Jun 23, 2020 at 08:51:33PM -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>
> ---
> 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 | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 7badd6dfe544..c4c2477a94ae 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -45,9 +45,30 @@ 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.
> + */
> + error = filemap_fdatawrite(mapping);
> + if (!error)
> + error = filemap_fdatawait_keep_errors(mapping);
> + if (error == -ENOSPC || error == -EIO) {
> + /*
> + * 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.
> + */
> + } else if (error)
> goto out;
> }
Functionality is fine, but the if() that does nothing but contain a
comment is a bit weird. Promote the error handling comment, and that
can formatted as:
/*
* 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;
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
