Skip to contentSkip to conversation

RFC: NFS - filelayout DS rpc mountstats support

PATCH In review Weston Andros Adamson 5 participants 11 messages Feb 8, 2012 – Feb 14, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 RFC: NFS - filelayout DS rpc mountstats support

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

3) currently this will display stats twice when DS == MDS.

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

 fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
 fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
 fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
 fs/nfs/super.c             |    1 +
 include/linux/nfs_iostat.h |    2 +-
 5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
  * Remove a deviceid from cache
  *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
  * NFS byte counters
Signature · 1 line
1.7.4.4
#1 lore raw

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

Quoted 13 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Quoted 4 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Quoted 7 lines

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Quoted 2 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Agree.

Quoted 177 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#2 lore raw
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 10 lines
Quoted 8 lines

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 5 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#3 lore raw
Quoted 29 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 16 lines
Quoted 16 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

This method has issues:

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

What do you think should happen if statsvers > supported? Just print an error, or print a warning and try to parse anyway?

Thanks!

-dros

Quoted 235 lines
Quoted 4 lines
Quoted 4 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 10 lines
Quoted 8 lines

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 5 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#4 lore raw
Quoted 36 lines · Dros
Quoted 33 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 20 lines
Quoted 19 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

User land is broken then. :-)

Agreed!

What do you think should happen if statsvers > supported? Just print an error, or print a warning and try to parse anyway?

Hard to say. Do we want it to go on and possibly print wrong data, or stop and insist on an update? I prefer the latter, but some may not. I think distributions should do integration testing to prevent the latter from occurring on a customer's system.

Quoted 241 lines

Thanks!

-dros

Quoted 235 lines
Quoted 4 lines
Quoted 4 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 10 lines
Quoted 8 lines

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 5 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#5 lore raw
In reply to Adamson, Dros

Sorry for the late response.

Quoted 54 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 4 lines
Quoted 4 lines

So... What should I do here?

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Benny

Quoted 206 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 5 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#6 lore raw
Quoted 67 lines · Benny Halevy

Sorry for the late response.

Quoted 64 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 10 lines
Quoted 8 lines

So... What should I do here?

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 1 line
Quoted 7 lines
Quoted 1 line
Quoted 1 line

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Benny

Agreed! It just can't go into mountstats right away (due to statsvers= not being interpreted in mountstats(8)). I'll make a second patch on top of my mountstats changes to add per-DS stats in /proc/fs/nfsfs/ and we can figure out if we want to add it to mountstats later.

Does this sound OK?

-dros

Quoted 210 lines
Quoted 11 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 4 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Quoted 195 lines
Quoted 1 line

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

--
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

#7 lore raw
Quoted 75 lines · Dros
Quoted 72 lines · Benny Halevy

Sorry for the late response.

Quoted 54 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 4 lines
Quoted 4 lines

So... What should I do here?

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Quoted 6 lines
Quoted 2 lines

Benny

Agreed! It just can't go into mountstats right away (due to statsvers= not being interpreted in mountstats(8)). I'll make a second patch on top of my mountstats changes to add per-DS stats in /proc/fs/nfsfs/ and we can figure out if we want to add it to mountstats later.

Does this sound OK?

Absolutely. Thanks!

Benny

Quoted 214 lines

-dros

Quoted 210 lines
Quoted 11 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 4 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Quoted 195 lines
Quoted 1 line

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 178 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	u32 i;
+
+	dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+	for (i = 0; i < dsaddr->ds_num; i++) {
+		ds = dsaddr->ds_list[i];
+
+		if (ds && ds->ds_clp) {
+			seq_printf(m, "  pnfs dataserver %s\n",
+				   ds->ds_remotestr);
+			rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+		}
+	}
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
 	.free_deviceid_node	= filelayout_free_deveiceid_node,
+	.ds_rpc_print_iostats	= filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	void (*ds_rpc_print_iostats) (struct seq_file *,
+				      struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				     const struct nfs_client *clp,
+				     const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+				const struct nfs_client *clp,
+				const struct pnfs_layoutdriver_type *ld)
+{
+	struct nfs4_deviceid_node *d;
+	struct hlist_node *n;
+	long h;
+
+	if (!ld->ds_rpc_print_iostats)
+		return;
+
+	rcu_read_lock();
+	for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+		hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+			if (d->ld == ld && d->nfs_client == clp) {
+				if (atomic_read(&d->ref))
+					ld->ds_rpc_print_iostats(seq, d);
+			}
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 	seq_printf(m, "\n");
 
 	rpc_print_iostats(m, nfss->client);
+	pnfs_rpc_print_iostats(m, nfss);
 
 	return 0;
 }
include/linux/nfs_iostat.h +1 -1
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS		"1.0"
+#define NFS_IOSTAT_VERS		"2.0"
 
 /*
 * NFS byte counters
Signature · 1 line
1.7.4.4
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

--
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

#8 lore raw

I have an early patch set that prints session stats for MDS and for DS. The DS pnfs_layoutdriver_type interface for session stats takes the same arguments as the RPC iostats interface, a seq_file* and a struct nfs4_deviceid_node.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

+ void (*ds_rpc_print_iostats) (struct seq_file *,
+                               struct nfs4_deviceid_node *);

We can use the same interface for RPC and session stats. I suggest you rename this to "ds_stats" and add an enum type to identify which stats are being requested.

-->Andy

Quoted 305 lines · Benny Halevy
Quoted 75 lines · Dros
Quoted 72 lines · Benny Halevy

Sorry for the late response.

Quoted 54 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client.  This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then.  :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 4 lines
Quoted 4 lines

So... What should I do here?

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client.  This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Quoted 6 lines
Quoted 2 lines

Benny

Agreed!  It just can't go into mountstats right away (due to statsvers= not being interpreted in mountstats(8)).  I'll make a second patch on top of my mountstats changes to add per-DS stats in /proc/fs/nfsfs/ and we can figure out if we want to add it to mountstats later.

Does this sound OK?

Absolutely.  Thanks!

Benny

Quoted 214 lines

-dros

Quoted 210 lines
Quoted 11 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line?  Obviously the different underlying rpc_clients will have different values.  I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go.  Do we keep it in /proc/self/mountstats?  I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 4 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Quoted 195 lines
Quoted 1 line

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 176 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++ fs/nfs/pnfs.h              |   20 ++++++++++++++++++++ fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++ fs/nfs/super.c             |    1 + include/linux/nfs_iostat.h |    2 +-

5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
   nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+ struct nfs4_file_layout_dsaddr *dsaddr;
+ struct nfs4_pnfs_ds *ds;
+ u32 i;
+
+ dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+ for (i = 0; i < dsaddr->ds_num; i++) {
+         ds = dsaddr->ds_list[i];
+
+         if (ds && ds->ds_clp) {
+                 seq_printf(m, "  pnfs dataserver %s\n",
+                            ds->ds_remotestr);
+                 rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+         }
+ }
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
   .id                     = LAYOUT_NFSV4_1_FILES,
   .name                   = "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
   .read_pagelist          = filelayout_read_pagelist,
   .write_pagelist         = filelayout_write_pagelist,
   .free_deviceid_node     = filelayout_free_deveiceid_node,
+ .ds_rpc_print_iostats   = filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
   void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
                                struct xdr_stream *xdr,
                                const struct nfs4_layoutcommit_args *args);
+
+ void (*ds_rpc_print_iostats) (struct seq_file *,
+                               struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                              const struct nfs_client *clp,
+                              const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
   return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+ if (!pnfs_enabled_sb(nfss))
+         return;
+ nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+                                 nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                         const struct nfs_client *clp,
+                         const struct pnfs_layoutdriver_type *ld)
+{
+ struct nfs4_deviceid_node *d;
+ struct hlist_node *n;
+ long h;
+
+ if (!ld->ds_rpc_print_iostats)
+         return;
+
+ rcu_read_lock();
+ for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+         hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+                 if (d->ld == ld && d->nfs_client == clp) {
+                         if (atomic_read(&d->ref))
+                                 ld->ds_rpc_print_iostats(seq, d);
+                 }
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
   seq_printf(m, "\n");
 
   rpc_print_iostats(m, nfss->client);
+ pnfs_rpc_print_iostats(m, nfss);
 
   return 0;
 }
include/linux/nfs_iostat.h +1 -2
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS          "1.0"
+#define NFS_IOSTAT_VERS          "2.0"
 
 /*
 * NFS byte counters
--
Quoted 2 lines

1.7.4.4

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

--
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

--
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

#9 lore raw
Quoted 5 lines · Andy Adamson

I have an early patch set that prints session stats for MDS and for DS. The DS pnfs_layoutdriver_type interface for session stats takes the same arguments as the RPC iostats interface, a seq_file* and a struct nfs4_deviceid_node.

Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

+ void (*ds_rpc_print_iostats) (struct seq_file *,
+ struct nfs4_deviceid_node *);

We can use the same interface for RPC and session stats. I suggest you rename this to "ds_stats" and add an enum type to identify which stats are being requested.

-->Andy

Well, this patch is dead. I reposted a better implementation yesterday (based on Trond's suggestions) that doesn't have to iterate through the DSs, but we can work together to make a common interface for more stats. Once we display per-DS rpc_iostats, there is no getting around the need to iterate through the DSs (like your session stats). I plan on looking at this later today.

-dros

Quoted 306 lines · Benny Halevy
Quoted 305 lines · Dros
Quoted 75 lines · Benny Halevy
Quoted 73 lines

Sorry for the late response.

Quoted 54 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client. This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then. :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 4 lines
Quoted 4 lines

So... What should I do here?

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client. This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Quoted 6 lines
Quoted 2 lines

Benny

Agreed! It just can't go into mountstats right away (due to statsvers= not being interpreted in mountstats(8)). I'll make a second patch on top of my mountstats changes to add per-DS stats in /proc/fs/nfsfs/ and we can figure out if we want to add it to mountstats later.

Does this sound OK?

Absolutely. Thanks!

Benny

Quoted 214 lines

-dros

Quoted 210 lines
Quoted 11 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line? Obviously the different underlying rpc_clients will have different values. I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go. Do we keep it in /proc/self/mountstats? I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 4 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Quoted 195 lines
Quoted 1 line

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 176 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++
fs/nfs/pnfs.h              |   20 ++++++++++++++++++++
fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++
fs/nfs/super.c             |    1 +
include/linux/nfs_iostat.h |    2 +-
5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
  nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+ struct nfs4_file_layout_dsaddr *dsaddr;
+ struct nfs4_pnfs_ds *ds;
+ u32 i;
+
+ dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+ for (i = 0; i < dsaddr->ds_num; i++) {
+         ds = dsaddr->ds_list[i];
+
+         if (ds && ds->ds_clp) {
+                 seq_printf(m, "  pnfs dataserver %s\n",
+                            ds->ds_remotestr);
+                 rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+         }
+ }
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
  .id                     = LAYOUT_NFSV4_1_FILES,
  .name                   = "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
  .read_pagelist          = filelayout_read_pagelist,
  .write_pagelist         = filelayout_write_pagelist,
  .free_deviceid_node     = filelayout_free_deveiceid_node,
+ .ds_rpc_print_iostats   = filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
  void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
                               struct xdr_stream *xdr,
                               const struct nfs4_layoutcommit_args *args);
+
+ void (*ds_rpc_print_iostats) (struct seq_file *,
+                               struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                              const struct nfs_client *clp,
+                              const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
  return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+ if (!pnfs_enabled_sb(nfss))
+         return;
+ nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+                                 nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                         const struct nfs_client *clp,
+                         const struct pnfs_layoutdriver_type *ld)
+{
+ struct nfs4_deviceid_node *d;
+ struct hlist_node *n;
+ long h;
+
+ if (!ld->ds_rpc_print_iostats)
+         return;
+
+ rcu_read_lock();
+ for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+         hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+                 if (d->ld == ld && d->nfs_client == clp) {
+                         if (atomic_read(&d->ref))
+                                 ld->ds_rpc_print_iostats(seq, d);
+                 }
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
  seq_printf(m, "\n");
 
  rpc_print_iostats(m, nfss->client);
+ pnfs_rpc_print_iostats(m, nfss);
 
  return 0;
 }
include/linux/nfs_iostat.h +1 -2
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS          "1.0"
+#define NFS_IOSTAT_VERS          "2.0"
 
 /*
 * NFS byte counters
--
Quoted 2 lines

1.7.4.4

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

--
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

--
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

#10 lore raw
Quoted 15 lines · Dros
Quoted 12 lines · Andy Adamson

I have an early patch set that prints session stats for MDS and for DS. The DS pnfs_layoutdriver_type interface for session stats takes the same arguments as the RPC iostats interface, a seq_file* and a struct nfs4_deviceid_node.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

+ void (*ds_rpc_print_iostats) (struct seq_file *,
+                               struct nfs4_deviceid_node *);

We can use the same interface for RPC and session stats.  I suggest you rename this to "ds_stats" and add an enum type to identify which stats are being requested.

Quoted 5 lines
Quoted 2 lines

-->Andy

Well, this patch is dead.  I reposted a better implementation yesterday (based on Trond's suggestions) that doesn't have to iterate through the DSs, but we can work together to make a common interface for more stats.  Once we display per-DS rpc_iostats, there is no getting around the need to iterate through the DSs (like your session stats).  I plan on looking at this later today.

OK - cool.

-->Andy

Quoted 311 lines

-dros

Quoted 306 lines · Benny Halevy
Quoted 305 lines · Dros
Quoted 75 lines · Benny Halevy
Quoted 73 lines

Sorry for the late response.

Quoted 54 lines · Dros
Quoted 7 lines · Chuck Lever

Hi-

Quoted 1 line · Weston Andros Adamson

The per-op stats displayed in /self/proc/mountstats are collected in the sunrpc

You mean /proc/self/mountstats here.

oops!

Quoted 14 lines
Quoted 14 lines

layer and are collected per rpc_client.  This has worked for nfs thus far since only one nfs_client was ever associated with a mountpoint, but with NFS4.1+PNFS+filelayout an nfs mount can have more than one nfs_client.

This can be seen with a pnfs mount where no data server is the same as the MDS - all reads, writes and commits will never make it into the current mountstats output.

I took the approach of keeping the stats from the MDS and DSs separate in the /proc/self/mountstats output to avoid doing too much in the kernel, and to expose the fact that these stats are from separate connections (maybe for later use).

Quoted 7 lines
Quoted 5 lines

This method has issues:

1) even changing the "statvers" of mountstats doesn't stop the userland mountstats(8) from trying to parse this output -- and it does so incorrectly.

User land is broken then.  :-)

Agreed!

Quoted 3 lines
Quoted 3 lines

2) when using DS multipath this method fails to count operations that happened on a different path to the same DS (after a failure).

I should have noted that this isn't a problem yet because I haven't submitted the multipath failover patches :)

Quoted 4 lines
Quoted 2 lines

3) currently this will display stats twice when DS == MDS.

Why is this case hard to detect in the kernel or in mountstats?

Not hard, just an issue with the current patch.

Quoted 4 lines
Quoted 4 lines

So... What should I do here?

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines

A different approach is to add support in net/sunrpc to specify a "parent" stat structure so that operations on DS nfs_clients bump stats on the main nfs_server->nfs_client->rpc_client.  This would take care of all the issues with the current patch, but seems like a hack.

Is this the same as displaying all data in one set of stats?

Yes

Quoted 2 lines

Seems like that might be the best next step, then we can split up the MDS and DS stats at some later point.

Splitting up the MDS/DS stats is important to tell when DS I/O is being done and how effective it is.

Quoted 6 lines
Quoted 2 lines

Benny

Agreed!  It just can't go into mountstats right away (due to statsvers= not being interpreted in mountstats(8)).  I'll make a second patch on top of my mountstats changes to add per-DS stats in /proc/fs/nfsfs/ and we can figure out if we want to add it to mountstats later.

Does this sound OK?

Absolutely.  Thanks!

Benny

Quoted 214 lines

-dros

Quoted 210 lines
Quoted 11 lines

Ok, I'll try to do that.

Any advice on how to handle the "xprt: …" line?  Obviously the different underlying rpc_clients will have different values.  I'd have to look at mountstats(8) some more, but I *think* we could just make it a comma separated list.

For the 'next step', we can use this patch - we just need to figure out where it should go.  Do we keep it in /proc/self/mountstats?  I think because of problem #2 we might want to move them out to somewhere in /proc/fs/nfsfs/ along with other per-DS statistics.

Quoted 4 lines
Quoted 3 lines

One task that seems like a good thing to do is to fix mountstats(8) to respect the "statsvers" value.

Quoted 195 lines
Quoted 1 line

Agree.

I'll do this first.

Thanks Chuck!

-dros

Quoted 186 lines
Quoted 176 lines

Thoughts?
---

I cleaned up this patch, but I still have reservations (noted in commit message)

fs/nfs/nfs4filelayout.c    |   23 +++++++++++++++++++++++ fs/nfs/pnfs.h              |   20 ++++++++++++++++++++ fs/nfs/pnfs_dev.c          |   25 +++++++++++++++++++++++++ fs/nfs/super.c             |    1 + include/linux/nfs_iostat.h |    2 +-

5 files changed, 70 insertions(+), 1 deletions(-)
fs/nfs/nfs4filelayout.c +23 -0
index 79be7ac..9410fd0 100644
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -918,6 +920,26 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
   nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static void
+filelayout_rpc_print_iostats(struct seq_file *m, struct nfs4_deviceid_node *d)
+{
+ struct nfs4_file_layout_dsaddr *dsaddr;
+ struct nfs4_pnfs_ds *ds;
+ u32 i;
+
+ dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
+
+ for (i = 0; i < dsaddr->ds_num; i++) {
+         ds = dsaddr->ds_list[i];
+
+         if (ds && ds->ds_clp) {
+                 seq_printf(m, "  pnfs dataserver %s\n",
+                            ds->ds_remotestr);
+                 rpc_print_iostats(m, ds->ds_clp->cl_rpcclient);
+         }
+ }
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
   .id                     = LAYOUT_NFSV4_1_FILES,
   .name                   = "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +954,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
   .read_pagelist          = filelayout_read_pagelist,
   .write_pagelist         = filelayout_write_pagelist,
   .free_deviceid_node     = filelayout_free_deveiceid_node,
+ .ds_rpc_print_iostats   = filelayout_rpc_print_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
fs/nfs/pnfs.h +20 -0
index 53d593a..0a5e020 100644
@@ -119,6 +119,9 @@ struct pnfs_layoutdriver_type {
   void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
                                struct xdr_stream *xdr,
                                const struct nfs4_layoutcommit_args *args);
+
+ void (*ds_rpc_print_iostats) (struct seq_file *,
+                               struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +242,10 @@ void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
 struct nfs4_deviceid_node *nfs4_insert_deviceid_node(struct nfs4_deviceid_node *);
 bool nfs4_put_deviceid_node(struct nfs4_deviceid_node *);
 void nfs4_deviceid_purge_client(const struct nfs_client *);
+void nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                              const struct nfs_client *clp,
+                              const struct pnfs_layoutdriver_type *ld);
+
 
 static inline int lo_fail_bit(u32 iomode)
 {
@@ -328,6 +335,14 @@ static inline int pnfs_return_layout(struct inode *ino)
   return 0;
 }
 
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+ if (!pnfs_enabled_sb(nfss))
+         return;
+ nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+                                 nfss->pnfs_curr_ld);
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +444,11 @@ static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 static inline void nfs4_deviceid_purge_client(struct nfs_client *ncl)
 {
 }
+
+static inline void
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
fs/nfs/pnfs_dev.c +25 -0
index 4f359d2..277de87 100644
@@ -115,6 +115,31 @@ nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
 }
 EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
 
+
+void
+nfs4_deviceid_rpc_print_iostats(struct seq_file *seq,
+                         const struct nfs_client *clp,
+                         const struct pnfs_layoutdriver_type *ld)
+{
+ struct nfs4_deviceid_node *d;
+ struct hlist_node *n;
+ long h;
+
+ if (!ld->ds_rpc_print_iostats)
+         return;
+
+ rcu_read_lock();
+ for (h = 0; h < NFS4_DEVICE_ID_HASH_SIZE; h++) {
+         hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[h], node)
+                 if (d->ld == ld && d->nfs_client == clp) {
+                         if (atomic_read(&d->ref))
+                                 ld->ds_rpc_print_iostats(seq, d);
+                 }
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
 * Remove a deviceid from cache
 *
fs/nfs/super.c +1 -0
index d18a90b..453e496 100644
@@ -871,6 +871,7 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
   seq_printf(m, "\n");
 
   rpc_print_iostats(m, nfss->client);
+ pnfs_rpc_print_iostats(m, nfss);
 
   return 0;
 }
include/linux/nfs_iostat.h +1 -2
index 8866bb3..7fe4605 100644
@@ -21,7 +21,7 @@
 #ifndef _LINUX_NFS_IOSTAT
 #define _LINUX_NFS_IOSTAT
 
-#define NFS_IOSTAT_VERS          "1.0"
+#define NFS_IOSTAT_VERS          "2.0"
 
 /*
 * NFS byte counters
--
Quoted 2 lines

1.7.4.4

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

--
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

--
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