Re: [PATCH 2/6] xfs: implement block reservation accounting for btrees we're staging

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Aug 07, 2023 at 04:58:50PM +1000, Dave Chinner wrote:
> On Thu, Jul 27, 2023 at 03:24:16PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create a new xrep_newbt structure to encapsulate a fake root for
> > creating a staged btree cursor as well as to track all the blocks that
> > we need to reserve in order to build that btree.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> .....
> > +/* Allocate disk space for our new file-based btree. */
> > +STATIC int
> > +xrep_newbt_alloc_file_blocks(
> > +	struct xrep_newbt	*xnr,
> > +	uint64_t		nr_blocks)
> > +{
> > +	struct xfs_scrub	*sc = xnr->sc;
> > +	int			error = 0;
> > +
> > +	while (nr_blocks > 0) {
> > +		struct xfs_alloc_arg	args = {
> > +			.tp		= sc->tp,
> > +			.mp		= sc->mp,
> > +			.oinfo		= xnr->oinfo,
> > +			.minlen		= 1,
> > +			.maxlen		= nr_blocks,
> > +			.prod		= 1,
> > +			.resv		= xnr->resv,
> > +		};
> > +		struct xfs_perag	*pag;
> > +
> > +		xrep_newbt_validate_file_alloc_hint(xnr);
> > +
> > +		error = xfs_alloc_vextent_start_ag(&args, xnr->alloc_hint);
> > +		if (error)
> > +			return error;
> > +		if (args.fsbno == NULLFSBLOCK)
> > +			return -ENOSPC;
> > +
> > +		trace_xrep_newbt_alloc_file_blocks(sc->mp, args.agno,
> > +				args.agbno, args.len, xnr->oinfo.oi_owner);
> > +
> > +		pag = xfs_perag_get(sc->mp, args.agno);
> 
> I don't think we should allow callers to trust args.agno and
> args.agbno after the allocation has completed. The result of the
> allocation is returned in args.fsbno, and there is no guarantee that
> args.agno and args.agbno will be valid at the completion of the
> allocation.
> 
> i.e. we set args.agno and args.agbno internally based on the target
> that is passed to xfs_alloc_vextent_start_ag(), and they change
> internally depending on the iterations being done during allocation.
> IOWs, those two fields are internal allocation state and not
> actually return values that the caller can rely on.
> 
> Hence I think this needs to do:
> 
> 	agno = XFS_FSB_TO_AGNO(mp, args.fsbno);
> 	agbno = XFS_FSB_TO_AGBNO(mp, args.fsbno);
> 
> before using those values.

Ok, fixed.  At some point we ought to double-underscore all the
private(ish) fields in xfs_alloc_args.  I'll also fix
xrep_newbt_alloc_ag_blocks.

> > +
> > +/*
> > + * How many extent freeing items can we attach to a transaction before we want
> > + * to finish the chain so that unreserving new btree blocks doesn't overrun
> > + * the transaction reservation?
> > + */
> > +#define XREP_REAP_MAX_NEWBT_EFIS	(128)
> 
> Should there be a common define for this for repair operations?

I had left them separate, but I don't think there's much of a point
anymore, since the newbt(ree) and reaping code both use tr_itruncate.

/*
 * This is the maximum number of deferred extent freeing item extents
 * (EFIs) that we'll attach to a transaction without rolling the
 * transaction to avoid overrunning a tr_itruncate reservation.
 */
#define XREP_MAX_ITRUNCATE_EFIS	(128)


--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com