Re: [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk()

[Trond Myklebust <trondmy@hammerspace.com>]

Hi Santosh,

On Tue, 2018-12-11 at 15:28 +0530, Santosh kumar pradhan wrote:
> From: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
> 
> Multipathing: In case of NFSv3, rpc_clnt_test_and_add_xprt() adds
> the xprt to xprt switch (i.e. xps) if rpc_call_null_helper() returns
> success. But in case of NFSv4.1, it needs to do EXCHANGEID to verify
> the path along with check for session trunking.
> 
> Add the xprt once nfs4_test_session_trunk() returns success.
> Also release refcount hold by rpc_clnt_setup_test_and_add_xprt().
> 
> Signed-off-by: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
> Tested-by: Suresh Jayaraman <suresh.jayaraman@wdc.com>
> ---
>  net/sunrpc/clnt.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d839c33ae7d9..053f594cc144 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2654,10 +2654,14 @@ int rpc_clnt_setup_test_and_add_xprt(struct
> rpc_clnt *clnt,
>  		goto out_err;
>  
>  	/* rpc_xprt_switch and rpc_xprt are deferrenced by
> add_xprt_test() */
> -	xtest->add_xprt_test(clnt, xprt, xtest->data);
> +	status = xtest->add_xprt_test(clnt, xprt, xtest->data);
> +	if (status != 0)
> +		goto out_err;
>  
> -	/* so that rpc_clnt_add_xprt does not call
> rpc_xprt_switch_add_xprt */
> -	return 1;
> +	xprt_put(xprt);
> +	xprt_switch_put(xps);
> +	/* so that rpc_clnt_add_xprt calls rpc_xprt_switch_add_xprt */
> +	return 0;
>  out_err:
>  	xprt_put(xprt);
>  	xprt_switch_put(xps);

I'd really prefer to have callbacks that don't return values here so
that they can be made asynchronous at some point in the future. For
that reason, I'd prefer a fix in the NFS code, not the RPC code.

Can you therefore please change struct rpc_add_xprt_test to something
like the following:

struct rpc_add_xprt_test {
        void (*add_xprt_test)(struct rpc_clnt *,
                struct rpc_xprt *,
                void *calldata);
        void *data;
};

and then simply have nfs4_test_session_trunk() call
rpc_clnt_xprt_switch_add_xprt() if the call to
nfs4_detect_session_trunking() is successful?

Thanks!
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com