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:
smime.p7s
Description: S/MIME cryptographic signature
