On Tue, Sep 10, 2024 at 07:28:45AM +0300, Christoph Hellwig wrote:
> There is no good reason to have two different routines for freeing perag
> structures for the unmount and error cases. Add two arguments to specify
> the range of AGs to free to xfs_free_perag, and use that to replace
> xfs_free_unused_perag_range.
>
> The addition RCU grace period for the error case is harmless, and the
> extra check for the AG to actually exist is not required now that the
> callers pass the exact known allocated range.
And I guess the extra xfs_perag_rele is ok because xfs_initialize_perag
sets it to 1 and the _unused free function didn't care what the active
refcount was?
If the answer is yes then I've understood this enough to say
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_ag.c | 40 ++++++++++------------------------------
> fs/xfs/libxfs/xfs_ag.h | 5 ++---
> fs/xfs/xfs_fsops.c | 2 +-
> fs/xfs/xfs_mount.c | 5 ++---
> 4 files changed, 15 insertions(+), 37 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 5186735da5d45a..3f695100d7ab58 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -185,17 +185,20 @@ xfs_initialize_perag_data(
> }
>
> /*
> - * Free up the per-ag resources associated with the mount structure.
> + * Free up the per-ag resources within the specified AG range.
> */
> void
> -xfs_free_perag(
> - struct xfs_mount *mp)
> +xfs_free_perag_range(
> + struct xfs_mount *mp,
> + xfs_agnumber_t first_agno,
> + xfs_agnumber_t end_agno)
> +
> {
> - struct xfs_perag *pag;
> xfs_agnumber_t agno;
>
> - for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> - pag = xa_erase(&mp->m_perags, agno);
> + for (agno = first_agno; agno < end_agno; agno++) {
> + struct xfs_perag *pag = xa_erase(&mp->m_perags, agno);
> +
> ASSERT(pag);
> XFS_IS_CORRUPT(pag->pag_mount, atomic_read(&pag->pag_ref) != 0);
> xfs_defer_drain_free(&pag->pag_intents_drain);
> @@ -270,29 +273,6 @@ xfs_agino_range(
> return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last);
> }
>
> -/*
> - * Free perag within the specified AG range, it is only used to free unused
> - * perags under the error handling path.
> - */
> -void
> -xfs_free_unused_perag_range(
> - struct xfs_mount *mp,
> - xfs_agnumber_t agstart,
> - xfs_agnumber_t agend)
> -{
> - struct xfs_perag *pag;
> - xfs_agnumber_t index;
> -
> - for (index = agstart; index < agend; index++) {
> - pag = xa_erase(&mp->m_perags, index);
> - if (!pag)
> - break;
> - xfs_buf_cache_destroy(&pag->pag_bcache);
> - xfs_defer_drain_free(&pag->pag_intents_drain);
> - kfree(pag);
> - }
> -}
> -
> int
> xfs_initialize_perag(
> struct xfs_mount *mp,
> @@ -369,7 +349,7 @@ xfs_initialize_perag(
> out_free_pag:
> kfree(pag);
> out_unwind_new_pags:
> - xfs_free_unused_perag_range(mp, old_agcount, index);
> + xfs_free_perag_range(mp, old_agcount, index);
> return error;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 69fc31e7b84728..6e68d6a3161a0f 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -144,13 +144,12 @@ __XFS_AG_OPSTATE(prefers_metadata, PREFERS_METADATA)
> __XFS_AG_OPSTATE(allows_inodes, ALLOWS_INODES)
> __XFS_AG_OPSTATE(agfl_needs_reset, AGFL_NEEDS_RESET)
>
> -void xfs_free_unused_perag_range(struct xfs_mount *mp, xfs_agnumber_t agstart,
> - xfs_agnumber_t agend);
> int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t old_agcount,
> xfs_agnumber_t agcount, xfs_rfsblock_t dcount,
> xfs_agnumber_t *maxagi);
> +void xfs_free_perag_range(struct xfs_mount *mp, xfs_agnumber_t first_agno,
> + xfs_agnumber_t end_agno);
> int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno);
> -void xfs_free_perag(struct xfs_mount *mp);
>
> /* Passive AG references */
> struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index de2bf0594cb474..b247d895c276d2 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -229,7 +229,7 @@ xfs_growfs_data_private(
> xfs_trans_cancel(tp);
> out_free_unused_perag:
> if (nagcount > oagcount)
> - xfs_free_unused_perag_range(mp, oagcount, nagcount);
> + xfs_free_perag_range(mp, oagcount, nagcount);
> return error;
> }
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 0f4f56a7f02d9a..6671ee3849c239 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1044,7 +1044,7 @@ xfs_mountfs(
> xfs_buftarg_drain(mp->m_logdev_targp);
> xfs_buftarg_drain(mp->m_ddev_targp);
> out_free_perag:
> - xfs_free_perag(mp);
> + xfs_free_perag_range(mp, 0, mp->m_sb.sb_agcount);
> out_free_dir:
> xfs_da_unmount(mp);
> out_remove_uuid:
> @@ -1125,8 +1125,7 @@ xfs_unmountfs(
> xfs_errortag_clearall(mp);
> #endif
> shrinker_free(mp->m_inodegc_shrinker);
> - xfs_free_perag(mp);
> -
> + xfs_free_perag_range(mp, 0, mp->m_sb.sb_agcount);
> xfs_errortag_del(mp);
> xfs_error_sysfs_del(mp);
> xchk_stats_unregister(mp->m_scrub_stats);
> --
> 2.45.2
>
>
