Re: [PATCH 02/22] xfs: add helpers to allocate and initialize fresh btree roots

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, May 16, 2018 at 05:07:46PM +1000, Dave Chinner wrote:
> On Tue, May 15, 2018 at 03:33:51PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a pair of helper functions to allocate and initialize fresh btree
> > roots.  The repair functions will use these as part of recreating
> > corrupted metadata.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/repair.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/repair.h |    6 ++++
> >  2 files changed, 87 insertions(+)
> 
> Looks good, but....
> 
> > +/* Initialize a new AG btree root block with zero entries. */
> > +int
> > +xfs_repair_init_btblock(
> > +	struct xfs_scrub_context	*sc,
> > +	xfs_fsblock_t			fsb,
> > +	struct xfs_buf			**bpp,
> > +	xfs_btnum_t			btnum,
> > +	const struct xfs_buf_ops	*ops)
> > +{
> > +	struct xfs_trans		*tp = sc->tp;
> > +	struct xfs_mount		*mp = sc->mp;
> > +	struct xfs_buf			*bp;
> > +
> > +	trace_xfs_repair_init_btblock(mp, XFS_FSB_TO_AGNO(mp, fsb),
> > +			XFS_FSB_TO_AGBNO(mp, fsb), btnum);
> > +
> > +	ASSERT(XFS_FSB_TO_AGNO(mp, fsb) == sc->sa.agno);
> > +	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, XFS_FSB_TO_DADDR(mp, fsb),
> > +			XFS_FSB_TO_BB(mp, 1), 0);
> > +	xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> > +	xfs_btree_init_block(mp, bp, btnum, 0, 0, sc->sa.agno,
> > +			XFS_BTREE_CRC_BLOCKS);
> 
> This flag does nothing in xfs_btree_init_block(). Any reason for
> setting it?

Nope.  Will fix.

> With that fixed, though,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> > +	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_BTREE_BUF);
> > +	xfs_trans_log_buf(tp, bp, 0, bp->b_length);
> > +	bp->b_ops = ops;
> > +	*bpp = bp;
> > +
> > +	return 0;
> > +}
> 
> For followup patches, I think there's some overlap with the new
> libxfs/xfs_ag.c functions for initialising new btree root blocks for
> growfs? e.g. Make the struct xfs_aghdr_grow_data aghdr_data[] array
> a static global, and then we can do something like:
> 
> 	bp = xfs_ag_init_btroot(mp, tp, agno, agbno, btnum);
> 	xfs_trans_log_buf(tp, bp, 0, bp->b_length);
> 
> and all the details of reading the btree block and setting it up go
> away from this code....

I'm not sure aghdr_data can be made static global since parts of it
depend on the struct xfs_mount, but at the very least we should try to
refactor this (in the followup series) to simplify the repair code.

There may be complications relating to the xfs_*root_init functions
initializing default records that are appropriate for a freshly created
AG that are totally wrong for regenerating a broken AG, and in any case
repair really requires totally empty root blocks since it has already
generated the list of records it's going to put into the new btree.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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