On Thu, 2023-01-19 at 09:44 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> So that they all output the same information in the traces to make
> debugging refcount issues easier.
>
> This means that all the lookup/drop functions no longer need to use
> the full memory barrier atomic operations (atomic*_return()) so
> will have less overhead when tracing is off. The set/clear tag
> tracepoints no longer abuse the reference count to pass the tag -
> the tag being cleared is obvious from the _RET_IP_ that is recorded
> in the trace point.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> fs/xfs/libxfs/xfs_ag.c | 25 +++++++++----------------
> fs/xfs/xfs_icache.c | 4 ++--
> fs/xfs/xfs_trace.h | 21 +++++++++++----------
> 3 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 46e25c682bf4..7cff61875340 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -44,16 +44,15 @@ xfs_perag_get(
> xfs_agnumber_t agno)
> {
> struct xfs_perag *pag;
> - int ref = 0;
>
> rcu_read_lock();
> pag = radix_tree_lookup(&mp->m_perag_tree, agno);
> if (pag) {
> + trace_xfs_perag_get(pag, _RET_IP_);
> ASSERT(atomic_read(&pag->pag_ref) >= 0);
> - ref = atomic_inc_return(&pag->pag_ref);
> + atomic_inc(&pag->pag_ref);
> }
> rcu_read_unlock();
> - trace_xfs_perag_get(mp, agno, ref, _RET_IP_);
> return pag;
> }
>
> @@ -68,7 +67,6 @@ xfs_perag_get_tag(
> {
> struct xfs_perag *pag;
> int found;
> - int ref;
>
> rcu_read_lock();
> found = radix_tree_gang_lookup_tag(&mp->m_perag_tree,
> @@ -77,9 +75,9 @@ xfs_perag_get_tag(
> rcu_read_unlock();
> return NULL;
> }
> - ref = atomic_inc_return(&pag->pag_ref);
> + trace_xfs_perag_get_tag(pag, _RET_IP_);
> + atomic_inc(&pag->pag_ref);
> rcu_read_unlock();
> - trace_xfs_perag_get_tag(mp, pag->pag_agno, ref, _RET_IP_);
> return pag;
> }
>
> @@ -87,11 +85,9 @@ void
> xfs_perag_put(
> struct xfs_perag *pag)
> {
> - int ref;
> -
> + trace_xfs_perag_put(pag, _RET_IP_);
> ASSERT(atomic_read(&pag->pag_ref) > 0);
> - ref = atomic_dec_return(&pag->pag_ref);
> - trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref,
> _RET_IP_);
> + atomic_dec(&pag->pag_ref);
> }
>
> /*
> @@ -110,8 +106,7 @@ xfs_perag_grab(
> rcu_read_lock();
> pag = radix_tree_lookup(&mp->m_perag_tree, agno);
> if (pag) {
> - trace_xfs_perag_grab(mp, pag->pag_agno,
> - atomic_read(&pag->pag_active_ref),
> _RET_IP_);
> + trace_xfs_perag_grab(pag, _RET_IP_);
> if (!atomic_inc_not_zero(&pag->pag_active_ref))
> pag = NULL;
> }
> @@ -138,8 +133,7 @@ xfs_perag_grab_tag(
> rcu_read_unlock();
> return NULL;
> }
> - trace_xfs_perag_grab_tag(mp, pag->pag_agno,
> - atomic_read(&pag->pag_active_ref), _RET_IP_);
> + trace_xfs_perag_grab_tag(pag, _RET_IP_);
> if (!atomic_inc_not_zero(&pag->pag_active_ref))
> pag = NULL;
> rcu_read_unlock();
> @@ -150,8 +144,7 @@ void
> xfs_perag_rele(
> struct xfs_perag *pag)
> {
> - trace_xfs_perag_rele(pag->pag_mount, pag->pag_agno,
> - atomic_read(&pag->pag_active_ref), _RET_IP_);
> + trace_xfs_perag_rele(pag, _RET_IP_);
> if (atomic_dec_and_test(&pag->pag_active_ref))
> wake_up(&pag->pag_active_wq);
> }
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 0f4a014dded3..8b2823d85a68 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -255,7 +255,7 @@ xfs_perag_set_inode_tag(
> break;
> }
>
> - trace_xfs_perag_set_inode_tag(mp, pag->pag_agno, tag,
> _RET_IP_);
> + trace_xfs_perag_set_inode_tag(pag, _RET_IP_);
> }
>
> /* Clear a tag on both the AG incore inode tree and the AG radix
> tree. */
> @@ -289,7 +289,7 @@ xfs_perag_clear_inode_tag(
> radix_tree_tag_clear(&mp->m_perag_tree, pag->pag_agno, tag);
> spin_unlock(&mp->m_perag_lock);
>
> - trace_xfs_perag_clear_inode_tag(mp, pag->pag_agno, tag,
> _RET_IP_);
> + trace_xfs_perag_clear_inode_tag(pag, _RET_IP_);
> }
>
> /*
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index f0b62054ea68..c921e9a5256d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -159,33 +159,34 @@ TRACE_EVENT(xlog_intent_recovery_failed,
> );
>
> DECLARE_EVENT_CLASS(xfs_perag_class,
> - TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, int
> refcount,
> - unsigned long caller_ip),
> - TP_ARGS(mp, agno, refcount, caller_ip),
> + TP_PROTO(struct xfs_perag *pag, unsigned long caller_ip),
> + TP_ARGS(pag, caller_ip),
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(xfs_agnumber_t, agno)
> __field(int, refcount)
> + __field(int, active_refcount)
> __field(unsigned long, caller_ip)
> ),
> TP_fast_assign(
> - __entry->dev = mp->m_super->s_dev;
> - __entry->agno = agno;
> - __entry->refcount = refcount;
> + __entry->dev = pag->pag_mount->m_super->s_dev;
> + __entry->agno = pag->pag_agno;
> + __entry->refcount = atomic_read(&pag->pag_ref);
> + __entry->active_refcount = atomic_read(&pag-
> >pag_active_ref);
> __entry->caller_ip = caller_ip;
> ),
> - TP_printk("dev %d:%d agno 0x%x refcount %d caller %pS",
> + TP_printk("dev %d:%d agno 0x%x passive refs %d active refs %d
> caller %pS",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->agno,
> __entry->refcount,
> + __entry->active_refcount,
> (char *)__entry->caller_ip)
> );
>
> #define DEFINE_PERAG_REF_EVENT(name) \
> DEFINE_EVENT(xfs_perag_class, name, \
> - TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, int
> refcount, \
> - unsigned long
> caller_ip), \
> - TP_ARGS(mp, agno, refcount, caller_ip))
> + 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_put);
