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: <1621283385-24390-1-git-send-email-dwysocha@redhat.com>
 <1621283385-24390-2-git-send-email-dwysocha@redhat.com> <20210525205845.GB4162@fieldses.org>
 <6C2F8C95-E29F-4BB3-9127-6ED5D825ACB7@oracle.com> <20210526183011.GA7823@fieldses.org>
In-Reply-To: <20210526183011.GA7823@fieldses.org>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Wed, 26 May 2021 15:13:24 -0400
Message-ID: <CALF+zOnwEgMEv6FS=888x6_YHFTama-qdeoCWroiHnrtxe6QVA@mail.gmail.com>
Subject: Re: [PATCH v2 1/1] nfsd4: Expose the callback address and state of
 each NFS4 client
To:     Bruce Fields <bfields@fieldses.org>
Cc:     Chuck Lever III <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 26, 2021 at 2:30 PM Bruce Fields <bfields@fieldses.org> wrote:
>
> On Tue, May 25, 2021 at 09:48:38PM +0000, Chuck Lever III wrote:
> >
> >
> > > On May 25, 2021, at 4:58 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > When I run trace-cmd report I get output like:
> > >
> > >  [nfsd:nfsd_cb_state] function cb_state2str not defined
> > >  [nfsd:nfsd_cb_shutdown] function cb_state2str not defined
> > >  [nfsd:nfsd_cb_probe] function cb_state2str not defined
> > >  [nfsd:nfsd_cb_lost] function cb_state2str not defined
> > >
> > > I don't know how this is supposed to work.  Is it OK for tracepoint definitions
> > > to reference kernel functions if they're defined in the right way somehow?  If
> > > not, I don't know what the solution would be for sharing this--define a macro
> > > that expands to the array literal and use that in both places?  Or maybe just
> > > live with the the redundancy.
> >
> > Living with the redundancy is OK with me.
>
> OK, I'll revert back to Dave's first version of this patch.
>

I'm ok with the above.

If reuse was desired we could do something like this, but it's a bit
ugly / convoluted:

diff --git a/fs/nfsd/trace.c b/fs/nfsd/trace.c
index f008b95ceec2..54c31b5f42d4 100644
--- a/fs/nfsd/trace.c
+++ b/fs/nfsd/trace.c
@@ -2,3 +2,16 @@

 #define CREATE_TRACE_POINTS
 #include "trace.h"
+
+const char *cb_state2str(const int state)
+{
+       int i;
+       const struct trace_print_flags state_array[] = { CB_STATE_ARRAY,
+                                                        { -1, NULL } };
+
+       for (i = 0; state_array[i].name; i++)
+               if (state == state_array[i].mask)
+                       return state_array[i].name;
+
+       return "UNDEFINED";
+}
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 10cc3aaf1089..f21841f4ae3b 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -876,12 +876,12 @@ TRACE_EVENT(nfsd_cb_nodelegs,
        TP_printk("client %08x:%08x", __entry->cl_boot, __entry->cl_id)
 )

-#define show_cb_state(val)                                             \
-       __print_symbolic(val,                                           \
-               { NFSD4_CB_UP,          "UP" },                         \
-               { NFSD4_CB_UNKNOWN,     "UNKNOWN" },                    \
-               { NFSD4_CB_DOWN,        "DOWN" },                       \
-               { NFSD4_CB_FAULT,       "FAULT"})
+#define CB_STATE_ARRAY \
+       { NFSD4_CB_UP,          "UP" },                 \
+       { NFSD4_CB_UNKNOWN,     "UNKNOWN" },            \
+       { NFSD4_CB_DOWN,        "DOWN" },               \
+       { NFSD4_CB_FAULT,       "FAULT"}
+extern const char *cb_state2str(const int state);

 DECLARE_EVENT_CLASS(nfsd_cb_class,
        TP_PROTO(const struct nfs4_client *clp),
@@ -901,7 +901,7 @@ DECLARE_EVENT_CLASS(nfsd_cb_class,
        ),
        TP_printk("addr=%pISpc client %08x:%08x state=%s",
                __entry->addr, __entry->cl_boot, __entry->cl_id,
-               show_cb_state(__entry->state))
+               __print_symbolic(__entry->state, CB_STATE_ARRAY))
 );

 #define DEFINE_NFSD_CB_EVENT(name)                     \


> --b.
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 49c052243b5c..89a7cada334d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2357,6 +2357,21 @@ static void seq_quote_mem(struct seq_file *m, char *data, int len)
>         seq_printf(m, "\"");
>  }
>
> +static const char *cb_state_str(int state)
> +{
> +       switch (state) {
> +               case NFSD4_CB_UP:
> +                       return "UP";
> +               case NFSD4_CB_UNKNOWN:
> +                       return "UNKNOWN";
> +               case NFSD4_CB_DOWN:
> +                       return "DOWN";
> +               case NFSD4_CB_FAULT:
> +                       return "FAULT";
> +       }
> +       return "UNDEFINED";
> +}
> +
>  static int client_info_show(struct seq_file *m, void *v)
>  {
>         struct inode *inode = m->private;
> @@ -2385,6 +2400,8 @@ static int client_info_show(struct seq_file *m, void *v)
>                 seq_printf(m, "\nImplementation time: [%lld, %ld]\n",
>                         clp->cl_nii_time.tv_sec, clp->cl_nii_time.tv_nsec);
>         }
> +       seq_printf(m, "callback state: %s\n", cb_state_str(clp->cl_cb_state));
> +       seq_printf(m, "callback address: %pISpc\n", &clp->cl_cb_conn.cb_addr);
>         drop_client(clp);
>
>         return 0;
> --
> 1.8.3.1
>