Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 25 Sep 2020 10:32:18 -0400
From:   Bruce Fields <bfields@fieldses.org>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Bill Baker <Bill.Baker@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 00/27] NFSD operation monitoring tracepoints
Message-ID: <20200925143218.GD1096@fieldses.org>
References: <160071167664.1468.1365570508917640511.stgit@klimt.1015granger.net>
 <20200924213617.GA12407@fieldses.org>
 <945A7DE6-909D-4177-852F-F80EF7DFE6B3@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <945A7DE6-909D-4177-852F-F80EF7DFE6B3@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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@fieldses.org> 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?

--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@intel.com>
> >> * 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
> 
>