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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> .....
> > +/* 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@xxxxxxxxxxxxx
