> On Apr 23, 2020, at 3:50 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>> ---
>> 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.
Agreed. RPC_TASK_SOFT is still set, but it's been moved to
rpc_call_null_helper() because every one of its callers is a soft
caller.
>> 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@xxxxxxxxxxxxxxx
--
Chuck Lever
