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