Re: [PATCH 10/12] xfs_repair: rebuild refcount btrees with bulk loader

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: sandeen@sandeen.net, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 10/12] xfs_repair: rebuild refcount btrees with bulk loader
Date: Thu, 18 Jun 2020 09:56:22 -0700	[thread overview]
Message-ID: <20200618165622.GX11245@magnolia> (raw)
In-Reply-To: <20200618152617.GD32216@bfoster>

On Thu, Jun 18, 2020 at 11:26:17AM -0400, Brian Foster wrote:
> On Mon, Jun 01, 2020 at 09:27:57PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Use the btree bulk loading functions to rebuild the refcount btrees
> > and drop the open-coded implementation.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/libxfs_api_defs.h |    1 
> >  repair/agbtree.c         |   71 ++++++++++
> >  repair/agbtree.h         |    5 +
> >  repair/phase5.c          |  341 ++--------------------------------------------
> >  4 files changed, 93 insertions(+), 325 deletions(-)
> > 
> > 
> ...
> > diff --git a/repair/phase5.c b/repair/phase5.c
> > index 1c6448f4..ad009416 100644
> > --- a/repair/phase5.c
> > +++ b/repair/phase5.c
> ...
> > @@ -817,10 +510,14 @@ build_agf_agfl(
> >  				cpu_to_be32(btr_rmap->newbt.afake.af_blocks);
> >  	}
> >  
> > -	agf->agf_refcount_root = cpu_to_be32(refcnt_bt->root);
> > -	agf->agf_refcount_level = cpu_to_be32(refcnt_bt->num_levels);
> > -	agf->agf_refcount_blocks = cpu_to_be32(refcnt_bt->num_tot_blocks -
> > -			refcnt_bt->num_free_blocks);
> > +	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> > +		agf->agf_refcount_root =
> > +				cpu_to_be32(btr_refc->newbt.afake.af_root);
> > +		agf->agf_refcount_level =
> > +				cpu_to_be32(btr_refc->newbt.afake.af_levels);
> > +		agf->agf_refcount_blocks =
> > +				cpu_to_be32(btr_refc->newbt.afake.af_blocks);
> > +	}
> 
> It looks like the previous cursor variant (refcnt_bt) would be zeroed
> out if the feature isn't enabled (causing this to zero out the agf
> fields on disk), whereas now we only write the fields when the feature
> is enabled. Any concern over removing that zeroing behavior? Also note
> that an assert further down unconditionally reads the
> ->agf_refcount_root field.
> 
> BTW, I suppose the same question may apply to the previous patch as
> well...

I'll double check, but we do memset the AGF (and AGI) to zero before we
start initializing things, so the asserts should be fine even on
!reflink filesystems.

--D

> Brian
> 
> >  
> >  	/*
> >  	 * Count and record the number of btree blocks consumed if required.
> > @@ -981,7 +678,7 @@ phase5_func(
> >  	struct bt_rebuild	btr_ino;
> >  	struct bt_rebuild	btr_fino;
> >  	struct bt_rebuild	btr_rmap;
> > -	bt_status_t		refcnt_btree_curs;
> > +	struct bt_rebuild	btr_refc;
> >  	int			extra_blocks = 0;
> >  	uint			num_freeblocks;
> >  	xfs_agblock_t		num_extents;
> > @@ -1017,11 +714,7 @@ _("unable to rebuild AG %u.  Not enough free space in on-disk AG.\n"),
> >  
> >  	init_rmapbt_cursor(&sc, agno, num_freeblocks, &btr_rmap);
> >  
> > -	/*
> > -	 * Set up the btree cursors for the on-disk refcount btrees,
> > -	 * which includes pre-allocating all required blocks.
> > -	 */
> > -	init_refc_cursor(mp, agno, &refcnt_btree_curs);
> > +	init_refc_cursor(&sc, agno, num_freeblocks, &btr_refc);
> >  
> >  	num_extents = count_bno_extents_blocks(agno, &num_freeblocks);
> >  	/*
> > @@ -1085,16 +778,14 @@ _("unable to rebuild AG %u.  Not enough free space in on-disk AG.\n"),
> >  		sb_fdblocks_ag[agno] += btr_rmap.newbt.afake.af_blocks - 1;
> >  	}
> >  
> > -	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> > -		build_refcount_tree(mp, agno, &refcnt_btree_curs);
> > -		write_cursor(&refcnt_btree_curs);
> > -	}
> > +	if (xfs_sb_version_hasreflink(&mp->m_sb))
> > +		build_refcount_tree(&sc, agno, &btr_refc);
> >  
> >  	/*
> >  	 * set up agf and agfl
> >  	 */
> > -	build_agf_agfl(mp, agno, &btr_bno, &btr_cnt, &btr_rmap,
> > -			&refcnt_btree_curs, lost_fsb);
> > +	build_agf_agfl(mp, agno, &btr_bno, &btr_cnt, &btr_rmap, &btr_refc,
> > +			lost_fsb);
> >  
> >  	build_inode_btrees(&sc, agno, &btr_ino, &btr_fino);
> >  
> > @@ -1112,7 +803,7 @@ _("unable to rebuild AG %u.  Not enough free space in on-disk AG.\n"),
> >  	if (xfs_sb_version_hasrmapbt(&mp->m_sb))
> >  		finish_rebuild(mp, &btr_rmap, lost_fsb);
> >  	if (xfs_sb_version_hasreflink(&mp->m_sb))
> > -		finish_cursor(&refcnt_btree_curs);
> > +		finish_rebuild(mp, &btr_refc, lost_fsb);
> >  
> >  	/*
> >  	 * release the incore per-AG bno/bcnt trees so the extent nodes
> > 
> 