Re: [PATCH 11/25] xfs: scrub the AGI

[Dave Chinner <david@fromorbit.com>]

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@oracle.com>
> > > 
> > > 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@oracle.com>
> > > ---
> > >  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.

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...

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com