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@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
>
> > --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;
> > > > }
> > > >
