On 3/31/20 5:09 AM, xiakaixu1987@xxxxxxxxx wrote:
> From: Kaixu Xia <kaixuxia@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>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)
>
