Re: [PATCH 2/2] xfs_repair: try to fill the AGFL before we fix the freelist

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

On Mon, Jun 29, 2020 at 08:22:28AM -0400, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 01:52:39PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In commit 9851fd79bfb1, we added a slight amount of slack to the free
> > space btrees being reconstructed so that the initial fix_freelist call
> > (which is run against a totally empty AGFL) would never have to split
> > either free space btree in order to populate the free list.
> > 
> > The new btree bulk loading code in xfs_repair can re-create this
> > situation because it can set the slack values to zero if the filesystem
> > is very full.  However, these days repair has the infrastructure needed
> > to ensure that overestimations of the btree block counts end up on the
> > AGFL or get freed back into the filesystem at the end of phase 5.
> > 
> > Fix this problem by reserving blocks to a separate AGFL block
> > reservation, and checking that between this new reservation and any
> > overages in the bnobt/cntbt fakeroots, we have enough blocks sitting
> > around to populate the AGFL with the minimum number of blocks it needs
> > to handle a split in the bno/cnt/rmap btrees.
> > 
> > Note that we reserve blocks for the new bnobt/cntbt/AGFL at the very end
> > of the reservation steps in phase 5, so the extra allocation should not
> > cause repair to fail if it can't find blocks for btrees.
> > 
> > Fixes: 9851fd79bfb1 ("repair: AGFL rebuild fails if btree split required")
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/agbtree.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 68 insertions(+), 10 deletions(-)
> > 
> > 
> > diff --git a/repair/agbtree.c b/repair/agbtree.c
> > index 339b1489..7a4f316c 100644
> > --- a/repair/agbtree.c
> > +++ b/repair/agbtree.c
> ...
> > @@ -262,25 +286,59 @@ _("Unable to compute free space by block btree geometry, error %d.\n"), -error);
> ...
> > +
> > +		/*
> > +		 * Now try to fill the bnobt/cntbt cursors with extra blocks to
> > +		 * populate the AGFL.  If we don't get all the blocks we want,
> > +		 * stop trying to fill the AGFL.
> > +		 */
> > +		wanted = (int64_t)btr_bno->bload.nr_blocks +
> > +				(min_agfl_len / 2) - bno_blocks;
> > +		if (wanted > 0 && fill_agfl) {
> > +			got = reserve_agblocks(sc->mp, agno, btr_bno, wanted);
> > +			if (wanted > got)
> > +				fill_agfl = false;
> > +			btr_bno->bload.nr_blocks += got;
> > +		}
> > +
> > +		wanted = (int64_t)btr_cnt->bload.nr_blocks +
> > +				(min_agfl_len / 2) - cnt_blocks;
> > +		if (wanted > 0 && fill_agfl) {
> > +			got = reserve_agblocks(sc->mp, agno, btr_cnt, wanted);
> > +			if (wanted > got)
> > +				fill_agfl = false;
> > +			btr_cnt->bload.nr_blocks += got;
> > +		}
> 
> It's a little hard to follow this with the nr_blocks sampling and
> whatnot, but I think I get the idea. What's the reason for splitting the
> AGFL res requirement evenly across the two cursors? These AGFL blocks
> all fall into the same overflow pool, right? I was wondering why we
> couldn't just attach the overflow to one, or check one for the full res
> and then the other if more blocks are needed.

I chose to stuff the excess blocks into the bnobt and cntbt bulkload
cursors to avoid having to initialize a semi-phony "bulkload cursor" for
the agfl, and I decided to split them evenly between the two cursors so
that I wouldn't have someday to deal with a bug report about how one
cursor somehow ran out of blocks but the other one had plenty more.

> In thinking about it a bit more, wouldn't the whole algorithm be more
> simple if we reserved the min AGFL requirement first, optionally passed
> 'agfl_res' to reserve_btblocks() such that subsequent reservations can
> steal from it (and then fail if it depletes), then stuff what's left in
> one (or both, if there's a reason for that) of the cursors at the end?

Hmm.  I hadn't thought about that.  In general I wanted the AGFL
reservations to go last because I'd rather we set off with an underfull
AGFL than totally fail because we couldn't get blocks for the
bnobt/cntbt, but I suppose you're right that we could steal from it as
needed to prevent repair failure.

So, uh, I could rework this patch to create a phony agfl bulk load
cursor, fill it before the loop, steal blocks from it to fill the
bnobt/cntbt to satisfy failed allocations, and then dump any remainders
into the bnobt/cntbt cursors afterwards.  How does that sound?

--D

> Brian
> 
> >  
> >  		/* Ok, now how many free space records do we have? */
> >  		*nr_extents = count_bno_extents_blocks(agno, &num_freeblocks);
> >  	} while (1);
> > -
> > -	*extra_blocks = (bno_blocks - btr_bno->bload.nr_blocks) +
> > -			(cnt_blocks - btr_cnt->bload.nr_blocks);
> >  }
> >  
> >  /* Rebuild the free space btrees. */
> > 
>