Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

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

On Tue, Aug 27, 2019 at 11:21:47AM -0400, Brian Foster wrote:
> On Tue, Aug 27, 2019 at 08:18:12AM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 27, 2019 at 09:14:03AM -0400, Brian Foster wrote:
> > > On Mon, Aug 26, 2019 at 02:48:49PM -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>
> > > > ---
> > > >  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
> > > ...
> > > > @@ -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)

Heh, I didn't even spot the backwards logic here.

If the bmap scrub just loaded the extent tree then was_loaded will be
false, so this needs to be "if (!info->was_loaded) return 0;".

I fixed it before I sent it to for-next.

--D

> > > > +		return 0;
> > > > +
> > > 
> > > This all looks fine to me except that I don't follow the reasoning for
> > > skipping the lookup from the comment. Are we just saying that if we
> > > loaded the extent tree, then we can assume it reflects what is on disk?
> > > If so, can we fix up the last sentence in the comment to explain?
> > 
> > Yes.
> > 
> 
> Ok.
> 
> > "If the incore extent tree was just loaded from disk by the scrubber,
> > we assume that its contents match what's on disk and skip the
> > equivalence check." ?
> > 
> 
> Sounds good. With that tweak:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > --D
> > 
> > > Brian
> > > 
> > > >  	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;
> > > >  	}
> > > >