On Wed, Oct 09, 2019 at 09:49:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Remove the for_each_xbitmap_ macros in favor of proper iterator
> functions. We'll soon be switching this data structure over to an
> interval tree implementation, which means that we can't allow callers to
> modify the bitmap during iteration without telling us.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/agheader_repair.c | 73 ++++++++++++++++++++++++----------------
> fs/xfs/scrub/bitmap.c | 59 ++++++++++++++++++++++++++++++++
> fs/xfs/scrub/bitmap.h | 22 ++++++++----
> fs/xfs/scrub/repair.c | 60 +++++++++++++++++----------------
> 4 files changed, 148 insertions(+), 66 deletions(-)
>
>
...
> diff --git a/fs/xfs/scrub/bitmap.h b/fs/xfs/scrub/bitmap.h
> index 900646b72de1..27fde5b4a753 100644
> --- a/fs/xfs/scrub/bitmap.h
> +++ b/fs/xfs/scrub/bitmap.h
...
> @@ -34,4 +27,19 @@ int xbitmap_set_btblocks(struct xbitmap *bitmap,
> struct xfs_btree_cur *cur);
> uint64_t xbitmap_hweight(struct xbitmap *bitmap);
>
> +/*
> + * Return codes for the bitmap iterator functions are 0 to continue iterating,
> + * and non-zero to stop iterating. Any non-zero value will be passed up to the
> + * iteration caller. The special value -ECANCELED can be used to stop
> + * iteration, because neither bitmap iterator ever generates that error code on
> + * its own.
> + */
> +typedef int (*xbitmap_walk_run_fn)(uint64_t start, uint64_t len, void *priv);
> +int xbitmap_iter_set(struct xbitmap *bitmap, xbitmap_walk_run_fn fn,
> + void *priv);
> +
> +typedef int (*xbitmap_walk_bit_fn)(uint64_t bit, void *priv);
> +int xbitmap_iter_set_bits(struct xbitmap *bitmap, xbitmap_walk_bit_fn fn,
> + void *priv);
> +
Somewhat of a nit, but I read "set" as a verb in the above function
names which tends to confuse me over what these functions do (i.e.
iterate bits, not set bits). Could we call them something a bit more
neutral, like xbitmap[_bit]_iter() perhaps? That aside the rest of the
patch looks Ok to me.
Brian
> #endif /* __XFS_SCRUB_BITMAP_H__ */
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index d41da4c44f10..588bc054db5c 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -507,15 +507,21 @@ xrep_reap_invalidate_block(
> xfs_trans_binval(sc->tp, bp);
> }
>
> +struct xrep_reap_block {
> + struct xfs_scrub *sc;
> + const struct xfs_owner_info *oinfo;
> + enum xfs_ag_resv_type resv;
> + unsigned int deferred;
> +};
> +
> /* Dispose of a single block. */
> STATIC int
> xrep_reap_block(
> - struct xfs_scrub *sc,
> - xfs_fsblock_t fsbno,
> - const struct xfs_owner_info *oinfo,
> - enum xfs_ag_resv_type resv,
> - unsigned int *deferred)
> + uint64_t fsbno,
> + void *priv)
> {
> + struct xrep_reap_block *rb = priv;
> + struct xfs_scrub *sc = rb->sc;
> struct xfs_btree_cur *cur;
> struct xfs_buf *agf_bp = NULL;
> xfs_agnumber_t agno;
> @@ -527,6 +533,10 @@ xrep_reap_block(
> agno = XFS_FSB_TO_AGNO(sc->mp, fsbno);
> agbno = XFS_FSB_TO_AGBNO(sc->mp, fsbno);
>
> + ASSERT(sc->ip != NULL || agno == sc->sa.agno);
> +
> + trace_xrep_dispose_btree_extent(sc->mp, agno, agbno, 1);
> +
> /*
> * If we are repairing per-inode metadata, we need to read in the AGF
> * buffer. Otherwise, we're repairing a per-AG structure, so reuse
> @@ -544,7 +554,8 @@ xrep_reap_block(
> cur = xfs_rmapbt_init_cursor(sc->mp, sc->tp, agf_bp, agno);
>
> /* Can we find any other rmappings? */
> - error = xfs_rmap_has_other_keys(cur, agbno, 1, oinfo, &has_other_rmap);
> + error = xfs_rmap_has_other_keys(cur, agbno, 1, rb->oinfo,
> + &has_other_rmap);
> xfs_btree_del_cursor(cur, error);
> if (error)
> goto out_free;
> @@ -563,8 +574,9 @@ xrep_reap_block(
> * to run xfs_repair.
> */
> if (has_other_rmap) {
> - error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1, oinfo);
> - } else if (resv == XFS_AG_RESV_AGFL) {
> + error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1,
> + rb->oinfo);
> + } else if (rb->resv == XFS_AG_RESV_AGFL) {
> xrep_reap_invalidate_block(sc, fsbno);
> error = xrep_put_freelist(sc, agbno);
> } else {
> @@ -576,16 +588,16 @@ xrep_reap_block(
> * reservation.
> */
> xrep_reap_invalidate_block(sc, fsbno);
> - __xfs_bmap_add_free(sc->tp, fsbno, 1, oinfo, true);
> - (*deferred)++;
> - need_roll = *deferred > 100;
> + __xfs_bmap_add_free(sc->tp, fsbno, 1, rb->oinfo, true);
> + rb->deferred++;
> + need_roll = rb->deferred > 100;
> }
> if (agf_bp != sc->sa.agf_bp)
> xfs_trans_brelse(sc->tp, agf_bp);
> if (error || !need_roll)
> return error;
>
> - *deferred = 0;
> + rb->deferred = 0;
> if (sc->ip)
> return xfs_trans_roll_inode(&sc->tp, sc->ip);
> return xrep_roll_ag_trans(sc);
> @@ -604,27 +616,17 @@ xrep_reap_extents(
> const struct xfs_owner_info *oinfo,
> enum xfs_ag_resv_type type)
> {
> - struct xbitmap_range *bmr;
> - struct xbitmap_range *n;
> - xfs_fsblock_t fsbno;
> - unsigned int deferred = 0;
> + struct xrep_reap_block rb = {
> + .sc = sc,
> + .oinfo = oinfo,
> + .resv = type,
> + };
> int error = 0;
>
> ASSERT(xfs_sb_version_hasrmapbt(&sc->mp->m_sb));
>
> - for_each_xbitmap_block(fsbno, bmr, n, bitmap) {
> - ASSERT(sc->ip != NULL ||
> - XFS_FSB_TO_AGNO(sc->mp, fsbno) == sc->sa.agno);
> - trace_xrep_dispose_btree_extent(sc->mp,
> - XFS_FSB_TO_AGNO(sc->mp, fsbno),
> - XFS_FSB_TO_AGBNO(sc->mp, fsbno), 1);
> -
> - error = xrep_reap_block(sc, fsbno, oinfo, type, &deferred);
> - if (error)
> - break;
> - }
> -
> - if (error || deferred == 0)
> + error = xbitmap_iter_set_bits(bitmap, xrep_reap_block, &rb);
> + if (error || rb.deferred == 0)
> return error;
>
> if (sc->ip)
>
