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

[Benny Halevy]

On Mar. 06, 2008, 1:21 +0200, "Labiaga, Ricardo" <Ricardo.Labiaga at netapp.com> wrote:
>> -----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.

Thanks a bunch!

Benny

> 
>> 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
>>