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;
MIME-Version: 1.0
References: <20211018220314.85115-1-olga.kornievskaia@gmail.com>
 <20211018220314.85115-6-olga.kornievskaia@gmail.com> <9B0CDAB8-670F-4D34-815D-49AC44B4DFC6@oracle.com>
In-Reply-To: <9B0CDAB8-670F-4D34-815D-49AC44B4DFC6@oracle.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Tue, 19 Oct 2021 12:01:01 -0400
Message-ID: <CAN-5tyH90dMe2QyLXVjZBZU_BV34a1=xc_NhwxDQmt1MbcnXQQ@mail.gmail.com>
Subject: Re: [PATCH 5/7] NFSv4.2 add tracepoint to CB_OFFLOAD
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Steve Dickson <steved@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 19, 2021 at 11:17 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 18, 2021, at 6:03 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Add a tracepoint to the CB_OFFLOAD operation.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> > fs/nfs/callback_proc.c |  3 +++
> > fs/nfs/nfs4trace.h     | 50 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> > index ed9d580826f5..09c5b1cb3e07 100644
> > --- a/fs/nfs/callback_proc.c
> > +++ b/fs/nfs/callback_proc.c
> > @@ -739,6 +739,9 @@ __be32 nfs4_callback_offload(void *data, void *dummy,
> >               kfree(copy);
> >       spin_unlock(&cps->clp->cl_lock);
> >
> > +     trace_nfs4_cb_offload(&args->coa_fh, &args->coa_stateid,
> > +                     args->wr_count, args->error,
> > +                     args->wr_writeverf.committed);
> >       return 0;
> > }
> > #endif /* CONFIG_NFS_V4_2 */
> > diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
> > index cc6537a20ebe..33f52d486528 100644
> > --- a/fs/nfs/nfs4trace.h
> > +++ b/fs/nfs/nfs4trace.h
> > @@ -2714,6 +2714,56 @@ TRACE_EVENT(nfs4_clone,
> >               )
> > );
> >
> > +#define show_write_mode(how)                 \
> > +        __print_symbolic(how,                        \
> > +                { NFS_UNSTABLE, "UNSTABLE" },        \
> > +                { NFS_DATA_SYNC, "DATA_SYNC" },      \
> > +             { NFS_FILE_SYNC, "FILE_SYNC"})
>
> Is there no way to reuse fs/nfs/nfstrace.h::nfs_show_stable() ?
>
> Btw, I have patches that move some NFS trace infrastructure
> into include/trace/events so that it can be shared between the
> NFS client and server trace subsystems. They might be useful
> here too.
>
> The generic FS macros are moved in this commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-more-tracepoints&id=495731e1332c7e26af1e04a88eb65e3c08dfbf53
>
> Some NFS macros are moved in this commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-more-tracepoints&id=24763f8889e0a18a8d06ddcd05bac06a7d043515
>
> Additional macros are introduced later in that same branch.
>
> I don't have any opinion about whether these should be
> applied before your patches or after them.

It sounds like, if there was already a show_nfs_stable_how() that I
could call then I don't need to define what I'm defining? So if I
apply your patches and and then my patches on top of that, that seems
like the way to go? That depends on if your patch(es) are ready to be
submitted or not? Alternatively, your patch(es) can fix my code. I
don't have a preference either way.

>
>
> > +
> > +TRACE_EVENT(nfs4_cb_offload,
> > +             TP_PROTO(
> > +                     const struct nfs_fh *cb_fh,
> > +                     const nfs4_stateid *cb_stateid,
> > +                     uint64_t cb_count,
> > +                     int cb_error,
> > +                     int cb_how_stable
> > +             ),
> > +
> > +             TP_ARGS(cb_fh, cb_stateid, cb_count, cb_error,
> > +                     cb_how_stable),
> > +
> > +             TP_STRUCT__entry(
> > +                     __field(unsigned long, error)
> > +                     __field(u32, fhandle)
> > +                     __field(loff_t, cb_count)
> > +                     __field(int, cb_how)
> > +                     __field(int, cb_stateid_seq)
> > +                     __field(u32, cb_stateid_hash)
> > +             ),
> > +
> > +             TP_fast_assign(
> > +                     __entry->error = cb_error < 0 ? -cb_error : 0;
> > +                     __entry->fhandle = nfs_fhandle_hash(cb_fh);
> > +                     __entry->cb_stateid_seq =
> > +                             be32_to_cpu(cb_stateid->seqid);
> > +                     __entry->cb_stateid_hash =
> > +                             nfs_stateid_hash(cb_stateid);
> > +                     __entry->cb_count = cb_count;
> > +                     __entry->cb_how = cb_how_stable;
> > +             ),
> > +
> > +             TP_printk(
> > +                     "error=%ld (%s) fhandle=0x%08x cb_stateid=%d:0x%08x "
> > +                     "cb_count=%llu cb_how=%s",
> > +                     -__entry->error,
> > +                     show_nfsv4_errors(__entry->error),
> > +                     __entry->fhandle,
> > +                     __entry->cb_stateid_seq, __entry->cb_stateid_hash,
> > +                     __entry->cb_count,
> > +                     show_write_mode(__entry->cb_how)
> > +             )
> > +);
> > +
> > #endif /* CONFIG_NFS_V4_1 */
> >
> > #endif /* _TRACE_NFS4_H */
> > --
> > 2.27.0
> >
>
> --
> Chuck Lever
>
>
>