[PATCH] NFS: include filelayout DS rpc stats in mountstats

[PATCH] NFS: include filelayout DS rpc stats in mountstats

[Weston Andros Adamson]

Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
filelayout mounts.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
Cleaned up version after comments from Trond.  This patch accumulates 
reads, writes and commits (the only DS operations) on the rpc_iostats of 
the nfs_server associated with the mount.

I was confused last week about where we breaking counting stats only for a 
mountpoint -- What I was seeing was two mounts sharing stats if they were the
same server AND the same filesystem on the server.  This patch does not address
that issue.

This patch still counts DS operations on the DS::nfs_client::rpc_clnt's stats
structures.  Assuming this patch is OK, I plan on submitting a patch that will 
show MDS-only and individual DS stats somewhere in /proc/fs/nfsfs/.

 fs/nfs/nfs4filelayout.c        |   19 +++++++++++++++++++
 include/linux/sunrpc/metrics.h |    6 ++++--
 include/linux/sunrpc/sched.h   |    1 +
 net/sunrpc/stats.c             |    8 ++++----
 net/sunrpc/xprt.c              |    5 ++++-
 5 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 79be7ac..47e8f34 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -189,6 +191,13 @@ static void filelayout_read_call_done(struct rpc_task *task, void *data)
 	rdata->mds_ops->rpc_call_done(task, data);
 }
 
+static void filelayout_read_count_stats(struct rpc_task *task, void *data)
+{
+	struct nfs_read_data *rdata = (struct nfs_read_data *)data;
+
+	rpc_count_iostats(task, NFS_SERVER(rdata->inode)->client->cl_metrics);
+}
+
 static void filelayout_read_release(void *data)
 {
 	struct nfs_read_data *rdata = (struct nfs_read_data *)data;
@@ -268,6 +277,13 @@ static void filelayout_write_call_done(struct rpc_task *task, void *data)
 	wdata->mds_ops->rpc_call_done(task, data);
 }
 
+static void filelayout_write_count_stats(struct rpc_task *task, void *data)
+{
+	struct nfs_write_data *wdata = (struct nfs_write_data *)data;
+
+	rpc_count_iostats(task, NFS_SERVER(wdata->inode)->client->cl_metrics);
+}
+
 static void filelayout_write_release(void *data)
 {
 	struct nfs_write_data *wdata = (struct nfs_write_data *)data;
@@ -288,18 +304,21 @@ static void filelayout_commit_release(void *data)
 struct rpc_call_ops filelayout_read_call_ops = {
 	.rpc_call_prepare = filelayout_read_prepare,
 	.rpc_call_done = filelayout_read_call_done,
+	.rpc_count_stats = filelayout_read_count_stats,
 	.rpc_release = filelayout_read_release,
 };
 
 struct rpc_call_ops filelayout_write_call_ops = {
 	.rpc_call_prepare = filelayout_write_prepare,
 	.rpc_call_done = filelayout_write_call_done,
+	.rpc_count_stats = filelayout_write_count_stats,
 	.rpc_release = filelayout_write_release,
 };
 
 struct rpc_call_ops filelayout_commit_call_ops = {
 	.rpc_call_prepare = filelayout_write_prepare,
 	.rpc_call_done = filelayout_write_call_done,
+	.rpc_count_stats = filelayout_write_count_stats,
 	.rpc_release = filelayout_commit_release,
 };
 
diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
index b6edbc0..1565bbe 100644
--- a/include/linux/sunrpc/metrics.h
+++ b/include/linux/sunrpc/metrics.h
@@ -74,14 +74,16 @@ struct rpc_clnt;
 #ifdef CONFIG_PROC_FS
 
 struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
-void			rpc_count_iostats(struct rpc_task *);
+void			rpc_count_iostats(const struct rpc_task *,
+					  struct rpc_iostats *);
 void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
 void			rpc_free_iostats(struct rpc_iostats *);
 
 #else  /*  CONFIG_PROC_FS  */
 
 static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
-static inline void rpc_count_iostats(struct rpc_task *task) {}
+static inline void rpc_count_iostats(const struct rpc_task *task,
+				     struct rpc_iostats *stats) {}
 static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
 static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
 
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 22dfc24..dc0c3cc 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -103,6 +103,7 @@ typedef void			(*rpc_action)(struct rpc_task *);
 struct rpc_call_ops {
 	void (*rpc_call_prepare)(struct rpc_task *, void *);
 	void (*rpc_call_done)(struct rpc_task *, void *);
+	void (*rpc_count_stats)(struct rpc_task *, void *);
 	void (*rpc_release)(void *);
 };
 
diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index 3c4f688..1eb3304 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -133,20 +133,19 @@ EXPORT_SYMBOL_GPL(rpc_free_iostats);
 /**
  * rpc_count_iostats - tally up per-task stats
  * @task: completed rpc_task
+ * @stats: array of stat structures
  *
  * Relies on the caller for serialization.
  */
-void rpc_count_iostats(struct rpc_task *task)
+void rpc_count_iostats(const struct rpc_task *task, struct rpc_iostats *stats)
 {
 	struct rpc_rqst *req = task->tk_rqstp;
-	struct rpc_iostats *stats;
 	struct rpc_iostats *op_metrics;
 	ktime_t delta;
 
-	if (!task->tk_client || !task->tk_client->cl_metrics || !req)
+	if (!stats || !req)
 		return;
 
-	stats = task->tk_client->cl_metrics;
 	op_metrics = &stats[task->tk_msg.rpc_proc->p_statidx];
 
 	op_metrics->om_ops++;
@@ -164,6 +163,7 @@ void rpc_count_iostats(struct rpc_task *task)
 	delta = ktime_sub(ktime_get(), task->tk_start);
 	op_metrics->om_execute = ktime_add(op_metrics->om_execute, delta);
 }
+EXPORT_SYMBOL_GPL(rpc_count_iostats);
 
 static void _print_name(struct seq_file *seq, unsigned int op,
 			struct rpc_procinfo *procs)
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index efe5495..ab7a28f 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1132,7 +1132,10 @@ void xprt_release(struct rpc_task *task)
 		return;
 
 	xprt = req->rq_xprt;
-	rpc_count_iostats(task);
+	if (task->tk_client)
+		rpc_count_iostats(task, task->tk_client->cl_metrics);
+	if (task->tk_ops->rpc_count_stats != NULL)
+		task->tk_ops->rpc_count_stats(task, task->tk_calldata);
 	spin_lock_bh(&xprt->transport_lock);
 	xprt->ops->release_xprt(xprt, task);
 	if (xprt->ops->release_request)
-- 
1.7.4.4

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTAyLTEzIGF0IDE3OjIyIC0wNTAwLCBXZXN0b24gQW5kcm9zIEFkYW1zb24g
d3JvdGU6DQo+IGRpZmYgLS1naXQgYS9uZXQvc3VucnBjL3hwcnQuYyBiL25ldC9zdW5ycGMveHBy
dC5jDQo+IGluZGV4IGVmZTU0OTUuLmFiN2EyOGYgMTAwNjQ0DQo+IC0tLSBhL25ldC9zdW5ycGMv
eHBydC5jDQo+ICsrKyBiL25ldC9zdW5ycGMveHBydC5jDQo+IEBAIC0xMTMyLDcgKzExMzIsMTAg
QEAgdm9pZCB4cHJ0X3JlbGVhc2Uoc3RydWN0IHJwY190YXNrICp0YXNrKQ0KPiAgCQlyZXR1cm47
DQo+ICANCj4gIAl4cHJ0ID0gcmVxLT5ycV94cHJ0Ow0KPiAtCXJwY19jb3VudF9pb3N0YXRzKHRh
c2spOw0KPiArCWlmICh0YXNrLT50a19jbGllbnQpDQo+ICsJCXJwY19jb3VudF9pb3N0YXRzKHRh
c2ssIHRhc2stPnRrX2NsaWVudC0+Y2xfbWV0cmljcyk7DQo+ICsJaWYgKHRhc2stPnRrX29wcy0+
cnBjX2NvdW50X3N0YXRzICE9IE5VTEwpDQo+ICsJCXRhc2stPnRrX29wcy0+cnBjX2NvdW50X3N0
YXRzKHRhc2ssIHRhc2stPnRrX2NhbGxkYXRhKTsNCj4gIAlzcGluX2xvY2tfYmgoJnhwcnQtPnRy
YW5zcG9ydF9sb2NrKTsNCj4gIAl4cHJ0LT5vcHMtPnJlbGVhc2VfeHBydCh4cHJ0LCB0YXNrKTsN
Cj4gIAlpZiAoeHBydC0+b3BzLT5yZWxlYXNlX3JlcXVlc3QpDQoNCjIgbml0czoNCg0KICAgICAx
LiBBcmVuJ3Qgd2UgZ3VhcmFudGVlZCB0aGF0IHRhc2stPnRrX2NsaWVudCAhPSBOVUxMIGluDQog
ICAgICAgIHhwcnRfcmVsZWFzZSgpPw0KICAgICAyLiBTaG91bGRuJ3Qgd2UgYmUgY2FsbGluZyBf
ZWl0aGVyXyBycGNfY291bnRfaW9zdGF0cygpLCBfb3JfDQogICAgICAgIHRhc2stPnRrX29wcy0+
cnBjX2NvdW50X3N0YXRzKCk/IEFzIGZhciBhcyBJIGNhbiBzZWUsIHRoZQ0KICAgICAgICBuZnNz
dGF0IG91dHB1dCB3aWxsIG5vdyBiZSBkb3VibGUtY291bnRpbmcgYW5kIHBORlMgcmVhZHMsDQog
ICAgICAgIHdyaXRlcyBhbmQgY29tbWl0cyB0aGF0IGFyZSBzZW50IHRvIHRoZSBEUy4NCg0KQ2hl
ZXJzDQogIFRyb25kDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1h
aW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFw
cC5jb20NCg0K

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]


On Feb 16, 2012, at 2:48 PM, Myklebust, Trond wrote:

> On Mon, 2012-02-13 at 17:22 -0500, Weston Andros Adamson wrote:
>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>> index efe5495..ab7a28f 100644
>> --- a/net/sunrpc/xprt.c
>> +++ b/net/sunrpc/xprt.c
>> @@ -1132,7 +1132,10 @@ void xprt_release(struct rpc_task *task)
>> 		return;
>> 
>> 	xprt = req->rq_xprt;
>> -	rpc_count_iostats(task);
>> +	if (task->tk_client)
>> +		rpc_count_iostats(task, task->tk_client->cl_metrics);
>> +	if (task->tk_ops->rpc_count_stats != NULL)
>> +		task->tk_ops->rpc_count_stats(task, task->tk_calldata);
>> 	spin_lock_bh(&xprt->transport_lock);
>> 	xprt->ops->release_xprt(xprt, task);
>> 	if (xprt->ops->release_request)
> 
> 2 nits:
> 
>     1. Aren't we guaranteed that task->tk_client != NULL in
>        xprt_release()?

I wasn't sure -- there is no other reference to tk_client in the xprt_release() path.
It worked fine for *me* without the if statement, but the old rpc_count_iostats (of which this was the only caller) checked if tk_client was NULL and I didn't want to break anything.

>     2. Shouldn't we be calling _either_ rpc_count_iostats(), _or_
>        task->tk_ops->rpc_count_stats()? As far as I can see, the
>        nfsstat output will now be double-counting and pNFS reads,
>        writes and commits that are sent to the DS.


No, this doesn't double count with nfsstats.  nfsstats uses rpc_clnt::cl_stats (and not rpc_clnt::cl_metrics).

I probably should have done an if/else for this patch -- with the current code, the DSs' rpc_clnt:cl_metrics will never be used.
I left it accumulating here because we want to have per-DS stats eventually and my plan was to print out stats in /proc/fs/nfsfs per *client* (so not separated by mountpoint).

I can repost with if/else, but looking at this some more made me realize that we are *still* doing this wrong :)

The callback method in this patch fails to accumulate stats for operations to the DS other than read/write/commit -- that seems right, but what about null, exchange_id, session heartbeats, etc?

In order to properly accumulate those we are back to two obvious choices:
  1) add a count_iostats callback to the ~25 other rpc_call_ops in nfs-land (yuck)
  2) add an 'additional stats' pointer to the rpc_task structure (trond already said you don't want to add to task struct)

Or do we just not care about displaying those operations?  For my purposes (nfsometer perf testing), it'd be nice to have *all* of the operations.

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]


On Feb 16, 2012, at 3:44 PM, Adamson, Dros wrote:

> 
> On Feb 16, 2012, at 2:48 PM, Myklebust, Trond wrote:
> 
>> On Mon, 2012-02-13 at 17:22 -0500, Weston Andros Adamson wrote:
>>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>>> index efe5495..ab7a28f 100644
>>> --- a/net/sunrpc/xprt.c
>>> +++ b/net/sunrpc/xprt.c
>>> @@ -1132,7 +1132,10 @@ void xprt_release(struct rpc_task *task)
>>> 		return;
>>> 
>>> 	xprt = req->rq_xprt;
>>> -	rpc_count_iostats(task);
>>> +	if (task->tk_client)
>>> +		rpc_count_iostats(task, task->tk_client->cl_metrics);
>>> +	if (task->tk_ops->rpc_count_stats != NULL)
>>> +		task->tk_ops->rpc_count_stats(task, task->tk_calldata);
>>> 	spin_lock_bh(&xprt->transport_lock);
>>> 	xprt->ops->release_xprt(xprt, task);
>>> 	if (xprt->ops->release_request)
>> 
>> 2 nits:
>> 
>>    1. Aren't we guaranteed that task->tk_client != NULL in
>>       xprt_release()?
> 
> I wasn't sure -- there is no other reference to tk_client in the xprt_release() path.
> It worked fine for *me* without the if statement, but the old rpc_count_iostats (of which this was the only caller) checked if tk_client was NULL and I didn't want to break anything.
> 
>>    2. Shouldn't we be calling _either_ rpc_count_iostats(), _or_
>>       task->tk_ops->rpc_count_stats()? As far as I can see, the
>>       nfsstat output will now be double-counting and pNFS reads,
>>       writes and commits that are sent to the DS.
> 
> 
> No, this doesn't double count with nfsstats.  nfsstats uses rpc_clnt::cl_stats (and not rpc_clnt::cl_metrics).
> 
> I probably should have done an if/else for this patch -- with the current code, the DSs' rpc_clnt:cl_metrics will never be used.
> I left it accumulating here because we want to have per-DS stats eventually and my plan was to print out stats in /proc/fs/nfsfs per *client* (so not separated by mountpoint).
> 
> I can repost with if/else, but looking at this some more made me realize that we are *still* doing this wrong :)
> 
> The callback method in this patch fails to accumulate stats for operations to the DS other than read/write/commit -- that seems right, but what about null, exchange_id, session heartbeats, etc?
> 
> In order to properly accumulate those we are back to two obvious choices:
>  1) add a count_iostats callback to the ~25 other rpc_call_ops in nfs-land (yuck)

^^ i guess it wouldn't have to be all of them, but this gets ugly quick.

>  2) add an 'additional stats' pointer to the rpc_task structure (trond already said you don't want to add to task struct)
> 
> Or do we just not care about displaying those operations?  For my purposes (nfsometer perf testing), it'd be nice to have *all* of the operations.
> 
> -dros


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTE2IGF0IDIwOjQ0ICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiBPbiBGZWIgMTYsIDIwMTIsIGF0IDI6NDggUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+
ID4gICAgIDIuIFNob3VsZG4ndCB3ZSBiZSBjYWxsaW5nIF9laXRoZXJfIHJwY19jb3VudF9pb3N0
YXRzKCksIF9vcl8NCj4gPiAgICAgICAgdGFzay0+dGtfb3BzLT5ycGNfY291bnRfc3RhdHMoKT8g
QXMgZmFyIGFzIEkgY2FuIHNlZSwgdGhlDQo+ID4gICAgICAgIG5mc3N0YXQgb3V0cHV0IHdpbGwg
bm93IGJlIGRvdWJsZS1jb3VudGluZyBhbmQgcE5GUyByZWFkcywNCj4gPiAgICAgICAgd3JpdGVz
IGFuZCBjb21taXRzIHRoYXQgYXJlIHNlbnQgdG8gdGhlIERTLg0KPiANCj4gDQo+IE5vLCB0aGlz
IGRvZXNuJ3QgZG91YmxlIGNvdW50IHdpdGggbmZzc3RhdHMuICBuZnNzdGF0cyB1c2VzIHJwY19j
bG50OjpjbF9zdGF0cyAoYW5kIG5vdCBycGNfY2xudDo6Y2xfbWV0cmljcykuDQo+IA0KPiBJIHBy
b2JhYmx5IHNob3VsZCBoYXZlIGRvbmUgYW4gaWYvZWxzZSBmb3IgdGhpcyBwYXRjaCAtLSB3aXRo
IHRoZSBjdXJyZW50IGNvZGUsIHRoZSBEU3MnIHJwY19jbG50OmNsX21ldHJpY3Mgd2lsbCBuZXZl
ciBiZSB1c2VkLg0KPiBJIGxlZnQgaXQgYWNjdW11bGF0aW5nIGhlcmUgYmVjYXVzZSB3ZSB3YW50
IHRvIGhhdmUgcGVyLURTIHN0YXRzIGV2ZW50dWFsbHkgYW5kIG15IHBsYW4gd2FzIHRvIHByaW50
IG91dCBzdGF0cyBpbiAvcHJvYy9mcy9uZnNmcyBwZXIgKmNsaWVudCogKHNvIG5vdCBzZXBhcmF0
ZWQgYnkgbW91bnRwb2ludCkuDQo+IA0KPiBJIGNhbiByZXBvc3Qgd2l0aCBpZi9lbHNlLCBidXQg
bG9va2luZyBhdCB0aGlzIHNvbWUgbW9yZSBtYWRlIG1lIHJlYWxpemUgdGhhdCB3ZSBhcmUgKnN0
aWxsKiBkb2luZyB0aGlzIHdyb25nIDopDQo+IA0KPiBUaGUgY2FsbGJhY2sgbWV0aG9kIGluIHRo
aXMgcGF0Y2ggZmFpbHMgdG8gYWNjdW11bGF0ZSBzdGF0cyBmb3Igb3BlcmF0aW9ucyB0byB0aGUg
RFMgb3RoZXIgdGhhbiByZWFkL3dyaXRlL2NvbW1pdCAtLSB0aGF0IHNlZW1zIHJpZ2h0LCBidXQg
d2hhdCBhYm91dCBudWxsLCBleGNoYW5nZV9pZCwgc2Vzc2lvbiBoZWFydGJlYXRzLCBldGM/DQo+
IA0KPiBJbiBvcmRlciB0byBwcm9wZXJseSBhY2N1bXVsYXRlIHRob3NlIHdlIGFyZSBiYWNrIHRv
IHR3byBvYnZpb3VzIGNob2ljZXM6DQo+ICAgMSkgYWRkIGEgY291bnRfaW9zdGF0cyBjYWxsYmFj
ayB0byB0aGUgfjI1IG90aGVyIHJwY19jYWxsX29wcyBpbiBuZnMtbGFuZCAoeXVjaykNCj4gICAy
KSBhZGQgYW4gJ2FkZGl0aW9uYWwgc3RhdHMnIHBvaW50ZXIgdG8gdGhlIHJwY190YXNrIHN0cnVj
dHVyZSAodHJvbmQgYWxyZWFkeSBzYWlkIHlvdSBkb24ndCB3YW50IHRvIGFkZCB0byB0YXNrIHN0
cnVjdCkNCj4gDQo+IE9yIGRvIHdlIGp1c3Qgbm90IGNhcmUgYWJvdXQgZGlzcGxheWluZyB0aG9z
ZSBvcGVyYXRpb25zPyAgRm9yIG15IHB1cnBvc2VzIChuZnNvbWV0ZXIgcGVyZiB0ZXN0aW5nKSwg
aXQnZCBiZSBuaWNlIHRvIGhhdmUgKmFsbCogb2YgdGhlIG9wZXJhdGlvbnMuDQoNCkFzIGZhciBh
cyBJJ20gY29uY2VybmVkLCB0aGUgYWRtaW5pc3RyYXRpdmUgdHJhZmZpYyB0byB0aGUgRFMgc2hv
dWxkIG5vdA0KYmUgYWNjb3VudGVkIGZvciBpbiB0aGUgbW91bnRzdGF0czogdGhhdCB3b3VsZCBi
ZSB3cm9uZyBzaW5jZSBEU2VzIGNhbg0KYmUgc2hhcmVkIG5vdCBvbmx5IGJ5IGRpZmZlcmVudCBm
aWxlc3lzdGVtcyBidXQgZXZlbiBieSBkaWZmZXJlbnQgTURTZXMuDQoNClNvIHRoZSBwZXJmb3Jt
YW5jZSBvdmVyaGVhZCBvZiBsZWFzZSBhbmQgc2Vzc2lvbiBzZXR1cCB0byB0aGUgRFNlcyBuZWVk
cw0KdG8gYmUgYWNjb3VudGVkIGZvciBieSBzb21lIG90aGVyIG1lY2hhbmlzbS4NCg0KLS0gDQpU
cm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRy
b25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]


On Feb 16, 2012, at 4:01 PM, Myklebust, Trond wrote:

> On Thu, 2012-02-16 at 20:44 +0000, Adamson, Dros wrote:
>> On Feb 16, 2012, at 2:48 PM, Myklebust, Trond wrote:
>>>    2. Shouldn't we be calling _either_ rpc_count_iostats(), _or_
>>>       task->tk_ops->rpc_count_stats()? As far as I can see, the
>>>       nfsstat output will now be double-counting and pNFS reads,
>>>       writes and commits that are sent to the DS.
>> 
>> 
>> No, this doesn't double count with nfsstats.  nfsstats uses rpc_clnt::cl_stats (and not rpc_clnt::cl_metrics).
>> 
>> I probably should have done an if/else for this patch -- with the current code, the DSs' rpc_clnt:cl_metrics will never be used.
>> I left it accumulating here because we want to have per-DS stats eventually and my plan was to print out stats in /proc/fs/nfsfs per *client* (so not separated by mountpoint).
>> 
>> I can repost with if/else, but looking at this some more made me realize that we are *still* doing this wrong :)
>> 
>> The callback method in this patch fails to accumulate stats for operations to the DS other than read/write/commit -- that seems right, but what about null, exchange_id, session heartbeats, etc?
>> 
>> In order to properly accumulate those we are back to two obvious choices:
>>  1) add a count_iostats callback to the ~25 other rpc_call_ops in nfs-land (yuck)
>>  2) add an 'additional stats' pointer to the rpc_task structure (trond already said you don't want to add to task struct)
>> 
>> Or do we just not care about displaying those operations?  For my purposes (nfsometer perf testing), it'd be nice to have *all* of the operations.
> 
> As far as I'm concerned, the administrative traffic to the DS should not
> be accounted for in the mountstats: that would be wrong since DSes can
> be shared not only by different filesystems but even by different MDSes.
> 
> So the performance overhead of lease and session setup to the DSes needs
> to be accounted for by some other mechanism.

Fair enough.  The per-client stats should help me with that.

I'll repost with if/else and change it back with the next stats patch.

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

[PATCH] NFS: include filelayout DS rpc stats in mountstats

[Weston Andros Adamson]

Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
filelayout mounts.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
Update: don't increment tk_client stats if callback is used.

This still checks task->tk_client != NULL -- I'm not convinced it's unneeded
as it existed in rpc_count_iostats.

 fs/nfs/nfs4filelayout.c        |   19 +++++++++++++++++++
 include/linux/sunrpc/metrics.h |    6 ++++--
 include/linux/sunrpc/sched.h   |    1 +
 net/sunrpc/stats.c             |    8 ++++----
 net/sunrpc/xprt.c              |    5 ++++-
 5 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 79be7ac..47e8f34 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -33,6 +33,8 @@
 #include <linux/nfs_page.h>
 #include <linux/module.h>
 
+#include <linux/sunrpc/metrics.h>
+
 #include "internal.h"
 #include "nfs4filelayout.h"
 
@@ -189,6 +191,13 @@ static void filelayout_read_call_done(struct rpc_task *task, void *data)
 	rdata->mds_ops->rpc_call_done(task, data);
 }
 
+static void filelayout_read_count_stats(struct rpc_task *task, void *data)
+{
+	struct nfs_read_data *rdata = (struct nfs_read_data *)data;
+
+	rpc_count_iostats(task, NFS_SERVER(rdata->inode)->client->cl_metrics);
+}
+
 static void filelayout_read_release(void *data)
 {
 	struct nfs_read_data *rdata = (struct nfs_read_data *)data;
@@ -268,6 +277,13 @@ static void filelayout_write_call_done(struct rpc_task *task, void *data)
 	wdata->mds_ops->rpc_call_done(task, data);
 }
 
+static void filelayout_write_count_stats(struct rpc_task *task, void *data)
+{
+	struct nfs_write_data *wdata = (struct nfs_write_data *)data;
+
+	rpc_count_iostats(task, NFS_SERVER(wdata->inode)->client->cl_metrics);
+}
+
 static void filelayout_write_release(void *data)
 {
 	struct nfs_write_data *wdata = (struct nfs_write_data *)data;
@@ -288,18 +304,21 @@ static void filelayout_commit_release(void *data)
 struct rpc_call_ops filelayout_read_call_ops = {
 	.rpc_call_prepare = filelayout_read_prepare,
 	.rpc_call_done = filelayout_read_call_done,
+	.rpc_count_stats = filelayout_read_count_stats,
 	.rpc_release = filelayout_read_release,
 };
 
 struct rpc_call_ops filelayout_write_call_ops = {
 	.rpc_call_prepare = filelayout_write_prepare,
 	.rpc_call_done = filelayout_write_call_done,
+	.rpc_count_stats = filelayout_write_count_stats,
 	.rpc_release = filelayout_write_release,
 };
 
 struct rpc_call_ops filelayout_commit_call_ops = {
 	.rpc_call_prepare = filelayout_write_prepare,
 	.rpc_call_done = filelayout_write_call_done,
+	.rpc_count_stats = filelayout_write_count_stats,
 	.rpc_release = filelayout_commit_release,
 };
 
diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
index b6edbc0..1565bbe 100644
--- a/include/linux/sunrpc/metrics.h
+++ b/include/linux/sunrpc/metrics.h
@@ -74,14 +74,16 @@ struct rpc_clnt;
 #ifdef CONFIG_PROC_FS
 
 struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
-void			rpc_count_iostats(struct rpc_task *);
+void			rpc_count_iostats(const struct rpc_task *,
+					  struct rpc_iostats *);
 void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
 void			rpc_free_iostats(struct rpc_iostats *);
 
 #else  /*  CONFIG_PROC_FS  */
 
 static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
-static inline void rpc_count_iostats(struct rpc_task *task) {}
+static inline void rpc_count_iostats(const struct rpc_task *task,
+				     struct rpc_iostats *stats) {}
 static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
 static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
 
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 22dfc24..dc0c3cc 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -103,6 +103,7 @@ typedef void			(*rpc_action)(struct rpc_task *);
 struct rpc_call_ops {
 	void (*rpc_call_prepare)(struct rpc_task *, void *);
 	void (*rpc_call_done)(struct rpc_task *, void *);
+	void (*rpc_count_stats)(struct rpc_task *, void *);
 	void (*rpc_release)(void *);
 };
 
diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index 3c4f688..1eb3304 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -133,20 +133,19 @@ EXPORT_SYMBOL_GPL(rpc_free_iostats);
 /**
  * rpc_count_iostats - tally up per-task stats
  * @task: completed rpc_task
+ * @stats: array of stat structures
  *
  * Relies on the caller for serialization.
  */
-void rpc_count_iostats(struct rpc_task *task)
+void rpc_count_iostats(const struct rpc_task *task, struct rpc_iostats *stats)
 {
 	struct rpc_rqst *req = task->tk_rqstp;
-	struct rpc_iostats *stats;
 	struct rpc_iostats *op_metrics;
 	ktime_t delta;
 
-	if (!task->tk_client || !task->tk_client->cl_metrics || !req)
+	if (!stats || !req)
 		return;
 
-	stats = task->tk_client->cl_metrics;
 	op_metrics = &stats[task->tk_msg.rpc_proc->p_statidx];
 
 	op_metrics->om_ops++;
@@ -164,6 +163,7 @@ void rpc_count_iostats(struct rpc_task *task)
 	delta = ktime_sub(ktime_get(), task->tk_start);
 	op_metrics->om_execute = ktime_add(op_metrics->om_execute, delta);
 }
+EXPORT_SYMBOL_GPL(rpc_count_iostats);
 
 static void _print_name(struct seq_file *seq, unsigned int op,
 			struct rpc_procinfo *procs)
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index efe5495..100b469 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1132,7 +1132,10 @@ void xprt_release(struct rpc_task *task)
 		return;
 
 	xprt = req->rq_xprt;
-	rpc_count_iostats(task);
+	if (task->tk_ops->rpc_count_stats != NULL)
+		task->tk_ops->rpc_count_stats(task, task->tk_calldata);
+	else if (task->tk_client)
+		rpc_count_iostats(task, task->tk_client->cl_metrics);
 	spin_lock_bh(&xprt->transport_lock);
 	xprt->ops->release_xprt(xprt, task);
 	if (xprt->ops->release_request)
-- 
1.7.4.4

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTAyLTE3IGF0IDEzOjE1IC0wNTAwLCBXZXN0b24gQW5kcm9zIEFkYW1zb24g
d3JvdGU6DQo+IEluY2x1ZGUgUlBDIHN0YXRpc3RpY3MgZnJvbSBhbGwgZGF0YSBzZXJ2ZXJzIGlu
IC9wcm9jL3NlbGYvbW91bnRzdGF0cyBmb3IgcE5GUw0KPiBmaWxlbGF5b3V0IG1vdW50cy4NCj4g
DQo+IFNpZ25lZC1vZmYtYnk6IFdlc3RvbiBBbmRyb3MgQWRhbXNvbiA8ZHJvc0BuZXRhcHAuY29t
Pg0KPiAtLS0NCj4gVXBkYXRlOiBkb24ndCBpbmNyZW1lbnQgdGtfY2xpZW50IHN0YXRzIGlmIGNh
bGxiYWNrIGlzIHVzZWQuDQo+IA0KPiBUaGlzIHN0aWxsIGNoZWNrcyB0YXNrLT50a19jbGllbnQg
IT0gTlVMTCAtLSBJJ20gbm90IGNvbnZpbmNlZCBpdCdzIHVubmVlZGVkDQo+IGFzIGl0IGV4aXN0
ZWQgaW4gcnBjX2NvdW50X2lvc3RhdHMuDQoNCkknbGwgcmVtb3ZlIGl0IGJlZm9yZSBhcHBseWlu
Zy4geHBydF9yZXF1ZXN0X2luaXQgd2lsbCBPb3BzIHdpdGhvdXQNCnRhc2stPnRrX2NsaWVudCwg
c28geW91J3JlIGd1YXJhbnRlZWQgdGhhdCBpZiB0YXNrLT50a19ycXN0cCBpcw0Kbm9uLW51bGws
IHRoZW4gdGFzay0+dGtfY2xpZW50IGlzIGEgdmFsaWQgcG9pbnRlci4uLg0KDQo+ICBmcy9uZnMv
bmZzNGZpbGVsYXlvdXQuYyAgICAgICAgfCAgIDE5ICsrKysrKysrKysrKysrKysrKysNCj4gIGlu
Y2x1ZGUvbGludXgvc3VucnBjL21ldHJpY3MuaCB8ICAgIDYgKysrKy0tDQo+ICBpbmNsdWRlL2xp
bnV4L3N1bnJwYy9zY2hlZC5oICAgfCAgICAxICsNCj4gIG5ldC9zdW5ycGMvc3RhdHMuYyAgICAg
ICAgICAgICB8ICAgIDggKysrKy0tLS0NCj4gIG5ldC9zdW5ycGMveHBydC5jICAgICAgICAgICAg
ICB8ICAgIDUgKysrKy0NCj4gIDUgZmlsZXMgY2hhbmdlZCwgMzIgaW5zZXJ0aW9ucygrKSwgNyBk
ZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYyBi
L2ZzL25mcy9uZnM0ZmlsZWxheW91dC5jDQo+IGluZGV4IDc5YmU3YWMuLjQ3ZThmMzQgMTAwNjQ0
DQo+IC0tLSBhL2ZzL25mcy9uZnM0ZmlsZWxheW91dC5jDQo+ICsrKyBiL2ZzL25mcy9uZnM0Zmls
ZWxheW91dC5jDQo+IEBAIC0zMyw2ICszMyw4IEBADQo+ICAjaW5jbHVkZSA8bGludXgvbmZzX3Bh
Z2UuaD4NCj4gICNpbmNsdWRlIDxsaW51eC9tb2R1bGUuaD4NCj4gIA0KPiArI2luY2x1ZGUgPGxp
bnV4L3N1bnJwYy9tZXRyaWNzLmg+DQo+ICsNCj4gICNpbmNsdWRlICJpbnRlcm5hbC5oIg0KPiAg
I2luY2x1ZGUgIm5mczRmaWxlbGF5b3V0LmgiDQo+ICANCj4gQEAgLTE4OSw2ICsxOTEsMTMgQEAg
c3RhdGljIHZvaWQgZmlsZWxheW91dF9yZWFkX2NhbGxfZG9uZShzdHJ1Y3QgcnBjX3Rhc2sgKnRh
c2ssIHZvaWQgKmRhdGEpDQo+ICAJcmRhdGEtPm1kc19vcHMtPnJwY19jYWxsX2RvbmUodGFzaywg
ZGF0YSk7DQo+ICB9DQo+ICANCj4gK3N0YXRpYyB2b2lkIGZpbGVsYXlvdXRfcmVhZF9jb3VudF9z
dGF0cyhzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2ssIHZvaWQgKmRhdGEpDQo+ICt7DQo+ICsJc3RydWN0
IG5mc19yZWFkX2RhdGEgKnJkYXRhID0gKHN0cnVjdCBuZnNfcmVhZF9kYXRhICopZGF0YTsNCj4g
Kw0KPiArCXJwY19jb3VudF9pb3N0YXRzKHRhc2ssIE5GU19TRVJWRVIocmRhdGEtPmlub2RlKS0+
Y2xpZW50LT5jbF9tZXRyaWNzKTsNCj4gK30NCj4gKw0KPiAgc3RhdGljIHZvaWQgZmlsZWxheW91
dF9yZWFkX3JlbGVhc2Uodm9pZCAqZGF0YSkNCj4gIHsNCj4gIAlzdHJ1Y3QgbmZzX3JlYWRfZGF0
YSAqcmRhdGEgPSAoc3RydWN0IG5mc19yZWFkX2RhdGEgKilkYXRhOw0KPiBAQCAtMjY4LDYgKzI3
NywxMyBAQCBzdGF0aWMgdm9pZCBmaWxlbGF5b3V0X3dyaXRlX2NhbGxfZG9uZShzdHJ1Y3QgcnBj
X3Rhc2sgKnRhc2ssIHZvaWQgKmRhdGEpDQo+ICAJd2RhdGEtPm1kc19vcHMtPnJwY19jYWxsX2Rv
bmUodGFzaywgZGF0YSk7DQo+ICB9DQo+ICANCj4gK3N0YXRpYyB2b2lkIGZpbGVsYXlvdXRfd3Jp
dGVfY291bnRfc3RhdHMoc3RydWN0IHJwY190YXNrICp0YXNrLCB2b2lkICpkYXRhKQ0KPiArew0K
PiArCXN0cnVjdCBuZnNfd3JpdGVfZGF0YSAqd2RhdGEgPSAoc3RydWN0IG5mc193cml0ZV9kYXRh
ICopZGF0YTsNCj4gKw0KPiArCXJwY19jb3VudF9pb3N0YXRzKHRhc2ssIE5GU19TRVJWRVIod2Rh
dGEtPmlub2RlKS0+Y2xpZW50LT5jbF9tZXRyaWNzKTsNCj4gK30NCj4gKw0KPiAgc3RhdGljIHZv
aWQgZmlsZWxheW91dF93cml0ZV9yZWxlYXNlKHZvaWQgKmRhdGEpDQo+ICB7DQo+ICAJc3RydWN0
IG5mc193cml0ZV9kYXRhICp3ZGF0YSA9IChzdHJ1Y3QgbmZzX3dyaXRlX2RhdGEgKilkYXRhOw0K
PiBAQCAtMjg4LDE4ICszMDQsMjEgQEAgc3RhdGljIHZvaWQgZmlsZWxheW91dF9jb21taXRfcmVs
ZWFzZSh2b2lkICpkYXRhKQ0KPiAgc3RydWN0IHJwY19jYWxsX29wcyBmaWxlbGF5b3V0X3JlYWRf
Y2FsbF9vcHMgPSB7DQo+ICAJLnJwY19jYWxsX3ByZXBhcmUgPSBmaWxlbGF5b3V0X3JlYWRfcHJl
cGFyZSwNCj4gIAkucnBjX2NhbGxfZG9uZSA9IGZpbGVsYXlvdXRfcmVhZF9jYWxsX2RvbmUsDQo+
ICsJLnJwY19jb3VudF9zdGF0cyA9IGZpbGVsYXlvdXRfcmVhZF9jb3VudF9zdGF0cywNCj4gIAku
cnBjX3JlbGVhc2UgPSBmaWxlbGF5b3V0X3JlYWRfcmVsZWFzZSwNCj4gIH07DQo+ICANCj4gIHN0
cnVjdCBycGNfY2FsbF9vcHMgZmlsZWxheW91dF93cml0ZV9jYWxsX29wcyA9IHsNCj4gIAkucnBj
X2NhbGxfcHJlcGFyZSA9IGZpbGVsYXlvdXRfd3JpdGVfcHJlcGFyZSwNCj4gIAkucnBjX2NhbGxf
ZG9uZSA9IGZpbGVsYXlvdXRfd3JpdGVfY2FsbF9kb25lLA0KPiArCS5ycGNfY291bnRfc3RhdHMg
PSBmaWxlbGF5b3V0X3dyaXRlX2NvdW50X3N0YXRzLA0KPiAgCS5ycGNfcmVsZWFzZSA9IGZpbGVs
YXlvdXRfd3JpdGVfcmVsZWFzZSwNCj4gIH07DQo+ICANCj4gIHN0cnVjdCBycGNfY2FsbF9vcHMg
ZmlsZWxheW91dF9jb21taXRfY2FsbF9vcHMgPSB7DQo+ICAJLnJwY19jYWxsX3ByZXBhcmUgPSBm
aWxlbGF5b3V0X3dyaXRlX3ByZXBhcmUsDQo+ICAJLnJwY19jYWxsX2RvbmUgPSBmaWxlbGF5b3V0
X3dyaXRlX2NhbGxfZG9uZSwNCj4gKwkucnBjX2NvdW50X3N0YXRzID0gZmlsZWxheW91dF93cml0
ZV9jb3VudF9zdGF0cywNCj4gIAkucnBjX3JlbGVhc2UgPSBmaWxlbGF5b3V0X2NvbW1pdF9yZWxl
YXNlLA0KPiAgfTsNCj4gIA0KPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9zdW5ycGMvbWV0
cmljcy5oIGIvaW5jbHVkZS9saW51eC9zdW5ycGMvbWV0cmljcy5oDQo+IGluZGV4IGI2ZWRiYzAu
LjE1NjViYmUgMTAwNjQ0DQo+IC0tLSBhL2luY2x1ZGUvbGludXgvc3VucnBjL21ldHJpY3MuaA0K
PiArKysgYi9pbmNsdWRlL2xpbnV4L3N1bnJwYy9tZXRyaWNzLmgNCj4gQEAgLTc0LDE0ICs3NCwx
NiBAQCBzdHJ1Y3QgcnBjX2NsbnQ7DQo+ICAjaWZkZWYgQ09ORklHX1BST0NfRlMNCj4gIA0KPiAg
c3RydWN0IHJwY19pb3N0YXRzICoJcnBjX2FsbG9jX2lvc3RhdHMoc3RydWN0IHJwY19jbG50ICop
Ow0KPiAtdm9pZAkJCXJwY19jb3VudF9pb3N0YXRzKHN0cnVjdCBycGNfdGFzayAqKTsNCj4gK3Zv
aWQJCQlycGNfY291bnRfaW9zdGF0cyhjb25zdCBzdHJ1Y3QgcnBjX3Rhc2sgKiwNCj4gKwkJCQkJ
ICBzdHJ1Y3QgcnBjX2lvc3RhdHMgKik7DQo+ICB2b2lkCQkJcnBjX3ByaW50X2lvc3RhdHMoc3Ry
dWN0IHNlcV9maWxlICosIHN0cnVjdCBycGNfY2xudCAqKTsNCj4gIHZvaWQJCQlycGNfZnJlZV9p
b3N0YXRzKHN0cnVjdCBycGNfaW9zdGF0cyAqKTsNCj4gIA0KPiAgI2Vsc2UgIC8qICBDT05GSUdf
UFJPQ19GUyAgKi8NCj4gIA0KPiAgc3RhdGljIGlubGluZSBzdHJ1Y3QgcnBjX2lvc3RhdHMgKnJw
Y19hbGxvY19pb3N0YXRzKHN0cnVjdCBycGNfY2xudCAqY2xudCkgeyByZXR1cm4gTlVMTDsgfQ0K
PiAtc3RhdGljIGlubGluZSB2b2lkIHJwY19jb3VudF9pb3N0YXRzKHN0cnVjdCBycGNfdGFzayAq
dGFzaykge30NCj4gK3N0YXRpYyBpbmxpbmUgdm9pZCBycGNfY291bnRfaW9zdGF0cyhjb25zdCBz
dHJ1Y3QgcnBjX3Rhc2sgKnRhc2ssDQo+ICsJCQkJICAgICBzdHJ1Y3QgcnBjX2lvc3RhdHMgKnN0
YXRzKSB7fQ0KPiAgc3RhdGljIGlubGluZSB2b2lkIHJwY19wcmludF9pb3N0YXRzKHN0cnVjdCBz
ZXFfZmlsZSAqc2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQpIHt9DQo+ICBzdGF0aWMgaW5saW5l
IHZvaWQgcnBjX2ZyZWVfaW9zdGF0cyhzdHJ1Y3QgcnBjX2lvc3RhdHMgKnN0YXRzKSB7fQ0KPiAg
DQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L3N1bnJwYy9zY2hlZC5oIGIvaW5jbHVkZS9s
aW51eC9zdW5ycGMvc2NoZWQuaA0KPiBpbmRleCAyMmRmYzI0Li5kYzBjM2NjIDEwMDY0NA0KPiAt
LS0gYS9pbmNsdWRlL2xpbnV4L3N1bnJwYy9zY2hlZC5oDQo+ICsrKyBiL2luY2x1ZGUvbGludXgv
c3VucnBjL3NjaGVkLmgNCj4gQEAgLTEwMyw2ICsxMDMsNyBAQCB0eXBlZGVmIHZvaWQJCQkoKnJw
Y19hY3Rpb24pKHN0cnVjdCBycGNfdGFzayAqKTsNCj4gIHN0cnVjdCBycGNfY2FsbF9vcHMgew0K
PiAgCXZvaWQgKCpycGNfY2FsbF9wcmVwYXJlKShzdHJ1Y3QgcnBjX3Rhc2sgKiwgdm9pZCAqKTsN
Cj4gIAl2b2lkICgqcnBjX2NhbGxfZG9uZSkoc3RydWN0IHJwY190YXNrICosIHZvaWQgKik7DQo+
ICsJdm9pZCAoKnJwY19jb3VudF9zdGF0cykoc3RydWN0IHJwY190YXNrICosIHZvaWQgKik7DQo+
ICAJdm9pZCAoKnJwY19yZWxlYXNlKSh2b2lkICopOw0KPiAgfTsNCj4gIA0KPiBkaWZmIC0tZ2l0
IGEvbmV0L3N1bnJwYy9zdGF0cy5jIGIvbmV0L3N1bnJwYy9zdGF0cy5jDQo+IGluZGV4IDNjNGY2
ODguLjFlYjMzMDQgMTAwNjQ0DQo+IC0tLSBhL25ldC9zdW5ycGMvc3RhdHMuYw0KPiArKysgYi9u
ZXQvc3VucnBjL3N0YXRzLmMNCj4gQEAgLTEzMywyMCArMTMzLDE5IEBAIEVYUE9SVF9TWU1CT0xf
R1BMKHJwY19mcmVlX2lvc3RhdHMpOw0KPiAgLyoqDQo+ICAgKiBycGNfY291bnRfaW9zdGF0cyAt
IHRhbGx5IHVwIHBlci10YXNrIHN0YXRzDQo+ICAgKiBAdGFzazogY29tcGxldGVkIHJwY190YXNr
DQo+ICsgKiBAc3RhdHM6IGFycmF5IG9mIHN0YXQgc3RydWN0dXJlcw0KPiAgICoNCj4gICAqIFJl
bGllcyBvbiB0aGUgY2FsbGVyIGZvciBzZXJpYWxpemF0aW9uLg0KPiAgICovDQo+IC12b2lkIHJw
Y19jb3VudF9pb3N0YXRzKHN0cnVjdCBycGNfdGFzayAqdGFzaykNCj4gK3ZvaWQgcnBjX2NvdW50
X2lvc3RhdHMoY29uc3Qgc3RydWN0IHJwY190YXNrICp0YXNrLCBzdHJ1Y3QgcnBjX2lvc3RhdHMg
KnN0YXRzKQ0KPiAgew0KPiAgCXN0cnVjdCBycGNfcnFzdCAqcmVxID0gdGFzay0+dGtfcnFzdHA7
DQo+IC0Jc3RydWN0IHJwY19pb3N0YXRzICpzdGF0czsNCj4gIAlzdHJ1Y3QgcnBjX2lvc3RhdHMg
Km9wX21ldHJpY3M7DQo+ICAJa3RpbWVfdCBkZWx0YTsNCj4gIA0KPiAtCWlmICghdGFzay0+dGtf
Y2xpZW50IHx8ICF0YXNrLT50a19jbGllbnQtPmNsX21ldHJpY3MgfHwgIXJlcSkNCj4gKwlpZiAo
IXN0YXRzIHx8ICFyZXEpDQo+ICAJCXJldHVybjsNCj4gIA0KPiAtCXN0YXRzID0gdGFzay0+dGtf
Y2xpZW50LT5jbF9tZXRyaWNzOw0KPiAgCW9wX21ldHJpY3MgPSAmc3RhdHNbdGFzay0+dGtfbXNn
LnJwY19wcm9jLT5wX3N0YXRpZHhdOw0KPiAgDQo+ICAJb3BfbWV0cmljcy0+b21fb3BzKys7DQo+
IEBAIC0xNjQsNiArMTYzLDcgQEAgdm9pZCBycGNfY291bnRfaW9zdGF0cyhzdHJ1Y3QgcnBjX3Rh
c2sgKnRhc2spDQo+ICAJZGVsdGEgPSBrdGltZV9zdWIoa3RpbWVfZ2V0KCksIHRhc2stPnRrX3N0
YXJ0KTsNCj4gIAlvcF9tZXRyaWNzLT5vbV9leGVjdXRlID0ga3RpbWVfYWRkKG9wX21ldHJpY3Mt
Pm9tX2V4ZWN1dGUsIGRlbHRhKTsNCj4gIH0NCj4gK0VYUE9SVF9TWU1CT0xfR1BMKHJwY19jb3Vu
dF9pb3N0YXRzKTsNCj4gIA0KPiAgc3RhdGljIHZvaWQgX3ByaW50X25hbWUoc3RydWN0IHNlcV9m
aWxlICpzZXEsIHVuc2lnbmVkIGludCBvcCwNCj4gIAkJCXN0cnVjdCBycGNfcHJvY2luZm8gKnBy
b2NzKQ0KPiBkaWZmIC0tZ2l0IGEvbmV0L3N1bnJwYy94cHJ0LmMgYi9uZXQvc3VucnBjL3hwcnQu
Yw0KPiBpbmRleCBlZmU1NDk1Li4xMDBiNDY5IDEwMDY0NA0KPiAtLS0gYS9uZXQvc3VucnBjL3hw
cnQuYw0KPiArKysgYi9uZXQvc3VucnBjL3hwcnQuYw0KPiBAQCAtMTEzMiw3ICsxMTMyLDEwIEBA
IHZvaWQgeHBydF9yZWxlYXNlKHN0cnVjdCBycGNfdGFzayAqdGFzaykNCj4gIAkJcmV0dXJuOw0K
PiAgDQo+ICAJeHBydCA9IHJlcS0+cnFfeHBydDsNCj4gLQlycGNfY291bnRfaW9zdGF0cyh0YXNr
KTsNCj4gKwlpZiAodGFzay0+dGtfb3BzLT5ycGNfY291bnRfc3RhdHMgIT0gTlVMTCkNCj4gKwkJ
dGFzay0+dGtfb3BzLT5ycGNfY291bnRfc3RhdHModGFzaywgdGFzay0+dGtfY2FsbGRhdGEpOw0K
PiArCWVsc2UgaWYgKHRhc2stPnRrX2NsaWVudCkNCj4gKwkJcnBjX2NvdW50X2lvc3RhdHModGFz
aywgdGFzay0+dGtfY2xpZW50LT5jbF9tZXRyaWNzKTsNCj4gIAlzcGluX2xvY2tfYmgoJnhwcnQt
PnRyYW5zcG9ydF9sb2NrKTsNCj4gIAl4cHJ0LT5vcHMtPnJlbGVhc2VfeHBydCh4cHJ0LCB0YXNr
KTsNCj4gIAlpZiAoeHBydC0+b3BzLT5yZWxlYXNlX3JlcXVlc3QpDQoNCi0tIA0KVHJvbmQgTXlr
bGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWts
ZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On Feb 17, 2012, at 1:47 PM, Myklebust, Trond wrote:

> On Fri, 2012-02-17 at 13:15 -0500, Weston Andros Adamson wrote:
>> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
>> filelayout mounts.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> Update: don't increment tk_client stats if callback is used.
>> 
>> This still checks task->tk_client != NULL -- I'm not convinced it's unneeded
>> as it existed in rpc_count_iostats.
> 
> I'll remove it before applying.

I can repost the patch, if that's easier.

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

RE: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBBZGFtc29uLCBEcm9zDQo+IFNl
bnQ6IEZyaWRheSwgRmVicnVhcnkgMTcsIDIwMTIgMjowMiBQTQ0KPiBUbzogTXlrbGVidXN0LCBU
cm9uZA0KPiBDYzogQWRhbXNvbiwgRHJvczsgbGludXgtbmZzQHZnZXIua2VybmVsLm9yZw0KPiBT
dWJqZWN0OiBSZTogW1BBVENIXSBORlM6IGluY2x1ZGUgZmlsZWxheW91dCBEUyBycGMgc3RhdHMg
aW4gbW91bnRzdGF0cw0KPiANCj4gT24gRmViIDE3LCAyMDEyLCBhdCAxOjQ3IFBNLCBNeWtsZWJ1
c3QsIFRyb25kIHdyb3RlOg0KPiANCj4gPiBPbiBGcmksIDIwMTItMDItMTcgYXQgMTM6MTUgLTA1
MDAsIFdlc3RvbiBBbmRyb3MgQWRhbXNvbiB3cm90ZToNCj4gPj4gSW5jbHVkZSBSUEMgc3RhdGlz
dGljcyBmcm9tIGFsbCBkYXRhIHNlcnZlcnMgaW4gL3Byb2Mvc2VsZi9tb3VudHN0YXRzIGZvcg0K
PiBwTkZTDQo+ID4+IGZpbGVsYXlvdXQgbW91bnRzLg0KPiA+Pg0KPiA+PiBTaWduZWQtb2ZmLWJ5
OiBXZXN0b24gQW5kcm9zIEFkYW1zb24gPGRyb3NAbmV0YXBwLmNvbT4NCj4gPj4gLS0tDQo+ID4+
IFVwZGF0ZTogZG9uJ3QgaW5jcmVtZW50IHRrX2NsaWVudCBzdGF0cyBpZiBjYWxsYmFjayBpcyB1
c2VkLg0KPiA+Pg0KPiA+PiBUaGlzIHN0aWxsIGNoZWNrcyB0YXNrLT50a19jbGllbnQgIT0gTlVM
TCAtLSBJJ20gbm90IGNvbnZpbmNlZCBpdCdzIHVubmVlZGVkDQo+ID4+IGFzIGl0IGV4aXN0ZWQg
aW4gcnBjX2NvdW50X2lvc3RhdHMuDQo+ID4NCj4gPiBJJ2xsIHJlbW92ZSBpdCBiZWZvcmUgYXBw
bHlpbmcuDQo+IA0KPiBJIGNhbiByZXBvc3QgdGhlIHBhdGNoLCBpZiB0aGF0J3MgZWFzaWVyLg0K
DQpOYWguIEkndmUgYWxyZWFkeSBhcHBsaWVkIGFuZCBwdXNoZWQgdG8gdGhlIGdpdCByZXBvc2l0
b3J5Lg0KDQo=

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]


On Feb 17, 2012, at 2:04 PM, Myklebust, Trond wrote:

>> -----Original Message-----
>> From: Adamson, Dros
>> Sent: Friday, February 17, 2012 2:02 PM
>> To: Myklebust, Trond
>> Cc: Adamson, Dros; linux-nfs@vger.kernel.org
>> Subject: Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats
>> 
>> On Feb 17, 2012, at 1:47 PM, Myklebust, Trond wrote:
>> 
>>> On Fri, 2012-02-17 at 13:15 -0500, Weston Andros Adamson wrote:
>>>> Include RPC statistics from all data servers in /proc/self/mountstats for
>> pNFS
>>>> filelayout mounts.
>>>> 
>>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>>> ---
>>>> Update: don't increment tk_client stats if callback is used.
>>>> 
>>>> This still checks task->tk_client != NULL -- I'm not convinced it's unneeded
>>>> as it existed in rpc_count_iostats.
>>> 
>>> I'll remove it before applying.
>> 
>> I can repost the patch, if that's easier.
> 
> Nah. I've already applied and pushed to the git repository.
> 

Cool, thanks!

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

[PATCH] NFS: include filelayout DS rpc stats in mountstats

[Weston Andros Adamson]

Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
filelayout mounts.

The additional info printed on the "per-op statistics" line does not break
current mountstats(8) from nfs-utils.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---

This is clearly a better approach than I was taking trying to keep DS rpc stats
separate in /proc/self/mountstats.

FYI, there are no other callers of rpc_print_iostats().

The new output of /proc/self/mountstats works fine with current mountstats 
userland tool.  The additional data on the per-op statistics line doesn't make
it into the output, but I left it in /proc/self/mountstats because it doesn't
break anything and can be included in newer versions of mountstats.

I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
soon.

 fs/nfs/nfs4filelayout.c        |   30 ++++++++++++++++++++++++++
 fs/nfs/pnfs.h                  |   23 ++++++++++++++++++++
 fs/nfs/pnfs_dev.c              |   45 ++++++++++++++++++++++++++++++++++++++++
 fs/nfs/super.c                 |    3 +-
 include/linux/sunrpc/metrics.h |   13 +++++++++-
 net/sunrpc/stats.c             |   38 ++++++++++++++++++++++++++++++++-
 6 files changed, 147 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 79be7ac..51c71e5 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -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,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
 }
 
+static unsigned int
+filelayout_rpc_add_iostats(struct rpc_iostats *total,
+			   unsigned int maxproc,
+			   struct nfs4_deviceid_node *d)
+{
+	struct nfs4_file_layout_dsaddr *dsaddr;
+	struct nfs4_pnfs_ds *ds;
+	unsigned int num_ds = 0;
+	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];
+
+		/* only add iostats if nfs_client is different from MDS */
+		if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
+			struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
+			rpc_add_iostats(total, clnt->cl_metrics,
+					min(maxproc, clnt->cl_maxproc));
+			num_ds++;
+		}
+	}
+
+	return num_ds;
+}
+
 static struct pnfs_layoutdriver_type filelayout_type = {
 	.id			= LAYOUT_NFSV4_1_FILES,
 	.name			= "LAYOUT_NFSV4_1_FILES",
@@ -932,6 +961,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_add_iostats	= filelayout_rpc_add_iostats,
 };
 
 static int __init nfs4filelayout_init(void)
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 53d593a..d1b82eb 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -72,6 +72,7 @@ enum layoutdriver_policy_flags {
 };
 
 struct nfs4_deviceid_node;
+struct rpc_iostats;
 
 /* Per-layout driver specific registration structure */
 struct pnfs_layoutdriver_type {
@@ -119,6 +120,9 @@ struct pnfs_layoutdriver_type {
 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct nfs4_layoutcommit_args *args);
+
+	unsigned int (*ds_rpc_add_iostats) (struct rpc_iostats *, unsigned int,
+					    struct nfs4_deviceid_node *);
 };
 
 struct pnfs_layout_hdr {
@@ -239,6 +243,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 +336,15 @@ static inline int pnfs_return_layout(struct inode *ino)
 	return 0;
 }
 
+static inline int
+pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
+{
+	if (!pnfs_enabled_sb(nfss))
+		return 0;
+	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
+					nfss->pnfs_curr_ld);
+	return 1;
+}
 #else  /* CONFIG_NFS_V4_1 */
 
 static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
@@ -429,6 +446,12 @@ 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)
+{
+	return 0;
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 #endif /* FS_NFS_PNFS_H */
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 4f359d2..aa5d102 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -29,6 +29,9 @@
  */
 
 #include <linux/export.h>
+
+#include <linux/sunrpc/metrics.h>
+
 #include "pnfs.h"
 
 #define NFSDBG_FACILITY		NFSDBG_PNFS
@@ -115,6 +118,48 @@ 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;
+	struct rpc_iostats *totals;
+	char msgbuf[64];
+	unsigned int num_ds = 0;
+	unsigned int maxproc;
+	long h;
+
+	if (!ld->ds_rpc_add_iostats)
+		return;
+	totals = rpc_alloc_iostats(clp->cl_rpcclient);
+	if (!totals)
+		return;
+	maxproc = clp->cl_rpcclient->cl_maxproc;
+	rpc_add_iostats(totals, clp->cl_rpcclient->cl_metrics, maxproc);
+
+	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))
+					num_ds += ld->ds_rpc_add_iostats(totals,
+							maxproc, d);
+			}
+		}
+	}
+	rcu_read_unlock();
+
+	snprintf(msgbuf, sizeof(msgbuf),
+		 " for the metadata server and %u data server%s", num_ds,
+		 (num_ds != 1) ? "s" : "");
+	rpc_print_iostats(seq, clp->cl_rpcclient, totals, msgbuf);
+	rpc_free_iostats(totals);
+}
+EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
+
 /*
  * Remove a deviceid from cache
  *
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d18a90b..76d0850 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -870,7 +870,8 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
 #endif
 	seq_printf(m, "\n");
 
-	rpc_print_iostats(m, nfss->client);
+	if (!pnfs_rpc_print_iostats(m, nfss))
+		rpc_print_iostats(m, nfss->client, NULL, NULL);
 
 	return 0;
 }
diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
index b6edbc0..e6e04e4 100644
--- a/include/linux/sunrpc/metrics.h
+++ b/include/linux/sunrpc/metrics.h
@@ -75,14 +75,23 @@ struct rpc_clnt;
 
 struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
 void			rpc_count_iostats(struct rpc_task *);
-void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
+void			rpc_print_iostats(struct seq_file *,
+					  struct rpc_clnt *,
+					  struct rpc_iostats *,
+					  const char *);
+void			rpc_add_iostats(struct rpc_iostats *,
+					struct rpc_iostats *, unsigned int);
 void			rpc_free_iostats(struct rpc_iostats *);
 
 #else  /*  CONFIG_PROC_FS  */
 
 static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
 static inline void rpc_count_iostats(struct rpc_task *task) {}
-static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
+static inline void rpc_print_iostats(struct seq_file *seq,
+				     struct rpc_iostats *stat) {}
+static inline void rpc_add_iostats(struct rpc_iostats *total,
+				   struct rpc_iostats *new,
+				   unsigned int maxproc) {}
 static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
 
 #endif  /*  CONFIG_PROC_FS  */
diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index 3c4f688..342df57 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -176,12 +176,16 @@ static void _print_name(struct seq_file *seq, unsigned int op,
 		seq_printf(seq, "\t%12u: ", op);
 }
 
-void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
+void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt,
+		       struct rpc_iostats *altstats,
+		       const char *altstats_msg)
 {
 	struct rpc_iostats *stats = clnt->cl_metrics;
 	struct rpc_xprt *xprt = clnt->cl_xprt;
 	unsigned int op, maxproc = clnt->cl_maxproc;
 
+	if (altstats)
+		stats = altstats;
 	if (!stats)
 		return;
 
@@ -192,7 +196,8 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
 	if (xprt)
 		xprt->ops->print_stats(xprt, seq);
 
-	seq_printf(seq, "\tper-op statistics\n");
+	seq_printf(seq, "\tper-op statistics%s\n",
+		   (altstats && altstats_msg) ? altstats_msg : "");
 	for (op = 0; op < maxproc; op++) {
 		struct rpc_iostats *metrics = &stats[op];
 		_print_name(seq, op, clnt->cl_procinfo);
@@ -209,6 +214,35 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
 }
 EXPORT_SYMBOL_GPL(rpc_print_iostats);
 
+void
+rpc_add_iostats(struct rpc_iostats *totals, struct rpc_iostats *new,
+		unsigned int maxproc)
+{
+	unsigned int op;
+
+	for (op = 0; op < maxproc; op++) {
+		struct rpc_iostats *op_totals = &totals[op],
+				   *op_new = &new[op];
+
+		op_totals->om_ops += op_new->om_ops;
+		op_totals->om_ntrans += op_new->om_ntrans;
+		op_totals->om_timeouts += op_new->om_timeouts;
+
+		op_totals->om_bytes_sent += op_new->om_bytes_sent;
+		op_totals->om_bytes_recv += op_new->om_bytes_recv;
+
+		op_totals->om_queue = ktime_add(op_totals->om_queue,
+					       op_new->om_queue);
+
+		op_totals->om_rtt = ktime_add(op_totals->om_rtt,
+					      op_new->om_rtt);
+
+		op_totals->om_execute = ktime_add(op_totals->om_execute,
+						 op_new->om_execute);
+	}
+}
+EXPORT_SYMBOL_GPL(rpc_add_iostats);
+
 /*
  * Register/unregister RPC proc files
  */
-- 
1.7.4.4

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Chuck Lever]

On Feb 9, 2012, at 11:41 AM, Weston Andros Adamson wrote:

> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
> filelayout mounts.
> 
> The additional info printed on the "per-op statistics" line does not break
> current mountstats(8) from nfs-utils.
> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
> 
> This is clearly a better approach than I was taking trying to keep DS rpc stats
> separate in /proc/self/mountstats.

I guess you could argue that since these are "mount" stats, it's a per mount point thing, not necessarily a per server thing.  We've simply had it that way because prior to pNFS, you couldn't have more than one server per mount point.

At some point we might consider also displaying some fs_locations data here, and perhaps a count of how many migration or replication failover events have been seen.  For another day.

> FYI, there are no other callers of rpc_print_iostats().
> 
> The new output of /proc/self/mountstats works fine with current mountstats 
> userland tool.  The additional data on the per-op statistics line doesn't make
> it into the output, but I left it in /proc/self/mountstats because it doesn't
> break anything and can be included in newer versions of mountstats.
> 
> I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
> soon.
> 
> fs/nfs/nfs4filelayout.c        |   30 ++++++++++++++++++++++++++
> fs/nfs/pnfs.h                  |   23 ++++++++++++++++++++
> fs/nfs/pnfs_dev.c              |   45 ++++++++++++++++++++++++++++++++++++++++
> fs/nfs/super.c                 |    3 +-
> include/linux/sunrpc/metrics.h |   13 +++++++++-
> net/sunrpc/stats.c             |   38 ++++++++++++++++++++++++++++++++-
> 6 files changed, 147 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index 79be7ac..51c71e5 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -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,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
> 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
> }
> 
> +static unsigned int
> +filelayout_rpc_add_iostats(struct rpc_iostats *total,
> +			   unsigned int maxproc,
> +			   struct nfs4_deviceid_node *d)
> +{
> +	struct nfs4_file_layout_dsaddr *dsaddr;
> +	struct nfs4_pnfs_ds *ds;
> +	unsigned int num_ds = 0;
> +	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];
> +
> +		/* only add iostats if nfs_client is different from MDS */
> +		if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
> +			struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
> +			rpc_add_iostats(total, clnt->cl_metrics,
> +					min(maxproc, clnt->cl_maxproc));
> +			num_ds++;
> +		}
> +	}
> +
> +	return num_ds;
> +}
> +
> static struct pnfs_layoutdriver_type filelayout_type = {
> 	.id			= LAYOUT_NFSV4_1_FILES,
> 	.name			= "LAYOUT_NFSV4_1_FILES",
> @@ -932,6 +961,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_add_iostats	= filelayout_rpc_add_iostats,
> };
> 
> static int __init nfs4filelayout_init(void)
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index 53d593a..d1b82eb 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -72,6 +72,7 @@ enum layoutdriver_policy_flags {
> };
> 
> struct nfs4_deviceid_node;
> +struct rpc_iostats;
> 
> /* Per-layout driver specific registration structure */
> struct pnfs_layoutdriver_type {
> @@ -119,6 +120,9 @@ struct pnfs_layoutdriver_type {
> 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
> 				     struct xdr_stream *xdr,
> 				     const struct nfs4_layoutcommit_args *args);
> +
> +	unsigned int (*ds_rpc_add_iostats) (struct rpc_iostats *, unsigned int,
> +					    struct nfs4_deviceid_node *);
> };
> 
> struct pnfs_layout_hdr {
> @@ -239,6 +243,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 +336,15 @@ static inline int pnfs_return_layout(struct inode *ino)
> 	return 0;
> }
> 
> +static inline int
> +pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
> +{
> +	if (!pnfs_enabled_sb(nfss))
> +		return 0;
> +	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
> +					nfss->pnfs_curr_ld);
> +	return 1;
> +}
> #else  /* CONFIG_NFS_V4_1 */
> 
> static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
> @@ -429,6 +446,12 @@ 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)
> +{
> +	return 0;
> +}
> #endif /* CONFIG_NFS_V4_1 */
> 
> #endif /* FS_NFS_PNFS_H */
> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
> index 4f359d2..aa5d102 100644
> --- a/fs/nfs/pnfs_dev.c
> +++ b/fs/nfs/pnfs_dev.c
> @@ -29,6 +29,9 @@
>  */
> 
> #include <linux/export.h>
> +
> +#include <linux/sunrpc/metrics.h>
> +
> #include "pnfs.h"
> 
> #define NFSDBG_FACILITY		NFSDBG_PNFS
> @@ -115,6 +118,48 @@ 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;
> +	struct rpc_iostats *totals;
> +	char msgbuf[64];
> +	unsigned int num_ds = 0;
> +	unsigned int maxproc;
> +	long h;
> +
> +	if (!ld->ds_rpc_add_iostats)
> +		return;
> +	totals = rpc_alloc_iostats(clp->cl_rpcclient);
> +	if (!totals)
> +		return;
> +	maxproc = clp->cl_rpcclient->cl_maxproc;
> +	rpc_add_iostats(totals, clp->cl_rpcclient->cl_metrics, maxproc);
> +
> +	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))
> +					num_ds += ld->ds_rpc_add_iostats(totals,
> +							maxproc, d);
> +			}
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	snprintf(msgbuf, sizeof(msgbuf),
> +		 " for the metadata server and %u data server%s", num_ds,
> +		 (num_ds != 1) ? "s" : "");
> +	rpc_print_iostats(seq, clp->cl_rpcclient, totals, msgbuf);
> +	rpc_free_iostats(totals);
> +}
> +EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
> +
> /*
>  * Remove a deviceid from cache
>  *
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index d18a90b..76d0850 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -870,7 +870,8 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
> #endif
> 	seq_printf(m, "\n");
> 
> -	rpc_print_iostats(m, nfss->client);
> +	if (!pnfs_rpc_print_iostats(m, nfss))
> +		rpc_print_iostats(m, nfss->client, NULL, NULL);
> 
> 	return 0;
> }
> diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
> index b6edbc0..e6e04e4 100644
> --- a/include/linux/sunrpc/metrics.h
> +++ b/include/linux/sunrpc/metrics.h
> @@ -75,14 +75,23 @@ struct rpc_clnt;
> 
> struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
> void			rpc_count_iostats(struct rpc_task *);
> -void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
> +void			rpc_print_iostats(struct seq_file *,
> +					  struct rpc_clnt *,
> +					  struct rpc_iostats *,
> +					  const char *);
> +void			rpc_add_iostats(struct rpc_iostats *,
> +					struct rpc_iostats *, unsigned int);
> void			rpc_free_iostats(struct rpc_iostats *);
> 
> #else  /*  CONFIG_PROC_FS  */
> 
> static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
> static inline void rpc_count_iostats(struct rpc_task *task) {}
> -static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
> +static inline void rpc_print_iostats(struct seq_file *seq,
> +				     struct rpc_iostats *stat) {}
> +static inline void rpc_add_iostats(struct rpc_iostats *total,
> +				   struct rpc_iostats *new,
> +				   unsigned int maxproc) {}
> static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
> 
> #endif  /*  CONFIG_PROC_FS  */
> diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
> index 3c4f688..342df57 100644
> --- a/net/sunrpc/stats.c
> +++ b/net/sunrpc/stats.c
> @@ -176,12 +176,16 @@ static void _print_name(struct seq_file *seq, unsigned int op,
> 		seq_printf(seq, "\t%12u: ", op);
> }
> 
> -void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
> +void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt,
> +		       struct rpc_iostats *altstats,
> +		       const char *altstats_msg)
> {
> 	struct rpc_iostats *stats = clnt->cl_metrics;
> 	struct rpc_xprt *xprt = clnt->cl_xprt;
> 	unsigned int op, maxproc = clnt->cl_maxproc;
> 
> +	if (altstats)
> +		stats = altstats;
> 	if (!stats)
> 		return;
> 
> @@ -192,7 +196,8 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
> 	if (xprt)
> 		xprt->ops->print_stats(xprt, seq);
> 
> -	seq_printf(seq, "\tper-op statistics\n");
> +	seq_printf(seq, "\tper-op statistics%s\n",
> +		   (altstats && altstats_msg) ? altstats_msg : "");
> 	for (op = 0; op < maxproc; op++) {
> 		struct rpc_iostats *metrics = &stats[op];
> 		_print_name(seq, op, clnt->cl_procinfo);
> @@ -209,6 +214,35 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
> }
> EXPORT_SYMBOL_GPL(rpc_print_iostats);
> 
> +void
> +rpc_add_iostats(struct rpc_iostats *totals, struct rpc_iostats *new,
> +		unsigned int maxproc)
> +{
> +	unsigned int op;
> +
> +	for (op = 0; op < maxproc; op++) {
> +		struct rpc_iostats *op_totals = &totals[op],
> +				   *op_new = &new[op];
> +
> +		op_totals->om_ops += op_new->om_ops;
> +		op_totals->om_ntrans += op_new->om_ntrans;
> +		op_totals->om_timeouts += op_new->om_timeouts;
> +
> +		op_totals->om_bytes_sent += op_new->om_bytes_sent;
> +		op_totals->om_bytes_recv += op_new->om_bytes_recv;
> +
> +		op_totals->om_queue = ktime_add(op_totals->om_queue,
> +					       op_new->om_queue);
> +
> +		op_totals->om_rtt = ktime_add(op_totals->om_rtt,
> +					      op_new->om_rtt);
> +
> +		op_totals->om_execute = ktime_add(op_totals->om_execute,
> +						 op_new->om_execute);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(rpc_add_iostats);
> +
> /*
>  * Register/unregister RPC proc files
>  */
> -- 
> 1.7.4.4
> 

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

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 12251 bytes --]


On Feb 9, 2012, at 11:49 AM, Chuck Lever wrote:

> 
> On Feb 9, 2012, at 11:41 AM, Weston Andros Adamson wrote:
> 
>> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
>> filelayout mounts.
>> 
>> The additional info printed on the "per-op statistics" line does not break
>> current mountstats(8) from nfs-utils.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> 
>> This is clearly a better approach than I was taking trying to keep DS rpc stats
>> separate in /proc/self/mountstats.
> 
> I guess you could argue that since these are "mount" stats, it's a per mount point thing, not necessarily a per server thing.  We've simply had it that way because prior to pNFS, you couldn't have more than one server per mount point.

Right, Trond made that point to me a while ago :)  

For some reason I was afraid to change the sunrpc metrics interface until I realized that there were no users other than NFS.

> 
> At some point we might consider also displaying some fs_locations data here, and perhaps a count of how many migration or replication failover events have been seen.  For another day.

Agreed.  First we need to fix the statsvers= issue with mountstats(8).  I plan on poking at that today.

> 
>> FYI, there are no other callers of rpc_print_iostats().
>> 
>> The new output of /proc/self/mountstats works fine with current mountstats 
>> userland tool.  The additional data on the per-op statistics line doesn't make
>> it into the output, but I left it in /proc/self/mountstats because it doesn't
>> break anything and can be included in newer versions of mountstats.
>> 
>> I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
>> soon.
>> 
>> fs/nfs/nfs4filelayout.c        |   30 ++++++++++++++++++++++++++
>> fs/nfs/pnfs.h                  |   23 ++++++++++++++++++++
>> fs/nfs/pnfs_dev.c              |   45 ++++++++++++++++++++++++++++++++++++++++
>> fs/nfs/super.c                 |    3 +-
>> include/linux/sunrpc/metrics.h |   13 +++++++++-
>> net/sunrpc/stats.c             |   38 ++++++++++++++++++++++++++++++++-
>> 6 files changed, 147 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index 79be7ac..51c71e5 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -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,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
>> 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
>> }
>> 
>> +static unsigned int
>> +filelayout_rpc_add_iostats(struct rpc_iostats *total,
>> +			   unsigned int maxproc,
>> +			   struct nfs4_deviceid_node *d)
>> +{
>> +	struct nfs4_file_layout_dsaddr *dsaddr;
>> +	struct nfs4_pnfs_ds *ds;
>> +	unsigned int num_ds = 0;
>> +	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];
>> +
>> +		/* only add iostats if nfs_client is different from MDS */
>> +		if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
>> +			struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
>> +			rpc_add_iostats(total, clnt->cl_metrics,
>> +					min(maxproc, clnt->cl_maxproc));
>> +			num_ds++;
>> +		}
>> +	}
>> +
>> +	return num_ds;
>> +}
>> +
>> static struct pnfs_layoutdriver_type filelayout_type = {
>> 	.id			= LAYOUT_NFSV4_1_FILES,
>> 	.name			= "LAYOUT_NFSV4_1_FILES",
>> @@ -932,6 +961,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_add_iostats	= filelayout_rpc_add_iostats,
>> };
>> 
>> static int __init nfs4filelayout_init(void)
>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>> index 53d593a..d1b82eb 100644
>> --- a/fs/nfs/pnfs.h
>> +++ b/fs/nfs/pnfs.h
>> @@ -72,6 +72,7 @@ enum layoutdriver_policy_flags {
>> };
>> 
>> struct nfs4_deviceid_node;
>> +struct rpc_iostats;
>> 
>> /* Per-layout driver specific registration structure */
>> struct pnfs_layoutdriver_type {
>> @@ -119,6 +120,9 @@ struct pnfs_layoutdriver_type {
>> 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
>> 				     struct xdr_stream *xdr,
>> 				     const struct nfs4_layoutcommit_args *args);
>> +
>> +	unsigned int (*ds_rpc_add_iostats) (struct rpc_iostats *, unsigned int,
>> +					    struct nfs4_deviceid_node *);
>> };
>> 
>> struct pnfs_layout_hdr {
>> @@ -239,6 +243,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 +336,15 @@ static inline int pnfs_return_layout(struct inode *ino)
>> 	return 0;
>> }
>> 
>> +static inline int
>> +pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
>> +{
>> +	if (!pnfs_enabled_sb(nfss))
>> +		return 0;
>> +	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
>> +					nfss->pnfs_curr_ld);
>> +	return 1;
>> +}
>> #else  /* CONFIG_NFS_V4_1 */
>> 
>> static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
>> @@ -429,6 +446,12 @@ 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)
>> +{
>> +	return 0;
>> +}
>> #endif /* CONFIG_NFS_V4_1 */
>> 
>> #endif /* FS_NFS_PNFS_H */
>> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
>> index 4f359d2..aa5d102 100644
>> --- a/fs/nfs/pnfs_dev.c
>> +++ b/fs/nfs/pnfs_dev.c
>> @@ -29,6 +29,9 @@
>> */
>> 
>> #include <linux/export.h>
>> +
>> +#include <linux/sunrpc/metrics.h>
>> +
>> #include "pnfs.h"
>> 
>> #define NFSDBG_FACILITY		NFSDBG_PNFS
>> @@ -115,6 +118,48 @@ 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;
>> +	struct rpc_iostats *totals;
>> +	char msgbuf[64];
>> +	unsigned int num_ds = 0;
>> +	unsigned int maxproc;
>> +	long h;
>> +
>> +	if (!ld->ds_rpc_add_iostats)
>> +		return;
>> +	totals = rpc_alloc_iostats(clp->cl_rpcclient);
>> +	if (!totals)
>> +		return;
>> +	maxproc = clp->cl_rpcclient->cl_maxproc;
>> +	rpc_add_iostats(totals, clp->cl_rpcclient->cl_metrics, maxproc);
>> +
>> +	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))
>> +					num_ds += ld->ds_rpc_add_iostats(totals,
>> +							maxproc, d);
>> +			}
>> +		}
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	snprintf(msgbuf, sizeof(msgbuf),
>> +		 " for the metadata server and %u data server%s", num_ds,
>> +		 (num_ds != 1) ? "s" : "");
>> +	rpc_print_iostats(seq, clp->cl_rpcclient, totals, msgbuf);
>> +	rpc_free_iostats(totals);
>> +}
>> +EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
>> +
>> /*
>> * Remove a deviceid from cache
>> *
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index d18a90b..76d0850 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -870,7 +870,8 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
>> #endif
>> 	seq_printf(m, "\n");
>> 
>> -	rpc_print_iostats(m, nfss->client);
>> +	if (!pnfs_rpc_print_iostats(m, nfss))
>> +		rpc_print_iostats(m, nfss->client, NULL, NULL);
>> 
>> 	return 0;
>> }
>> diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
>> index b6edbc0..e6e04e4 100644
>> --- a/include/linux/sunrpc/metrics.h
>> +++ b/include/linux/sunrpc/metrics.h
>> @@ -75,14 +75,23 @@ struct rpc_clnt;
>> 
>> struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
>> void			rpc_count_iostats(struct rpc_task *);
>> -void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
>> +void			rpc_print_iostats(struct seq_file *,
>> +					  struct rpc_clnt *,
>> +					  struct rpc_iostats *,
>> +					  const char *);
>> +void			rpc_add_iostats(struct rpc_iostats *,
>> +					struct rpc_iostats *, unsigned int);
>> void			rpc_free_iostats(struct rpc_iostats *);
>> 
>> #else  /*  CONFIG_PROC_FS  */
>> 
>> static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
>> static inline void rpc_count_iostats(struct rpc_task *task) {}
>> -static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
>> +static inline void rpc_print_iostats(struct seq_file *seq,
>> +				     struct rpc_iostats *stat) {}
>> +static inline void rpc_add_iostats(struct rpc_iostats *total,
>> +				   struct rpc_iostats *new,
>> +				   unsigned int maxproc) {}
>> static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
>> 
>> #endif  /*  CONFIG_PROC_FS  */
>> diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
>> index 3c4f688..342df57 100644
>> --- a/net/sunrpc/stats.c
>> +++ b/net/sunrpc/stats.c
>> @@ -176,12 +176,16 @@ static void _print_name(struct seq_file *seq, unsigned int op,
>> 		seq_printf(seq, "\t%12u: ", op);
>> }
>> 
>> -void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> +void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt,
>> +		       struct rpc_iostats *altstats,
>> +		       const char *altstats_msg)
>> {
>> 	struct rpc_iostats *stats = clnt->cl_metrics;
>> 	struct rpc_xprt *xprt = clnt->cl_xprt;
>> 	unsigned int op, maxproc = clnt->cl_maxproc;
>> 
>> +	if (altstats)
>> +		stats = altstats;
>> 	if (!stats)
>> 		return;
>> 
>> @@ -192,7 +196,8 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> 	if (xprt)
>> 		xprt->ops->print_stats(xprt, seq);
>> 
>> -	seq_printf(seq, "\tper-op statistics\n");
>> +	seq_printf(seq, "\tper-op statistics%s\n",
>> +		   (altstats && altstats_msg) ? altstats_msg : "");
>> 	for (op = 0; op < maxproc; op++) {
>> 		struct rpc_iostats *metrics = &stats[op];
>> 		_print_name(seq, op, clnt->cl_procinfo);
>> @@ -209,6 +214,35 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> }
>> EXPORT_SYMBOL_GPL(rpc_print_iostats);
>> 
>> +void
>> +rpc_add_iostats(struct rpc_iostats *totals, struct rpc_iostats *new,
>> +		unsigned int maxproc)
>> +{
>> +	unsigned int op;
>> +
>> +	for (op = 0; op < maxproc; op++) {
>> +		struct rpc_iostats *op_totals = &totals[op],
>> +				   *op_new = &new[op];
>> +
>> +		op_totals->om_ops += op_new->om_ops;
>> +		op_totals->om_ntrans += op_new->om_ntrans;
>> +		op_totals->om_timeouts += op_new->om_timeouts;
>> +
>> +		op_totals->om_bytes_sent += op_new->om_bytes_sent;
>> +		op_totals->om_bytes_recv += op_new->om_bytes_recv;
>> +
>> +		op_totals->om_queue = ktime_add(op_totals->om_queue,
>> +					       op_new->om_queue);
>> +
>> +		op_totals->om_rtt = ktime_add(op_totals->om_rtt,
>> +					      op_new->om_rtt);
>> +
>> +		op_totals->om_execute = ktime_add(op_totals->om_execute,
>> +						 op_new->om_execute);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(rpc_add_iostats);
>> +
>> /*
>> * Register/unregister RPC proc files
>> */
>> -- 
>> 1.7.4.4
>> 
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTA5IGF0IDExOjQxIC0wNTAwLCBXZXN0b24gQW5kcm9zIEFkYW1zb24g
d3JvdGU6DQo+IEluY2x1ZGUgUlBDIHN0YXRpc3RpY3MgZnJvbSBhbGwgZGF0YSBzZXJ2ZXJzIGlu
IC9wcm9jL3NlbGYvbW91bnRzdGF0cyBmb3IgcE5GUw0KPiBmaWxlbGF5b3V0IG1vdW50cy4NCj4g
DQo+IFRoZSBhZGRpdGlvbmFsIGluZm8gcHJpbnRlZCBvbiB0aGUgInBlci1vcCBzdGF0aXN0aWNz
IiBsaW5lIGRvZXMgbm90IGJyZWFrDQo+IGN1cnJlbnQgbW91bnRzdGF0cyg4KSBmcm9tIG5mcy11
dGlscy4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IFdlc3RvbiBBbmRyb3MgQWRhbXNvbiA8ZHJvc0Bu
ZXRhcHAuY29tPg0KPiAtLS0NCj4gDQo+IFRoaXMgaXMgY2xlYXJseSBhIGJldHRlciBhcHByb2Fj
aCB0aGFuIEkgd2FzIHRha2luZyB0cnlpbmcgdG8ga2VlcCBEUyBycGMgc3RhdHMNCj4gc2VwYXJh
dGUgaW4gL3Byb2Mvc2VsZi9tb3VudHN0YXRzLg0KPiANCj4gRllJLCB0aGVyZSBhcmUgbm8gb3Ro
ZXIgY2FsbGVycyBvZiBycGNfcHJpbnRfaW9zdGF0cygpLg0KPiANCj4gVGhlIG5ldyBvdXRwdXQg
b2YgL3Byb2Mvc2VsZi9tb3VudHN0YXRzIHdvcmtzIGZpbmUgd2l0aCBjdXJyZW50IG1vdW50c3Rh
dHMgDQo+IHVzZXJsYW5kIHRvb2wuICBUaGUgYWRkaXRpb25hbCBkYXRhIG9uIHRoZSBwZXItb3Ag
c3RhdGlzdGljcyBsaW5lIGRvZXNuJ3QgbWFrZQ0KPiBpdCBpbnRvIHRoZSBvdXRwdXQsIGJ1dCBJ
IGxlZnQgaXQgaW4gL3Byb2Mvc2VsZi9tb3VudHN0YXRzIGJlY2F1c2UgaXQgZG9lc24ndA0KPiBi
cmVhayBhbnl0aGluZyBhbmQgY2FuIGJlIGluY2x1ZGVkIGluIG5ld2VyIHZlcnNpb25zIG9mIG1v
dW50c3RhdHMuDQo+IA0KPiBJIHBsYW4gb24gYWRkaW5nIHRoZSBwZXItRFMgc3RhdHMgKHJwYyBh
bmQgb3RoZXJzKSBzb21ld2hlcmUgaW4gL3Byb2MvZnMvbmZzZnMvDQo+IHNvb24uDQo+IA0KPiAg
ZnMvbmZzL25mczRmaWxlbGF5b3V0LmMgICAgICAgIHwgICAzMCArKysrKysrKysrKysrKysrKysr
KysrKysrKw0KPiAgZnMvbmZzL3BuZnMuaCAgICAgICAgICAgICAgICAgIHwgICAyMyArKysrKysr
KysrKysrKysrKysrKw0KPiAgZnMvbmZzL3BuZnNfZGV2LmMgICAgICAgICAgICAgIHwgICA0NSAr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQo+ICBmcy9uZnMvc3VwZXIu
YyAgICAgICAgICAgICAgICAgfCAgICAzICstDQo+ICBpbmNsdWRlL2xpbnV4L3N1bnJwYy9tZXRy
aWNzLmggfCAgIDEzICsrKysrKysrKy0NCj4gIG5ldC9zdW5ycGMvc3RhdHMuYyAgICAgICAgICAg
ICB8ICAgMzggKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKystDQo+ICA2IGZpbGVzIGNo
YW5nZWQsIDE0NyBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdp
dCBhL2ZzL25mcy9uZnM0ZmlsZWxheW91dC5jIGIvZnMvbmZzL25mczRmaWxlbGF5b3V0LmMNCj4g
aW5kZXggNzliZTdhYy4uNTFjNzFlNSAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL25mczRmaWxlbGF5
b3V0LmMNCj4gKysrIGIvZnMvbmZzL25mczRmaWxlbGF5b3V0LmMNCj4gQEAgLTMzLDYgKzMzLDgg
QEANCj4gICNpbmNsdWRlIDxsaW51eC9uZnNfcGFnZS5oPg0KPiAgI2luY2x1ZGUgPGxpbnV4L21v
ZHVsZS5oPg0KPiAgDQo+ICsjaW5jbHVkZSA8bGludXgvc3VucnBjL21ldHJpY3MuaD4NCj4gKw0K
PiAgI2luY2x1ZGUgImludGVybmFsLmgiDQo+ICAjaW5jbHVkZSAibmZzNGZpbGVsYXlvdXQuaCIN
Cj4gIA0KPiBAQCAtOTE4LDYgKzkyMCwzMyBAQCBmaWxlbGF5b3V0X2ZyZWVfZGV2ZWljZWlkX25v
ZGUoc3RydWN0IG5mczRfZGV2aWNlaWRfbm9kZSAqZCkNCj4gIAluZnM0X2ZsX2ZyZWVfZGV2aWNl
aWQoY29udGFpbmVyX29mKGQsIHN0cnVjdCBuZnM0X2ZpbGVfbGF5b3V0X2RzYWRkciwgaWRfbm9k
ZSkpOw0KPiAgfQ0KPiAgDQo+ICtzdGF0aWMgdW5zaWduZWQgaW50DQo+ICtmaWxlbGF5b3V0X3Jw
Y19hZGRfaW9zdGF0cyhzdHJ1Y3QgcnBjX2lvc3RhdHMgKnRvdGFsLA0KPiArCQkJICAgdW5zaWdu
ZWQgaW50IG1heHByb2MsDQo+ICsJCQkgICBzdHJ1Y3QgbmZzNF9kZXZpY2VpZF9ub2RlICpkKQ0K
PiArew0KPiArCXN0cnVjdCBuZnM0X2ZpbGVfbGF5b3V0X2RzYWRkciAqZHNhZGRyOw0KPiArCXN0
cnVjdCBuZnM0X3BuZnNfZHMgKmRzOw0KPiArCXVuc2lnbmVkIGludCBudW1fZHMgPSAwOw0KPiAr
CXUzMiBpOw0KPiArDQo+ICsJZHNhZGRyID0gY29udGFpbmVyX29mKGQsIHN0cnVjdCBuZnM0X2Zp
bGVfbGF5b3V0X2RzYWRkciwgaWRfbm9kZSk7DQo+ICsNCj4gKwlmb3IgKGkgPSAwOyBpIDwgZHNh
ZGRyLT5kc19udW07IGkrKykgew0KPiArCQlkcyA9IGRzYWRkci0+ZHNfbGlzdFtpXTsNCj4gKw0K
PiArCQkvKiBvbmx5IGFkZCBpb3N0YXRzIGlmIG5mc19jbGllbnQgaXMgZGlmZmVyZW50IGZyb20g
TURTICovDQo+ICsJCWlmIChkcyAmJiBkcy0+ZHNfY2xwICYmIGQtPm5mc19jbGllbnQgIT0gZHMt
PmRzX2NscCkgew0KPiArCQkJc3RydWN0IHJwY19jbG50ICpjbG50ID0gZHMtPmRzX2NscC0+Y2xf
cnBjY2xpZW50Ow0KDQpIbW1tLi4uLiBTaG91bGRuJ3QgdGhpcyByYXRoZXIgYmUgdGhlIE5GU19D
TElFTlQoaW5vZGUpPyBBbHNvLCB3aHkgbm90DQpkbyB0aGlzIGluIHRoZSByZWFkL3dyaXRlIGNv
bXBsZXRpb24gY29kZT8gV2h5IHdhbGsgdGhlIGRldmljZWlkIGxpc3QgYXQNCmFsbD8NCg0KPiAr
CQkJcnBjX2FkZF9pb3N0YXRzKHRvdGFsLCBjbG50LT5jbF9tZXRyaWNzLA0KPiArCQkJCQltaW4o
bWF4cHJvYywgY2xudC0+Y2xfbWF4cHJvYykpOw0KPiArCQkJbnVtX2RzKys7DQo+ICsJCX0NCj4g
Kwl9DQo+ICsNCj4gKwlyZXR1cm4gbnVtX2RzOw0KPiArfQ0KPiArDQo+ICBzdGF0aWMgc3RydWN0
IHBuZnNfbGF5b3V0ZHJpdmVyX3R5cGUgZmlsZWxheW91dF90eXBlID0gew0KPiAgCS5pZAkJCT0g
TEFZT1VUX05GU1Y0XzFfRklMRVMsDQo+ICAJLm5hbWUJCQk9ICJMQVlPVVRfTkZTVjRfMV9GSUxF
UyIsDQo+IEBAIC05MzIsNiArOTYxLDcgQEAgc3RhdGljIHN0cnVjdCBwbmZzX2xheW91dGRyaXZl
cl90eXBlIGZpbGVsYXlvdXRfdHlwZSA9IHsNCj4gIAkucmVhZF9wYWdlbGlzdAkJPSBmaWxlbGF5
b3V0X3JlYWRfcGFnZWxpc3QsDQo+ICAJLndyaXRlX3BhZ2VsaXN0CQk9IGZpbGVsYXlvdXRfd3Jp
dGVfcGFnZWxpc3QsDQo+ICAJLmZyZWVfZGV2aWNlaWRfbm9kZQk9IGZpbGVsYXlvdXRfZnJlZV9k
ZXZlaWNlaWRfbm9kZSwNCj4gKwkuZHNfcnBjX2FkZF9pb3N0YXRzCT0gZmlsZWxheW91dF9ycGNf
YWRkX2lvc3RhdHMsDQo+ICB9Ow0KPiAgDQo+ICBzdGF0aWMgaW50IF9faW5pdCBuZnM0ZmlsZWxh
eW91dF9pbml0KHZvaWQpDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvcG5mcy5oIGIvZnMvbmZzL3Bu
ZnMuaA0KPiBpbmRleCA1M2Q1OTNhLi5kMWI4MmViIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvcG5m
cy5oDQo+ICsrKyBiL2ZzL25mcy9wbmZzLmgNCj4gQEAgLTcyLDYgKzcyLDcgQEAgZW51bSBsYXlv
dXRkcml2ZXJfcG9saWN5X2ZsYWdzIHsNCj4gIH07DQo+ICANCj4gIHN0cnVjdCBuZnM0X2Rldmlj
ZWlkX25vZGU7DQo+ICtzdHJ1Y3QgcnBjX2lvc3RhdHM7DQo+ICANCj4gIC8qIFBlci1sYXlvdXQg
ZHJpdmVyIHNwZWNpZmljIHJlZ2lzdHJhdGlvbiBzdHJ1Y3R1cmUgKi8NCj4gIHN0cnVjdCBwbmZz
X2xheW91dGRyaXZlcl90eXBlIHsNCj4gQEAgLTExOSw2ICsxMjAsOSBAQCBzdHJ1Y3QgcG5mc19s
YXlvdXRkcml2ZXJfdHlwZSB7DQo+ICAJdm9pZCAoKmVuY29kZV9sYXlvdXRjb21taXQpIChzdHJ1
Y3QgcG5mc19sYXlvdXRfaGRyICpsYXlvdXRpZCwNCj4gIAkJCQkgICAgIHN0cnVjdCB4ZHJfc3Ry
ZWFtICp4ZHIsDQo+ICAJCQkJICAgICBjb25zdCBzdHJ1Y3QgbmZzNF9sYXlvdXRjb21taXRfYXJn
cyAqYXJncyk7DQo+ICsNCj4gKwl1bnNpZ25lZCBpbnQgKCpkc19ycGNfYWRkX2lvc3RhdHMpIChz
dHJ1Y3QgcnBjX2lvc3RhdHMgKiwgdW5zaWduZWQgaW50LA0KPiArCQkJCQkgICAgc3RydWN0IG5m
czRfZGV2aWNlaWRfbm9kZSAqKTsNCj4gIH07DQo+ICANCj4gIHN0cnVjdCBwbmZzX2xheW91dF9o
ZHIgew0KPiBAQCAtMjM5LDYgKzI0MywxMCBAQCB2b2lkIG5mczRfaW5pdF9kZXZpY2VpZF9ub2Rl
KHN0cnVjdCBuZnM0X2RldmljZWlkX25vZGUgKiwNCj4gIHN0cnVjdCBuZnM0X2RldmljZWlkX25v
ZGUgKm5mczRfaW5zZXJ0X2RldmljZWlkX25vZGUoc3RydWN0IG5mczRfZGV2aWNlaWRfbm9kZSAq
KTsNCj4gIGJvb2wgbmZzNF9wdXRfZGV2aWNlaWRfbm9kZShzdHJ1Y3QgbmZzNF9kZXZpY2VpZF9u
b2RlICopOw0KPiAgdm9pZCBuZnM0X2RldmljZWlkX3B1cmdlX2NsaWVudChjb25zdCBzdHJ1Y3Qg
bmZzX2NsaWVudCAqKTsNCj4gK3ZvaWQgbmZzNF9kZXZpY2VpZF9ycGNfcHJpbnRfaW9zdGF0cyhz
dHJ1Y3Qgc2VxX2ZpbGUgKnNlcSwNCj4gKwkJCQkgICAgIGNvbnN0IHN0cnVjdCBuZnNfY2xpZW50
ICpjbHAsDQo+ICsJCQkJICAgICBjb25zdCBzdHJ1Y3QgcG5mc19sYXlvdXRkcml2ZXJfdHlwZSAq
bGQpOw0KPiArDQo+ICANCj4gIHN0YXRpYyBpbmxpbmUgaW50IGxvX2ZhaWxfYml0KHUzMiBpb21v
ZGUpDQo+ICB7DQo+IEBAIC0zMjgsNiArMzM2LDE1IEBAIHN0YXRpYyBpbmxpbmUgaW50IHBuZnNf
cmV0dXJuX2xheW91dChzdHJ1Y3QgaW5vZGUgKmlubykNCj4gIAlyZXR1cm4gMDsNCj4gIH0NCj4g
IA0KPiArc3RhdGljIGlubGluZSBpbnQNCj4gK3BuZnNfcnBjX3ByaW50X2lvc3RhdHMoc3RydWN0
IHNlcV9maWxlICptLCBzdHJ1Y3QgbmZzX3NlcnZlciAqbmZzcykNCj4gK3sNCj4gKwlpZiAoIXBu
ZnNfZW5hYmxlZF9zYihuZnNzKSkNCj4gKwkJcmV0dXJuIDA7DQo+ICsJbmZzNF9kZXZpY2VpZF9y
cGNfcHJpbnRfaW9zdGF0cyhtLCBuZnNzLT5uZnNfY2xpZW50LA0KPiArCQkJCQluZnNzLT5wbmZz
X2N1cnJfbGQpOw0KPiArCXJldHVybiAxOw0KPiArfQ0KPiAgI2Vsc2UgIC8qIENPTkZJR19ORlNf
VjRfMSAqLw0KPiAgDQo+ICBzdGF0aWMgaW5saW5lIHZvaWQgcG5mc19kZXN0cm95X2FsbF9sYXlv
dXRzKHN0cnVjdCBuZnNfY2xpZW50ICpjbHApDQo+IEBAIC00MjksNiArNDQ2LDEyIEBAIHN0YXRp
YyBpbmxpbmUgaW50IHBuZnNfbGF5b3V0Y29tbWl0X2lub2RlKHN0cnVjdCBpbm9kZSAqaW5vZGUs
IGJvb2wgc3luYykNCj4gIHN0YXRpYyBpbmxpbmUgdm9pZCBuZnM0X2RldmljZWlkX3B1cmdlX2Ns
aWVudChzdHJ1Y3QgbmZzX2NsaWVudCAqbmNsKQ0KPiAgew0KPiAgfQ0KPiArDQo+ICtzdGF0aWMg
aW5saW5lIHZvaWQNCj4gK3BuZnNfcnBjX3ByaW50X2lvc3RhdHMoc3RydWN0IHNlcV9maWxlICpt
LCBzdHJ1Y3QgbmZzX3NlcnZlciAqbmZzcykNCj4gK3sNCj4gKwlyZXR1cm4gMDsNCj4gK30NCj4g
ICNlbmRpZiAvKiBDT05GSUdfTkZTX1Y0XzEgKi8NCj4gIA0KPiAgI2VuZGlmIC8qIEZTX05GU19Q
TkZTX0ggKi8NCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9wbmZzX2Rldi5jIGIvZnMvbmZzL3BuZnNf
ZGV2LmMNCj4gaW5kZXggNGYzNTlkMi4uYWE1ZDEwMiAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL3Bu
ZnNfZGV2LmMNCj4gKysrIGIvZnMvbmZzL3BuZnNfZGV2LmMNCj4gQEAgLTI5LDYgKzI5LDkgQEAN
Cj4gICAqLw0KPiAgDQo+ICAjaW5jbHVkZSA8bGludXgvZXhwb3J0Lmg+DQo+ICsNCj4gKyNpbmNs
dWRlIDxsaW51eC9zdW5ycGMvbWV0cmljcy5oPg0KPiArDQo+ICAjaW5jbHVkZSAicG5mcy5oIg0K
PiAgDQo+ICAjZGVmaW5lIE5GU0RCR19GQUNJTElUWQkJTkZTREJHX1BORlMNCj4gQEAgLTExNSw2
ICsxMTgsNDggQEAgbmZzNF9maW5kX2dldF9kZXZpY2VpZChjb25zdCBzdHJ1Y3QgcG5mc19sYXlv
dXRkcml2ZXJfdHlwZSAqbGQsDQo+ICB9DQo+ICBFWFBPUlRfU1lNQk9MX0dQTChuZnM0X2ZpbmRf
Z2V0X2RldmljZWlkKTsNCj4gIA0KPiArDQo+ICt2b2lkDQo+ICtuZnM0X2RldmljZWlkX3JwY19w
cmludF9pb3N0YXRzKHN0cnVjdCBzZXFfZmlsZSAqc2VxLA0KPiArCQkJCWNvbnN0IHN0cnVjdCBu
ZnNfY2xpZW50ICpjbHAsDQo+ICsJCQkJY29uc3Qgc3RydWN0IHBuZnNfbGF5b3V0ZHJpdmVyX3R5
cGUgKmxkKQ0KPiArew0KPiArCXN0cnVjdCBuZnM0X2RldmljZWlkX25vZGUgKmQ7DQo+ICsJc3Ry
dWN0IGhsaXN0X25vZGUgKm47DQo+ICsJc3RydWN0IHJwY19pb3N0YXRzICp0b3RhbHM7DQo+ICsJ
Y2hhciBtc2didWZbNjRdOw0KPiArCXVuc2lnbmVkIGludCBudW1fZHMgPSAwOw0KPiArCXVuc2ln
bmVkIGludCBtYXhwcm9jOw0KPiArCWxvbmcgaDsNCj4gKw0KPiArCWlmICghbGQtPmRzX3JwY19h
ZGRfaW9zdGF0cykNCj4gKwkJcmV0dXJuOw0KPiArCXRvdGFscyA9IHJwY19hbGxvY19pb3N0YXRz
KGNscC0+Y2xfcnBjY2xpZW50KTsNCj4gKwlpZiAoIXRvdGFscykNCj4gKwkJcmV0dXJuOw0KPiAr
CW1heHByb2MgPSBjbHAtPmNsX3JwY2NsaWVudC0+Y2xfbWF4cHJvYzsNCj4gKwlycGNfYWRkX2lv
c3RhdHModG90YWxzLCBjbHAtPmNsX3JwY2NsaWVudC0+Y2xfbWV0cmljcywgbWF4cHJvYyk7DQo+
ICsNCj4gKwlyY3VfcmVhZF9sb2NrKCk7DQo+ICsJZm9yIChoID0gMDsgaCA8IE5GUzRfREVWSUNF
X0lEX0hBU0hfU0laRTsgaCsrKSB7DQo+ICsJCWhsaXN0X2Zvcl9lYWNoX2VudHJ5X3JjdShkLCBu
LCAmbmZzNF9kZXZpY2VpZF9jYWNoZVtoXSwgbm9kZSkgew0KPiArCQkJaWYgKGQtPmxkID09IGxk
ICYmIGQtPm5mc19jbGllbnQgPT0gY2xwKSB7DQo+ICsJCQkJaWYgKGF0b21pY19yZWFkKCZkLT5y
ZWYpKQ0KPiArCQkJCQludW1fZHMgKz0gbGQtPmRzX3JwY19hZGRfaW9zdGF0cyh0b3RhbHMsDQo+
ICsJCQkJCQkJbWF4cHJvYywgZCk7DQo+ICsJCQl9DQo+ICsJCX0NCj4gKwl9DQo+ICsJcmN1X3Jl
YWRfdW5sb2NrKCk7DQo+ICsNCj4gKwlzbnByaW50Zihtc2didWYsIHNpemVvZihtc2didWYpLA0K
PiArCQkgIiBmb3IgdGhlIG1ldGFkYXRhIHNlcnZlciBhbmQgJXUgZGF0YSBzZXJ2ZXIlcyIsIG51
bV9kcywNCj4gKwkJIChudW1fZHMgIT0gMSkgPyAicyIgOiAiIik7DQo+ICsJcnBjX3ByaW50X2lv
c3RhdHMoc2VxLCBjbHAtPmNsX3JwY2NsaWVudCwgdG90YWxzLCBtc2didWYpOw0KPiArCXJwY19m
cmVlX2lvc3RhdHModG90YWxzKTsNCj4gK30NCj4gK0VYUE9SVF9TWU1CT0xfR1BMKG5mczRfZGV2
aWNlaWRfcnBjX3ByaW50X2lvc3RhdHMpOw0KDQpJcyB0aGlzIG5lZWRlZD8gQUZBSUNTIGl0IGlz
IHVzZWQgaW4gZnMvbmZzL3N1cGVyLmMsIHdoaWNoIGlzIGluIHRoZQ0Kc2FtZSBtb2R1bGUgKGku
ZS4gbmZzLmtvKS4NCg0KPiAgLyoNCj4gICAqIFJlbW92ZSBhIGRldmljZWlkIGZyb20gY2FjaGUN
Cj4gICAqDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvc3VwZXIuYyBiL2ZzL25mcy9zdXBlci5jDQo+
IGluZGV4IGQxOGE5MGIuLjc2ZDA4NTAgMTAwNjQ0DQo+IC0tLSBhL2ZzL25mcy9zdXBlci5jDQo+
ICsrKyBiL2ZzL25mcy9zdXBlci5jDQo+IEBAIC04NzAsNyArODcwLDggQEAgc3RhdGljIGludCBu
ZnNfc2hvd19zdGF0cyhzdHJ1Y3Qgc2VxX2ZpbGUgKm0sIHN0cnVjdCBkZW50cnkgKnJvb3QpDQo+
ICAjZW5kaWYNCj4gIAlzZXFfcHJpbnRmKG0sICJcbiIpOw0KPiAgDQo+IC0JcnBjX3ByaW50X2lv
c3RhdHMobSwgbmZzcy0+Y2xpZW50KTsNCj4gKwlpZiAoIXBuZnNfcnBjX3ByaW50X2lvc3RhdHMo
bSwgbmZzcykpDQo+ICsJCXJwY19wcmludF9pb3N0YXRzKG0sIG5mc3MtPmNsaWVudCwgTlVMTCwg
TlVMTCk7DQo+ICANCj4gIAlyZXR1cm4gMDsNCj4gIH0NCj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUv
bGludXgvc3VucnBjL21ldHJpY3MuaCBiL2luY2x1ZGUvbGludXgvc3VucnBjL21ldHJpY3MuaA0K
PiBpbmRleCBiNmVkYmMwLi5lNmUwNGU0IDEwMDY0NA0KPiAtLS0gYS9pbmNsdWRlL2xpbnV4L3N1
bnJwYy9tZXRyaWNzLmgNCj4gKysrIGIvaW5jbHVkZS9saW51eC9zdW5ycGMvbWV0cmljcy5oDQo+
IEBAIC03NSwxNCArNzUsMjMgQEAgc3RydWN0IHJwY19jbG50Ow0KPiAgDQo+ICBzdHJ1Y3QgcnBj
X2lvc3RhdHMgKglycGNfYWxsb2NfaW9zdGF0cyhzdHJ1Y3QgcnBjX2NsbnQgKik7DQo+ICB2b2lk
CQkJcnBjX2NvdW50X2lvc3RhdHMoc3RydWN0IHJwY190YXNrICopOw0KPiAtdm9pZAkJCXJwY19w
cmludF9pb3N0YXRzKHN0cnVjdCBzZXFfZmlsZSAqLCBzdHJ1Y3QgcnBjX2NsbnQgKik7DQo+ICt2
b2lkCQkJcnBjX3ByaW50X2lvc3RhdHMoc3RydWN0IHNlcV9maWxlICosDQo+ICsJCQkJCSAgc3Ry
dWN0IHJwY19jbG50ICosDQo+ICsJCQkJCSAgc3RydWN0IHJwY19pb3N0YXRzICosDQo+ICsJCQkJ
CSAgY29uc3QgY2hhciAqKTsNCj4gK3ZvaWQJCQlycGNfYWRkX2lvc3RhdHMoc3RydWN0IHJwY19p
b3N0YXRzICosDQo+ICsJCQkJCXN0cnVjdCBycGNfaW9zdGF0cyAqLCB1bnNpZ25lZCBpbnQpOw0K
PiAgdm9pZAkJCXJwY19mcmVlX2lvc3RhdHMoc3RydWN0IHJwY19pb3N0YXRzICopOw0KPiAgDQo+
ICAjZWxzZSAgLyogIENPTkZJR19QUk9DX0ZTICAqLw0KPiAgDQo+ICBzdGF0aWMgaW5saW5lIHN0
cnVjdCBycGNfaW9zdGF0cyAqcnBjX2FsbG9jX2lvc3RhdHMoc3RydWN0IHJwY19jbG50ICpjbG50
KSB7IHJldHVybiBOVUxMOyB9DQo+ICBzdGF0aWMgaW5saW5lIHZvaWQgcnBjX2NvdW50X2lvc3Rh
dHMoc3RydWN0IHJwY190YXNrICp0YXNrKSB7fQ0KPiAtc3RhdGljIGlubGluZSB2b2lkIHJwY19w
cmludF9pb3N0YXRzKHN0cnVjdCBzZXFfZmlsZSAqc2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQp
IHt9DQo+ICtzdGF0aWMgaW5saW5lIHZvaWQgcnBjX3ByaW50X2lvc3RhdHMoc3RydWN0IHNlcV9m
aWxlICpzZXEsDQo+ICsJCQkJICAgICBzdHJ1Y3QgcnBjX2lvc3RhdHMgKnN0YXQpIHt9DQo+ICtz
dGF0aWMgaW5saW5lIHZvaWQgcnBjX2FkZF9pb3N0YXRzKHN0cnVjdCBycGNfaW9zdGF0cyAqdG90
YWwsDQo+ICsJCQkJICAgc3RydWN0IHJwY19pb3N0YXRzICpuZXcsDQo+ICsJCQkJICAgdW5zaWdu
ZWQgaW50IG1heHByb2MpIHt9DQo+ICBzdGF0aWMgaW5saW5lIHZvaWQgcnBjX2ZyZWVfaW9zdGF0
cyhzdHJ1Y3QgcnBjX2lvc3RhdHMgKnN0YXRzKSB7fQ0KPiAgDQo+ICAjZW5kaWYgIC8qICBDT05G
SUdfUFJPQ19GUyAgKi8NCj4gZGlmZiAtLWdpdCBhL25ldC9zdW5ycGMvc3RhdHMuYyBiL25ldC9z
dW5ycGMvc3RhdHMuYw0KPiBpbmRleCAzYzRmNjg4Li4zNDJkZjU3IDEwMDY0NA0KPiAtLS0gYS9u
ZXQvc3VucnBjL3N0YXRzLmMNCj4gKysrIGIvbmV0L3N1bnJwYy9zdGF0cy5jDQo+IEBAIC0xNzYs
MTIgKzE3NiwxNiBAQCBzdGF0aWMgdm9pZCBfcHJpbnRfbmFtZShzdHJ1Y3Qgc2VxX2ZpbGUgKnNl
cSwgdW5zaWduZWQgaW50IG9wLA0KPiAgCQlzZXFfcHJpbnRmKHNlcSwgIlx0JTEydTogIiwgb3Ap
Ow0KPiAgfQ0KPiAgDQo+IC12b2lkIHJwY19wcmludF9pb3N0YXRzKHN0cnVjdCBzZXFfZmlsZSAq
c2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQpDQo+ICt2b2lkIHJwY19wcmludF9pb3N0YXRzKHN0
cnVjdCBzZXFfZmlsZSAqc2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQsDQo+ICsJCSAgICAgICBz
dHJ1Y3QgcnBjX2lvc3RhdHMgKmFsdHN0YXRzLA0KPiArCQkgICAgICAgY29uc3QgY2hhciAqYWx0
c3RhdHNfbXNnKQ0KPiAgew0KPiAgCXN0cnVjdCBycGNfaW9zdGF0cyAqc3RhdHMgPSBjbG50LT5j
bF9tZXRyaWNzOw0KPiAgCXN0cnVjdCBycGNfeHBydCAqeHBydCA9IGNsbnQtPmNsX3hwcnQ7DQo+
ICAJdW5zaWduZWQgaW50IG9wLCBtYXhwcm9jID0gY2xudC0+Y2xfbWF4cHJvYzsNCj4gIA0KPiAr
CWlmIChhbHRzdGF0cykNCj4gKwkJc3RhdHMgPSBhbHRzdGF0czsNCj4gIAlpZiAoIXN0YXRzKQ0K
PiAgCQlyZXR1cm47DQo+ICANCj4gQEAgLTE5Miw3ICsxOTYsOCBAQCB2b2lkIHJwY19wcmludF9p
b3N0YXRzKHN0cnVjdCBzZXFfZmlsZSAqc2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQpDQo+ICAJ
aWYgKHhwcnQpDQo+ICAJCXhwcnQtPm9wcy0+cHJpbnRfc3RhdHMoeHBydCwgc2VxKTsNCj4gIA0K
PiAtCXNlcV9wcmludGYoc2VxLCAiXHRwZXItb3Agc3RhdGlzdGljc1xuIik7DQo+ICsJc2VxX3By
aW50ZihzZXEsICJcdHBlci1vcCBzdGF0aXN0aWNzJXNcbiIsDQo+ICsJCSAgIChhbHRzdGF0cyAm
JiBhbHRzdGF0c19tc2cpID8gYWx0c3RhdHNfbXNnIDogIiIpOw0KPiAgCWZvciAob3AgPSAwOyBv
cCA8IG1heHByb2M7IG9wKyspIHsNCj4gIAkJc3RydWN0IHJwY19pb3N0YXRzICptZXRyaWNzID0g
JnN0YXRzW29wXTsNCj4gIAkJX3ByaW50X25hbWUoc2VxLCBvcCwgY2xudC0+Y2xfcHJvY2luZm8p
Ow0KPiBAQCAtMjA5LDYgKzIxNCwzNSBAQCB2b2lkIHJwY19wcmludF9pb3N0YXRzKHN0cnVjdCBz
ZXFfZmlsZSAqc2VxLCBzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQpDQo+ICB9DQo+ICBFWFBPUlRfU1lN
Qk9MX0dQTChycGNfcHJpbnRfaW9zdGF0cyk7DQo+ICANCj4gK3ZvaWQNCj4gK3JwY19hZGRfaW9z
dGF0cyhzdHJ1Y3QgcnBjX2lvc3RhdHMgKnRvdGFscywgc3RydWN0IHJwY19pb3N0YXRzICpuZXcs
DQo+ICsJCXVuc2lnbmVkIGludCBtYXhwcm9jKQ0KPiArew0KPiArCXVuc2lnbmVkIGludCBvcDsN
Cj4gKw0KPiArCWZvciAob3AgPSAwOyBvcCA8IG1heHByb2M7IG9wKyspIHsNCj4gKwkJc3RydWN0
IHJwY19pb3N0YXRzICpvcF90b3RhbHMgPSAmdG90YWxzW29wXSwNCj4gKwkJCQkgICAqb3BfbmV3
ID0gJm5ld1tvcF07DQo+ICsNCj4gKwkJb3BfdG90YWxzLT5vbV9vcHMgKz0gb3BfbmV3LT5vbV9v
cHM7DQo+ICsJCW9wX3RvdGFscy0+b21fbnRyYW5zICs9IG9wX25ldy0+b21fbnRyYW5zOw0KPiAr
CQlvcF90b3RhbHMtPm9tX3RpbWVvdXRzICs9IG9wX25ldy0+b21fdGltZW91dHM7DQo+ICsNCj4g
KwkJb3BfdG90YWxzLT5vbV9ieXRlc19zZW50ICs9IG9wX25ldy0+b21fYnl0ZXNfc2VudDsNCj4g
KwkJb3BfdG90YWxzLT5vbV9ieXRlc19yZWN2ICs9IG9wX25ldy0+b21fYnl0ZXNfcmVjdjsNCj4g
Kw0KPiArCQlvcF90b3RhbHMtPm9tX3F1ZXVlID0ga3RpbWVfYWRkKG9wX3RvdGFscy0+b21fcXVl
dWUsDQo+ICsJCQkJCSAgICAgICBvcF9uZXctPm9tX3F1ZXVlKTsNCj4gKw0KPiArCQlvcF90b3Rh
bHMtPm9tX3J0dCA9IGt0aW1lX2FkZChvcF90b3RhbHMtPm9tX3J0dCwNCj4gKwkJCQkJICAgICAg
b3BfbmV3LT5vbV9ydHQpOw0KPiArDQo+ICsJCW9wX3RvdGFscy0+b21fZXhlY3V0ZSA9IGt0aW1l
X2FkZChvcF90b3RhbHMtPm9tX2V4ZWN1dGUsDQo+ICsJCQkJCQkgb3BfbmV3LT5vbV9leGVjdXRl
KTsNCj4gKwl9DQo+ICt9DQo+ICtFWFBPUlRfU1lNQk9MX0dQTChycGNfYWRkX2lvc3RhdHMpOw0K
PiArDQo+ICAvKg0KPiAgICogUmVnaXN0ZXIvdW5yZWdpc3RlciBSUEMgcHJvYyBmaWxlcw0KPiAg
ICovDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIN
Cg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 12989 bytes --]


On Feb 9, 2012, at 1:16 PM, Myklebust, Trond wrote:

> On Thu, 2012-02-09 at 11:41 -0500, Weston Andros Adamson wrote:
>> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
>> filelayout mounts.
>> 
>> The additional info printed on the "per-op statistics" line does not break
>> current mountstats(8) from nfs-utils.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> 
>> This is clearly a better approach than I was taking trying to keep DS rpc stats
>> separate in /proc/self/mountstats.
>> 
>> FYI, there are no other callers of rpc_print_iostats().
>> 
>> The new output of /proc/self/mountstats works fine with current mountstats 
>> userland tool.  The additional data on the per-op statistics line doesn't make
>> it into the output, but I left it in /proc/self/mountstats because it doesn't
>> break anything and can be included in newer versions of mountstats.
>> 
>> I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
>> soon.
>> 
>> fs/nfs/nfs4filelayout.c        |   30 ++++++++++++++++++++++++++
>> fs/nfs/pnfs.h                  |   23 ++++++++++++++++++++
>> fs/nfs/pnfs_dev.c              |   45 ++++++++++++++++++++++++++++++++++++++++
>> fs/nfs/super.c                 |    3 +-
>> include/linux/sunrpc/metrics.h |   13 +++++++++-
>> net/sunrpc/stats.c             |   38 ++++++++++++++++++++++++++++++++-
>> 6 files changed, 147 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index 79be7ac..51c71e5 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -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,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
>> 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
>> }
>> 
>> +static unsigned int
>> +filelayout_rpc_add_iostats(struct rpc_iostats *total,
>> +			   unsigned int maxproc,
>> +			   struct nfs4_deviceid_node *d)
>> +{
>> +	struct nfs4_file_layout_dsaddr *dsaddr;
>> +	struct nfs4_pnfs_ds *ds;
>> +	unsigned int num_ds = 0;
>> +	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];
>> +
>> +		/* only add iostats if nfs_client is different from MDS */
>> +		if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
>> +			struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
> 
> Hmmm.... Shouldn't this rather be the NFS_CLIENT(inode)? Also, why not
> do this in the read/write completion code? Why walk the deviceid list at
> all?

I'm not really following you.  Note that these are rpc_iostats not nfs_iostats.  
nfs_iostats have been properly counting operations to DSs for some time now.
rpc_iostats count operations, rtt, exectime, etc per rpc procedure (or in the case of NFSv4.X there is a hack to use operations within compounds).

We have to walk the device list to get access to the underlying DS's nfs_client->rpc_client -- I can't just put this in the read/write completion because, unlike nfs_iostats, rpc_iostats count all operations and not just read/write events/bytes (not to mention that the accumulation is done at the rpc layer).

I do have an implementation that doesn't need to walk the deviceid list by allowing a shared rpc_iostats struct between multiple rpc_clients (in addition to the current rpc_iostats structure), but that required adding locking and reference counting -- all for printing stats (obviously not what we want).

Does that make sense?  If so, I'll repost with the fix from below.

Also, it just occurred to me that you might want me to separate this into two patches one for changes in net/sunrpc and one for fs/nfs...

-dros

> 
>> +			rpc_add_iostats(total, clnt->cl_metrics,
>> +					min(maxproc, clnt->cl_maxproc));
>> +			num_ds++;
>> +		}
>> +	}
>> +
>> +	return num_ds;
>> +}
>> +
>> static struct pnfs_layoutdriver_type filelayout_type = {
>> 	.id			= LAYOUT_NFSV4_1_FILES,
>> 	.name			= "LAYOUT_NFSV4_1_FILES",
>> @@ -932,6 +961,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_add_iostats	= filelayout_rpc_add_iostats,
>> };
>> 
>> static int __init nfs4filelayout_init(void)
>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>> index 53d593a..d1b82eb 100644
>> --- a/fs/nfs/pnfs.h
>> +++ b/fs/nfs/pnfs.h
>> @@ -72,6 +72,7 @@ enum layoutdriver_policy_flags {
>> };
>> 
>> struct nfs4_deviceid_node;
>> +struct rpc_iostats;
>> 
>> /* Per-layout driver specific registration structure */
>> struct pnfs_layoutdriver_type {
>> @@ -119,6 +120,9 @@ struct pnfs_layoutdriver_type {
>> 	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
>> 				     struct xdr_stream *xdr,
>> 				     const struct nfs4_layoutcommit_args *args);
>> +
>> +	unsigned int (*ds_rpc_add_iostats) (struct rpc_iostats *, unsigned int,
>> +					    struct nfs4_deviceid_node *);
>> };
>> 
>> struct pnfs_layout_hdr {
>> @@ -239,6 +243,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 +336,15 @@ static inline int pnfs_return_layout(struct inode *ino)
>> 	return 0;
>> }
>> 
>> +static inline int
>> +pnfs_rpc_print_iostats(struct seq_file *m, struct nfs_server *nfss)
>> +{
>> +	if (!pnfs_enabled_sb(nfss))
>> +		return 0;
>> +	nfs4_deviceid_rpc_print_iostats(m, nfss->nfs_client,
>> +					nfss->pnfs_curr_ld);
>> +	return 1;
>> +}
>> #else  /* CONFIG_NFS_V4_1 */
>> 
>> static inline void pnfs_destroy_all_layouts(struct nfs_client *clp)
>> @@ -429,6 +446,12 @@ 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)
>> +{
>> +	return 0;
>> +}
>> #endif /* CONFIG_NFS_V4_1 */
>> 
>> #endif /* FS_NFS_PNFS_H */
>> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
>> index 4f359d2..aa5d102 100644
>> --- a/fs/nfs/pnfs_dev.c
>> +++ b/fs/nfs/pnfs_dev.c
>> @@ -29,6 +29,9 @@
>>  */
>> 
>> #include <linux/export.h>
>> +
>> +#include <linux/sunrpc/metrics.h>
>> +
>> #include "pnfs.h"
>> 
>> #define NFSDBG_FACILITY		NFSDBG_PNFS
>> @@ -115,6 +118,48 @@ 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;
>> +	struct rpc_iostats *totals;
>> +	char msgbuf[64];
>> +	unsigned int num_ds = 0;
>> +	unsigned int maxproc;
>> +	long h;
>> +
>> +	if (!ld->ds_rpc_add_iostats)
>> +		return;
>> +	totals = rpc_alloc_iostats(clp->cl_rpcclient);
>> +	if (!totals)
>> +		return;
>> +	maxproc = clp->cl_rpcclient->cl_maxproc;
>> +	rpc_add_iostats(totals, clp->cl_rpcclient->cl_metrics, maxproc);
>> +
>> +	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))
>> +					num_ds += ld->ds_rpc_add_iostats(totals,
>> +							maxproc, d);
>> +			}
>> +		}
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	snprintf(msgbuf, sizeof(msgbuf),
>> +		 " for the metadata server and %u data server%s", num_ds,
>> +		 (num_ds != 1) ? "s" : "");
>> +	rpc_print_iostats(seq, clp->cl_rpcclient, totals, msgbuf);
>> +	rpc_free_iostats(totals);
>> +}
>> +EXPORT_SYMBOL_GPL(nfs4_deviceid_rpc_print_iostats);
> 
> Is this needed? AFAICS it is used in fs/nfs/super.c, which is in the
> same module (i.e. nfs.ko).

oops!

> 
>> /*
>>  * Remove a deviceid from cache
>>  *
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index d18a90b..76d0850 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -870,7 +870,8 @@ static int nfs_show_stats(struct seq_file *m, struct dentry *root)
>> #endif
>> 	seq_printf(m, "\n");
>> 
>> -	rpc_print_iostats(m, nfss->client);
>> +	if (!pnfs_rpc_print_iostats(m, nfss))
>> +		rpc_print_iostats(m, nfss->client, NULL, NULL);
>> 
>> 	return 0;
>> }
>> diff --git a/include/linux/sunrpc/metrics.h b/include/linux/sunrpc/metrics.h
>> index b6edbc0..e6e04e4 100644
>> --- a/include/linux/sunrpc/metrics.h
>> +++ b/include/linux/sunrpc/metrics.h
>> @@ -75,14 +75,23 @@ struct rpc_clnt;
>> 
>> struct rpc_iostats *	rpc_alloc_iostats(struct rpc_clnt *);
>> void			rpc_count_iostats(struct rpc_task *);
>> -void			rpc_print_iostats(struct seq_file *, struct rpc_clnt *);
>> +void			rpc_print_iostats(struct seq_file *,
>> +					  struct rpc_clnt *,
>> +					  struct rpc_iostats *,
>> +					  const char *);
>> +void			rpc_add_iostats(struct rpc_iostats *,
>> +					struct rpc_iostats *, unsigned int);
>> void			rpc_free_iostats(struct rpc_iostats *);
>> 
>> #else  /*  CONFIG_PROC_FS  */
>> 
>> static inline struct rpc_iostats *rpc_alloc_iostats(struct rpc_clnt *clnt) { return NULL; }
>> static inline void rpc_count_iostats(struct rpc_task *task) {}
>> -static inline void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt) {}
>> +static inline void rpc_print_iostats(struct seq_file *seq,
>> +				     struct rpc_iostats *stat) {}
>> +static inline void rpc_add_iostats(struct rpc_iostats *total,
>> +				   struct rpc_iostats *new,
>> +				   unsigned int maxproc) {}
>> static inline void rpc_free_iostats(struct rpc_iostats *stats) {}
>> 
>> #endif  /*  CONFIG_PROC_FS  */
>> diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
>> index 3c4f688..342df57 100644
>> --- a/net/sunrpc/stats.c
>> +++ b/net/sunrpc/stats.c
>> @@ -176,12 +176,16 @@ static void _print_name(struct seq_file *seq, unsigned int op,
>> 		seq_printf(seq, "\t%12u: ", op);
>> }
>> 
>> -void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> +void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt,
>> +		       struct rpc_iostats *altstats,
>> +		       const char *altstats_msg)
>> {
>> 	struct rpc_iostats *stats = clnt->cl_metrics;
>> 	struct rpc_xprt *xprt = clnt->cl_xprt;
>> 	unsigned int op, maxproc = clnt->cl_maxproc;
>> 
>> +	if (altstats)
>> +		stats = altstats;
>> 	if (!stats)
>> 		return;
>> 
>> @@ -192,7 +196,8 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> 	if (xprt)
>> 		xprt->ops->print_stats(xprt, seq);
>> 
>> -	seq_printf(seq, "\tper-op statistics\n");
>> +	seq_printf(seq, "\tper-op statistics%s\n",
>> +		   (altstats && altstats_msg) ? altstats_msg : "");
>> 	for (op = 0; op < maxproc; op++) {
>> 		struct rpc_iostats *metrics = &stats[op];
>> 		_print_name(seq, op, clnt->cl_procinfo);
>> @@ -209,6 +214,35 @@ void rpc_print_iostats(struct seq_file *seq, struct rpc_clnt *clnt)
>> }
>> EXPORT_SYMBOL_GPL(rpc_print_iostats);
>> 
>> +void
>> +rpc_add_iostats(struct rpc_iostats *totals, struct rpc_iostats *new,
>> +		unsigned int maxproc)
>> +{
>> +	unsigned int op;
>> +
>> +	for (op = 0; op < maxproc; op++) {
>> +		struct rpc_iostats *op_totals = &totals[op],
>> +				   *op_new = &new[op];
>> +
>> +		op_totals->om_ops += op_new->om_ops;
>> +		op_totals->om_ntrans += op_new->om_ntrans;
>> +		op_totals->om_timeouts += op_new->om_timeouts;
>> +
>> +		op_totals->om_bytes_sent += op_new->om_bytes_sent;
>> +		op_totals->om_bytes_recv += op_new->om_bytes_recv;
>> +
>> +		op_totals->om_queue = ktime_add(op_totals->om_queue,
>> +					       op_new->om_queue);
>> +
>> +		op_totals->om_rtt = ktime_add(op_totals->om_rtt,
>> +					      op_new->om_rtt);
>> +
>> +		op_totals->om_execute = ktime_add(op_totals->om_execute,
>> +						 op_new->om_execute);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(rpc_add_iostats);
>> +
>> /*
>>  * Register/unregister RPC proc files
>>  */
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTA5IGF0IDE5OjEzICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiBPbiBGZWIgOSwgMjAxMiwgYXQgMToxNiBQTSwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g
DQo+ID4gT24gVGh1LCAyMDEyLTAyLTA5IGF0IDExOjQxIC0wNTAwLCBXZXN0b24gQW5kcm9zIEFk
YW1zb24gd3JvdGU6DQo+ID4+IEluY2x1ZGUgUlBDIHN0YXRpc3RpY3MgZnJvbSBhbGwgZGF0YSBz
ZXJ2ZXJzIGluIC9wcm9jL3NlbGYvbW91bnRzdGF0cyBmb3IgcE5GUw0KPiA+PiBmaWxlbGF5b3V0
IG1vdW50cy4NCj4gPj4gDQo+ID4+IFRoZSBhZGRpdGlvbmFsIGluZm8gcHJpbnRlZCBvbiB0aGUg
InBlci1vcCBzdGF0aXN0aWNzIiBsaW5lIGRvZXMgbm90IGJyZWFrDQo+ID4+IGN1cnJlbnQgbW91
bnRzdGF0cyg4KSBmcm9tIG5mcy11dGlscy4NCj4gPj4gDQo+ID4+IFNpZ25lZC1vZmYtYnk6IFdl
c3RvbiBBbmRyb3MgQWRhbXNvbiA8ZHJvc0BuZXRhcHAuY29tPg0KPiA+PiAtLS0NCj4gPj4gDQo+
ID4+IFRoaXMgaXMgY2xlYXJseSBhIGJldHRlciBhcHByb2FjaCB0aGFuIEkgd2FzIHRha2luZyB0
cnlpbmcgdG8ga2VlcCBEUyBycGMgc3RhdHMNCj4gPj4gc2VwYXJhdGUgaW4gL3Byb2Mvc2VsZi9t
b3VudHN0YXRzLg0KPiA+PiANCj4gPj4gRllJLCB0aGVyZSBhcmUgbm8gb3RoZXIgY2FsbGVycyBv
ZiBycGNfcHJpbnRfaW9zdGF0cygpLg0KPiA+PiANCj4gPj4gVGhlIG5ldyBvdXRwdXQgb2YgL3By
b2Mvc2VsZi9tb3VudHN0YXRzIHdvcmtzIGZpbmUgd2l0aCBjdXJyZW50IG1vdW50c3RhdHMgDQo+
ID4+IHVzZXJsYW5kIHRvb2wuICBUaGUgYWRkaXRpb25hbCBkYXRhIG9uIHRoZSBwZXItb3Agc3Rh
dGlzdGljcyBsaW5lIGRvZXNuJ3QgbWFrZQ0KPiA+PiBpdCBpbnRvIHRoZSBvdXRwdXQsIGJ1dCBJ
IGxlZnQgaXQgaW4gL3Byb2Mvc2VsZi9tb3VudHN0YXRzIGJlY2F1c2UgaXQgZG9lc24ndA0KPiA+
PiBicmVhayBhbnl0aGluZyBhbmQgY2FuIGJlIGluY2x1ZGVkIGluIG5ld2VyIHZlcnNpb25zIG9m
IG1vdW50c3RhdHMuDQo+ID4+IA0KPiA+PiBJIHBsYW4gb24gYWRkaW5nIHRoZSBwZXItRFMgc3Rh
dHMgKHJwYyBhbmQgb3RoZXJzKSBzb21ld2hlcmUgaW4gL3Byb2MvZnMvbmZzZnMvDQo+ID4+IHNv
b24uDQo+ID4+IA0KPiA+PiBmcy9uZnMvbmZzNGZpbGVsYXlvdXQuYyAgICAgICAgfCAgIDMwICsr
KysrKysrKysrKysrKysrKysrKysrKysrDQo+ID4+IGZzL25mcy9wbmZzLmggICAgICAgICAgICAg
ICAgICB8ICAgMjMgKysrKysrKysrKysrKysrKysrKysNCj4gPj4gZnMvbmZzL3BuZnNfZGV2LmMg
ICAgICAgICAgICAgIHwgICA0NSArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrDQo+ID4+IGZzL25mcy9zdXBlci5jICAgICAgICAgICAgICAgICB8ICAgIDMgKy0NCj4gPj4g
aW5jbHVkZS9saW51eC9zdW5ycGMvbWV0cmljcy5oIHwgICAxMyArKysrKysrKystDQo+ID4+IG5l
dC9zdW5ycGMvc3RhdHMuYyAgICAgICAgICAgICB8ICAgMzggKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKystDQo+ID4+IDYgZmlsZXMgY2hhbmdlZCwgMTQ3IGluc2VydGlvbnMoKyksIDUg
ZGVsZXRpb25zKC0pDQo+ID4+IA0KPiA+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRmaWxlbGF5
b3V0LmMgYi9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYw0KPiA+PiBpbmRleCA3OWJlN2FjLi41MWM3
MWU1IDEwMDY0NA0KPiA+PiAtLS0gYS9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYw0KPiA+PiArKysg
Yi9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYw0KPiA+PiBAQCAtMzMsNiArMzMsOCBAQA0KPiA+PiAj
aW5jbHVkZSA8bGludXgvbmZzX3BhZ2UuaD4NCj4gPj4gI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5o
Pg0KPiA+PiANCj4gPj4gKyNpbmNsdWRlIDxsaW51eC9zdW5ycGMvbWV0cmljcy5oPg0KPiA+PiAr
DQo+ID4+ICNpbmNsdWRlICJpbnRlcm5hbC5oIg0KPiA+PiAjaW5jbHVkZSAibmZzNGZpbGVsYXlv
dXQuaCINCj4gPj4gDQo+ID4+IEBAIC05MTgsNiArOTIwLDMzIEBAIGZpbGVsYXlvdXRfZnJlZV9k
ZXZlaWNlaWRfbm9kZShzdHJ1Y3QgbmZzNF9kZXZpY2VpZF9ub2RlICpkKQ0KPiA+PiAJbmZzNF9m
bF9mcmVlX2RldmljZWlkKGNvbnRhaW5lcl9vZihkLCBzdHJ1Y3QgbmZzNF9maWxlX2xheW91dF9k
c2FkZHIsIGlkX25vZGUpKTsNCj4gPj4gfQ0KPiA+PiANCj4gPj4gK3N0YXRpYyB1bnNpZ25lZCBp
bnQNCj4gPj4gK2ZpbGVsYXlvdXRfcnBjX2FkZF9pb3N0YXRzKHN0cnVjdCBycGNfaW9zdGF0cyAq
dG90YWwsDQo+ID4+ICsJCQkgICB1bnNpZ25lZCBpbnQgbWF4cHJvYywNCj4gPj4gKwkJCSAgIHN0
cnVjdCBuZnM0X2RldmljZWlkX25vZGUgKmQpDQo+ID4+ICt7DQo+ID4+ICsJc3RydWN0IG5mczRf
ZmlsZV9sYXlvdXRfZHNhZGRyICpkc2FkZHI7DQo+ID4+ICsJc3RydWN0IG5mczRfcG5mc19kcyAq
ZHM7DQo+ID4+ICsJdW5zaWduZWQgaW50IG51bV9kcyA9IDA7DQo+ID4+ICsJdTMyIGk7DQo+ID4+
ICsNCj4gPj4gKwlkc2FkZHIgPSBjb250YWluZXJfb2YoZCwgc3RydWN0IG5mczRfZmlsZV9sYXlv
dXRfZHNhZGRyLCBpZF9ub2RlKTsNCj4gPj4gKw0KPiA+PiArCWZvciAoaSA9IDA7IGkgPCBkc2Fk
ZHItPmRzX251bTsgaSsrKSB7DQo+ID4+ICsJCWRzID0gZHNhZGRyLT5kc19saXN0W2ldOw0KPiA+
PiArDQo+ID4+ICsJCS8qIG9ubHkgYWRkIGlvc3RhdHMgaWYgbmZzX2NsaWVudCBpcyBkaWZmZXJl
bnQgZnJvbSBNRFMgKi8NCj4gPj4gKwkJaWYgKGRzICYmIGRzLT5kc19jbHAgJiYgZC0+bmZzX2Ns
aWVudCAhPSBkcy0+ZHNfY2xwKSB7DQo+ID4+ICsJCQlzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQgPSBk
cy0+ZHNfY2xwLT5jbF9ycGNjbGllbnQ7DQo+ID4gDQo+ID4gSG1tbS4uLi4gU2hvdWxkbid0IHRo
aXMgcmF0aGVyIGJlIHRoZSBORlNfQ0xJRU5UKGlub2RlKT8gQWxzbywgd2h5IG5vdA0KPiA+IGRv
IHRoaXMgaW4gdGhlIHJlYWQvd3JpdGUgY29tcGxldGlvbiBjb2RlPyBXaHkgd2FsayB0aGUgZGV2
aWNlaWQgbGlzdCBhdA0KPiA+IGFsbD8NCj4gDQo+IEknbSBub3QgcmVhbGx5IGZvbGxvd2luZyB5
b3UuICBOb3RlIHRoYXQgdGhlc2UgYXJlIHJwY19pb3N0YXRzIG5vdCBuZnNfaW9zdGF0cy4gIA0K
PiBuZnNfaW9zdGF0cyBoYXZlIGJlZW4gcHJvcGVybHkgY291bnRpbmcgb3BlcmF0aW9ucyB0byBE
U3MgZm9yIHNvbWUgdGltZSBub3cuDQo+IHJwY19pb3N0YXRzIGNvdW50IG9wZXJhdGlvbnMsIHJ0
dCwgZXhlY3RpbWUsIGV0YyBwZXIgcnBjIHByb2NlZHVyZSAob3IgaW4gdGhlIGNhc2Ugb2YgTkZT
djQuWCB0aGVyZSBpcyBhIGhhY2sgdG8gdXNlIG9wZXJhdGlvbnMgd2l0aGluIGNvbXBvdW5kcyku
DQo+IA0KPiBXZSBoYXZlIHRvIHdhbGsgdGhlIGRldmljZSBsaXN0IHRvIGdldCBhY2Nlc3MgdG8g
dGhlIHVuZGVybHlpbmcgRFMncyBuZnNfY2xpZW50LT5ycGNfY2xpZW50IC0tIEkgY2FuJ3QganVz
dCBwdXQgdGhpcyBpbiB0aGUgcmVhZC93cml0ZSBjb21wbGV0aW9uIGJlY2F1c2UsIHVubGlrZSBu
ZnNfaW9zdGF0cywgcnBjX2lvc3RhdHMgY291bnQgYWxsIG9wZXJhdGlvbnMgYW5kIG5vdCBqdXN0
IHJlYWQvd3JpdGUgZXZlbnRzL2J5dGVzIChub3QgdG8gbWVudGlvbiB0aGF0IHRoZSBhY2N1bXVs
YXRpb24gaXMgZG9uZSBhdCB0aGUgcnBjIGxheWVyKS4NCg0KSW4gZG9pbmcgdGhpcywgeW91IGFy
ZSBhc3N1bWluZyB0aGF0IERTZXMgYXJlIG5vdCBzaGFyZWQgYW1vbmcgbXVsdGlwbGUNCmZpbGVz
eXN0ZW1zIG9yIE1EU2VzIChpLmUuIHRoYXQgZWFjaCBEUyBpcyB1c2VkIGJ5IGEgc2luZ2xlIHN0
cnVjdA0KbmZzX3NlcnZlcikuIEZvciBzb21ldGhpbmcgbGlrZSBhIE5ldEFwcCBmaWxlciBpbiBj
LW1vZGUsIHRoYXQgaXMNCmNsZWFybHkgbm90IGEgdmFsaWQgYXNzdW1wdGlvbi4NCg0KPiBJIGRv
IGhhdmUgYW4gaW1wbGVtZW50YXRpb24gdGhhdCBkb2Vzbid0IG5lZWQgdG8gd2FsayB0aGUgZGV2
aWNlaWQgbGlzdCBieSBhbGxvd2luZyBhIHNoYXJlZCBycGNfaW9zdGF0cyBzdHJ1Y3QgYmV0d2Vl
biBtdWx0aXBsZSBycGNfY2xpZW50cyAoaW4gYWRkaXRpb24gdG8gdGhlIGN1cnJlbnQgcnBjX2lv
c3RhdHMgc3RydWN0dXJlKSwgYnV0IHRoYXQgcmVxdWlyZWQgYWRkaW5nIGxvY2tpbmcgYW5kIHJl
ZmVyZW5jZSBjb3VudGluZyAtLSBhbGwgZm9yIHByaW50aW5nIHN0YXRzIChvYnZpb3VzbHkgbm90
IHdoYXQgd2Ugd2FudCkuDQoNClRoaXMgbWlnaHQgYmUgbW9yZSBpbiBsaW5lIHdpdGggd2hhdCB3
ZSB3YW50LiBOb3RlIHRoYXQgaXQgc2hvdWxkIGJlDQplYXN5IHRvIGNsb25lIGFuIHJwY19jbGll
bnQgYW5kIHRoZW4gcmVwbGFjZSBpdHMgcnBjX2lvc3RhdHMgc3RydWN0LiBJDQpkb24ndCB0aGlu
ayB0aGF0IG5lZWRzIGFueSBleHRyYSBsb2NraW5nLiBXZSdyZSBhbHJlYWR5IGlnbm9yaW5nIGxv
Y2tpbmcNCmhlcmUgYmV0d2VlbiBkaWZmZXJlbnQgcnBjX3Rhc2tzLCBzbyB0aHJvd2luZyBpbiBk
aWZmZXJlbnQgcnBjX2NsaWVudHMNCnRvIHRoZSBtaXggd2lsbCBtYWtlIG5vIGRpZmZlcmVuY2Uu
DQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0K
DQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 5590 bytes --]


On Feb 9, 2012, at 2:27 PM, Myklebust, Trond wrote:

> On Thu, 2012-02-09 at 19:13 +0000, Adamson, Dros wrote:
>> On Feb 9, 2012, at 1:16 PM, Myklebust, Trond wrote:
>> 
>>> On Thu, 2012-02-09 at 11:41 -0500, Weston Andros Adamson wrote:
>>>> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
>>>> filelayout mounts.
>>>> 
>>>> The additional info printed on the "per-op statistics" line does not break
>>>> current mountstats(8) from nfs-utils.
>>>> 
>>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>>> ---
>>>> 
>>>> This is clearly a better approach than I was taking trying to keep DS rpc stats
>>>> separate in /proc/self/mountstats.
>>>> 
>>>> FYI, there are no other callers of rpc_print_iostats().
>>>> 
>>>> The new output of /proc/self/mountstats works fine with current mountstats 
>>>> userland tool.  The additional data on the per-op statistics line doesn't make
>>>> it into the output, but I left it in /proc/self/mountstats because it doesn't
>>>> break anything and can be included in newer versions of mountstats.
>>>> 
>>>> I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
>>>> soon.
>>>> 
>>>> fs/nfs/nfs4filelayout.c        |   30 ++++++++++++++++++++++++++
>>>> fs/nfs/pnfs.h                  |   23 ++++++++++++++++++++
>>>> fs/nfs/pnfs_dev.c              |   45 ++++++++++++++++++++++++++++++++++++++++
>>>> fs/nfs/super.c                 |    3 +-
>>>> include/linux/sunrpc/metrics.h |   13 +++++++++-
>>>> net/sunrpc/stats.c             |   38 ++++++++++++++++++++++++++++++++-
>>>> 6 files changed, 147 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>>> index 79be7ac..51c71e5 100644
>>>> --- a/fs/nfs/nfs4filelayout.c
>>>> +++ b/fs/nfs/nfs4filelayout.c
>>>> @@ -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,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
>>>> 	nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
>>>> }
>>>> 
>>>> +static unsigned int
>>>> +filelayout_rpc_add_iostats(struct rpc_iostats *total,
>>>> +			   unsigned int maxproc,
>>>> +			   struct nfs4_deviceid_node *d)
>>>> +{
>>>> +	struct nfs4_file_layout_dsaddr *dsaddr;
>>>> +	struct nfs4_pnfs_ds *ds;
>>>> +	unsigned int num_ds = 0;
>>>> +	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];
>>>> +
>>>> +		/* only add iostats if nfs_client is different from MDS */
>>>> +		if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
>>>> +			struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
>>> 
>>> Hmmm.... Shouldn't this rather be the NFS_CLIENT(inode)? Also, why not
>>> do this in the read/write completion code? Why walk the deviceid list at
>>> all?
>> 
>> I'm not really following you.  Note that these are rpc_iostats not nfs_iostats.  
>> nfs_iostats have been properly counting operations to DSs for some time now.
>> rpc_iostats count operations, rtt, exectime, etc per rpc procedure (or in the case of NFSv4.X there is a hack to use operations within compounds).
>> 
>> We have to walk the device list to get access to the underlying DS's nfs_client->rpc_client -- I can't just put this in the read/write completion because, unlike nfs_iostats, rpc_iostats count all operations and not just read/write events/bytes (not to mention that the accumulation is done at the rpc layer).
> 
> In doing this, you are assuming that DSes are not shared among multiple
> filesystems or MDSes (i.e. that each DS is used by a single struct
> nfs_server). For something like a NetApp filer in c-mode, that is
> clearly not a valid assumption.

True - I had thought of this, but I thought it was fine since I was just carrying this assumption forward...

The current mountstats code already prints nfs_server->nfs_client->rpc_client->rpc_iostats -- which clearly doesn't differentiate between multiple nfs_servers that could be sharing the nfs_client.

> 
>> I do have an implementation that doesn't need to walk the deviceid list by allowing a shared rpc_iostats struct between multiple rpc_clients (in addition to the current rpc_iostats structure), but that required adding locking and reference counting -- all for printing stats (obviously not what we want).
> 
> This might be more in line with what we want. Note that it should be
> easy to clone an rpc_client and then replace its rpc_iostats struct. I
> don't think that needs any extra locking. We're already ignoring locking
> here between different rpc_tasks, so throwing in different rpc_clients
> to the mix will make no difference.

Yeah, that's easy enough and i guess we could ignore locking, but we are still left with the same problem: how is this supposed to account for different mount points using the same nfs_client?  nfs_client only has one rpc_client member.  I doubt we want to make a hash lookup on nfs_server to get the right rpc_client (which could all use the same underlying xprt).

Maybe it's time to move these stats into fs/nfs/ where they really belong?  We could get rid of the hack that overloads procnum with opnum from inside the compound for v4+ and finally only show a specific mount's RPC stats.

Thoughts?

-dros



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTA5IGF0IDE5OjQ4ICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiA+IA0KPiA+PiBJIGRvIGhhdmUgYW4gaW1wbGVtZW50YXRpb24gdGhhdCBkb2Vzbid0IG5lZWQg
dG8gd2FsayB0aGUgZGV2aWNlaWQgbGlzdCBieSBhbGxvd2luZyBhIHNoYXJlZCBycGNfaW9zdGF0
cyBzdHJ1Y3QgYmV0d2VlbiBtdWx0aXBsZSBycGNfY2xpZW50cyAoaW4gYWRkaXRpb24gdG8gdGhl
IGN1cnJlbnQgcnBjX2lvc3RhdHMgc3RydWN0dXJlKSwgYnV0IHRoYXQgcmVxdWlyZWQgYWRkaW5n
IGxvY2tpbmcgYW5kIHJlZmVyZW5jZSBjb3VudGluZyAtLSBhbGwgZm9yIHByaW50aW5nIHN0YXRz
IChvYnZpb3VzbHkgbm90IHdoYXQgd2Ugd2FudCkuDQo+ID4gDQo+ID4gVGhpcyBtaWdodCBiZSBt
b3JlIGluIGxpbmUgd2l0aCB3aGF0IHdlIHdhbnQuIE5vdGUgdGhhdCBpdCBzaG91bGQgYmUNCj4g
PiBlYXN5IHRvIGNsb25lIGFuIHJwY19jbGllbnQgYW5kIHRoZW4gcmVwbGFjZSBpdHMgcnBjX2lv
c3RhdHMgc3RydWN0LiBJDQo+ID4gZG9uJ3QgdGhpbmsgdGhhdCBuZWVkcyBhbnkgZXh0cmEgbG9j
a2luZy4gV2UncmUgYWxyZWFkeSBpZ25vcmluZyBsb2NraW5nDQo+ID4gaGVyZSBiZXR3ZWVuIGRp
ZmZlcmVudCBycGNfdGFza3MsIHNvIHRocm93aW5nIGluIGRpZmZlcmVudCBycGNfY2xpZW50cw0K
PiA+IHRvIHRoZSBtaXggd2lsbCBtYWtlIG5vIGRpZmZlcmVuY2UuDQo+IA0KPiBZZWFoLCB0aGF0
J3MgZWFzeSBlbm91Z2ggYW5kIGkgZ3Vlc3Mgd2UgY291bGQgaWdub3JlIGxvY2tpbmcsIGJ1dCB3
ZSBhcmUgc3RpbGwgbGVmdCB3aXRoIHRoZSBzYW1lIHByb2JsZW06IGhvdyBpcyB0aGlzIHN1cHBv
c2VkIHRvIGFjY291bnQgZm9yIGRpZmZlcmVudCBtb3VudCBwb2ludHMgdXNpbmcgdGhlIHNhbWUg
bmZzX2NsaWVudD8gIG5mc19jbGllbnQgb25seSBoYXMgb25lIHJwY19jbGllbnQgbWVtYmVyLiAg
SSBkb3VidCB3ZSB3YW50IHRvIG1ha2UgYSBoYXNoIGxvb2t1cCBvbiBuZnNfc2VydmVyIHRvIGdl
dCB0aGUgcmlnaHQgcnBjX2NsaWVudCAod2hpY2ggY291bGQgYWxsIHVzZSB0aGUgc2FtZSB1bmRl
cmx5aW5nIHhwcnQpLg0KPiANCj4gTWF5YmUgaXQncyB0aW1lIHRvIG1vdmUgdGhlc2Ugc3RhdHMg
aW50byBmcy9uZnMvIHdoZXJlIHRoZXkgcmVhbGx5IGJlbG9uZz8gIFdlIGNvdWxkIGdldCByaWQg
b2YgdGhlIGhhY2sgdGhhdCBvdmVybG9hZHMgcHJvY251bSB3aXRoIG9wbnVtIGZyb20gaW5zaWRl
IHRoZSBjb21wb3VuZCBmb3IgdjQrIGFuZCBmaW5hbGx5IG9ubHkgc2hvdyBhIHNwZWNpZmljIG1v
dW50J3MgUlBDIHN0YXRzLg0KDQpBY3R1YWxseSwgaG93IGFib3V0IGp1c3QgYWRkaW5nIGEgY2Fs
bGJhY2sgaW50byBzdHJ1Y3QgcnBjX2NhbGxfb3BzDQp0aGF0LCBpZiBpdCBpcyBkZWZpbmVkLCB3
b3VsZCBiZSBjYWxsZWQgaW5zdGVhZCBvZiBycGNfY291bnRfaW9zdGF0cygpLiANCklmIHlvdSB0
aGVuIG1vZGlmeSBycGNfY291bnRfaW9zdGF0cygpIHRvIHRha2UgdGhlICdzdGF0cycgdmFyaWFi
bGUgYXMgYQ0KcGFyYW1ldGVyLCB5b3UgY2FuIHNpbXBseSBoYXZlIHRoZSBuZXcgY2FsbGJhY2sg
Y2FsbCBycGNfY291bnRfaW9zdGF0cw0Kd2l0aCB0aGUgcmlnaHQgYXJndW1lbnRzLg0KDQotLSAN
ClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0K
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]


On Feb 9, 2012, at 2:58 PM, Myklebust, Trond wrote:

> On Thu, 2012-02-09 at 19:48 +0000, Adamson, Dros wrote:
>>> 
>>>> I do have an implementation that doesn't need to walk the deviceid list by allowing a shared rpc_iostats struct between multiple rpc_clients (in addition to the current rpc_iostats structure), but that required adding locking and reference counting -- all for printing stats (obviously not what we want).
>>> 
>>> This might be more in line with what we want. Note that it should be
>>> easy to clone an rpc_client and then replace its rpc_iostats struct. I
>>> don't think that needs any extra locking. We're already ignoring locking
>>> here between different rpc_tasks, so throwing in different rpc_clients
>>> to the mix will make no difference.
>> 
>> Yeah, that's easy enough and i guess we could ignore locking, but we are still left with the same problem: how is this supposed to account for different mount points using the same nfs_client?  nfs_client only has one rpc_client member.  I doubt we want to make a hash lookup on nfs_server to get the right rpc_client (which could all use the same underlying xprt).
>> 
>> Maybe it's time to move these stats into fs/nfs/ where they really belong?  We could get rid of the hack that overloads procnum with opnum from inside the compound for v4+ and finally only show a specific mount's RPC stats.
> 
> Actually, how about just adding a callback into struct rpc_call_ops
> that, if it is defined, would be called instead of rpc_count_iostats(). 
> If you then modify rpc_count_iostats() to take the 'stats' variable as a
> parameter, you can simply have the new callback call rpc_count_iostats
> with the right arguments.

Yeah, that could work!  On my walk home from the cafe (they always seem to help) I came up with a slightly different strategy:

- remove the cl_metrics (struct rpc_iostats) member from rpc_clnt
- add an *optional* rpc_iostats pointer to rpc_task (ignored if NULL)
- allocate one rpc_iostats structure (really array of structs, but you know what I mean) per nfs_server structure
- when setting up rpc_tasks in nfs-land, pass the correct rpc_iostats

with this strategy we don't accumulate stats when they aren't ever used (like an rpc_client used for mount protocol).  again, only NFS uses the rpc stats interface.

I kind of like this better than a callback, but either way is fine with me.  Which way would you prefer?

-dros

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTA5IGF0IDIwOjIzICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiBPbiBGZWIgOSwgMjAxMiwgYXQgMjo1OCBQTSwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g
DQo+ID4gT24gVGh1LCAyMDEyLTAyLTA5IGF0IDE5OjQ4ICswMDAwLCBBZGFtc29uLCBEcm9zIHdy
b3RlOg0KPiA+Pj4gDQo+ID4+Pj4gSSBkbyBoYXZlIGFuIGltcGxlbWVudGF0aW9uIHRoYXQgZG9l
c24ndCBuZWVkIHRvIHdhbGsgdGhlIGRldmljZWlkIGxpc3QgYnkgYWxsb3dpbmcgYSBzaGFyZWQg
cnBjX2lvc3RhdHMgc3RydWN0IGJldHdlZW4gbXVsdGlwbGUgcnBjX2NsaWVudHMgKGluIGFkZGl0
aW9uIHRvIHRoZSBjdXJyZW50IHJwY19pb3N0YXRzIHN0cnVjdHVyZSksIGJ1dCB0aGF0IHJlcXVp
cmVkIGFkZGluZyBsb2NraW5nIGFuZCByZWZlcmVuY2UgY291bnRpbmcgLS0gYWxsIGZvciBwcmlu
dGluZyBzdGF0cyAob2J2aW91c2x5IG5vdCB3aGF0IHdlIHdhbnQpLg0KPiA+Pj4gDQo+ID4+PiBU
aGlzIG1pZ2h0IGJlIG1vcmUgaW4gbGluZSB3aXRoIHdoYXQgd2Ugd2FudC4gTm90ZSB0aGF0IGl0
IHNob3VsZCBiZQ0KPiA+Pj4gZWFzeSB0byBjbG9uZSBhbiBycGNfY2xpZW50IGFuZCB0aGVuIHJl
cGxhY2UgaXRzIHJwY19pb3N0YXRzIHN0cnVjdC4gSQ0KPiA+Pj4gZG9uJ3QgdGhpbmsgdGhhdCBu
ZWVkcyBhbnkgZXh0cmEgbG9ja2luZy4gV2UncmUgYWxyZWFkeSBpZ25vcmluZyBsb2NraW5nDQo+
ID4+PiBoZXJlIGJldHdlZW4gZGlmZmVyZW50IHJwY190YXNrcywgc28gdGhyb3dpbmcgaW4gZGlm
ZmVyZW50IHJwY19jbGllbnRzDQo+ID4+PiB0byB0aGUgbWl4IHdpbGwgbWFrZSBubyBkaWZmZXJl
bmNlLg0KPiA+PiANCj4gPj4gWWVhaCwgdGhhdCdzIGVhc3kgZW5vdWdoIGFuZCBpIGd1ZXNzIHdl
IGNvdWxkIGlnbm9yZSBsb2NraW5nLCBidXQgd2UgYXJlIHN0aWxsIGxlZnQgd2l0aCB0aGUgc2Ft
ZSBwcm9ibGVtOiBob3cgaXMgdGhpcyBzdXBwb3NlZCB0byBhY2NvdW50IGZvciBkaWZmZXJlbnQg
bW91bnQgcG9pbnRzIHVzaW5nIHRoZSBzYW1lIG5mc19jbGllbnQ/ICBuZnNfY2xpZW50IG9ubHkg
aGFzIG9uZSBycGNfY2xpZW50IG1lbWJlci4gIEkgZG91YnQgd2Ugd2FudCB0byBtYWtlIGEgaGFz
aCBsb29rdXAgb24gbmZzX3NlcnZlciB0byBnZXQgdGhlIHJpZ2h0IHJwY19jbGllbnQgKHdoaWNo
IGNvdWxkIGFsbCB1c2UgdGhlIHNhbWUgdW5kZXJseWluZyB4cHJ0KS4NCj4gPj4gDQo+ID4+IE1h
eWJlIGl0J3MgdGltZSB0byBtb3ZlIHRoZXNlIHN0YXRzIGludG8gZnMvbmZzLyB3aGVyZSB0aGV5
IHJlYWxseSBiZWxvbmc/ICBXZSBjb3VsZCBnZXQgcmlkIG9mIHRoZSBoYWNrIHRoYXQgb3Zlcmxv
YWRzIHByb2NudW0gd2l0aCBvcG51bSBmcm9tIGluc2lkZSB0aGUgY29tcG91bmQgZm9yIHY0KyBh
bmQgZmluYWxseSBvbmx5IHNob3cgYSBzcGVjaWZpYyBtb3VudCdzIFJQQyBzdGF0cy4NCj4gPiAN
Cj4gPiBBY3R1YWxseSwgaG93IGFib3V0IGp1c3QgYWRkaW5nIGEgY2FsbGJhY2sgaW50byBzdHJ1
Y3QgcnBjX2NhbGxfb3BzDQo+ID4gdGhhdCwgaWYgaXQgaXMgZGVmaW5lZCwgd291bGQgYmUgY2Fs
bGVkIGluc3RlYWQgb2YgcnBjX2NvdW50X2lvc3RhdHMoKS4gDQo+ID4gSWYgeW91IHRoZW4gbW9k
aWZ5IHJwY19jb3VudF9pb3N0YXRzKCkgdG8gdGFrZSB0aGUgJ3N0YXRzJyB2YXJpYWJsZSBhcyBh
DQo+ID4gcGFyYW1ldGVyLCB5b3UgY2FuIHNpbXBseSBoYXZlIHRoZSBuZXcgY2FsbGJhY2sgY2Fs
bCBycGNfY291bnRfaW9zdGF0cw0KPiA+IHdpdGggdGhlIHJpZ2h0IGFyZ3VtZW50cy4NCj4gDQo+
IFllYWgsIHRoYXQgY291bGQgd29yayEgIE9uIG15IHdhbGsgaG9tZSBmcm9tIHRoZSBjYWZlICh0
aGV5IGFsd2F5cyBzZWVtIHRvIGhlbHApIEkgY2FtZSB1cCB3aXRoIGEgc2xpZ2h0bHkgZGlmZmVy
ZW50IHN0cmF0ZWd5Og0KPiANCj4gLSByZW1vdmUgdGhlIGNsX21ldHJpY3MgKHN0cnVjdCBycGNf
aW9zdGF0cykgbWVtYmVyIGZyb20gcnBjX2NsbnQNCj4gLSBhZGQgYW4gKm9wdGlvbmFsKiBycGNf
aW9zdGF0cyBwb2ludGVyIHRvIHJwY190YXNrIChpZ25vcmVkIGlmIE5VTEwpDQo+IC0gYWxsb2Nh
dGUgb25lIHJwY19pb3N0YXRzIHN0cnVjdHVyZSAocmVhbGx5IGFycmF5IG9mIHN0cnVjdHMsIGJ1
dCB5b3Uga25vdyB3aGF0IEkgbWVhbikgcGVyIG5mc19zZXJ2ZXIgc3RydWN0dXJlDQo+IC0gd2hl
biBzZXR0aW5nIHVwIHJwY190YXNrcyBpbiBuZnMtbGFuZCwgcGFzcyB0aGUgY29ycmVjdCBycGNf
aW9zdGF0cw0KPiANCj4gd2l0aCB0aGlzIHN0cmF0ZWd5IHdlIGRvbid0IGFjY3VtdWxhdGUgc3Rh
dHMgd2hlbiB0aGV5IGFyZW4ndCBldmVyIHVzZWQgKGxpa2UgYW4gcnBjX2NsaWVudCB1c2VkIGZv
ciBtb3VudCBwcm90b2NvbCkuICBhZ2Fpbiwgb25seSBORlMgdXNlcyB0aGUgcnBjIHN0YXRzIGlu
dGVyZmFjZS4NCj4gDQo+IEkga2luZCBvZiBsaWtlIHRoaXMgYmV0dGVyIHRoYW4gYSBjYWxsYmFj
aywgYnV0IGVpdGhlciB3YXkgaXMgZmluZSB3aXRoIG1lLiAgV2hpY2ggd2F5IHdvdWxkIHlvdSBw
cmVmZXI/DQoNCkknZCBwcmVmZXIgbm90IHRvIGhhdmUgdG8gZ3JvdyB0aGUgc2l6ZSBvZiB0aGUg
cnBjX3Rhc2sgdW5sZXNzIHdlIHJlYWxseQ0KbmVlZCB0by4gVGhhdCdzIHdoeSBJIHN1Z2dlc3Rl
ZCB0aGUgY2FsbGJhY2sgaW5zdGVhZC4NCg0KQ2hlZXJzDQogIFRyb25kDQoNCi0tIA0KVHJvbmQg
TXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5N
eWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Feb 9, 2012, at 3:37 PM, Myklebust, Trond wrote:

> On Thu, 2012-02-09 at 20:23 +0000, Adamson, Dros wrote:
>> On Feb 9, 2012, at 2:58 PM, Myklebust, Trond wrote:
>> 
>>> On Thu, 2012-02-09 at 19:48 +0000, Adamson, Dros wrote:
>>>>> 
>>>>>> I do have an implementation that doesn't need to walk the deviceid list by allowing a shared rpc_iostats struct between multiple rpc_clients (in addition to the current rpc_iostats structure), but that required adding locking and reference counting -- all for printing stats (obviously not what we want).
>>>>> 
>>>>> This might be more in line with what we want. Note that it should be
>>>>> easy to clone an rpc_client and then replace its rpc_iostats struct. I
>>>>> don't think that needs any extra locking. We're already ignoring locking
>>>>> here between different rpc_tasks, so throwing in different rpc_clients
>>>>> to the mix will make no difference.
>>>> 
>>>> Yeah, that's easy enough and i guess we could ignore locking, but we are still left with the same problem: how is this supposed to account for different mount points using the same nfs_client?  nfs_client only has one rpc_client member.  I doubt we want to make a hash lookup on nfs_server to get the right rpc_client (which could all use the same underlying xprt).
>>>> 
>>>> Maybe it's time to move these stats into fs/nfs/ where they really belong?  We could get rid of the hack that overloads procnum with opnum from inside the compound for v4+ and finally only show a specific mount's RPC stats.
>>> 
>>> Actually, how about just adding a callback into struct rpc_call_ops
>>> that, if it is defined, would be called instead of rpc_count_iostats(). 
>>> If you then modify rpc_count_iostats() to take the 'stats' variable as a
>>> parameter, you can simply have the new callback call rpc_count_iostats
>>> with the right arguments.
>> 
>> Yeah, that could work!  On my walk home from the cafe (they always seem to help) I came up with a slightly different strategy:
>> 
>> - remove the cl_metrics (struct rpc_iostats) member from rpc_clnt
>> - add an *optional* rpc_iostats pointer to rpc_task (ignored if NULL)
>> - allocate one rpc_iostats structure (really array of structs, but you know what I mean) per nfs_server structure
>> - when setting up rpc_tasks in nfs-land, pass the correct rpc_iostats
>> 
>> with this strategy we don't accumulate stats when they aren't ever used (like an rpc_client used for mount protocol).  again, only NFS uses the rpc stats interface.
>> 
>> I kind of like this better than a callback, but either way is fine with me.  Which way would you prefer?
> 
> I'd prefer not to have to grow the size of the rpc_task unless we really
> need to. That's why I suggested the callback instead.

Good to know!  I'll do the callback method.

Also, I'm going to go ahead with removing the cl_metrics member from rpc_clnt -- once we have the callback it'll never be used.

Thanks for pointing me in the right direction.  Expect a patch soon!

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH] NFS: include filelayout DS rpc stats in mountstats

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAyLTA5IGF0IDE5OjQ4ICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiA+IA0KPiA+PiBJIGRvIGhhdmUgYW4gaW1wbGVtZW50YXRpb24gdGhhdCBkb2Vzbid0IG5lZWQg
dG8gd2FsayB0aGUgZGV2aWNlaWQgbGlzdCBieSBhbGxvd2luZyBhIHNoYXJlZCBycGNfaW9zdGF0
cyBzdHJ1Y3QgYmV0d2VlbiBtdWx0aXBsZSBycGNfY2xpZW50cyAoaW4gYWRkaXRpb24gdG8gdGhl
IGN1cnJlbnQgcnBjX2lvc3RhdHMgc3RydWN0dXJlKSwgYnV0IHRoYXQgcmVxdWlyZWQgYWRkaW5n
IGxvY2tpbmcgYW5kIHJlZmVyZW5jZSBjb3VudGluZyAtLSBhbGwgZm9yIHByaW50aW5nIHN0YXRz
IChvYnZpb3VzbHkgbm90IHdoYXQgd2Ugd2FudCkuDQo+ID4gDQo+ID4gVGhpcyBtaWdodCBiZSBt
b3JlIGluIGxpbmUgd2l0aCB3aGF0IHdlIHdhbnQuIE5vdGUgdGhhdCBpdCBzaG91bGQgYmUNCj4g
PiBlYXN5IHRvIGNsb25lIGFuIHJwY19jbGllbnQgYW5kIHRoZW4gcmVwbGFjZSBpdHMgcnBjX2lv
c3RhdHMgc3RydWN0LiBJDQo+ID4gZG9uJ3QgdGhpbmsgdGhhdCBuZWVkcyBhbnkgZXh0cmEgbG9j
a2luZy4gV2UncmUgYWxyZWFkeSBpZ25vcmluZyBsb2NraW5nDQo+ID4gaGVyZSBiZXR3ZWVuIGRp
ZmZlcmVudCBycGNfdGFza3MsIHNvIHRocm93aW5nIGluIGRpZmZlcmVudCBycGNfY2xpZW50cw0K
PiA+IHRvIHRoZSBtaXggd2lsbCBtYWtlIG5vIGRpZmZlcmVuY2UuDQo+IA0KPiBZZWFoLCB0aGF0
J3MgZWFzeSBlbm91Z2ggYW5kIGkgZ3Vlc3Mgd2UgY291bGQgaWdub3JlIGxvY2tpbmcsIGJ1dCB3
ZSBhcmUgc3RpbGwgbGVmdCB3aXRoIHRoZSBzYW1lIHByb2JsZW06IGhvdyBpcyB0aGlzIHN1cHBv
c2VkIHRvIGFjY291bnQgZm9yIGRpZmZlcmVudCBtb3VudCBwb2ludHMgdXNpbmcgdGhlIHNhbWUg
bmZzX2NsaWVudD8gIG5mc19jbGllbnQgb25seSBoYXMgb25lIHJwY19jbGllbnQgbWVtYmVyLiAg
SSBkb3VidCB3ZSB3YW50IHRvIG1ha2UgYSBoYXNoIGxvb2t1cCBvbiBuZnNfc2VydmVyIHRvIGdl
dCB0aGUgcmlnaHQgcnBjX2NsaWVudCAod2hpY2ggY291bGQgYWxsIHVzZSB0aGUgc2FtZSB1bmRl
cmx5aW5nIHhwcnQpLg0KPiANCj4gTWF5YmUgaXQncyB0aW1lIHRvIG1vdmUgdGhlc2Ugc3RhdHMg
aW50byBmcy9uZnMvIHdoZXJlIHRoZXkgcmVhbGx5IGJlbG9uZz8gIFdlIGNvdWxkIGdldCByaWQg
b2YgdGhlIGhhY2sgdGhhdCBvdmVybG9hZHMgcHJvY251bSB3aXRoIG9wbnVtIGZyb20gaW5zaWRl
IHRoZSBjb21wb3VuZCBmb3IgdjQrIGFuZCBmaW5hbGx5IG9ubHkgc2hvdyBhIHNwZWNpZmljIG1v
dW50J3MgUlBDIHN0YXRzLg0KDQpBY3R1YWxseSwgaG93IGFib3V0IGp1c3QgYWRkaW5nIGEgY2Fs
bGJhY2sgaW50byBzdHJ1Y3QgcnBjX2NhbGxfb3BzDQp0aGF0LCBpZiBpdCBpcyBkZWZpbmVkLCB3
b3VsZCBiZSBjYWxsZWQgaW5zdGVhZCBvZiBycGNfY291bnRfaW9zdGF0cygpLiANCklmIHlvdSB0
aGVuIG1vZGlmeSBycGNfY291bnRfaW9zdGF0cygpIHRvIHRha2UgdGhlICdzdGF0cycgdmFyaWFi
bGUgYXMgYQ0KcGFyYW1ldGVyLCB5b3UgY2FuIHNpbXBseSBoYXZlIHRoZSBuZXcgY2FsbGJhY2sg
Y2FsbCBycGNfY291bnRfaW9zdGF0cw0Kd2l0aCB0aGUgcmlnaHQgYXJndW1lbnRzLg0KDQotLSAN
ClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0K
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg0K