On Wed, Aug 21, 2024 at 08:38:30AM +0200, Christoph Hellwig wrote:
> Pass the old perag structure to the tagged loop helpers so that they can
> grab the old agno before releasing the reference. This removes the need
> to separately track the agno and the iterator macro, and thus also
> obsoletes the for_each_perag_tag syntactic sugar.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_icache.c | 69 +++++++++++++++++++++------------------------
> fs/xfs/xfs_trace.h | 4 +--
> 2 files changed, 34 insertions(+), 39 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index ac604640d36229..4d71fbfe71299a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -296,60 +296,63 @@ xfs_perag_clear_inode_tag(
> * Search from @first to find the next perag with the given tag set.
> */
> static struct xfs_perag *
> -xfs_perag_get_tag(
> +xfs_perag_get_next_tag(
> struct xfs_mount *mp,
> - xfs_agnumber_t first,
> + struct xfs_perag *pag,
> unsigned int tag)
> {
> - struct xfs_perag *pag;
> + unsigned long index = 0;
> int found;
>
> + if (pag) {
> + index = pag->pag_agno + 1;
> + xfs_perag_rele(pag);
> + }
Please update the comment to reflect the replacement of @first with
@pag, and be sure to note that one starts the iteration by passing in
@pag == null, like you did below.
--D
> +
> rcu_read_lock();
> found = radix_tree_gang_lookup_tag(&mp->m_perag_tree,
> - (void **)&pag, first, 1, tag);
> + (void **)&pag, index, 1, tag);
> if (found <= 0) {
> rcu_read_unlock();
> return NULL;
> }
> - trace_xfs_perag_get_tag(pag, _RET_IP_);
> + trace_xfs_perag_get_next_tag(pag, _RET_IP_);
> atomic_inc(&pag->pag_ref);
> rcu_read_unlock();
> return pag;
> }
>
> /*
> - * Search from @first to find the next perag with the given tag set.
> + * Find the next AG after @pag, or the first AG if @pag is NULL.
> */
> static struct xfs_perag *
> -xfs_perag_grab_tag(
> +xfs_perag_grab_next_tag(
> struct xfs_mount *mp,
> - xfs_agnumber_t first,
> + struct xfs_perag *pag,
> int tag)
> {
> - struct xfs_perag *pag;
> + unsigned long index = 0;
> int found;
>
> + if (pag) {
> + index = pag->pag_agno + 1;
> + xfs_perag_rele(pag);
> + }
> +
> rcu_read_lock();
> found = radix_tree_gang_lookup_tag(&mp->m_perag_tree,
> - (void **)&pag, first, 1, tag);
> + (void **)&pag, index, 1, tag);
> if (found <= 0) {
> rcu_read_unlock();
> return NULL;
> }
> - trace_xfs_perag_grab_tag(pag, _RET_IP_);
> + trace_xfs_perag_grab_next_tag(pag, _RET_IP_);
> if (!atomic_inc_not_zero(&pag->pag_active_ref))
> pag = NULL;
> rcu_read_unlock();
> return pag;
> }
>
> -#define for_each_perag_tag(mp, agno, pag, tag) \
> - for ((agno) = 0, (pag) = xfs_perag_grab_tag((mp), 0, (tag)); \
> - (pag) != NULL; \
> - (agno) = (pag)->pag_agno + 1, \
> - xfs_perag_rele(pag), \
> - (pag) = xfs_perag_grab_tag((mp), (agno), (tag)))
> -
> /*
> * When we recycle a reclaimable inode, we need to re-initialise the VFS inode
> * part of the structure. This is made more complex by the fact we store
> @@ -1077,15 +1080,11 @@ long
> xfs_reclaim_inodes_count(
> struct xfs_mount *mp)
> {
> - struct xfs_perag *pag;
> - xfs_agnumber_t ag = 0;
> + struct xfs_perag *pag = NULL;
> long reclaimable = 0;
>
> - while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
> - ag = pag->pag_agno + 1;
> + while ((pag = xfs_perag_get_next_tag(mp, pag, XFS_ICI_RECLAIM_TAG)))
> reclaimable += pag->pag_ici_reclaimable;
> - xfs_perag_put(pag);
> - }
> return reclaimable;
> }
>
> @@ -1427,14 +1426,13 @@ void
> xfs_blockgc_start(
> struct xfs_mount *mp)
> {
> - struct xfs_perag *pag;
> - xfs_agnumber_t agno;
> + struct xfs_perag *pag = NULL;
>
> if (xfs_set_blockgc_enabled(mp))
> return;
>
> trace_xfs_blockgc_start(mp, __return_address);
> - for_each_perag_tag(mp, agno, pag, XFS_ICI_BLOCKGC_TAG)
> + while ((pag = xfs_perag_grab_next_tag(mp, pag, XFS_ICI_BLOCKGC_TAG)))
> xfs_blockgc_queue(pag);
> }
>
> @@ -1550,21 +1548,19 @@ int
> xfs_blockgc_flush_all(
> struct xfs_mount *mp)
> {
> - struct xfs_perag *pag;
> - xfs_agnumber_t agno;
> + struct xfs_perag *pag = NULL;
>
> trace_xfs_blockgc_flush_all(mp, __return_address);
>
> /*
> - * For each blockgc worker, move its queue time up to now. If it
> - * wasn't queued, it will not be requeued. Then flush whatever's
> - * left.
> + * For each blockgc worker, move its queue time up to now. If it wasn't
> + * queued, it will not be requeued. Then flush whatever is left.
> */
> - for_each_perag_tag(mp, agno, pag, XFS_ICI_BLOCKGC_TAG)
> + while ((pag = xfs_perag_grab_next_tag(mp, pag, XFS_ICI_BLOCKGC_TAG)))
> mod_delayed_work(pag->pag_mount->m_blockgc_wq,
> &pag->pag_blockgc_work, 0);
>
> - for_each_perag_tag(mp, agno, pag, XFS_ICI_BLOCKGC_TAG)
> + while ((pag = xfs_perag_grab_next_tag(mp, pag, XFS_ICI_BLOCKGC_TAG)))
> flush_delayed_work(&pag->pag_blockgc_work);
>
> return xfs_inodegc_flush(mp);
> @@ -1810,12 +1806,11 @@ xfs_icwalk(
> enum xfs_icwalk_goal goal,
> struct xfs_icwalk *icw)
> {
> - struct xfs_perag *pag;
> + struct xfs_perag *pag = NULL;
> int error = 0;
> int last_error = 0;
> - xfs_agnumber_t agno;
>
> - for_each_perag_tag(mp, agno, pag, goal) {
> + while ((pag = xfs_perag_grab_next_tag(mp, pag, goal))) {
> error = xfs_icwalk_ag(pag, goal, icw);
> if (error) {
> last_error = error;
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 180ce697305a92..002d012ebd83cb 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -210,11 +210,11 @@ DEFINE_EVENT(xfs_perag_class, name, \
> TP_PROTO(struct xfs_perag *pag, unsigned long caller_ip), \
> TP_ARGS(pag, caller_ip))
> DEFINE_PERAG_REF_EVENT(xfs_perag_get);
> -DEFINE_PERAG_REF_EVENT(xfs_perag_get_tag);
> +DEFINE_PERAG_REF_EVENT(xfs_perag_get_next_tag);
> DEFINE_PERAG_REF_EVENT(xfs_perag_hold);
> DEFINE_PERAG_REF_EVENT(xfs_perag_put);
> DEFINE_PERAG_REF_EVENT(xfs_perag_grab);
> -DEFINE_PERAG_REF_EVENT(xfs_perag_grab_tag);
> +DEFINE_PERAG_REF_EVENT(xfs_perag_grab_next_tag);
> DEFINE_PERAG_REF_EVENT(xfs_perag_rele);
> DEFINE_PERAG_REF_EVENT(xfs_perag_set_inode_tag);
> DEFINE_PERAG_REF_EVENT(xfs_perag_clear_inode_tag);
> --
> 2.43.0
>
>
