Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH RFC2 1/2] SUNRPC: Set SOFTCONN when destroying GSS
 contexts
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <1420699d1256f1808e5790e172bb3abd99ec7259.camel@hammerspace.com>
Date:   Thu, 23 Apr 2020 15:52:29 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F3E313AF-E44E-4337-8F4E-62BF5C4DD4EE@oracle.com>
References: <20200423194311.7849.36326.stgit@manet.1015granger.net>
 <1420699d1256f1808e5790e172bb3abd99ec7259.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Apr 23, 2020, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> 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.
>>=20
>> I noticed that callers of rpc_call_null_helper() use a common set of
>> flags, so I collected them all in that helper function.
>>=20
>> 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(-)
>>=20
>> 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 =3D RPC_GSS_PROC_DESTROY;
>>=20
>> 		task =3D rpc_call_null(gss_auth->client, &new->gc_base,
>> -				RPC_TASK_ASYNC|RPC_TASK_SOFT);
>> +				     RPC_TASK_ASYNC);
>=20
> 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);
>>=20
>> 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 =3D cred,
>> 		.callback_ops =3D (ops !=3D NULL) ? ops : =
&rpc_default_ops,
>> 		.callback_data =3D data,
>> -		.flags =3D flags | RPC_TASK_NULLCREDS,
>> +		.flags =3D flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
>> +			 RPC_TASK_NULLCREDS,
>> 	};
>>=20
>> 	return rpc_run_task(&task_setup_data);
>> @@ -2805,8 +2806,7 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt
>> *clnt,
>> 		goto success;
>> 	}
>>=20
>> -	task =3D rpc_call_null_helper(clnt, xprt, NULL,
>> -			RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC|
>> RPC_TASK_NULLCREDS,
>> +	task =3D 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;
>>=20
>> 	/* Test the connection */
>> -	task =3D rpc_call_null_helper(clnt, xprt, NULL,
>> -				    RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
>> RPC_TASK_NULLCREDS,
>> -				    NULL, NULL);
>> +	task =3D rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL);
>> 	if (IS_ERR(task)) {
>> 		status =3D PTR_ERR(task);
>> 		goto out_err;
>>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever