Re: [PATCH 06/27] xfs: create helpers to record and deal with scrub problems

[Dave Chinner <david@fromorbit.com>]

On Wed, Sep 20, 2017 at 05:18:14PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create helper functions to record crc and corruption problems, and
> deal with any other runtime errors that arise.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/common.c |  243 +++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/scrub/common.h |   39 ++++++++
>  fs/xfs/scrub/trace.h  |  193 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 475 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 13ccb36..cf3f1365 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -47,6 +47,249 @@
>  
>  /* Common code for the metadata scrubbers. */
>  
> +/* Check for operational errors. */
> +bool
> +xfs_scrub_op_ok(
> +	struct xfs_scrub_context	*sc,
> +	xfs_agnumber_t			agno,
> +	xfs_agblock_t			bno,
> +	int				*error)
> +{
> +	switch (*error) {
> +	case 0:
> +		return true;
> +	case -EDEADLOCK:
> +		/* Used to restart an op with deadlock avoidance. */
> +		trace_xfs_scrub_deadlock_retry(sc->ip, sc->sm, *error);
> +		break;
> +	case -EFSBADCRC:
> +	case -EFSCORRUPTED:
> +		/* Note the badness but don't abort. */
> +		sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> +		*error = 0;
> +		/* fall through */
> +	default:
> +		trace_xfs_scrub_op_error(sc, agno, bno, *error,
> +				__return_address);
> +		break;
> +	}
> +	return false;
> +}

What are the semantics here w.r.t. *error? on some errors it's
cleared before we return, on others it's ignored. It's as clear as
mud what we should expect from these functions...

> +/* Check for metadata block optimization possibilities. */
> +bool
> +xfs_scrub_block_preen_ok(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*bp,
> +	bool				fs_ok)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	xfs_fsblock_t			fsbno;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			bno;
> +
> +	if (fs_ok)
> +		return fs_ok;
> +
> +	fsbno = XFS_DADDR_TO_FSB(mp, bp->b_bn);
> +	agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +	bno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +
> +	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_PREEN;
> +	trace_xfs_scrub_block_preen(sc, agno, bno, __return_address);
> +	return fs_ok;
> +}

Again, I'm not sure what the return value semantics of the functioon
are? Why does the fs_ok return shortcut exist?

Same for all the other functions...

> +
> +/* Check for inode metadata non-corruption problems. */
> +bool
> +xfs_scrub_ino_warn_ok(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*bp,
> +	bool				fs_ok)

Confusing. What's the difference between a corruption problem and a
"non-corruption problem" that requires a warning?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com