From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once
Date: Tue, 27 Aug 2019 11:21:47 -0400 [thread overview]
Message-ID: <20190827152147.GI10636@bfoster> (raw)
In-Reply-To: <20190827151812.GA1037350@magnolia>
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)
> > > + 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;
> > > }
> > >
next prev parent reply other threads:[~2019-08-27 15:21 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-26 21:48 [PATCH 0/4] xfs: fixes for 5.4 Darrick J. Wong
2019-08-26 21:48 ` [PATCH 1/4] xfs: bmap scrub should only scrub records once Darrick J. Wong
2019-08-26 23:08 ` Dave Chinner
2019-08-27 13:14 ` Brian Foster
2019-08-27 15:18 ` Darrick J. Wong
2019-08-27 15:21 ` Brian Foster [this message]
2019-08-28 16:01 ` Darrick J. Wong
2019-08-26 21:48 ` [PATCH 2/4] xfs: fix maxicount division by zero error Darrick J. Wong
2019-08-26 23:09 ` Dave Chinner
2019-08-26 21:49 ` [PATCH 3/4] xfs: don't return _QUERY_ABORT from xfs_rmap_has_other_keys Darrick J. Wong
2019-08-26 23:15 ` Dave Chinner
2019-08-26 21:49 ` [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys Darrick J. Wong
2019-08-26 23:15 ` Dave Chinner
2019-08-27 13:01 ` Eric Sandeen
2019-08-27 15:19 ` Darrick J. Wong
2019-08-27 15:20 ` Eric Sandeen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190827152147.GI10636@bfoster \
--to=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).