On Sat, Jun 11, 2022 at 11:26:14AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> xfs_alloc_read_agf() initialises the perag if it hasn't been done
> yet, so it makes sense to pass it the perag rather than pull a
> reference from the buffer. This allows callers to be per-ag centric
> rather than passing mount/agno pairs everywhere.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_ag.c | 19 +++++++--------
> fs/xfs/libxfs/xfs_ag_resv.c | 2 +-
> fs/xfs/libxfs/xfs_alloc.c | 30 ++++++++++-------------
> fs/xfs/libxfs/xfs_alloc.h | 13 ++--------
> fs/xfs/libxfs/xfs_bmap.c | 2 +-
> fs/xfs/libxfs/xfs_ialloc.c | 2 +-
> fs/xfs/libxfs/xfs_refcount.c | 6 ++---
> fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
> fs/xfs/libxfs/xfs_rmap_btree.c | 2 +-
> fs/xfs/scrub/agheader_repair.c | 6 ++---
> fs/xfs/scrub/bmap.c | 2 +-
> fs/xfs/scrub/common.c | 2 +-
> fs/xfs/scrub/fscounters.c | 2 +-
> fs/xfs/scrub/repair.c | 5 ++--
> fs/xfs/xfs_discard.c | 2 +-
> fs/xfs/xfs_extfree_item.c | 6 ++++-
> fs/xfs/xfs_filestream.c | 2 +-
> fs/xfs/xfs_fsmap.c | 3 +--
> fs/xfs/xfs_reflink.c | 38 +++++++++++++++++-------------
> fs/xfs/xfs_reflink.h | 3 ---
> 20 files changed, 68 insertions(+), 81 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 734ef170936e..c1a1c9f414c3 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -120,16 +120,13 @@ xfs_initialize_perag_data(
>
> for (index = 0; index < agcount; index++) {
> /*
> - * read the agf, then the agi. This gets us
> - * all the information we need and populates the
> - * per-ag structures for us.
> + * Read the AGF and AGI buffers to populate the per-ag
> + * structures for us.
> */
> - error = xfs_alloc_read_agf(mp, NULL, index, 0, NULL);
> - if (error)
> - return error;
> -
> pag = xfs_perag_get(mp, index);
> - error = xfs_ialloc_read_agi(pag, NULL, NULL);
> + error = xfs_alloc_read_agf(pag, NULL, 0, NULL);
> + if (!error)
> + error = xfs_ialloc_read_agi(pag, NULL, NULL);
> if (error) {
> xfs_perag_put(pag);
> return error;
> @@ -792,7 +789,7 @@ xfs_ag_shrink_space(
>
> agi = agibp->b_addr;
>
> - error = xfs_alloc_read_agf(mp, *tpp, pag->pag_agno, 0, &agfbp);
> + error = xfs_alloc_read_agf(pag, *tpp, 0, &agfbp);
> if (error)
> return error;
>
> @@ -909,7 +906,7 @@ xfs_ag_extend_space(
> /*
> * Change agf length.
> */
> - error = xfs_alloc_read_agf(pag->pag_mount, tp, pag->pag_agno, 0, &bp);
> + error = xfs_alloc_read_agf(pag, tp, 0, &bp);
> if (error)
> return error;
>
> @@ -952,7 +949,7 @@ xfs_ag_get_geometry(
> error = xfs_ialloc_read_agi(pag, NULL, &agi_bp);
> if (error)
> return error;
> - error = xfs_alloc_read_agf(pag->pag_mount, NULL, pag->pag_agno, 0, &agf_bp);
> + error = xfs_alloc_read_agf(pag, NULL, 0, &agf_bp);
> if (error)
> goto out_agi;
>
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index ce28bf8f72dc..5af123d13a63 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -322,7 +322,7 @@ xfs_ag_resv_init(
> * address.
> */
> if (has_resv) {
> - error2 = xfs_alloc_read_agf(mp, tp, pag->pag_agno, 0, NULL);
> + error2 = xfs_alloc_read_agf(pag, tp, 0, NULL);
> if (error2)
> return error2;
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index f7853ab7b962..5d6ca86c4882 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2609,7 +2609,7 @@ xfs_alloc_fix_freelist(
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>
> if (!pag->pagf_init) {
> - error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> + error = xfs_alloc_read_agf(pag, tp, flags, &agbp);
> if (error) {
> /* Couldn't lock the AGF so skip this AG. */
> if (error == -EAGAIN)
> @@ -2639,7 +2639,7 @@ xfs_alloc_fix_freelist(
> * Can fail if we're not blocking on locks, and it's held.
> */
> if (!agbp) {
> - error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> + error = xfs_alloc_read_agf(pag, tp, flags, &agbp);
> if (error) {
> /* Couldn't lock the AGF so skip this AG. */
> if (error == -EAGAIN)
> @@ -3080,34 +3080,30 @@ xfs_read_agf(
> * perag structure if necessary. If the caller provides @agfbpp, then return the
> * locked buffer to the caller, otherwise free it.
> */
> -int /* error */
> +int
> xfs_alloc_read_agf(
> - struct xfs_mount *mp, /* mount point structure */
> - struct xfs_trans *tp, /* transaction pointer */
> - xfs_agnumber_t agno, /* allocation group number */
> - int flags, /* XFS_ALLOC_FLAG_... */
> + struct xfs_perag *pag,
> + struct xfs_trans *tp,
> + int flags,
> struct xfs_buf **agfbpp)
> {
> struct xfs_buf *agfbp;
> - struct xfs_agf *agf; /* ag freelist header */
> - struct xfs_perag *pag; /* per allocation group data */
> + struct xfs_agf *agf;
> int error;
> int allocbt_blks;
>
> - trace_xfs_alloc_read_agf(mp, agno);
> + trace_xfs_alloc_read_agf(pag->pag_mount, pag->pag_agno);
>
> /* We don't support trylock when freeing. */
> ASSERT((flags & (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK)) !=
> (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK));
> - ASSERT(agno != NULLAGNUMBER);
> - error = xfs_read_agf(mp, tp, agno,
> + error = xfs_read_agf(pag->pag_mount, tp, pag->pag_agno,
> (flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
> &agfbp);
> if (error)
> return error;
>
> agf = agfbp->b_addr;
> - pag = agfbp->b_pag;
> if (!pag->pagf_init) {
> pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
> pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
> @@ -3121,7 +3117,7 @@ xfs_alloc_read_agf(
> be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAPi]);
> pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
> pag->pagf_init = 1;
> - pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> + pag->pagf_agflreset = xfs_agfl_needs_reset(pag->pag_mount, agf);
>
> /*
> * Update the in-core allocbt counter. Filter out the rmapbt
> @@ -3131,13 +3127,13 @@ xfs_alloc_read_agf(
> * counter only tracks non-root blocks.
> */
> allocbt_blks = pag->pagf_btreeblks;
> - if (xfs_has_rmapbt(mp))
> + if (xfs_has_rmapbt(pag->pag_mount))
> allocbt_blks -= be32_to_cpu(agf->agf_rmap_blocks) - 1;
> if (allocbt_blks > 0)
> - atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> + atomic64_add(allocbt_blks, &pag->pag_mount->m_allocbt_blks);
Overly long line here. I think in general this function would benefit
from a local xfs_mount *mp variable anyway.
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index bea65f2fe657..65c5dfe17ecf 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -16,9 +16,6 @@ static inline bool xfs_is_cow_inode(struct xfs_inode *ip)
> return xfs_is_reflink_inode(ip) || xfs_is_always_cow_inode(ip);
> }
>
> -extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp,
> - xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen,
> - xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);
Dropping this extern seems unrelated, and should move into a separate
patch together with actually marking it static.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
