On Wed, Oct 04, 2017 at 05:43:33PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 09:25:01PM -0700, Darrick J. Wong wrote:
> > On Wed, Oct 04, 2017 at 12:43:47PM +1100, Dave Chinner wrote:
> > > On Tue, Oct 03, 2017 at 01:41:59PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > >
> > > > Add a forgotten check to the AGI verifier, then wire up the scrub
> > > > infrastructure to check the AGI contents.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > > ---
> > > > fs/xfs/libxfs/xfs_fs.h | 3 +-
> > > > fs/xfs/scrub/agheader.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > fs/xfs/scrub/common.c | 6 ++-
> > > > fs/xfs/scrub/scrub.c | 4 ++
> > > > fs/xfs/scrub/scrub.h | 1 +
> > > > 5 files changed, 99 insertions(+), 3 deletions(-)
> > > >
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > > index aeb2a66..1e326dd 100644
> > > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > > @@ -487,9 +487,10 @@ struct xfs_scrub_metadata {
> > > > #define XFS_SCRUB_TYPE_SB 1 /* superblock */
> > > > #define XFS_SCRUB_TYPE_AGF 2 /* AG free header */
> > > > #define XFS_SCRUB_TYPE_AGFL 3 /* AG free list */
> > > > +#define XFS_SCRUB_TYPE_AGI 4 /* AG inode header */
> > > >
> > > > /* Number of scrub subcommands. */
> > > > -#define XFS_SCRUB_TYPE_NR 4
> > > > +#define XFS_SCRUB_TYPE_NR 5
> > > >
> > > > /* i: Repair this metadata. */
> > > > #define XFS_SCRUB_IFLAG_REPAIR (1 << 0)
> > > > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> > > > index 7fe6630..3d269c2 100644
> > > > --- a/fs/xfs/scrub/agheader.c
> > > > +++ b/fs/xfs/scrub/agheader.c
> > > > @@ -535,3 +535,91 @@ xfs_scrub_agfl(
> > > > out:
> > > > return error;
> > > > }
> > > > +
> > > > +/* AGI */
> > > > +
> > > > +/* Scrub the AGI. */
> > > > +int
> > > > +xfs_scrub_agi(
> > > > + struct xfs_scrub_context *sc)
> > > > +{
> > > > + struct xfs_mount *mp = sc->mp;
> > > > + struct xfs_agi *agi;
> > > > + xfs_daddr_t daddr;
> > > > + xfs_daddr_t eofs;
> > > > + xfs_agnumber_t agno;
> > > > + xfs_agblock_t agbno;
> > > > + xfs_agblock_t eoag;
> > > > + xfs_agino_t agino;
> > > > + xfs_agino_t first_agino;
> > > > + xfs_agino_t last_agino;
> > > > + int i;
> > > > + int level;
> > > > + int error = 0;
> > > > +
> > > > + agno = sc->sm->sm_agno;
> > > > + error = xfs_scrub_load_ag_headers(sc, agno, XFS_SCRUB_TYPE_AGI);
> > > > + if (!xfs_scrub_op_ok(sc, agno, XFS_AGI_BLOCK(sc->mp), &error))
> > > > + goto out;
> > > > +
> > > > + agi = XFS_BUF_TO_AGI(sc->sa.agi_bp);
> > > > + eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
> > > > +
> > > > + /* Check the AG length */
> > > > + eoag = be32_to_cpu(agi->agi_length);
> > > > + if (eoag != xfs_scrub_ag_blocks(mp, agno))
> > > > + xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> > >
> > > Should we be cross checking that the AGI and AGF both have
> > > the same length here?
> >
> > Isn't that what this does? Albeit indirectly?
>
> I was kinda thinking of explicit checks, but you are right, it's
> indirectly verified....
>
> > xfs_scrub_ag_blocks returns sb_agcount for every AG except the last one.
> > For the last AG it returns (sb_dblocks - (all blocks in the other AGs))
> > which should be the same as agf->agf_length, right?
>
> ... which assumes we've validated sb_agblocks and sb_dblocks in some
> way, which we haven't really done in the superblock scrubber.
Yes.
> It seems to me that we're using the superblock 0 values as the
> golden master because it's a mounted filesystem, and then comparing
> everything else against it. Maybe we should at least check a couple
> of secondary superblocks to see that they match the primary
> superblock - that way we'll have some confidence that at least
> things like agcount, agblocks, dblocks, etc are valid before we go
> any further...
xfs_scrub_superblock does check the secondary superblock geometry
against whatever's in mp->m_sb, which came from sb 0.
> BUt maybe all we need is comment in the overall scrub description -
> that we're pretty much assuming that sb 0 is intact because we write
> what is in memory back to it and so we can simply validate
> everything else against the primary superblock contents...
Correct. Since scrub is run against a mounted live filesystem we assume
that the mount code fully validated sb 0 and therefore we can rely on it
not being wrong.
If OTOH sb 0 *is* wrong then the admin is better off running xfs_repair
because there's too much whirring machinery to go changing fundamental
geometry.
Ok more comments are coming.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
