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 > >