On Thu, May 28, 2020 at 11:09:21AM -0400, Brian Foster wrote:
> On Wed, May 27, 2020 at 03:07:33PM -0700, Darrick J. Wong wrote:
> > On Wed, May 27, 2020 at 08:18:04AM -0400, Brian Foster wrote:
> > > On Tue, May 19, 2020 at 06:51:02PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > >
> > > > Create some new support structures and functions to assist phase5 in
> > > > using the btree bulk loader to reconstruct metadata btrees. This is the
> > > > first step in removing the open-coded rebuilding code.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > > ---
> > > > repair/phase5.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > 1 file changed, 218 insertions(+), 21 deletions(-)
> > > >
> > > >
> > > > diff --git a/repair/phase5.c b/repair/phase5.c
> > > > index 84c05a13..8f5e5f59 100644
> > > > --- a/repair/phase5.c
> > > > +++ b/repair/phase5.c
> > > > @@ -18,6 +18,7 @@
> > > > #include "progress.h"
> > > > #include "slab.h"
> > > > #include "rmap.h"
> > > > +#include "bload.h"
> > > >
> > > > /*
> > > > * we maintain the current slice (path from root to leaf)
> > > ...
> > > > @@ -306,6 +324,156 @@ _("error - not enough free space in filesystem\n"));
> > > > #endif
> > > > }
> > > >
> > > ...
> > > > +static void
> > > > +consume_freespace(
> > > > + xfs_agnumber_t agno,
> > > > + struct extent_tree_node *ext_ptr,
> > > > + uint32_t len)
> > > > +{
> > > > + struct extent_tree_node *bno_ext_ptr;
> > > > + xfs_agblock_t new_start = ext_ptr->ex_startblock + len;
> > > > + xfs_extlen_t new_len = ext_ptr->ex_blockcount - len;
> > > > +
> > > > + /* Delete the used-up extent from both extent trees. */
> > > > +#ifdef XR_BLD_FREE_TRACE
> > > > + fprintf(stderr, "releasing extent: %u [%u %u]\n", agno,
> > > > + ext_ptr->ex_startblock, ext_ptr->ex_blockcount);
> > > > +#endif
> > > > + bno_ext_ptr = find_bno_extent(agno, ext_ptr->ex_startblock);
> > > > + ASSERT(bno_ext_ptr != NULL);
> > > > + get_bno_extent(agno, bno_ext_ptr);
> > > > + release_extent_tree_node(bno_ext_ptr);
> > > > +
> > > > + ext_ptr = get_bcnt_extent(agno, ext_ptr->ex_startblock,
> > > > + ext_ptr->ex_blockcount);
> > > > + release_extent_tree_node(ext_ptr);
> > > > +
> > >
> > > Not having looked too deeply at the in-core extent tracking structures,
> > > is there any particular reason we unconditionally remove and reinsert
> > > new records each time around? Is it because we're basically changing the
> > > extent index in the tree? If so, comment please (an update to the
> > > comment below is probably fine). :)
> >
> > Yes. We're changing the free space tree records, and the incore bno and
> > cnt trees maintain the records in sorted order. Therefore, if we want
> > to change a record we have to delete the record from the tree and
> > reinsert it.
> >
> > > > + /*
> > > > + * If we only used part of this last extent, then we must reinsert the
> > > > + * extent in the extent trees.
> >
> > /*
> > * If we only used part of this last extent, then we must reinsert the
> > * extent to maintain proper sorting order.
> > */
> >
> > How about that?
> >
>
> Works for me, thanks.
>
> > > > + */
> > > > + if (new_len > 0) {
> > > > + add_bno_extent(agno, new_start, new_len);
> > > > + add_bcnt_extent(agno, new_start, new_len);
> > > > + }
> > > > +}
> > > > +
> > > > +/* Reserve blocks for the new btree. */
> > > > +static void
> > > > +setup_rebuild(
> > > > + struct xfs_mount *mp,
> > > > + xfs_agnumber_t agno,
> > > > + struct bt_rebuild *btr,
> > > > + uint32_t nr_blocks)
> > > > +{
> > > > + struct extent_tree_node *ext_ptr;
> > > > + uint32_t blocks_allocated = 0;
> > > > + uint32_t len;
> > > > + int error;
> > > > +
> > > > + while (blocks_allocated < nr_blocks) {
> > > > + /*
> > > > + * Grab the smallest extent and use it up, then get the
> > > > + * next smallest. This mimics the init_*_cursor code.
> > > > + */
> > > > + ext_ptr = findfirst_bcnt_extent(agno);
> > >
> > > Extra whitespace ^
> > >
> > > > + if (!ext_ptr)
> > > > + do_error(
> > > > +_("error - not enough free space in filesystem\n"));
> > > > +
> > > > + /* Use up the extent we've got. */
> > > > + len = min(ext_ptr->ex_blockcount, nr_blocks - blocks_allocated);
> > > > + error = xrep_newbt_add_blocks(&btr->newbt,
> > > > + XFS_AGB_TO_FSB(mp, agno,
> > > > + ext_ptr->ex_startblock),
> > > > + len);
> > >
> > > Alignment.
> >
> > Will fix both of these.
> >
> > > > + if (error)
> > > > + do_error(_("could not set up btree reservation: %s\n"),
> > > > + strerror(-error));
> > > > +
> > > > + error = rmap_add_ag_rec(mp, agno, ext_ptr->ex_startblock, len,
> > > > + btr->newbt.oinfo.oi_owner);
> > > > + if (error)
> > > > + do_error(_("could not set up btree rmaps: %s\n"),
> > > > + strerror(-error));
> > > > +
> > > > + consume_freespace(agno, ext_ptr, len);
> > > > + blocks_allocated += len;
> > > > + }
> > > > +#ifdef XR_BLD_FREE_TRACE
> > > > + fprintf(stderr, "blocks_allocated = %d\n",
> > > > + blocks_allocated);
> > > > +#endif
> > > > +}
> > > > +
> > > > +/* Feed one of the new btree blocks to the bulk loader. */
> > > > +static int
> > > > +rebuild_claim_block(
> > > > + struct xfs_btree_cur *cur,
> > > > + union xfs_btree_ptr *ptr,
> > > > + void *priv)
> > > > +{
> > > > + struct bt_rebuild *btr = priv;
> > > > +
> > > > + return xrep_newbt_claim_block(cur, &btr->newbt, ptr);
> > > > +}
> > > > +
> > >
> > > Seems like an unnecessary helper, unless this grows more code in later
> > > patches..?
> >
> > It doesn't grow any more code, but keep in mind that get_record,
> > claim_block, and iroot_size are all callbacks of xfs_btree_bload(). The
> > priv parameter passed to that function are passed unchanged to the three
> > callbacks. The bulk load code doesn't know anything about where the
> > blocks or the records come from, so this is how both repairs will pass
> > that information to the callbacks.
> >
>
> Ok.
>
> > > > static void
> > > > write_cursor(bt_status_t *curs)
> > > > {
> > > ...
> > > > @@ -2287,28 +2483,29 @@ keep_fsinos(xfs_mount_t *mp)
> > > >
> > > > static void
> > > > phase5_func(
> > > > - xfs_mount_t *mp,
> > > > - xfs_agnumber_t agno,
> > > > - struct xfs_slab *lost_fsb)
> > > > + struct xfs_mount *mp,
> > > > + xfs_agnumber_t agno,
> > > > + struct xfs_slab *lost_fsb)
> > > > {
> > > > - uint64_t num_inos;
> > > > - uint64_t num_free_inos;
> > > > - uint64_t finobt_num_inos;
> > > > - uint64_t finobt_num_free_inos;
> > > > - bt_status_t bno_btree_curs;
> > > > - bt_status_t bcnt_btree_curs;
> > > > - bt_status_t ino_btree_curs;
> > > > - bt_status_t fino_btree_curs;
> > > > - bt_status_t rmap_btree_curs;
> > > > - bt_status_t refcnt_btree_curs;
> > > > - int extra_blocks = 0;
> > > > - uint num_freeblocks;
> > > > - xfs_extlen_t freeblks1;
> > > > + struct repair_ctx sc = { .mp = mp, };
> > >
> > > I don't see any reason to add sc here when it's still unused. It's not
> > > as if a single variable is saving complexity somewhere else. I guess
> > > I'll defer to Eric on the approach wrt to the other unused warnings.
> >
> > <shrug> I'll ask. It seems dumb to have a prep patch that adds a bunch
> > of symbols that won't get used until the next patch, but OTOH combining
> > the two will make for a ~40K patch.
> >
>
> I've no strong preference either way in general (the single variable
> thing aside) as long as each patch compiles and functions correctly and
> warnings are addressed by the end of the series. I do think that if we
> keep separate patches, it should probably be documented in the commit
> log that unused infrastructure is introduced (i.e. warnings expected)
> and users are introduced in a following patch. It's usually easier to
> squash patches than separate, so the maintainer can always squash them
> post review if he wanted to eliminate the warnings from the commit
> history.
Ok, will do.
--D
> Brian
>
> > > Also, what's the purpose of the rmap change below? I'm wondering if that
> > > (along with all of the indentation cleanup) should be its own patch with
> > > appropriate explanation.
> >
> > Errk, that one definitely should be separate.
> >
> > > Brian
> > >
> > > > + struct agi_stat agi_stat = {0,};
> > > > + uint64_t num_inos;
> > > > + uint64_t num_free_inos;
> > > > + uint64_t finobt_num_inos;
> > > > + uint64_t finobt_num_free_inos;
> > > > + bt_status_t bno_btree_curs;
> > > > + bt_status_t bcnt_btree_curs;
> > > > + bt_status_t ino_btree_curs;
> > > > + bt_status_t fino_btree_curs;
> > > > + bt_status_t rmap_btree_curs;
> > > > + bt_status_t refcnt_btree_curs;
> > > > + int extra_blocks = 0;
> > > > + uint num_freeblocks;
> > > > + xfs_extlen_t freeblks1;
> > > > #ifdef DEBUG
> > > > - xfs_extlen_t freeblks2;
> > > > + xfs_extlen_t freeblks2;
> > > > #endif
> > > > - xfs_agblock_t num_extents;
> > > > - struct agi_stat agi_stat = {0,};
> > > > + xfs_agblock_t num_extents;
> > > >
> > > > if (verbose)
> > > > do_log(_(" - agno = %d\n"), agno);
> > > > @@ -2516,8 +2713,8 @@ inject_lost_blocks(
> > > > if (error)
> > > > goto out_cancel;
> > > >
> > > > - error = -libxfs_free_extent(tp, *fsb, 1, &XFS_RMAP_OINFO_AG,
> > > > - XFS_AG_RESV_NONE);
> > > > + error = -libxfs_free_extent(tp, *fsb, 1,
> > > > + &XFS_RMAP_OINFO_ANY_OWNER, XFS_AG_RESV_NONE);
> > > > if (error)
> > > > goto out_cancel;
> > > >
> > > >
> > >
> >
>
