On Thu, Oct 05, 2023 at 03:53:10PM +1100, Dave Chinner wrote:
> On Tue, Sep 26, 2023 at 04:32:25PM -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>
> > ---
> > fs/xfs/Makefile | 1
> > fs/xfs/libxfs/xfs_btree_staging.h | 7 -
> > fs/xfs/scrub/agheader_repair.c | 1
> > fs/xfs/scrub/common.c | 1
> > fs/xfs/scrub/newbt.c | 492 +++++++++++++++++++++++++++++++++++++
> > fs/xfs/scrub/newbt.h | 62 +++++
> > fs/xfs/scrub/scrub.c | 2
> > fs/xfs/scrub/trace.h | 37 +++
> > 8 files changed, 598 insertions(+), 5 deletions(-)
> > create mode 100644 fs/xfs/scrub/newbt.c
> > create mode 100644 fs/xfs/scrub/newbt.h
>
> Looks reasonable to me. It all makes sense and nothing is obviously
> wrong.
>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
Thanks!
>
> Some notes on the extent allocation API bits - the rework of the
> high level allocation primitives I just posted intersects with this
> code in some interesting ways....
>
> > +
> > +/* Allocate disk space for a new per-AG btree. */
> > +STATIC int
> > +xrep_newbt_alloc_ag_blocks(
> > + struct xrep_newbt *xnr,
> > + uint64_t nr_blocks)
> > +{
> > + struct xfs_scrub *sc = xnr->sc;
> > + struct xfs_mount *mp = sc->mp;
> > + int error = 0;
> > +
> > + ASSERT(sc->sa.pag != NULL);
> > +
> > + while (nr_blocks > 0) {
> > + struct xfs_alloc_arg args = {
> > + .tp = sc->tp,
> > + .mp = mp,
> > + .oinfo = xnr->oinfo,
> > + .minlen = 1,
> > + .maxlen = nr_blocks,
> > + .prod = 1,
> > + .resv = xnr->resv,
> > + };
> > + xfs_agnumber_t agno;
> > +
> > + xrep_newbt_validate_ag_alloc_hint(xnr);
> > +
> > + error = xfs_alloc_vextent_near_bno(&args, xnr->alloc_hint);
>
> This would require a perag to be held by the caller (sc->sa.pag)
> and attached to the args. The target also changes to an agbno
> (IIRC).
<nod> Pretty straightforward.
> > + if (error)
> > + return error;
> > + if (args.fsbno == NULLFSBLOCK)
> > + return -ENOSPC;
>
> This will need to change to handling ENOSPC as the error directly on
> failure.
<nod>
> > +
> > + agno = XFS_FSB_TO_AGNO(mp, args.fsbno);
> > +
> > + trace_xrep_newbt_alloc_ag_blocks(mp, agno,
> > + XFS_FSB_TO_AGBNO(mp, args.fsbno), args.len,
> > + xnr->oinfo.oi_owner);
> > +
> > + if (agno != sc->sa.pag->pag_agno) {
> > + ASSERT(agno == sc->sa.pag->pag_agno);
> > + return -EFSCORRUPTED;
> > + }
>
> This can go away, because it simply isn't possible - it will
> allocate a block in sc->sa.pag or fail with ENOSPC.
>
> Hence this will probably simplify down a bit.
Yessssssss
> > +
> > + error = xrep_newbt_add_blocks(xnr, sc->sa.pag, &args);
> > + if (error)
> > + return error;
> > +
> > + nr_blocks -= args.len;
> > + xnr->alloc_hint = args.fsbno + args.len;
> > +
> > + error = xrep_defer_finish(sc);
> > + if (error)
> > + return error;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Don't let our allocation hint take us beyond EOFS */
> > +static inline void
> > +xrep_newbt_validate_file_alloc_hint(
> > + struct xrep_newbt *xnr)
> > +{
> > + struct xfs_scrub *sc = xnr->sc;
> > +
> > + if (xfs_verify_fsbno(sc->mp, xnr->alloc_hint))
> > + return;
> > +
> > + xnr->alloc_hint = XFS_AGB_TO_FSB(sc->mp, 0, XFS_AGFL_BLOCK(sc->mp) + 1);
> > +}
> > +
> > +/* 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;
> > + struct xfs_mount *mp = sc->mp;
> > + int error = 0;
> > +
> > + while (nr_blocks > 0) {
> > + struct xfs_alloc_arg args = {
> > + .tp = sc->tp,
> > + .mp = mp,
> > + .oinfo = xnr->oinfo,
> > + .minlen = 1,
> > + .maxlen = nr_blocks,
> > + .prod = 1,
> > + .resv = xnr->resv,
> > + };
> > + struct xfs_perag *pag;
> > + xfs_agnumber_t agno;
> > +
> > + 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;
>
> Similar target/errno changes will be needed here, and ....
> > +
> > + agno = XFS_FSB_TO_AGNO(mp, args.fsbno);
> > +
> > + trace_xrep_newbt_alloc_file_blocks(mp, agno,
> > + XFS_FSB_TO_AGBNO(mp, args.fsbno), args.len,
> > + xnr->oinfo.oi_owner);
> > +
> > + pag = xfs_perag_get(mp, agno);
> > + if (!pag) {
> > + ASSERT(0);
> > + return -EFSCORRUPTED;
> > + }
> > +
> > + error = xrep_newbt_add_blocks(xnr, pag, &args);
> > + xfs_perag_put(pag);
> > + if (error)
> > + return error;
>
> I suspect it might be useful to have xfs_alloc_vextent_start_ag() be
> able to return the referenced perag that the allocation occurred in
> rather than having to split the result and look it up again....
Yeah, I think it's reasonable to return an active(?) reference to the
perag that we picked and the space allocated from that AG.
> Hust a heads up for now, thought, we can deal with these issues when
> merging for one or the other happens...
Ok.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
