From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, david@fromorbit.com,
allison.henderson@oracle.com
Subject: Re: [PATCH 02/14] xfs: repair the AGF
Date: Mon, 30 Jul 2018 14:33:01 -0400 [thread overview]
Message-ID: <20180730183300.GF35107@bfoster> (raw)
In-Reply-To: <20180730182231.GZ30972@magnolia>
On Mon, Jul 30, 2018 at 11:22:31AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 30, 2018 at 02:19:15PM -0400, Brian Foster wrote:
> > On Mon, Jul 30, 2018 at 10:31:11AM -0700, Darrick J. Wong wrote:
> > > On Mon, Jul 30, 2018 at 12:22:24PM -0400, Brian Foster wrote:
> > > > On Sun, Jul 29, 2018 at 10:48:02PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > >
> > > > > Regenerate the AGF from the rmap data.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > > fs/xfs/scrub/agheader_repair.c | 370 ++++++++++++++++++++++++++++++++++++++++
> > > > > fs/xfs/scrub/repair.c | 27 ++-
> > > > > fs/xfs/scrub/repair.h | 4
> > > > > fs/xfs/scrub/scrub.c | 2
> > > > > 4 files changed, 393 insertions(+), 10 deletions(-)
> > > > >
> > > > >
> > > > > diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> > > > > index 1e96621ece3a..4842fc598c9b 100644
> > > > > --- a/fs/xfs/scrub/agheader_repair.c
> > > > > +++ b/fs/xfs/scrub/agheader_repair.c
> > > > ...
> > > > > @@ -54,3 +61,366 @@ xrep_superblock(
> > > > ...
> > > > > +/* Repair the AGF. v5 filesystems only. */
> > > > > +int
> > > > > +xrep_agf(
> > > > > + struct xfs_scrub *sc)
> > > > > +{
> > > > ...
> > > > > + /* Start rewriting the header and implant the btrees we found. */
> > > > > + xrep_agf_init_header(sc, agf_bp, &old_agf);
> > > > > + xrep_agf_set_roots(sc, agf, fab);
> > > > > + error = xrep_agf_calc_from_btrees(sc, agf_bp);
> > > > > + if (error)
> > > > > + goto out_revert;
> > > > > +
> > > > > + /* Commit the changes and reinitialize incore state. */
> > > > > + return xrep_agf_commit_new(sc, agf_bp);
> > > > > +
> > > > > +out_revert:
> > > > > + /* Mark the incore AGF state stale and revert the AGF. */
> > > > > + sc->sa.pag->pagf_init = 0;
> > > >
> > > > Hmm, looking at this again I'm not sure it's safe to reset ->pagf_init
> > > > like this. The contexts where we hold agf might be Ok because I think
> > > > that might prevent some other thread from actually coming in and
> > > > resetting it, but look at xfs_alloc_read_agf() does in this case if the
> > > > agf becomes available with !pagf_init. Specifically, are we at risk of
> > > > corrupting a populated ->pagb_tree or causing other problems by
> > > > reinitializing the spinlock? Perhaps we need another patch to separate
> > > > out some of those fields that should only ever be initialized once.
> > >
> > > Yikes, the pagb_tree & spinlock should not get reinitialized. I don't
> > > see where we ever tear them down except for unmount, so I *think* we can
> > > move it to xfs_initialize_perag. It's a little mystifying why we don't
> > > initialze those things there like we do for the incore inode radix tree.
> > >
> > > Also it would finally fix the discrepancy with xfsprogs libxfs where
> > > they comment out the RB_ROOT initialization.
> > >
> > > > With something like that, it might subsequently make sense to factor the
> > > > reinit from disk bits into a helper to be shared between
> > > > xrep_agf_commit_new() and xfs_allo_read_agf(). I also wonder if it's
> > > > sufficient to just update the agf on disk and leave pagf_init == 0.
> > >
> > > Hmm, wasn't there some verifier that used pag*_init (can't remember
> > > which one) to decide if we were in log recovery?
> > >
> >
> > It looks like xfs_attr3_leaf_verify() might do something like that. But
> > don't we have to handle that either way if the error path leaves
> > pagf_init == 0 on return? Actually we might have to address it
> > regardless if we want to use pagf_init if that path isn't holding the
> > agf.
>
> <nod> I think we should simply add a xlog helper that decides if the log
> is in recovery and call it from xfs_attr3_leaf_verify rather than having
> an open-coded check on some other data structure.
>
Agreed.
Brian
> --D
>
> > Brian
> >
> > > --D
> > >
> > > > Otherwise the rest of this patch seems Ok to me.
> > > >
> > > > Brian
> > > >
> > > > > + memcpy(agf, &old_agf, sizeof(old_agf));
> > > > > + return error;
> > > > > +}
> > > > > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > > > > index 85b048b341a0..17cf48564390 100644
> > > > > --- a/fs/xfs/scrub/repair.c
> > > > > +++ b/fs/xfs/scrub/repair.c
> > > > > @@ -128,9 +128,12 @@ xrep_roll_ag_trans(
> > > > > int error;
> > > > >
> > > > > /* Keep the AG header buffers locked so we can keep going. */
> > > > > - xfs_trans_bhold(sc->tp, sc->sa.agi_bp);
> > > > > - xfs_trans_bhold(sc->tp, sc->sa.agf_bp);
> > > > > - xfs_trans_bhold(sc->tp, sc->sa.agfl_bp);
> > > > > + if (sc->sa.agi_bp)
> > > > > + xfs_trans_bhold(sc->tp, sc->sa.agi_bp);
> > > > > + if (sc->sa.agf_bp)
> > > > > + xfs_trans_bhold(sc->tp, sc->sa.agf_bp);
> > > > > + if (sc->sa.agfl_bp)
> > > > > + xfs_trans_bhold(sc->tp, sc->sa.agfl_bp);
> > > > >
> > > > > /* Roll the transaction. */
> > > > > error = xfs_trans_roll(&sc->tp);
> > > > > @@ -138,9 +141,12 @@ xrep_roll_ag_trans(
> > > > > goto out_release;
> > > > >
> > > > > /* Join AG headers to the new transaction. */
> > > > > - xfs_trans_bjoin(sc->tp, sc->sa.agi_bp);
> > > > > - xfs_trans_bjoin(sc->tp, sc->sa.agf_bp);
> > > > > - xfs_trans_bjoin(sc->tp, sc->sa.agfl_bp);
> > > > > + if (sc->sa.agi_bp)
> > > > > + xfs_trans_bjoin(sc->tp, sc->sa.agi_bp);
> > > > > + if (sc->sa.agf_bp)
> > > > > + xfs_trans_bjoin(sc->tp, sc->sa.agf_bp);
> > > > > + if (sc->sa.agfl_bp)
> > > > > + xfs_trans_bjoin(sc->tp, sc->sa.agfl_bp);
> > > > >
> > > > > return 0;
> > > > >
> > > > > @@ -150,9 +156,12 @@ xrep_roll_ag_trans(
> > > > > * buffers will be released during teardown on our way out
> > > > > * of the kernel.
> > > > > */
> > > > > - xfs_trans_bhold_release(sc->tp, sc->sa.agi_bp);
> > > > > - xfs_trans_bhold_release(sc->tp, sc->sa.agf_bp);
> > > > > - xfs_trans_bhold_release(sc->tp, sc->sa.agfl_bp);
> > > > > + if (sc->sa.agi_bp)
> > > > > + xfs_trans_bhold_release(sc->tp, sc->sa.agi_bp);
> > > > > + if (sc->sa.agf_bp)
> > > > > + xfs_trans_bhold_release(sc->tp, sc->sa.agf_bp);
> > > > > + if (sc->sa.agfl_bp)
> > > > > + xfs_trans_bhold_release(sc->tp, sc->sa.agfl_bp);
> > > > >
> > > > > return error;
> > > > > }
> > > > > diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> > > > > index 5a4e92221916..1d283360b5ab 100644
> > > > > --- a/fs/xfs/scrub/repair.h
> > > > > +++ b/fs/xfs/scrub/repair.h
> > > > > @@ -58,6 +58,8 @@ int xrep_ino_dqattach(struct xfs_scrub *sc);
> > > > >
> > > > > int xrep_probe(struct xfs_scrub *sc);
> > > > > int xrep_superblock(struct xfs_scrub *sc);
> > > > > +int xrep_agf(struct xfs_scrub *sc);
> > > > > +int xrep_agfl(struct xfs_scrub *sc);
> > > > >
> > > > > #else
> > > > >
> > > > > @@ -81,6 +83,8 @@ xrep_calc_ag_resblks(
> > > > >
> > > > > #define xrep_probe xrep_notsupported
> > > > > #define xrep_superblock xrep_notsupported
> > > > > +#define xrep_agf xrep_notsupported
> > > > > +#define xrep_agfl xrep_notsupported
> > > > >
> > > > > #endif /* CONFIG_XFS_ONLINE_REPAIR */
> > > > >
> > > > > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > > > > index 6efb926f3cf8..1e8a17c8e2b9 100644
> > > > > --- a/fs/xfs/scrub/scrub.c
> > > > > +++ b/fs/xfs/scrub/scrub.c
> > > > > @@ -214,7 +214,7 @@ static const struct xchk_meta_ops meta_scrub_ops[] = {
> > > > > .type = ST_PERAG,
> > > > > .setup = xchk_setup_fs,
> > > > > .scrub = xchk_agf,
> > > > > - .repair = xrep_notsupported,
> > > > > + .repair = xrep_agf,
> > > > > },
> > > > > [XFS_SCRUB_TYPE_AGFL]= { /* agfl */
> > > > > .type = ST_PERAG,
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-07-30 20:09 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-30 5:47 [PATCH v17.1 00/14] xfs-4.19: online repair support Darrick J. Wong
2018-07-30 5:47 ` [PATCH 01/14] xfs: refactor the xrep_extent_list into xfs_bitmap Darrick J. Wong
2018-07-30 16:21 ` Brian Foster
2018-07-30 5:48 ` [PATCH 02/14] xfs: repair the AGF Darrick J. Wong
2018-07-30 16:22 ` Brian Foster
2018-07-30 17:31 ` Darrick J. Wong
2018-07-30 18:19 ` Brian Foster
2018-07-30 18:22 ` Darrick J. Wong
2018-07-30 18:33 ` Brian Foster [this message]
2018-07-30 5:48 ` [PATCH 03/14] xfs: repair the AGFL Darrick J. Wong
2018-07-30 16:25 ` Brian Foster
2018-07-30 17:22 ` Darrick J. Wong
2018-07-31 15:10 ` Brian Foster
2018-08-07 22:02 ` Darrick J. Wong
2018-08-08 12:09 ` Brian Foster
2018-08-08 21:26 ` Darrick J. Wong
2018-08-09 11:14 ` Brian Foster
2018-07-30 5:48 ` [PATCH 04/14] xfs: repair the AGI Darrick J. Wong
2018-07-30 18:20 ` Brian Foster
2018-07-30 18:44 ` Darrick J. Wong
2018-07-30 5:48 ` [PATCH 05/14] xfs: repair free space btrees Darrick J. Wong
2018-07-31 17:47 ` Brian Foster
2018-07-31 22:01 ` Darrick J. Wong
2018-08-01 11:54 ` Brian Foster
2018-08-01 16:23 ` Darrick J. Wong
2018-08-01 18:39 ` Brian Foster
2018-08-02 6:28 ` Darrick J. Wong
2018-08-02 13:48 ` Brian Foster
2018-08-02 19:22 ` Darrick J. Wong
2018-08-03 10:49 ` Brian Foster
2018-08-07 23:34 ` Darrick J. Wong
2018-08-08 12:29 ` Brian Foster
2018-08-08 22:42 ` Darrick J. Wong
2018-08-09 12:00 ` Brian Foster
2018-08-09 15:59 ` Darrick J. Wong
2018-08-10 10:33 ` Brian Foster
2018-08-10 15:39 ` Darrick J. Wong
2018-08-10 19:07 ` Brian Foster
2018-08-10 19:36 ` Darrick J. Wong
2018-08-11 12:50 ` Brian Foster
2018-08-11 15:48 ` Darrick J. Wong
2018-08-13 2:46 ` Dave Chinner
2018-07-30 5:48 ` [PATCH 06/14] xfs: repair inode btrees Darrick J. Wong
2018-08-02 14:54 ` Brian Foster
2018-11-06 2:16 ` Darrick J. Wong
2018-07-30 5:48 ` [PATCH 07/14] xfs: repair refcount btrees Darrick J. Wong
2018-07-30 5:48 ` [PATCH 08/14] xfs: repair inode records Darrick J. Wong
2018-07-30 5:48 ` [PATCH 09/14] xfs: zap broken inode forks Darrick J. Wong
2018-07-30 5:49 ` [PATCH 10/14] xfs: repair inode block maps Darrick J. Wong
2018-07-30 5:49 ` [PATCH 11/14] xfs: repair damaged symlinks Darrick J. Wong
2018-07-30 5:49 ` [PATCH 12/14] xfs: repair extended attributes Darrick J. Wong
2018-07-30 5:49 ` [PATCH 13/14] xfs: scrub should set preen if attr leaf has holes Darrick J. Wong
2018-07-30 5:49 ` [PATCH 14/14] xfs: repair quotas Darrick J. Wong
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=20180730183300.GF35107@bfoster \
--to=bfoster@redhat.com \
--cc=allison.henderson@oracle.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.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).