On 2020/4/1 5:02, Eric Sandeen wrote:
> 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)
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@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)
>>
--
kaixuxia
