Re: [PATCH RFC2 1/2] SUNRPC: Set SOFTCONN when destroying GSS contexts

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2020-04-23 at 15:43 -0400, Chuck Lever wrote:
> Prevent a (temporary) hang when shutting down an rpc_clnt that has
> used one or more GSS creds.
> 
> I noticed that callers of rpc_call_null_helper() use a common set of
> flags, so I collected them all in that helper function.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/auth_gss/auth_gss.c |    2 +-
>  net/sunrpc/clnt.c              |   10 ++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index ac5cac0dd24b..16cec9755b86 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1285,7 +1285,7 @@ static void gss_pipe_free(struct gss_pipe *p)
>  		ctx->gc_proc = RPC_GSS_PROC_DESTROY;
>  
>  		task = rpc_call_null(gss_auth->client, &new->gc_base,
> -				RPC_TASK_ASYNC|RPC_TASK_SOFT);
> +				     RPC_TASK_ASYNC);

No. This means we can end with silently hanging clients that never go
away. Besides, this function is destroying the creds, so there should
be no problem with them timing out.

>  		if (!IS_ERR(task))
>  			rpc_put_task(task);
>  
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 325a0858700f..ddc98b97c170 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2742,7 +2742,8 @@ struct rpc_task *rpc_call_null_helper(struct
> rpc_clnt *clnt,
>  		.rpc_op_cred = cred,
>  		.callback_ops = (ops != NULL) ? ops : &rpc_default_ops,
>  		.callback_data = data,
> -		.flags = flags | RPC_TASK_NULLCREDS,
> +		.flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
> +			 RPC_TASK_NULLCREDS,
>  	};
>  
>  	return rpc_run_task(&task_setup_data);
> @@ -2805,8 +2806,7 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt
> *clnt,
>  		goto success;
>  	}
>  
> -	task = rpc_call_null_helper(clnt, xprt, NULL,
> -			RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC|
> RPC_TASK_NULLCREDS,
> +	task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC,
>  			&rpc_cb_add_xprt_call_ops, data);
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
> @@ -2850,9 +2850,7 @@ int rpc_clnt_setup_test_and_add_xprt(struct
> rpc_clnt *clnt,
>  		goto out_err;
>  
>  	/* Test the connection */
> -	task = rpc_call_null_helper(clnt, xprt, NULL,
> -				    RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
> RPC_TASK_NULLCREDS,
> -				    NULL, NULL);
> +	task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL);
>  	if (IS_ERR(task)) {
>  		status = PTR_ERR(task);
>  		goto out_err;
> 
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com