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>
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).
> + if (error)
> + return error;
> + if (args.fsbno == NULLFSBLOCK)
> + return -ENOSPC;
This will need to change to handling ENOSPC as the error directly on
failure.
> +
> + 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.
> +
> + 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....
Hust a heads up for now, thought, we can deal with these issues when
merging for one or the other happens...
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
