| Message ID | 1585649387-22890-1-git-send-email-kaixuxia@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: move trace_xfs_dquot_dqalloc() to proper place | expand |
On 3/31/20 5:09 AM, xiakaixu1987@gmail.com wrote: > From: Kaixu Xia <kaixuxia@tencent.com> > > The trace event xfs_dquot_dqalloc does not depend on the > value uq, so move it to proper place. Long ago, the tracing did depend on uq (see 0b1b213fcf3a): if (uq) - xfs_dqtrace_entry_ino(uq, "DQALLOC", ip); + trace_xfs_dquot_dqalloc(ip); and I agree that only tracing the inode if user quota is set seems wrong. (FWIW, the old tracepoint traced much more than just the inode, it got all the information from the quota) However, I'm not completely sure about moving the tracepoint higher in the function; now it tells us that we entered the function but not if we successfully allocated the quota? So my only concern is that it changes the meaning of this tracepoint a little bit, from "we completed the function" more to "we entered the function" Not sure how much that matters in practice. But that makes this change do 2 things in 1 patch: 1) don't depend on uq, and 2) change when we trace I'd rather see: [PATCH] xfs: trace quota allocations for all quota types - if (uq) - trace_xfs_dquot_dqalloc(ip); + trace_xfs_dquot_dqalloc(ip); and if there's a good reason to /move/ the tracepoint as well, do that separately. -Eric > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>m> > --- > fs/xfs/xfs_qm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c > index 0b09096..5569af9 100644 > --- a/fs/xfs/xfs_qm.c > +++ b/fs/xfs/xfs_qm.c > @@ -1631,6 +1631,8 @@ struct xfs_qm_isolate { > if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp)) > return 0; > > + trace_xfs_dquot_dqalloc(ip); > + > lockflags = XFS_ILOCK_EXCL; > xfs_ilock(ip, lockflags); > > @@ -1714,8 +1716,6 @@ struct xfs_qm_isolate { > pq = xfs_qm_dqhold(ip->i_pdquot); > } > } > - if (uq) > - trace_xfs_dquot_dqalloc(ip); > > xfs_iunlock(ip, lockflags); > if (O_udqpp) >
On 2020/4/1 5:02, Eric Sandeen wrote: > On 3/31/20 5:09 AM, xiakaixu1987@gmail.com wrote: >> From: Kaixu Xia <kaixuxia@tencent.com> >> >> The trace event xfs_dquot_dqalloc does not depend on the >> value uq, so move it to proper place. > > Long ago, the tracing did depend on uq (see 0b1b213fcf3a): > > if (uq) > - xfs_dqtrace_entry_ino(uq, "DQALLOC", ip); > + trace_xfs_dquot_dqalloc(ip); > > and I agree that only tracing the inode if user quota is set seems wrong. > > (FWIW, the old tracepoint traced much more than just the inode, it got all > the information from the quota) Yeah, the original tracepoint traced more data, and now it is just the inode event. > > However, I'm not completely sure about moving the tracepoint higher in the function; > now it tells us that we entered the function but not if we successfully allocated > the quota? > > So my only concern is that it changes the meaning of this tracepoint a little bit, > from "we completed the function" more to "we entered the function" > > Not sure how much that matters in practice. > > But that makes this change do 2 things in 1 patch: > > 1) don't depend on uq, and > 2) change when we trace > > I'd rather see: > > [PATCH] xfs: trace quota allocations for all quota types > > - if (uq) > - trace_xfs_dquot_dqalloc(ip); > + trace_xfs_dquot_dqalloc(ip); > Make more sense, thanks for your suggestion, will follow it in next version. > and if there's a good reason to /move/ the tracepoint as well, do that separately. > > -Eric > >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>m> >> --- >> fs/xfs/xfs_qm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c >> index 0b09096..5569af9 100644 >> --- a/fs/xfs/xfs_qm.c >> +++ b/fs/xfs/xfs_qm.c >> @@ -1631,6 +1631,8 @@ struct xfs_qm_isolate { >> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp)) >> return 0; >> >> + trace_xfs_dquot_dqalloc(ip); >> + >> lockflags = XFS_ILOCK_EXCL; >> xfs_ilock(ip, lockflags); >> >> @@ -1714,8 +1716,6 @@ struct xfs_qm_isolate { >> pq = xfs_qm_dqhold(ip->i_pdquot); >> } >> } >> - if (uq) >> - trace_xfs_dquot_dqalloc(ip); >> >> xfs_iunlock(ip, lockflags); >> if (O_udqpp) >>
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c index 0b09096..5569af9 100644 --- a/fs/xfs/xfs_qm.c +++ b/fs/xfs/xfs_qm.c @@ -1631,6 +1631,8 @@ struct xfs_qm_isolate { if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp)) return 0; + trace_xfs_dquot_dqalloc(ip); + lockflags = XFS_ILOCK_EXCL; xfs_ilock(ip, lockflags); @@ -1714,8 +1716,6 @@ struct xfs_qm_isolate { pq = xfs_qm_dqhold(ip->i_pdquot); } } - if (uq) - trace_xfs_dquot_dqalloc(ip); xfs_iunlock(ip, lockflags); if (O_udqpp)