On Thu, Jun 18, 2020 at 09:56:22AM -0700, Darrick J. Wong wrote:
> 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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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.
>
Ah, so the implicit per-field zeroing behavior of the old implementation
is superfluous. Assert aside, I just wanted to make sure we weren't
removing some subtle mechanism for clearing unused metadata fields if
they happened to contain garbage. That is not the case, so this one
looks fine to me as well:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> --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
> > >
> >
>
