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

[Benny Halevy]

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.

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

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

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

Benny