[pnfs] [PATCH 1/2] nfs41: Add a reference to svc_serv during callback service bring up

[Labiaga, Ricardo]

> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy at panasas.com] 
> Sent: Tuesday, March 04, 2008 11:53 PM
> To: Labiaga, Ricardo
> Cc: pnfs at linux-nfs.org
> Subject: Re: [pnfs] [PATCH 1/2] nfs41: Add a reference to 
> svc_serv during callback service bring up
> 
> Ricardo Labiaga wrote:
> > From: Ricardo Labiaga <Ricardo.Labiaga at netapp.com>
> > 
> > There is only one NFSv4.1 callback service.  The first caller of
> > nfs4_callback_up() creates the service, subsequent callers 
> increment a
> > reference count on the service.  The service is destroyed 
> when the last
> > caller invokes nfs_callback_down().
> > 
> > The transport needs to hold a reference to the callback 
> service in order
> > to invoke it during callback processing.  Currently this 
> reference is only
> > obtained when the service is first created.  This is 
> incorrect, since
> > subsequent registrations for other transports will leave 
> the xprt->serv
> > pointer uninitialized, leading to an oops when a callback arrives on
> > the "unreferenced" transport.
> > 
> > This patch fixes the problem by ensuring that a reference 
> to the service
> > is saved in xprt->serv, either because the service is 
> created by this
> > invocation to nfs4_callback_up() or by a prior invocation.
> > 
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga at netapp.com>
> > ---
> >  fs/nfs/callback.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index e6a55a6..4afd42c 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -186,12 +186,6 @@ int nfs4_callback_up(struct svc_serv *serv)
> >   */
> >  int nfs41_callback_up(struct svc_serv *serv, struct rpc_xprt *xprt)
> >  {
> > -	/*
> > -	 * Save the svc_serv in the transport so that it can
> > -	 * be referenced when the session backchannel is initialized
> > -	 */
> > -	xprt->serv = serv;
> > -
> 
> The xprt parameter is no longer used and can be removed.

True, will remove it.

> >  	INIT_LIST_HEAD(&serv->sv_cb_list);
> >  	spin_lock_init(&serv->sv_cb_lock);
> >  	init_waitqueue_head(&serv->sv_cb_waitq);
> > @@ -236,6 +230,22 @@ int nfs_callback_up(int minorversion, 
> void *args)
> >  	nfs_callback_info.serv = serv;
> >  	wait_for_completion(&nfs_callback_info.started);
> >  out:
> > +	if (ret == 0) {
> > +		switch (minorversion) {
> 
> The surrounding logic seem to be a bit overly complex for a 
> single statement
> that needs to be done for minorversion 1.  How about a simple 
> if () statement
> rather than the switch?

I'm just being consistent with the rest of the minorversion switching in
other places.  I guess we're preparing for v4.2 :-)  We use the switch
pretty much everywhere.  One advantage of using the switch statement is
that it "flags" the places that are minorversion aware - for future
maintenance.  Having said that, I don't really feel strong about it
either way.

> 
> > +		case 0:
> > +			break;
> > +#if defined(CONFIG_NFS_V4_1)
> > +		case 1:
> > +			/*
> > +			 * Save the svc_serv in the transport 
> so that it can
> > +			 * be referenced when the session backchannel is
> > +			 * initialized and when callbacks arrive.
> > +			 */
> > +			xprt->serv = nfs_callback_info.serv;
> > +			break;
> > +#endif /* CONFIG_NFS_V4_1 */
> > +		}
> > +	}
> 
> Also, you can define a new label here, say "out_common:", to 
> be gone to
> from the out_err path rather than goto out to save "if (ret) { }"
> 
> >  	mutex_unlock(&nfs_callback_mutex);
> >  	unlock_kernel();
> >  	return ret;
> 
> BTW, looking at this function I don't see how it's supposed 
> to work when mounting
> over both minor versions.  We call either 
> nfs4{,1}_callback_up first time around
> and the other is never called after the callback service is created.
> This needs to be re-written so we can bring either both 
> threads up and down
> together or independently on-demand maybe by having two 
> copies of nfs_callback_info,
> one for each minor version.  In which case I think they can 
> share the service
> which is created once in nfs_callback_up and never destroyed.

My bad, thanks for catching this.  I'll resubmit a fixed version.

> 
> One more todo item, while I'm at it is to replace the void 
> *args argument
> to nfs_callback_up() with struct rpc_xprt *xprt since it's 
> always passed
> by nfs4_init_client()

Okay.

- ricardo

> 
> Benny
>