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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
Subject: Re: [PATCH v2 00/27] NFSD operation monitoring tracepoints
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200924213617.GA12407@fieldses.org>
Date:   Fri, 25 Sep 2020 09:59:51 -0400
Cc:     Bill Baker <Bill.Baker@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <945A7DE6-909D-4177-852F-F80EF7DFE6B3@oracle.com>
References: <160071167664.1468.1365570508917640511.stgit@klimt.1015granger.net>
 <20200924213617.GA12407@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Precedence: bulk

Thanks Bruce, for your time, attention, and comments!

> On Sep 24, 2020, at 5:36 PM, J. Bruce Fields <bfields@fieldses.org> =
wrote:
>=20
> 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.
>>=20
>> 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:
>=20
> 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.
>=20
> 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.


> --b.
>=20
>>=20
>> - 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)
>>=20
>> The topic branch has been updated as well:
>>=20
>> git://git.linux-nfs.org/projects/cel/cel-2.6.git =
nfsd-more-tracepoints
>>=20
>>=20
>> 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
>>=20
>> ---
>>=20
>> 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
>>=20
>>=20
>> 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
>>=20
>> --
>> Chuck Lever

--
Chuck Lever