Re: [PATCH v2] xfs: bmap scrub should only scrub records once

["Darrick J. Wong" <darrick.wong@oracle.com>]

Meh, at this point I might as well just send these four fixes (and four
more cleanups) that I found as proper series.

--D

On Mon, Aug 26, 2019 at 09:22:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The inode block mapping scrub function does more work for btree format
> extent maps than is absolutely necessary -- first it will walk the bmbt
> and check all the entries, and then it will load the incore tree and
> check every entry in that tree, possibly for a second time.
> 
> Simplify the code and decrease check runtime by separating the two
> responsibilities.  The bmbt walk will make sure the incore extent
> mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> and check that every bmbt record has a corresponding incore extent map;
> and the incore extent map walk takes all the responsibility for checking
> the mapping records and cross referencing them with other AG metadata.
> 
> This enables us to clean up some messy parameter handling and reduce
> redundant code.  Rename a few functions to make the split of
> responsibilities clearer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: always load the extent cache and separate the bmbt/iext walk code
> ---
>  fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 1bd29fdc2ab5..f6ed6eb133a6 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -75,6 +75,7 @@ struct xchk_bmap_info {
>  	xfs_fileoff_t		lastoff;
>  	bool			is_rt;
>  	bool			is_shared;
> +	bool			was_loaded;
>  	int			whichfork;
>  };
>  
> @@ -213,25 +214,20 @@ xchk_bmap_xref_rmap(
>  
>  /* Cross-reference a single rtdev extent record. */
>  STATIC void
> -xchk_bmap_rt_extent_xref(
> -	struct xchk_bmap_info	*info,
> +xchk_bmap_rt_iextent_xref(
>  	struct xfs_inode	*ip,
> -	struct xfs_btree_cur	*cur,
> +	struct xchk_bmap_info	*info,
>  	struct xfs_bmbt_irec	*irec)
>  {
> -	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> -		return;
> -
>  	xchk_xref_is_used_rt_space(info->sc, irec->br_startblock,
>  			irec->br_blockcount);
>  }
>  
>  /* Cross-reference a single datadev extent record. */
>  STATIC void
> -xchk_bmap_extent_xref(
> -	struct xchk_bmap_info	*info,
> +xchk_bmap_iextent_xref(
>  	struct xfs_inode	*ip,
> -	struct xfs_btree_cur	*cur,
> +	struct xchk_bmap_info	*info,
>  	struct xfs_bmbt_irec	*irec)
>  {
>  	struct xfs_mount	*mp = info->sc->mp;
> @@ -240,9 +236,6 @@ xchk_bmap_extent_xref(
>  	xfs_extlen_t		len;
>  	int			error;
>  
> -	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> -		return;
> -
>  	agno = XFS_FSB_TO_AGNO(mp, irec->br_startblock);
>  	agbno = XFS_FSB_TO_AGBNO(mp, irec->br_startblock);
>  	len = irec->br_blockcount;
> @@ -300,20 +293,15 @@ xchk_bmap_dirattr_extent(
>  
>  /* Scrub a single extent record. */
>  STATIC int
> -xchk_bmap_extent(
> +xchk_bmap_iextent(
>  	struct xfs_inode	*ip,
> -	struct xfs_btree_cur	*cur,
>  	struct xchk_bmap_info	*info,
>  	struct xfs_bmbt_irec	*irec)
>  {
>  	struct xfs_mount	*mp = info->sc->mp;
> -	struct xfs_buf		*bp = NULL;
>  	xfs_filblks_t		end;
>  	int			error = 0;
>  
> -	if (cur)
> -		xfs_btree_get_block(cur, 0, &bp);
> -
>  	/*
>  	 * Check for out-of-order extents.  This record could have come
>  	 * from the incore list, for which there is no ordering check.
> @@ -364,10 +352,13 @@ xchk_bmap_extent(
>  		xchk_fblock_set_corrupt(info->sc, info->whichfork,
>  				irec->br_startoff);
>  
> +	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> +		return 0;
> +
>  	if (info->is_rt)
> -		xchk_bmap_rt_extent_xref(info, ip, cur, irec);
> +		xchk_bmap_rt_iextent_xref(ip, info, irec);
>  	else
> -		xchk_bmap_extent_xref(info, ip, cur, irec);
> +		xchk_bmap_iextent_xref(ip, info, irec);
>  
>  	info->lastoff = irec->br_startoff + irec->br_blockcount;
>  	return error;
> @@ -380,10 +371,13 @@ xchk_bmapbt_rec(
>  	union xfs_btree_rec	*rec)
>  {
>  	struct xfs_bmbt_irec	irec;
> +	struct xfs_bmbt_irec	iext_irec;
> +	struct xfs_iext_cursor	icur;
>  	struct xchk_bmap_info	*info = bs->private;
>  	struct xfs_inode	*ip = bs->cur->bc_private.b.ip;
>  	struct xfs_buf		*bp = NULL;
>  	struct xfs_btree_block	*block;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, info->whichfork);
>  	uint64_t		owner;
>  	int			i;
>  
> @@ -402,9 +396,25 @@ xchk_bmapbt_rec(
>  		}
>  	}
>  
> -	/* Set up the in-core record and scrub it. */
> +	/*
> +	 * Check that the incore extent tree contains an extent that matches
> +	 * this one exactly.  We validate those cached bmaps later, so we don't
> +	 * need to check them here.  If the extent tree was freshly loaded by
> +	 * the scrubber then we skip the check entirely.
> +	 */
> +	if (info->was_loaded)
> +		return 0;
> +
>  	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
> -	return xchk_bmap_extent(ip, bs->cur, info, &irec);
> +	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
> +				&iext_irec) ||
> +	    irec.br_startoff != iext_irec.br_startoff ||
> +	    irec.br_startblock != iext_irec.br_startblock ||
> +	    irec.br_blockcount != iext_irec.br_blockcount ||
> +	    irec.br_state != iext_irec.br_state)
> +		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
> +				irec.br_startoff);
> +	return 0;
>  }
>  
>  /* Scan the btree records. */
> @@ -415,15 +425,26 @@ xchk_bmap_btree(
>  	struct xchk_bmap_info	*info)
>  {
>  	struct xfs_owner_info	oinfo;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
>  	struct xfs_mount	*mp = sc->mp;
>  	struct xfs_inode	*ip = sc->ip;
>  	struct xfs_btree_cur	*cur;
>  	int			error;
>  
> +	/* Load the incore bmap cache if it's not loaded. */
> +	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
> +	if (!info->was_loaded) {
> +		error = xfs_iread_extents(sc->tp, ip, whichfork);
> +		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> +			goto out;
> +	}
> +
> +	/* Check the btree structure. */
>  	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
>  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
>  	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
>  	xfs_btree_del_cursor(cur, error);
> +out:
>  	return error;
>  }
>  
> @@ -671,13 +692,6 @@ xchk_bmap(
>  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  		goto out;
>  
> -	/* Now try to scrub the in-memory extent list. */
> -        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> -		error = xfs_iread_extents(sc->tp, ip, whichfork);
> -		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> -			goto out;
> -	}
> -
>  	/* Find the offset of the last extent in the mapping. */
>  	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
>  	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> @@ -689,7 +703,7 @@ xchk_bmap(
>  	for_each_xfs_iext(ifp, &icur, &irec) {
>  		if (xchk_should_terminate(sc, &error) ||
>  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> -			break;
> +			goto out;
>  		if (isnullstartblock(irec.br_startblock))
>  			continue;
>  		if (irec.br_startoff >= endoff) {
> @@ -697,7 +711,7 @@ xchk_bmap(
>  					irec.br_startoff);
>  			goto out;
>  		}
> -		error = xchk_bmap_extent(ip, NULL, &info, &irec);
> +		error = xchk_bmap_iextent(ip, &info, &irec);
>  		if (error)
>  			goto out;
>  	}