| Message ID | 1621283385-24390-2-git-send-email-dwysocha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add callback address and state to nfsd4 client info | expand |
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 <dwysocha@redhat.com> > --- > 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 <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. > --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 <dwysocha@redhat.com> >> --- >> 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 <dwysocha@redhat.com> > > --- > > 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 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. --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;
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 >
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) \
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 <dwysocha@redhat.com> --- fs/nfsd/nfs4state.c | 2 ++ fs/nfsd/trace.c | 15 +++++++++++++++ fs/nfsd/trace.h | 9 ++------- 3 files changed, 19 insertions(+), 7 deletions(-)