2010-11-09 18:50:07

by Harshula

[permalink] [raw]
Subject: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats

Hi,

The NFSv4 client procs/ops in "struct rpc_procinfo nfs4_procedures" is
used to generate the NFS client stats interface:
------------------------------------------------------------
# cat /proc/net/rpc/nfs
net 0 0 0 0
rpc 15 0 0
proc2 18 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
proc3 22 0 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 1 0
proc4 42 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
0 0 0
0 0 0 0 0 0 0
------------------------------------------------------------
Note, for proc4, the number 42. That is the number of stats that follow
on the same line. Currently nfsstat's has_stats() relies on this number
to be equal to CLTPROC4_SZ. Unfortunately this is not the case. I have
changed has_stats() not to rely on these two values being equal. This
should also allow nfsstat to work with different kernel versions that
expose a different number of NFS client ops.

* Fix has_stats()
* Stop print_clnt_list() printing server stats!
* Describe the option -3 and -4 completely in the nfsstat manpage.

Signed-off-by: Harshula Jayasuriya <[email protected]>
---
utils/nfsstat/nfsstat.c | 124 ++++++++++++++++++++-------------------------
utils/nfsstat/nfsstat.man | 6 ++-
2 files changed, 59 insertions(+), 71 deletions(-)

diff --git a/utils/nfsstat/nfsstat.c b/utils/nfsstat/nfsstat.c
index bacef8e..f31bb81 100644
--- a/utils/nfsstat/nfsstat.c
+++ b/utils/nfsstat/nfsstat.c
@@ -46,7 +46,7 @@ static unsigned int cltproc3info[CLTPROC3_SZ+2],
static unsigned int srvproc4info[SRVPROC4_SZ+2],
srvproc4info_old[SRVPROC4_SZ+2]; /* NFSv4 call counts ([0] == 2) */
static unsigned int cltproc4info[CLTPROC4_SZ+2],
- cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 48) */
+ cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 49) */
static unsigned int srvproc4opsinfo[SRVPROC4OPS_SZ+2],
srvproc4opsinfo_old[SRVPROC4OPS_SZ+2]; /* NFSv4 call counts ([0] == 59) */
static unsigned int srvnetinfo[5], srvnetinfo_old[5]; /* 0 # of received packets
@@ -221,8 +221,8 @@ DECLARE_CLT(cltinfo);
DECLARE_CLT(cltinfo, _old);

static void print_all_stats(int, int, int);
-static void print_server_stats(int, int);
-static void print_client_stats(int, int);
+static void print_server_stats(int);
+static void print_client_stats(int);
static void print_stats_list(int, int, int);
static void print_numbers(const char *, unsigned int *,
unsigned int);
@@ -239,7 +239,7 @@ static int mounts(const char *);

static void get_stats(const char *, struct statinfo *, int *, int,
int);
-static int has_stats(const unsigned int *);
+static int has_stats(const unsigned int *, int);
static int has_rpcstats(const unsigned int *, int);
static void diff_stats(struct statinfo *, struct statinfo *, int);
static void unpause(int);
@@ -468,7 +468,7 @@ main(int argc, char **argv)
pause();
}

- if (opt_since || opt_sleep) {
+ if (opt_since || (opt_sleep && !sleep_time)) {
if (opt_srv) {
get_stats(NFSSRVSTAT, serverinfo_tmp, &opt_srv, opt_clt, 1);
diff_stats(serverinfo_tmp, serverinfo, 1);
@@ -516,16 +516,16 @@ main(int argc, char **argv)
static void
print_all_stats (int opt_srv, int opt_clt, int opt_prt)
{
- print_server_stats(opt_srv, opt_prt);
- print_client_stats(opt_clt, opt_prt);
+ if (opt_srv)
+ print_server_stats(opt_prt);
+
+ if (opt_clt)
+ print_client_stats(opt_prt);
}

static void
-print_server_stats(int opt_srv, int opt_prt)
+print_server_stats(int opt_prt)
{
- if (!opt_srv)
- return;
-
if (opt_prt & PRNT_NET) {
if (opt_sleep && !has_rpcstats(srvnetinfo, 4)) {
} else {
@@ -582,31 +582,29 @@ print_server_stats(int opt_srv, int opt_prt)
printf("\n");
}
if (opt_prt & PRNT_CALLS) {
+ int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
+ int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
+ int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
+
if ((opt_prt & PRNT_V2) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
- if (opt_sleep && !has_stats(srvproc2info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
+ if (!opt_sleep || has_v2_stats) {
print_callstats(LABEL_srvproc2,
nfsv2name, srvproc2info + 1,
sizeof(nfsv2name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V3) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
- if (opt_sleep && !has_stats(srvproc3info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
+ if (!opt_sleep || has_v3_stats) {
print_callstats(LABEL_srvproc3,
nfsv3name, srvproc3info + 1,
sizeof(nfsv3name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V4) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc4info))) {
- if (opt_sleep && !has_stats(srvproc4info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
+ if (!opt_sleep || has_v4_stats) {
print_callstats( LABEL_srvproc4,
nfssrvproc4name, srvproc4info + 1,
sizeof(nfssrvproc4name)/sizeof(char *));
@@ -618,11 +616,8 @@ print_server_stats(int opt_srv, int opt_prt)
}
}
static void
-print_client_stats(int opt_clt, int opt_prt)
+print_client_stats(int opt_prt)
{
- if (!opt_clt)
- return;
-
if (opt_prt & PRNT_NET) {
if (opt_sleep && !has_rpcstats(cltnetinfo, 4)) {
;
@@ -644,31 +639,28 @@ print_client_stats(int opt_clt, int opt_prt)
}
}
if (opt_prt & PRNT_CALLS) {
+ int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
+ int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
+ int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
if ((opt_prt & PRNT_V2) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
- if (opt_sleep && !has_stats(cltproc2info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
+ if (!opt_sleep || has_v2_stats) {
print_callstats(LABEL_cltproc2,
nfsv2name, cltproc2info + 1,
sizeof(nfsv2name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V3) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
- if (opt_sleep && !has_stats(cltproc3info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
+ if (!opt_sleep || has_v3_stats) {
print_callstats(LABEL_cltproc3,
nfsv3name, cltproc3info + 1,
sizeof(nfsv3name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V4) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
- if (opt_sleep && !has_stats(cltproc4info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
+ if (!opt_sleep || has_v4_stats) {
print_callstats(LABEL_cltproc4,
nfscltproc4name, cltproc4info + 1,
sizeof(nfscltproc4name)/sizeof(char *));
@@ -681,34 +673,28 @@ static void
print_clnt_list(int opt_prt)
{
if (opt_prt & PRNT_CALLS) {
+ int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
+ int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
+ int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
if ((opt_prt & PRNT_V2) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
- if (opt_sleep && !has_stats(cltproc2info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
+ if (!opt_sleep || has_v2_stats) {
print_callstats_list("nfs v2 client",
nfsv2name, cltproc2info + 1,
sizeof(nfsv2name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V3) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
- if (opt_sleep && !has_stats(cltproc3info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
+ if (!opt_sleep || has_v3_stats) {
print_callstats_list("nfs v3 client",
nfsv3name, cltproc3info + 1,
sizeof(nfsv3name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V4) ||
- ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
- if (opt_sleep && !has_stats(cltproc4info)) {
- ;
- } else {
- print_callstats_list("nfs v4 ops",
- nfssrvproc4opname, srvproc4opsinfo + 1,
- sizeof(nfssrvproc4opname)/sizeof(char *));
+ ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
+ if (!opt_sleep || has_v4_stats) {
print_callstats_list("nfs v4 client",
nfscltproc4name, cltproc4info + 1,
sizeof(nfscltproc4name)/sizeof(char *));
@@ -720,32 +706,32 @@ static void
print_serv_list(int opt_prt)
{
if (opt_prt & PRNT_CALLS) {
+ int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
+ int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
+ int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
if ((opt_prt & PRNT_V2) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
- if (opt_sleep && !has_stats(srvproc2info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
+ if (!opt_sleep || has_v2_stats) {
print_callstats_list("nfs v2 server",
nfsv2name, srvproc2info + 1,
sizeof(nfsv2name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V3) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
- if (opt_sleep && !has_stats(srvproc3info)) {
- ;
- } else {
+ ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
+ if (!opt_sleep || has_v3_stats) {
print_callstats_list("nfs v3 server",
nfsv3name, srvproc3info + 1,
sizeof(nfsv3name)/sizeof(char *));
}
}
if ((opt_prt & PRNT_V4) ||
- ((opt_prt & PRNT_AUTO) && has_stats(srvproc4opsinfo))) {
- if (opt_sleep && !has_stats(srvproc4info)) {
- ;
- } else {
- print_callstats_list("nfs v4 ops",
+ ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
+ if (!opt_sleep || has_v4_stats) {
+ print_callstats_list("nfs v4 server",
+ nfssrvproc4name, srvproc4info + 1,
+ sizeof(nfssrvproc4name)/sizeof(char *));
+ print_callstats_list("nfs v4 servop",
nfssrvproc4opname, srvproc4opsinfo + 1,
sizeof(nfssrvproc4opname)/sizeof(char *));
}
@@ -1054,9 +1040,9 @@ out:
* there are stats if the sum's greater than the entry-count.
*/
static int
-has_stats(const unsigned int *info)
+has_stats(const unsigned int *info, int nr)
{
- return (info[0] && info[info[0] + 1] > info[0]);
+ return (info[0] && info[nr-1] > info[0]);
}
static int
has_rpcstats(const unsigned int *info, int size)
diff --git a/utils/nfsstat/nfsstat.man b/utils/nfsstat/nfsstat.man
index 52215a9..cd573b0 100644
--- a/utils/nfsstat/nfsstat.man
+++ b/utils/nfsstat/nfsstat.man
@@ -30,10 +30,12 @@ Print only NFS v2 statistics. The default is to only print information
about the versions of \fBNFS\fR that have non-zero counts.
.TP
.B \-3
-Print only NFS v3 statistics.
+Print only NFS v3 statistics. The default is to only print information
+about the versions of \fBNFS\fR that have non-zero counts.
.TP
.B \-4
-Print only NFS v4 statistics.
+Print only NFS v4 statistics. The default is to only print information
+about the versions of \fBNFS\fR that have non-zero counts.
.TP
.B \-m, \-\-mounts
Print information about each of the mounted \fBNFS\fR file systems.
--
1.7.3.2



2010-11-10 04:26:48

by Harshula

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats

Hi Chuck,

Are you referring to /proc/self/mountstats? I didn't know about it.
nfsstat's get_stats() would have to be re-written. I can add that to my
TODO list, but not sure when I'll get to it. In the meantime, this patch
will fix the previously mentioned bugs and make the existing code a
little clearer.

cya,
#

On Tue, 2010-11-09 at 14:29 -0500, Chuck Lever wrote:
> Hi-
>
> Why not use mountstats? The /proc/net/rpc/nfs is destined for deprecation, I thought.
>
> On Nov 9, 2010, at 1:50 PM, Harshula Jayasuriya wrote:
>
> > Hi,
> >
> > The NFSv4 client procs/ops in "struct rpc_procinfo nfs4_procedures" is
> > used to generate the NFS client stats interface:
> > ------------------------------------------------------------
> > # cat /proc/net/rpc/nfs
> > net 0 0 0 0
> > rpc 15 0 0
> > proc2 18 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
> > proc3 22 0 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 1 0
> > proc4 42 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
> > 0 0 0
> > 0 0 0 0 0 0 0
> > ------------------------------------------------------------
> > Note, for proc4, the number 42. That is the number of stats that follow
> > on the same line. Currently nfsstat's has_stats() relies on this number
> > to be equal to CLTPROC4_SZ. Unfortunately this is not the case. I have
> > changed has_stats() not to rely on these two values being equal. This
> > should also allow nfsstat to work with different kernel versions that
> > expose a different number of NFS client ops.
> >
> > * Fix has_stats()
> > * Stop print_clnt_list() printing server stats!
> > * Describe the option -3 and -4 completely in the nfsstat manpage.
> >
> > Signed-off-by: Harshula Jayasuriya <[email protected]>
> > ---
> > utils/nfsstat/nfsstat.c | 124 ++++++++++++++++++++-------------------------
> > utils/nfsstat/nfsstat.man | 6 ++-
> > 2 files changed, 59 insertions(+), 71 deletions(-)
> >
> > diff --git a/utils/nfsstat/nfsstat.c b/utils/nfsstat/nfsstat.c
> > index bacef8e..f31bb81 100644
> > --- a/utils/nfsstat/nfsstat.c
> > +++ b/utils/nfsstat/nfsstat.c
> > @@ -46,7 +46,7 @@ static unsigned int cltproc3info[CLTPROC3_SZ+2],
> > static unsigned int srvproc4info[SRVPROC4_SZ+2],
> > srvproc4info_old[SRVPROC4_SZ+2]; /* NFSv4 call counts ([0] == 2) */
> > static unsigned int cltproc4info[CLTPROC4_SZ+2],
> > - cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 48) */
> > + cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 49) */
> > static unsigned int srvproc4opsinfo[SRVPROC4OPS_SZ+2],
> > srvproc4opsinfo_old[SRVPROC4OPS_SZ+2]; /* NFSv4 call counts ([0] == 59) */
> > static unsigned int srvnetinfo[5], srvnetinfo_old[5]; /* 0 # of received packets
> > @@ -221,8 +221,8 @@ DECLARE_CLT(cltinfo);
> > DECLARE_CLT(cltinfo, _old);
> >
> > static void print_all_stats(int, int, int);
> > -static void print_server_stats(int, int);
> > -static void print_client_stats(int, int);
> > +static void print_server_stats(int);
> > +static void print_client_stats(int);
> > static void print_stats_list(int, int, int);
> > static void print_numbers(const char *, unsigned int *,
> > unsigned int);
> > @@ -239,7 +239,7 @@ static int mounts(const char *);
> >
> > static void get_stats(const char *, struct statinfo *, int *, int,
> > int);
> > -static int has_stats(const unsigned int *);
> > +static int has_stats(const unsigned int *, int);
> > static int has_rpcstats(const unsigned int *, int);
> > static void diff_stats(struct statinfo *, struct statinfo *, int);
> > static void unpause(int);
> > @@ -468,7 +468,7 @@ main(int argc, char **argv)
> > pause();
> > }
> >
> > - if (opt_since || opt_sleep) {
> > + if (opt_since || (opt_sleep && !sleep_time)) {
> > if (opt_srv) {
> > get_stats(NFSSRVSTAT, serverinfo_tmp, &opt_srv, opt_clt, 1);
> > diff_stats(serverinfo_tmp, serverinfo, 1);
> > @@ -516,16 +516,16 @@ main(int argc, char **argv)
> > static void
> > print_all_stats (int opt_srv, int opt_clt, int opt_prt)
> > {
> > - print_server_stats(opt_srv, opt_prt);
> > - print_client_stats(opt_clt, opt_prt);
> > + if (opt_srv)
> > + print_server_stats(opt_prt);
> > +
> > + if (opt_clt)
> > + print_client_stats(opt_prt);
> > }
> >
> > static void
> > -print_server_stats(int opt_srv, int opt_prt)
> > +print_server_stats(int opt_prt)
> > {
> > - if (!opt_srv)
> > - return;
> > -
> > if (opt_prt & PRNT_NET) {
> > if (opt_sleep && !has_rpcstats(srvnetinfo, 4)) {
> > } else {
> > @@ -582,31 +582,29 @@ print_server_stats(int opt_srv, int opt_prt)
> > printf("\n");
> > }
> > if (opt_prt & PRNT_CALLS) {
> > + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
> > + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
> > + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
> > +
> > if ((opt_prt & PRNT_V2) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
> > - if (opt_sleep && !has_stats(srvproc2info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> > + if (!opt_sleep || has_v2_stats) {
> > print_callstats(LABEL_srvproc2,
> > nfsv2name, srvproc2info + 1,
> > sizeof(nfsv2name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V3) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
> > - if (opt_sleep && !has_stats(srvproc3info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> > + if (!opt_sleep || has_v3_stats) {
> > print_callstats(LABEL_srvproc3,
> > nfsv3name, srvproc3info + 1,
> > sizeof(nfsv3name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V4) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4info))) {
> > - if (opt_sleep && !has_stats(srvproc4info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> > + if (!opt_sleep || has_v4_stats) {
> > print_callstats( LABEL_srvproc4,
> > nfssrvproc4name, srvproc4info + 1,
> > sizeof(nfssrvproc4name)/sizeof(char *));
> > @@ -618,11 +616,8 @@ print_server_stats(int opt_srv, int opt_prt)
> > }
> > }
> > static void
> > -print_client_stats(int opt_clt, int opt_prt)
> > +print_client_stats(int opt_prt)
> > {
> > - if (!opt_clt)
> > - return;
> > -
> > if (opt_prt & PRNT_NET) {
> > if (opt_sleep && !has_rpcstats(cltnetinfo, 4)) {
> > ;
> > @@ -644,31 +639,28 @@ print_client_stats(int opt_clt, int opt_prt)
> > }
> > }
> > if (opt_prt & PRNT_CALLS) {
> > + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
> > + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
> > + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
> > if ((opt_prt & PRNT_V2) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
> > - if (opt_sleep && !has_stats(cltproc2info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> > + if (!opt_sleep || has_v2_stats) {
> > print_callstats(LABEL_cltproc2,
> > nfsv2name, cltproc2info + 1,
> > sizeof(nfsv2name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V3) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
> > - if (opt_sleep && !has_stats(cltproc3info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> > + if (!opt_sleep || has_v3_stats) {
> > print_callstats(LABEL_cltproc3,
> > nfsv3name, cltproc3info + 1,
> > sizeof(nfsv3name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V4) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
> > - if (opt_sleep && !has_stats(cltproc4info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> > + if (!opt_sleep || has_v4_stats) {
> > print_callstats(LABEL_cltproc4,
> > nfscltproc4name, cltproc4info + 1,
> > sizeof(nfscltproc4name)/sizeof(char *));
> > @@ -681,34 +673,28 @@ static void
> > print_clnt_list(int opt_prt)
> > {
> > if (opt_prt & PRNT_CALLS) {
> > + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
> > + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
> > + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
> > if ((opt_prt & PRNT_V2) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
> > - if (opt_sleep && !has_stats(cltproc2info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> > + if (!opt_sleep || has_v2_stats) {
> > print_callstats_list("nfs v2 client",
> > nfsv2name, cltproc2info + 1,
> > sizeof(nfsv2name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V3) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
> > - if (opt_sleep && !has_stats(cltproc3info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> > + if (!opt_sleep || has_v3_stats) {
> > print_callstats_list("nfs v3 client",
> > nfsv3name, cltproc3info + 1,
> > sizeof(nfsv3name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V4) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
> > - if (opt_sleep && !has_stats(cltproc4info)) {
> > - ;
> > - } else {
> > - print_callstats_list("nfs v4 ops",
> > - nfssrvproc4opname, srvproc4opsinfo + 1,
> > - sizeof(nfssrvproc4opname)/sizeof(char *));
> > + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> > + if (!opt_sleep || has_v4_stats) {
> > print_callstats_list("nfs v4 client",
> > nfscltproc4name, cltproc4info + 1,
> > sizeof(nfscltproc4name)/sizeof(char *));
> > @@ -720,32 +706,32 @@ static void
> > print_serv_list(int opt_prt)
> > {
> > if (opt_prt & PRNT_CALLS) {
> > + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
> > + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
> > + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
> > if ((opt_prt & PRNT_V2) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
> > - if (opt_sleep && !has_stats(srvproc2info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> > + if (!opt_sleep || has_v2_stats) {
> > print_callstats_list("nfs v2 server",
> > nfsv2name, srvproc2info + 1,
> > sizeof(nfsv2name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V3) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
> > - if (opt_sleep && !has_stats(srvproc3info)) {
> > - ;
> > - } else {
> > + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> > + if (!opt_sleep || has_v3_stats) {
> > print_callstats_list("nfs v3 server",
> > nfsv3name, srvproc3info + 1,
> > sizeof(nfsv3name)/sizeof(char *));
> > }
> > }
> > if ((opt_prt & PRNT_V4) ||
> > - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4opsinfo))) {
> > - if (opt_sleep && !has_stats(srvproc4info)) {
> > - ;
> > - } else {
> > - print_callstats_list("nfs v4 ops",
> > + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> > + if (!opt_sleep || has_v4_stats) {
> > + print_callstats_list("nfs v4 server",
> > + nfssrvproc4name, srvproc4info + 1,
> > + sizeof(nfssrvproc4name)/sizeof(char *));
> > + print_callstats_list("nfs v4 servop",
> > nfssrvproc4opname, srvproc4opsinfo + 1,
> > sizeof(nfssrvproc4opname)/sizeof(char *));
> > }
> > @@ -1054,9 +1040,9 @@ out:
> > * there are stats if the sum's greater than the entry-count.
> > */
> > static int
> > -has_stats(const unsigned int *info)
> > +has_stats(const unsigned int *info, int nr)
> > {
> > - return (info[0] && info[info[0] + 1] > info[0]);
> > + return (info[0] && info[nr-1] > info[0]);
> > }
> > static int
> > has_rpcstats(const unsigned int *info, int size)
> > diff --git a/utils/nfsstat/nfsstat.man b/utils/nfsstat/nfsstat.man
> > index 52215a9..cd573b0 100644
> > --- a/utils/nfsstat/nfsstat.man
> > +++ b/utils/nfsstat/nfsstat.man
> > @@ -30,10 +30,12 @@ Print only NFS v2 statistics. The default is to only print information
> > about the versions of \fBNFS\fR that have non-zero counts.
> > .TP
> > .B \-3
> > -Print only NFS v3 statistics.
> > +Print only NFS v3 statistics. The default is to only print information
> > +about the versions of \fBNFS\fR that have non-zero counts.
> > .TP
> > .B \-4
> > -Print only NFS v4 statistics.
> > +Print only NFS v4 statistics. The default is to only print information
> > +about the versions of \fBNFS\fR that have non-zero counts.
> > .TP
> > .B \-m, \-\-mounts
> > Print information about each of the mounted \fBNFS\fR file systems.
> > --
> > 1.7.3.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>



2010-11-10 15:08:25

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats



On 11/09/2010 11:26 PM, Harshula Jayasuriya wrote:
> Are you referring to /proc/self/mountstats? I didn't know about it.
Take a look at the nfsiostat and mountstats commands... they know
how to read the /proc/self/mountstats file..

> nfsstat's get_stats() would have to be re-written. I can add that to my
> TODO list, but not sure when I'll get to it. In the meantime, this patch
> will fix the previously mentioned bugs and make the existing code a
> little clearer.
This is pretty much legacy code... I would not waste the time
on teaching nfsstat how to read the mountstats file... I would
rather see the time and effort be put into making nfsiostat and
mountstats more useful... maybe even sometime type of GUI...

steved.

>
> cya,
> #
>
> On Tue, 2010-11-09 at 14:29 -0500, Chuck Lever wrote:
>> Hi-
>>
>> Why not use mountstats? The /proc/net/rpc/nfs is destined for deprecation, I thought.
>>
>> On Nov 9, 2010, at 1:50 PM, Harshula Jayasuriya wrote:
>>
>>> Hi,
>>>
>>> The NFSv4 client procs/ops in "struct rpc_procinfo nfs4_procedures" is
>>> used to generate the NFS client stats interface:
>>> ------------------------------------------------------------
>>> # cat /proc/net/rpc/nfs
>>> net 0 0 0 0
>>> rpc 15 0 0
>>> proc2 18 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
>>> proc3 22 0 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 1 0
>>> proc4 42 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
>>> 0 0 0
>>> 0 0 0 0 0 0 0
>>> ------------------------------------------------------------
>>> Note, for proc4, the number 42. That is the number of stats that follow
>>> on the same line. Currently nfsstat's has_stats() relies on this number
>>> to be equal to CLTPROC4_SZ. Unfortunately this is not the case. I have
>>> changed has_stats() not to rely on these two values being equal. This
>>> should also allow nfsstat to work with different kernel versions that
>>> expose a different number of NFS client ops.
>>>
>>> * Fix has_stats()
>>> * Stop print_clnt_list() printing server stats!
>>> * Describe the option -3 and -4 completely in the nfsstat manpage.
>>>
>>> Signed-off-by: Harshula Jayasuriya <[email protected]>
>>> ---
>>> utils/nfsstat/nfsstat.c | 124 ++++++++++++++++++++-------------------------
>>> utils/nfsstat/nfsstat.man | 6 ++-
>>> 2 files changed, 59 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/utils/nfsstat/nfsstat.c b/utils/nfsstat/nfsstat.c
>>> index bacef8e..f31bb81 100644
>>> --- a/utils/nfsstat/nfsstat.c
>>> +++ b/utils/nfsstat/nfsstat.c
>>> @@ -46,7 +46,7 @@ static unsigned int cltproc3info[CLTPROC3_SZ+2],
>>> static unsigned int srvproc4info[SRVPROC4_SZ+2],
>>> srvproc4info_old[SRVPROC4_SZ+2]; /* NFSv4 call counts ([0] == 2) */
>>> static unsigned int cltproc4info[CLTPROC4_SZ+2],
>>> - cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 48) */
>>> + cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 49) */
>>> static unsigned int srvproc4opsinfo[SRVPROC4OPS_SZ+2],
>>> srvproc4opsinfo_old[SRVPROC4OPS_SZ+2]; /* NFSv4 call counts ([0] == 59) */
>>> static unsigned int srvnetinfo[5], srvnetinfo_old[5]; /* 0 # of received packets
>>> @@ -221,8 +221,8 @@ DECLARE_CLT(cltinfo);
>>> DECLARE_CLT(cltinfo, _old);
>>>
>>> static void print_all_stats(int, int, int);
>>> -static void print_server_stats(int, int);
>>> -static void print_client_stats(int, int);
>>> +static void print_server_stats(int);
>>> +static void print_client_stats(int);
>>> static void print_stats_list(int, int, int);
>>> static void print_numbers(const char *, unsigned int *,
>>> unsigned int);
>>> @@ -239,7 +239,7 @@ static int mounts(const char *);
>>>
>>> static void get_stats(const char *, struct statinfo *, int *, int,
>>> int);
>>> -static int has_stats(const unsigned int *);
>>> +static int has_stats(const unsigned int *, int);
>>> static int has_rpcstats(const unsigned int *, int);
>>> static void diff_stats(struct statinfo *, struct statinfo *, int);
>>> static void unpause(int);
>>> @@ -468,7 +468,7 @@ main(int argc, char **argv)
>>> pause();
>>> }
>>>
>>> - if (opt_since || opt_sleep) {
>>> + if (opt_since || (opt_sleep && !sleep_time)) {
>>> if (opt_srv) {
>>> get_stats(NFSSRVSTAT, serverinfo_tmp, &opt_srv, opt_clt, 1);
>>> diff_stats(serverinfo_tmp, serverinfo, 1);
>>> @@ -516,16 +516,16 @@ main(int argc, char **argv)
>>> static void
>>> print_all_stats (int opt_srv, int opt_clt, int opt_prt)
>>> {
>>> - print_server_stats(opt_srv, opt_prt);
>>> - print_client_stats(opt_clt, opt_prt);
>>> + if (opt_srv)
>>> + print_server_stats(opt_prt);
>>> +
>>> + if (opt_clt)
>>> + print_client_stats(opt_prt);
>>> }
>>>
>>> static void
>>> -print_server_stats(int opt_srv, int opt_prt)
>>> +print_server_stats(int opt_prt)
>>> {
>>> - if (!opt_srv)
>>> - return;
>>> -
>>> if (opt_prt & PRNT_NET) {
>>> if (opt_sleep && !has_rpcstats(srvnetinfo, 4)) {
>>> } else {
>>> @@ -582,31 +582,29 @@ print_server_stats(int opt_srv, int opt_prt)
>>> printf("\n");
>>> }
>>> if (opt_prt & PRNT_CALLS) {
>>> + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
>>> + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
>>> + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
>>> +
>>> if ((opt_prt & PRNT_V2) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
>>> - if (opt_sleep && !has_stats(srvproc2info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
>>> + if (!opt_sleep || has_v2_stats) {
>>> print_callstats(LABEL_srvproc2,
>>> nfsv2name, srvproc2info + 1,
>>> sizeof(nfsv2name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V3) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
>>> - if (opt_sleep && !has_stats(srvproc3info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
>>> + if (!opt_sleep || has_v3_stats) {
>>> print_callstats(LABEL_srvproc3,
>>> nfsv3name, srvproc3info + 1,
>>> sizeof(nfsv3name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V4) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4info))) {
>>> - if (opt_sleep && !has_stats(srvproc4info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
>>> + if (!opt_sleep || has_v4_stats) {
>>> print_callstats( LABEL_srvproc4,
>>> nfssrvproc4name, srvproc4info + 1,
>>> sizeof(nfssrvproc4name)/sizeof(char *));
>>> @@ -618,11 +616,8 @@ print_server_stats(int opt_srv, int opt_prt)
>>> }
>>> }
>>> static void
>>> -print_client_stats(int opt_clt, int opt_prt)
>>> +print_client_stats(int opt_prt)
>>> {
>>> - if (!opt_clt)
>>> - return;
>>> -
>>> if (opt_prt & PRNT_NET) {
>>> if (opt_sleep && !has_rpcstats(cltnetinfo, 4)) {
>>> ;
>>> @@ -644,31 +639,28 @@ print_client_stats(int opt_clt, int opt_prt)
>>> }
>>> }
>>> if (opt_prt & PRNT_CALLS) {
>>> + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
>>> + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
>>> + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
>>> if ((opt_prt & PRNT_V2) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
>>> - if (opt_sleep && !has_stats(cltproc2info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
>>> + if (!opt_sleep || has_v2_stats) {
>>> print_callstats(LABEL_cltproc2,
>>> nfsv2name, cltproc2info + 1,
>>> sizeof(nfsv2name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V3) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
>>> - if (opt_sleep && !has_stats(cltproc3info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
>>> + if (!opt_sleep || has_v3_stats) {
>>> print_callstats(LABEL_cltproc3,
>>> nfsv3name, cltproc3info + 1,
>>> sizeof(nfsv3name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V4) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
>>> - if (opt_sleep && !has_stats(cltproc4info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
>>> + if (!opt_sleep || has_v4_stats) {
>>> print_callstats(LABEL_cltproc4,
>>> nfscltproc4name, cltproc4info + 1,
>>> sizeof(nfscltproc4name)/sizeof(char *));
>>> @@ -681,34 +673,28 @@ static void
>>> print_clnt_list(int opt_prt)
>>> {
>>> if (opt_prt & PRNT_CALLS) {
>>> + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
>>> + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
>>> + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
>>> if ((opt_prt & PRNT_V2) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
>>> - if (opt_sleep && !has_stats(cltproc2info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
>>> + if (!opt_sleep || has_v2_stats) {
>>> print_callstats_list("nfs v2 client",
>>> nfsv2name, cltproc2info + 1,
>>> sizeof(nfsv2name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V3) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
>>> - if (opt_sleep && !has_stats(cltproc3info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
>>> + if (!opt_sleep || has_v3_stats) {
>>> print_callstats_list("nfs v3 client",
>>> nfsv3name, cltproc3info + 1,
>>> sizeof(nfsv3name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V4) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
>>> - if (opt_sleep && !has_stats(cltproc4info)) {
>>> - ;
>>> - } else {
>>> - print_callstats_list("nfs v4 ops",
>>> - nfssrvproc4opname, srvproc4opsinfo + 1,
>>> - sizeof(nfssrvproc4opname)/sizeof(char *));
>>> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
>>> + if (!opt_sleep || has_v4_stats) {
>>> print_callstats_list("nfs v4 client",
>>> nfscltproc4name, cltproc4info + 1,
>>> sizeof(nfscltproc4name)/sizeof(char *));
>>> @@ -720,32 +706,32 @@ static void
>>> print_serv_list(int opt_prt)
>>> {
>>> if (opt_prt & PRNT_CALLS) {
>>> + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
>>> + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
>>> + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
>>> if ((opt_prt & PRNT_V2) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
>>> - if (opt_sleep && !has_stats(srvproc2info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
>>> + if (!opt_sleep || has_v2_stats) {
>>> print_callstats_list("nfs v2 server",
>>> nfsv2name, srvproc2info + 1,
>>> sizeof(nfsv2name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V3) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
>>> - if (opt_sleep && !has_stats(srvproc3info)) {
>>> - ;
>>> - } else {
>>> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
>>> + if (!opt_sleep || has_v3_stats) {
>>> print_callstats_list("nfs v3 server",
>>> nfsv3name, srvproc3info + 1,
>>> sizeof(nfsv3name)/sizeof(char *));
>>> }
>>> }
>>> if ((opt_prt & PRNT_V4) ||
>>> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4opsinfo))) {
>>> - if (opt_sleep && !has_stats(srvproc4info)) {
>>> - ;
>>> - } else {
>>> - print_callstats_list("nfs v4 ops",
>>> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
>>> + if (!opt_sleep || has_v4_stats) {
>>> + print_callstats_list("nfs v4 server",
>>> + nfssrvproc4name, srvproc4info + 1,
>>> + sizeof(nfssrvproc4name)/sizeof(char *));
>>> + print_callstats_list("nfs v4 servop",
>>> nfssrvproc4opname, srvproc4opsinfo + 1,
>>> sizeof(nfssrvproc4opname)/sizeof(char *));
>>> }
>>> @@ -1054,9 +1040,9 @@ out:
>>> * there are stats if the sum's greater than the entry-count.
>>> */
>>> static int
>>> -has_stats(const unsigned int *info)
>>> +has_stats(const unsigned int *info, int nr)
>>> {
>>> - return (info[0] && info[info[0] + 1] > info[0]);
>>> + return (info[0] && info[nr-1] > info[0]);
>>> }
>>> static int
>>> has_rpcstats(const unsigned int *info, int size)
>>> diff --git a/utils/nfsstat/nfsstat.man b/utils/nfsstat/nfsstat.man
>>> index 52215a9..cd573b0 100644
>>> --- a/utils/nfsstat/nfsstat.man
>>> +++ b/utils/nfsstat/nfsstat.man
>>> @@ -30,10 +30,12 @@ Print only NFS v2 statistics. The default is to only print information
>>> about the versions of \fBNFS\fR that have non-zero counts.
>>> .TP
>>> .B \-3
>>> -Print only NFS v3 statistics.
>>> +Print only NFS v3 statistics. The default is to only print information
>>> +about the versions of \fBNFS\fR that have non-zero counts.
>>> .TP
>>> .B \-4
>>> -Print only NFS v4 statistics.
>>> +Print only NFS v4 statistics. The default is to only print information
>>> +about the versions of \fBNFS\fR that have non-zero counts.
>>> .TP
>>> .B \-m, \-\-mounts
>>> Print information about each of the mounted \fBNFS\fR file systems.
>>> --
>>> 1.7.3.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>

2010-11-10 16:27:08

by Jim Rees

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats

Harshula Jayasuriya wrote:

On Tue, 2010-11-09 at 14:29 -0500, Chuck Lever wrote:
> Hi-
>
> Why not use mountstats? The /proc/net/rpc/nfs is destined for deprecation, I thought.

Are you referring to /proc/self/mountstats? I didn't know about it.
nfsstat's get_stats() would have to be re-written. I can add that to my
TODO list, but not sure when I'll get to it. In the meantime, this patch
will fix the previously mentioned bugs and make the existing code a
little clearer.

We should either fix it or remove it. Leaving it in and broken seems wrong
even if the interface is depracated, especially since we have a fix.

2010-11-10 18:03:54

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats



On 11/10/2010 11:27 AM, Jim Rees wrote:
> Harshula Jayasuriya wrote:
>
> On Tue, 2010-11-09 at 14:29 -0500, Chuck Lever wrote:
> > Hi-
> >
> > Why not use mountstats? The /proc/net/rpc/nfs is destined for deprecation, I thought.
>
> Are you referring to /proc/self/mountstats? I didn't know about it.
> nfsstat's get_stats() would have to be re-written. I can add that to my
> TODO list, but not sure when I'll get to it. In the meantime, this patch
> will fix the previously mentioned bugs and make the existing code a
> little clearer.
>
> We should either fix it or remove it. Leaving it in and broken seems wrong
> even if the interface is depracated, especially since we have a fix.
Of course... I was not clear... I'll take the patch to fix the bug
but I don't think its worth whiled to teach nfsstat how read
stats from /proc/self/mountstats... I would rather put the effort into
improving both the nfsiostat and mountstats command.

steved.

2010-11-09 19:30:32

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats

Hi-

Why not use mountstats? The /proc/net/rpc/nfs is destined for deprecation, I thought.

On Nov 9, 2010, at 1:50 PM, Harshula Jayasuriya wrote:

> Hi,
>
> The NFSv4 client procs/ops in "struct rpc_procinfo nfs4_procedures" is
> used to generate the NFS client stats interface:
> ------------------------------------------------------------
> # cat /proc/net/rpc/nfs
> net 0 0 0 0
> rpc 15 0 0
> proc2 18 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
> proc3 22 0 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 1 0
> proc4 42 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
> 0 0 0
> 0 0 0 0 0 0 0
> ------------------------------------------------------------
> Note, for proc4, the number 42. That is the number of stats that follow
> on the same line. Currently nfsstat's has_stats() relies on this number
> to be equal to CLTPROC4_SZ. Unfortunately this is not the case. I have
> changed has_stats() not to rely on these two values being equal. This
> should also allow nfsstat to work with different kernel versions that
> expose a different number of NFS client ops.
>
> * Fix has_stats()
> * Stop print_clnt_list() printing server stats!
> * Describe the option -3 and -4 completely in the nfsstat manpage.
>
> Signed-off-by: Harshula Jayasuriya <[email protected]>
> ---
> utils/nfsstat/nfsstat.c | 124 ++++++++++++++++++++-------------------------
> utils/nfsstat/nfsstat.man | 6 ++-
> 2 files changed, 59 insertions(+), 71 deletions(-)
>
> diff --git a/utils/nfsstat/nfsstat.c b/utils/nfsstat/nfsstat.c
> index bacef8e..f31bb81 100644
> --- a/utils/nfsstat/nfsstat.c
> +++ b/utils/nfsstat/nfsstat.c
> @@ -46,7 +46,7 @@ static unsigned int cltproc3info[CLTPROC3_SZ+2],
> static unsigned int srvproc4info[SRVPROC4_SZ+2],
> srvproc4info_old[SRVPROC4_SZ+2]; /* NFSv4 call counts ([0] == 2) */
> static unsigned int cltproc4info[CLTPROC4_SZ+2],
> - cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 48) */
> + cltproc4info_old[CLTPROC4_SZ+2]; /* NFSv4 call counts ([0] == 49) */
> static unsigned int srvproc4opsinfo[SRVPROC4OPS_SZ+2],
> srvproc4opsinfo_old[SRVPROC4OPS_SZ+2]; /* NFSv4 call counts ([0] == 59) */
> static unsigned int srvnetinfo[5], srvnetinfo_old[5]; /* 0 # of received packets
> @@ -221,8 +221,8 @@ DECLARE_CLT(cltinfo);
> DECLARE_CLT(cltinfo, _old);
>
> static void print_all_stats(int, int, int);
> -static void print_server_stats(int, int);
> -static void print_client_stats(int, int);
> +static void print_server_stats(int);
> +static void print_client_stats(int);
> static void print_stats_list(int, int, int);
> static void print_numbers(const char *, unsigned int *,
> unsigned int);
> @@ -239,7 +239,7 @@ static int mounts(const char *);
>
> static void get_stats(const char *, struct statinfo *, int *, int,
> int);
> -static int has_stats(const unsigned int *);
> +static int has_stats(const unsigned int *, int);
> static int has_rpcstats(const unsigned int *, int);
> static void diff_stats(struct statinfo *, struct statinfo *, int);
> static void unpause(int);
> @@ -468,7 +468,7 @@ main(int argc, char **argv)
> pause();
> }
>
> - if (opt_since || opt_sleep) {
> + if (opt_since || (opt_sleep && !sleep_time)) {
> if (opt_srv) {
> get_stats(NFSSRVSTAT, serverinfo_tmp, &opt_srv, opt_clt, 1);
> diff_stats(serverinfo_tmp, serverinfo, 1);
> @@ -516,16 +516,16 @@ main(int argc, char **argv)
> static void
> print_all_stats (int opt_srv, int opt_clt, int opt_prt)
> {
> - print_server_stats(opt_srv, opt_prt);
> - print_client_stats(opt_clt, opt_prt);
> + if (opt_srv)
> + print_server_stats(opt_prt);
> +
> + if (opt_clt)
> + print_client_stats(opt_prt);
> }
>
> static void
> -print_server_stats(int opt_srv, int opt_prt)
> +print_server_stats(int opt_prt)
> {
> - if (!opt_srv)
> - return;
> -
> if (opt_prt & PRNT_NET) {
> if (opt_sleep && !has_rpcstats(srvnetinfo, 4)) {
> } else {
> @@ -582,31 +582,29 @@ print_server_stats(int opt_srv, int opt_prt)
> printf("\n");
> }
> if (opt_prt & PRNT_CALLS) {
> + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
> + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
> + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
> +
> if ((opt_prt & PRNT_V2) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
> - if (opt_sleep && !has_stats(srvproc2info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> + if (!opt_sleep || has_v2_stats) {
> print_callstats(LABEL_srvproc2,
> nfsv2name, srvproc2info + 1,
> sizeof(nfsv2name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V3) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
> - if (opt_sleep && !has_stats(srvproc3info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> + if (!opt_sleep || has_v3_stats) {
> print_callstats(LABEL_srvproc3,
> nfsv3name, srvproc3info + 1,
> sizeof(nfsv3name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V4) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4info))) {
> - if (opt_sleep && !has_stats(srvproc4info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> + if (!opt_sleep || has_v4_stats) {
> print_callstats( LABEL_srvproc4,
> nfssrvproc4name, srvproc4info + 1,
> sizeof(nfssrvproc4name)/sizeof(char *));
> @@ -618,11 +616,8 @@ print_server_stats(int opt_srv, int opt_prt)
> }
> }
> static void
> -print_client_stats(int opt_clt, int opt_prt)
> +print_client_stats(int opt_prt)
> {
> - if (!opt_clt)
> - return;
> -
> if (opt_prt & PRNT_NET) {
> if (opt_sleep && !has_rpcstats(cltnetinfo, 4)) {
> ;
> @@ -644,31 +639,28 @@ print_client_stats(int opt_clt, int opt_prt)
> }
> }
> if (opt_prt & PRNT_CALLS) {
> + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
> + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
> + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
> if ((opt_prt & PRNT_V2) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
> - if (opt_sleep && !has_stats(cltproc2info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> + if (!opt_sleep || has_v2_stats) {
> print_callstats(LABEL_cltproc2,
> nfsv2name, cltproc2info + 1,
> sizeof(nfsv2name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V3) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
> - if (opt_sleep && !has_stats(cltproc3info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> + if (!opt_sleep || has_v3_stats) {
> print_callstats(LABEL_cltproc3,
> nfsv3name, cltproc3info + 1,
> sizeof(nfsv3name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V4) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
> - if (opt_sleep && !has_stats(cltproc4info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> + if (!opt_sleep || has_v4_stats) {
> print_callstats(LABEL_cltproc4,
> nfscltproc4name, cltproc4info + 1,
> sizeof(nfscltproc4name)/sizeof(char *));
> @@ -681,34 +673,28 @@ static void
> print_clnt_list(int opt_prt)
> {
> if (opt_prt & PRNT_CALLS) {
> + int has_v2_stats = has_stats(cltproc2info, CLTPROC2_SZ+2);
> + int has_v3_stats = has_stats(cltproc3info, CLTPROC3_SZ+2);
> + int has_v4_stats = has_stats(cltproc4info, CLTPROC4_SZ+2);
> if ((opt_prt & PRNT_V2) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc2info))) {
> - if (opt_sleep && !has_stats(cltproc2info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> + if (!opt_sleep || has_v2_stats) {
> print_callstats_list("nfs v2 client",
> nfsv2name, cltproc2info + 1,
> sizeof(nfsv2name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V3) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc3info))) {
> - if (opt_sleep && !has_stats(cltproc3info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> + if (!opt_sleep || has_v3_stats) {
> print_callstats_list("nfs v3 client",
> nfsv3name, cltproc3info + 1,
> sizeof(nfsv3name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V4) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(cltproc4info))) {
> - if (opt_sleep && !has_stats(cltproc4info)) {
> - ;
> - } else {
> - print_callstats_list("nfs v4 ops",
> - nfssrvproc4opname, srvproc4opsinfo + 1,
> - sizeof(nfssrvproc4opname)/sizeof(char *));
> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> + if (!opt_sleep || has_v4_stats) {
> print_callstats_list("nfs v4 client",
> nfscltproc4name, cltproc4info + 1,
> sizeof(nfscltproc4name)/sizeof(char *));
> @@ -720,32 +706,32 @@ static void
> print_serv_list(int opt_prt)
> {
> if (opt_prt & PRNT_CALLS) {
> + int has_v2_stats = has_stats(srvproc2info, SRVPROC2_SZ+2);
> + int has_v3_stats = has_stats(srvproc3info, SRVPROC3_SZ+2);
> + int has_v4_stats = has_stats(srvproc4info, SRVPROC4_SZ+2);
> if ((opt_prt & PRNT_V2) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc2info))) {
> - if (opt_sleep && !has_stats(srvproc2info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v2_stats)) {
> + if (!opt_sleep || has_v2_stats) {
> print_callstats_list("nfs v2 server",
> nfsv2name, srvproc2info + 1,
> sizeof(nfsv2name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V3) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc3info))) {
> - if (opt_sleep && !has_stats(srvproc3info)) {
> - ;
> - } else {
> + ((opt_prt & PRNT_AUTO) && has_v3_stats)) {
> + if (!opt_sleep || has_v3_stats) {
> print_callstats_list("nfs v3 server",
> nfsv3name, srvproc3info + 1,
> sizeof(nfsv3name)/sizeof(char *));
> }
> }
> if ((opt_prt & PRNT_V4) ||
> - ((opt_prt & PRNT_AUTO) && has_stats(srvproc4opsinfo))) {
> - if (opt_sleep && !has_stats(srvproc4info)) {
> - ;
> - } else {
> - print_callstats_list("nfs v4 ops",
> + ((opt_prt & PRNT_AUTO) && has_v4_stats)) {
> + if (!opt_sleep || has_v4_stats) {
> + print_callstats_list("nfs v4 server",
> + nfssrvproc4name, srvproc4info + 1,
> + sizeof(nfssrvproc4name)/sizeof(char *));
> + print_callstats_list("nfs v4 servop",
> nfssrvproc4opname, srvproc4opsinfo + 1,
> sizeof(nfssrvproc4opname)/sizeof(char *));
> }
> @@ -1054,9 +1040,9 @@ out:
> * there are stats if the sum's greater than the entry-count.
> */
> static int
> -has_stats(const unsigned int *info)
> +has_stats(const unsigned int *info, int nr)
> {
> - return (info[0] && info[info[0] + 1] > info[0]);
> + return (info[0] && info[nr-1] > info[0]);
> }
> static int
> has_rpcstats(const unsigned int *info, int size)
> diff --git a/utils/nfsstat/nfsstat.man b/utils/nfsstat/nfsstat.man
> index 52215a9..cd573b0 100644
> --- a/utils/nfsstat/nfsstat.man
> +++ b/utils/nfsstat/nfsstat.man
> @@ -30,10 +30,12 @@ Print only NFS v2 statistics. The default is to only print information
> about the versions of \fBNFS\fR that have non-zero counts.
> .TP
> .B \-3
> -Print only NFS v3 statistics.
> +Print only NFS v3 statistics. The default is to only print information
> +about the versions of \fBNFS\fR that have non-zero counts.
> .TP
> .B \-4
> -Print only NFS v4 statistics.
> +Print only NFS v4 statistics. The default is to only print information
> +about the versions of \fBNFS\fR that have non-zero counts.
> .TP
> .B \-m, \-\-mounts
> Print information about each of the mounted \fBNFS\fR file systems.
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com





2010-11-22 17:16:23

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] nfs-utils: nfsstat: has_stats() does not function correctly for NFSv4 client stats



On 11/09/2010 01:50 PM, Harshula Jayasuriya wrote:
> Hi,
>
> The NFSv4 client procs/ops in "struct rpc_procinfo nfs4_procedures" is
> used to generate the NFS client stats interface:
> ------------------------------------------------------------
> # cat /proc/net/rpc/nfs
> net 0 0 0 0
> rpc 15 0 0
> proc2 18 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
> proc3 22 0 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 1 0
> proc4 42 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
> 0 0 0
> 0 0 0 0 0 0 0
> ------------------------------------------------------------
> Note, for proc4, the number 42. That is the number of stats that follow
> on the same line. Currently nfsstat's has_stats() relies on this number
> to be equal to CLTPROC4_SZ. Unfortunately this is not the case. I have
> changed has_stats() not to rely on these two values being equal. This
> should also allow nfsstat to work with different kernel versions that
> expose a different number of NFS client ops.
>
> * Fix has_stats()
> * Stop print_clnt_list() printing server stats!
> * Describe the option -3 and -4 completely in the nfsstat manpage.
>
> Signed-off-by: Harshula Jayasuriya <[email protected]>
Committed...

steved.