On Thu, Jan 25, 2024 at 04:49:17PM -0500, Benjamin Coddington wrote:
> On 25 Jan 2024, at 11:29, Chuck Lever wrote:
>
> > From: Chuck Lever <chuck.lever@xxxxxxxxxx>
> >
> > Help observe the flow of callback operations.
> >
> > bc_shutdown() records exactly when the backchannel RPC client is
> > destroyed and cl_cb_client is replaced with NULL.
> >
> > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > ---
> > fs/nfsd/nfs4callback.c | 7 +++++++
> > fs/nfsd/trace.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> > include/trace/misc/nfs.h | 34 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 83 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 1c85426830b1..4d5a6370b92c 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -887,6 +887,7 @@ static struct workqueue_struct *callback_wq;
> >
> > static bool nfsd4_queue_cb(struct nfsd4_callback *cb)
> > {
> > + trace_nfsd_cb_queue(cb->cb_clp, cb);
> > return queue_work(callback_wq, &cb->cb_work);
> > }
> >
> > @@ -1106,6 +1107,7 @@ static void nfsd41_destroy_cb(struct nfsd4_callback *cb)
> > {
> > struct nfs4_client *clp = cb->cb_clp;
> >
> > + trace_nfsd_cb_destroy(clp, cb);
> > nfsd41_cb_release_slot(cb);
> > if (cb->cb_ops && cb->cb_ops->release)
> > cb->cb_ops->release(cb);
> > @@ -1220,6 +1222,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > goto out;
> > need_restart:
> > if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
> > + trace_nfsd_cb_restart(clp, cb);
> > task->tk_status = 0;
> > cb->cb_need_restart = true;
>
> I think you want to call the tracepoint /after/ setting cb_need_restart here..
Maybe?
The trace point currently captures the value of cb_need_restart
before this logic overwrites it. Is that beneficial for
troubleshooting?
> > }
> > @@ -1333,11 +1336,14 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
> > struct nfsd4_conn *c;
> > int err;
> >
> > + trace_nfsd_cb_bc_update(clp, cb);
> > +
> > /*
> > * This is either an update, or the client dying; in either case,
> > * kill the old client:
> > */
> > if (clp->cl_cb_client) {
> > + trace_nfsd_cb_bc_shutdown(clp, cb);
> > rpc_shutdown_client(clp->cl_cb_client);
> > clp->cl_cb_client = NULL;
> > put_cred(clp->cl_cb_cred);
> > @@ -1349,6 +1355,7 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
> > }
> > if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
> > return;
> > +
>
> I'm in favor of this whitespace change, but did you mean to include it?
Yes, but it might be better off in one of the other patches.
--
Chuck Lever
