For troubleshooting, it is useful to show the callback address and state
inside the nfsd4 client info. Note the callback address and state is
also available via trace events, so use a common function and output.
Changes since v1:
- fix indents, run checkpatch (Chuck L)
- create cb_state2str() inside fs/nfsd/trace.c (Bruce F)
- rebase on Chuck v3 nfsd patches and test
Dave Wysochanski (1):
nfsd4: Expose the callback address and state of each NFS4 client
fs/nfsd/nfs4state.c | 2 ++
fs/nfsd/trace.c | 15 +++++++++++++++
fs/nfsd/trace.h | 9 ++-------
3 files changed, 19 insertions(+), 7 deletions(-)
--
1.8.3.1
In addition to the client's address, display the callback channel
state and address in the 'info' file. Define and use a common
function for this information that can be used by both callback
trace events and the NFS4 client 'info' file.
Signed-off-by: Dave Wysochanski <[email protected]>
---
fs/nfsd/nfs4state.c | 2 ++
fs/nfsd/trace.c | 15 +++++++++++++++
fs/nfsd/trace.h | 9 ++-------
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b2573d3ecd3c..f3b8221bb543 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2385,6 +2385,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_state2str(clp->cl_cb_state));
+ seq_printf(m, "callback address: %pISpc\n", &clp->cl_cb_conn.cb_addr);
drop_client(clp);
return 0;
diff --git a/fs/nfsd/trace.c b/fs/nfsd/trace.c
index f008b95ceec2..6291b5d10824 100644
--- a/fs/nfsd/trace.c
+++ b/fs/nfsd/trace.c
@@ -2,3 +2,18 @@
#define CREATE_TRACE_POINTS
#include "trace.h"
+
+const char *cb_state2str(const 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";
+}
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 10cc3aaf1089..8908d48b2aa6 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -876,12 +876,7 @@
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"})
+const char *cb_state2str(const int state);
DECLARE_EVENT_CLASS(nfsd_cb_class,
TP_PROTO(const struct nfs4_client *clp),
@@ -901,7 +896,7 @@
),
TP_printk("addr=%pISpc client %08x:%08x state=%s",
__entry->addr, __entry->cl_boot, __entry->cl_id,
- show_cb_state(__entry->state))
+ cb_state2str(__entry->state))
);
#define DEFINE_NFSD_CB_EVENT(name) \
--
1.8.3.1
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.
--b.
On Mon, May 17, 2021 at 04:29:45PM -0400, Dave Wysochanski wrote:
> In addition to the client's address, display the callback channel
> state and address in the 'info' file. Define and use a common
> function for this information that can be used by both callback
> trace events and the NFS4 client 'info' file.
>
> Signed-off-by: Dave Wysochanski <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 ++
> fs/nfsd/trace.c | 15 +++++++++++++++
> fs/nfsd/trace.h | 9 ++-------
> 3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b2573d3ecd3c..f3b8221bb543 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2385,6 +2385,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_state2str(clp->cl_cb_state));
> + seq_printf(m, "callback address: %pISpc\n", &clp->cl_cb_conn.cb_addr);
> drop_client(clp);
>
> return 0;
> diff --git a/fs/nfsd/trace.c b/fs/nfsd/trace.c
> index f008b95ceec2..6291b5d10824 100644
> --- a/fs/nfsd/trace.c
> +++ b/fs/nfsd/trace.c
> @@ -2,3 +2,18 @@
>
> #define CREATE_TRACE_POINTS
> #include "trace.h"
> +
> +const char *cb_state2str(const 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";
> +}
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 10cc3aaf1089..8908d48b2aa6 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -876,12 +876,7 @@
> 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"})
> +const char *cb_state2str(const int state);
>
> DECLARE_EVENT_CLASS(nfsd_cb_class,
> TP_PROTO(const struct nfs4_client *clp),
> @@ -901,7 +896,7 @@
> ),
> TP_printk("addr=%pISpc client %08x:%08x state=%s",
> __entry->addr, __entry->cl_boot, __entry->cl_id,
> - show_cb_state(__entry->state))
> + cb_state2str(__entry->state))
> );
>
> #define DEFINE_NFSD_CB_EVENT(name) \
> --
> 1.8.3.1
> On May 25, 2021, at 4:58 PM, Bruce Fields <[email protected]> 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.
> --b.
>
> On Mon, May 17, 2021 at 04:29:45PM -0400, Dave Wysochanski wrote:
>> In addition to the client's address, display the callback channel
>> state and address in the 'info' file. Define and use a common
>> function for this information that can be used by both callback
>> trace events and the NFS4 client 'info' file.
>>
>> Signed-off-by: Dave Wysochanski <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 2 ++
>> fs/nfsd/trace.c | 15 +++++++++++++++
>> fs/nfsd/trace.h | 9 ++-------
>> 3 files changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index b2573d3ecd3c..f3b8221bb543 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2385,6 +2385,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_state2str(clp->cl_cb_state));
>> + seq_printf(m, "callback address: %pISpc\n", &clp->cl_cb_conn.cb_addr);
>> drop_client(clp);
>>
>> return 0;
>> diff --git a/fs/nfsd/trace.c b/fs/nfsd/trace.c
>> index f008b95ceec2..6291b5d10824 100644
>> --- a/fs/nfsd/trace.c
>> +++ b/fs/nfsd/trace.c
>> @@ -2,3 +2,18 @@
>>
>> #define CREATE_TRACE_POINTS
>> #include "trace.h"
>> +
>> +const char *cb_state2str(const 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";
>> +}
>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>> index 10cc3aaf1089..8908d48b2aa6 100644
>> --- a/fs/nfsd/trace.h
>> +++ b/fs/nfsd/trace.h
>> @@ -876,12 +876,7 @@
>> 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"})
>> +const char *cb_state2str(const int state);
>>
>> DECLARE_EVENT_CLASS(nfsd_cb_class,
>> TP_PROTO(const struct nfs4_client *clp),
>> @@ -901,7 +896,7 @@
>> ),
>> TP_printk("addr=%pISpc client %08x:%08x state=%s",
>> __entry->addr, __entry->cl_boot, __entry->cl_id,
>> - show_cb_state(__entry->state))
>> + cb_state2str(__entry->state))
>> );
>>
>> #define DEFINE_NFSD_CB_EVENT(name) \
>> --
>> 1.8.3.1
--
Chuck Lever
On Tue, 2021-05-25 at 16:58 -0400, Bruce Fields 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.
>
You need to store the string in the TP_fast_assign() section if you
want to be able to display it. Otherwise 'perf' tries (and fails) to
find the cb_state2str function so it can invoke it.
> --b.
>
> On Mon, May 17, 2021 at 04:29:45PM -0400, Dave Wysochanski wrote:
> > In addition to the client's address, display the callback channel
> > state and address in the 'info' file. Define and use a common
> > function for this information that can be used by both callback
> > trace events and the NFS4 client 'info' file.
> >
> > Signed-off-by: Dave Wysochanski <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 2 ++
> > fs/nfsd/trace.c | 15 +++++++++++++++
> > fs/nfsd/trace.h | 9 ++-------
> > 3 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index b2573d3ecd3c..f3b8221bb543 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2385,6 +2385,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_state2str(clp-
> > >cl_cb_state));
> > + seq_printf(m, "callback address: %pISpc\n", &clp-
> > >cl_cb_conn.cb_addr);
> > drop_client(clp);
> >
> > return 0;
> > diff --git a/fs/nfsd/trace.c b/fs/nfsd/trace.c
> > index f008b95ceec2..6291b5d10824 100644
> > --- a/fs/nfsd/trace.c
> > +++ b/fs/nfsd/trace.c
> > @@ -2,3 +2,18 @@
> >
> > #define CREATE_TRACE_POINTS
> > #include "trace.h"
> > +
> > +const char *cb_state2str(const 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";
> > +}
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 10cc3aaf1089..8908d48b2aa6 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -876,12 +876,7 @@
> > 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"})
> > +const char *cb_state2str(const int state);
> >
> > DECLARE_EVENT_CLASS(nfsd_cb_class,
> > TP_PROTO(const struct nfs4_client *clp),
> > @@ -901,7 +896,7 @@
> > ),
> > TP_printk("addr=%pISpc client %08x:%08x state=%s",
> > __entry->addr, __entry->cl_boot, __entry->cl_id,
> > - show_cb_state(__entry->state))
> > + cb_state2str(__entry->state))
> > );
> >
> > #define DEFINE_NFSD_CB_EVENT(name) \
> > --
> > 1.8.3.1
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Tue, May 25, 2021 at 09:48:38PM +0000, Chuck Lever III wrote:
>
>
> > On May 25, 2021, at 4:58 PM, Bruce Fields <[email protected]> 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.
--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
On Wed, May 26, 2021 at 2:30 PM Bruce Fields <[email protected]> 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 <[email protected]> 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
>