> On Sep 25, 2020, at 10:32 AM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > On Fri, Sep 25, 2020 at 09:59:51AM -0400, Chuck Lever wrote: >> Thanks Bruce, for your time, attention, and comments! >> >>> On Sep 24, 2020, at 5:36 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: >>> >>> On Mon, Sep 21, 2020 at 02:10:49PM -0400, Chuck Lever wrote: >>>> As I've been working on various server bugs, I've been adding >>>> tracepoints that record NFS operation arguments. Here's an updated >>>> snapshot of this work for your review and comment. >>>> >>>> The idea here is to provide a degree of NFS traffic observability >>>> without needing network capture. Tracepoints are generally lighter- >>>> weight than full network capture, allowing effective capture-time >>>> data reduction: >>> >>> I do wonder when tracepoints seem to duplicate information you could get >>> from network traces, so thanks for taking the time to explain this. It >>> makes sense to me. >>> >>> The patches look fine. The only one I'm I'm on the fence about is the >>> last with the split up of the dispatch functions. I'll ask some >>> questions there.... >> >> To be clear to everyone, this series is still "preview". I expect >> more churn in these patches, thus I don't consider the series ready >> to be merged by any stretch. > > OK! > > One thing I was wondering about: how would you limit tracing to a single > client, say if you wanted to see all DELEGRETURNs from a single client? > I guess you'd probably turn on a tracepoint in the receive code, look > for your client's IP address, then mask the task id to match later > nfs-level tracepoints. Is there enough information in those tracepoints > (including network namespace) to uniquely identify a client? Client IP address information is in the RPC layer trace data. The DELEGRETURN trace record includes client ID. So maybe not as straightforward as it could be. > --b. > >>>> - One or a handful of these can be enabled at a time >>>> - Each tracepoint records much less data per operation than capture >>>> - Extra capture-time filtering can reduce data amount even further >>>> - Some of these operations are infrequent enough that their >>>> tracepoint could be enabled persistently without a significant >>>> performance impact (for example, for security auditing) >>>> >>>> The topic branch has been updated as well: >>>> >>>> git://git.linux-nfs.org/projects/cel/cel-2.6.git nfsd-more-tracepoints >>>> >>>> >>>> Changes since RFC: >>>> * s/SPDK/SPDX and corrected the spelling of Christoph's surname >>>> * Fixed a build error noticed by <lkp@xxxxxxxxx> >>>> * Introduced generic headers for VFS and NFS protocol display macros >>>> * nfsd4_compoundstatus now displays NFS4ERR codes symbolically >>>> * The svc_process tracepoint now displays the RPC procedure symbolically >>>> * NFSD dispatcher now displays procedure names and status codes symbolically >>>> * fh_verify tracepoint tentatively included; it adds a lot of noise, but perhaps not much value >>>> * Cleaned up the remaining PROC() macros in the server code >>>> * Removed trace_printk's that were introduced during the RFC series >>>> * Removed redundant nfsd4_close tracepoint >>>> >>>> --- >>>> >>>> Chuck Lever (27): >>>> NFS: Move generic FS show macros to global header >>>> NFS: Move NFS protocol display macros to global header >>>> NFSD: Add SPDX header for fs/nfsd/trace.c >>>> SUNRPC: Move the svc_xdr_recvfrom() tracepoint >>>> SUNRPC: Add svc_xdr_authenticate tracepoint >>>> lockd: Replace PROC() macro with open code >>>> NFSACL: Replace PROC() macro with open code >>>> SUNRPC: Make trace_svc_process() display the RPC procedure symbolically >>>> NFSD: Clean up the show_nf_may macro >>>> NFSD: Remove extra "0x" in tracepoint format specifier >>>> NFSD: Constify @fh argument of knfsd_fh_hash() >>>> NFSD: Add tracepoint in nfsd_setattr() >>>> NFSD: Add tracepoint for nfsd_access() >>>> NFSD: nfsd_compound_status tracepoint should record XID >>>> NFSD: Add client ID lifetime tracepoints >>>> NFSD: Add tracepoints to report NFSv4 session state >>>> NFSD: Add a tracepoint to report the current filehandle >>>> NFSD: Add GETATTR tracepoint >>>> NFSD: Add tracepoint in nfsd4_stateid_preprocess() >>>> NFSD: Add tracepoint to report arguments to NFSv4 OPEN >>>> NFSD: Add a tracepoint for DELEGRETURN >>>> NFSD: Add a lookup tracepoint >>>> NFSD: Add lock and locku tracepoints >>>> NFSD: Add tracepoints to record the result of TEST_STATEID and FREE_STATEID >>>> NFSD: Rename nfsd_ tracepoints to nfsd4_ >>>> NFSD: Add tracepoints in the NFS dispatcher >>>> NFSD: Replace dprintk callsites in fs/nfsd/nfsfh.c >>>> >>>> >>>> fs/lockd/svc4proc.c | 263 +++++++++-- >>>> fs/lockd/svcproc.c | 265 +++++++++-- >>>> fs/nfs/callback_xdr.c | 2 + >>>> fs/nfs/nfs4trace.h | 387 ++-------------- >>>> fs/nfs/nfstrace.h | 113 +---- >>>> fs/nfs/pnfs.h | 4 - >>>> fs/nfsd/nfs2acl.c | 79 +++- >>>> fs/nfsd/nfs3acl.c | 54 ++- >>>> fs/nfsd/nfs3proc.c | 25 + >>>> fs/nfsd/nfs4callback.c | 28 +- >>>> fs/nfsd/nfs4layouts.c | 16 +- >>>> fs/nfsd/nfs4proc.c | 43 +- >>>> fs/nfsd/nfs4state.c | 100 ++-- >>>> fs/nfsd/nfsd.h | 1 + >>>> fs/nfsd/nfsfh.c | 36 +- >>>> fs/nfsd/nfsfh.h | 7 +- >>>> fs/nfsd/nfsproc.c | 21 + >>>> fs/nfsd/nfssvc.c | 198 +++++--- >>>> fs/nfsd/trace.c | 1 + >>>> fs/nfsd/trace.h | 844 ++++++++++++++++++++++++++++++---- >>>> fs/nfsd/vfs.c | 18 +- >>>> fs/nfsd/xdr4.h | 3 +- >>>> include/linux/nfs4.h | 4 + >>>> include/linux/sunrpc/svc.h | 1 + >>>> include/trace/events/fs.h | 30 ++ >>>> include/trace/events/nfs.h | 511 ++++++++++++++++++++ >>>> include/trace/events/sunrpc.h | 33 +- >>>> include/uapi/linux/nfsacl.h | 2 + >>>> net/sunrpc/svc_xprt.c | 4 +- >>>> net/sunrpc/svcauth.c | 5 +- >>>> 30 files changed, 2187 insertions(+), 911 deletions(-) >>>> create mode 100644 include/trace/events/nfs.h >>>> >>>> -- >>>> Chuck Lever >> >> -- >> Chuck Lever -- Chuck Lever
