Re: [PATCH 1/3] NFSD: rename and update nfsd_read_vector_dio trace event to nfsd_analyze_dio

[Chuck Lever <chuck.lever@oracle.com>]

On 7/31/25 1:04 PM, Mike Snitzer wrote:
> On Thu, Jul 31, 2025 at 11:31:29AM -0400, Jeff Layton wrote:
>> On Wed, 2025-07-30 at 19:05 -0400, Mike Snitzer wrote:
>>> From: Mike Snitzer <snitzer@hammerspace.com>
>>>
>>> Rename nfsd_read_vector_dio trace event to nfsd_analyze_dio and update
>>> it so that it provides useful tracing for both READ and WRITE.  This
>>> prepares for nfsd_vfs_write() to also make use of it when handling
>>> misaligned WRITEs.
>>>
>>> Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
>>> ---
>>>  fs/nfsd/trace.h | 37 ++++++++++++++++++++++++-------------
>>>  fs/nfsd/vfs.c   | 11 ++++++-----
>>>  2 files changed, 30 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>>> index 55055482f8a8..51b47fd041a8 100644
>>> --- a/fs/nfsd/trace.h
>>> +++ b/fs/nfsd/trace.h
>>> @@ -473,41 +473,52 @@ DEFINE_NFSD_IO_EVENT(write_done);
>>>  DEFINE_NFSD_IO_EVENT(commit_start);
>>>  DEFINE_NFSD_IO_EVENT(commit_done);
>>>  
>>> -TRACE_EVENT(nfsd_read_vector_dio,
>>> +TRACE_EVENT(nfsd_analyze_dio,
>>>  	TP_PROTO(struct svc_rqst *rqstp,
>>>  		 struct svc_fh	*fhp,
>>> +		 u32		rw,
>>
>> I would do this a bit differently. You're hardcoding READ and WRITE
>> into both tracepoints. I would turn this trace event into a class a'la
>> DECLARE_EVENT_CLASS, and then just define two different tracepoints
>> (maybe trace_nfsd_analyze_read/write_dio). Then you can just drop the
>> above u32 field, and it'll still be evident whether it's a read or
>> write in the log.
> 
> Seems overkill to me, and also forces the need for user to enable
> discrete tracepoints.

Users can enable trace points with globs, so that's not a big deal in
most cases. What is more important is that, when you define individual
trace points, you can enable tracing for only reads or only writes.

The common trope is to define a class, as Jeff suggested. The I/O
direction is then recorded in the trace point and the extra field
is no longer necessary.


-- 
Chuck Lever