[PATCH] lockd: don't use timed rebind with TCP

[PATCH] lockd: don't use timed rebind with TCP

[Calum Mackay]

It is possible for nlm_bind_host() to clear XPRT_BOUND whilst a connection
worker is in the middle of trying to reconnect. When the latter notices
that XPRT_BOUND been cleared under it, in xs_tcp_finish_connecting(),
that results in:

	xs_tcp_setup_socket: connect returned unhandled error -107

Worse, it's possible that the two can get into lockstep, resulting in
the same behaviour repeated indefinitely, with the above error every
300 seconds, without ever recovering, and the connection never being
established. This is most likely to occur when there's a large number
of NLM client tasks following a server reboot.

Since the timed rebind would seem not to be needed for TCP in any case,
whilst the existing connection remains, restrict the timed rebinding to
UDP only.

For TCP, we will still rebind when needed, e.g. on timeout, connection
error (including closure), and in the reclaimer.

Whilst there, refactor some duplicate code.

Signed-off-by: Calum Mackay <calum.mackay@oracle.com>
---
 fs/lockd/host.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 0afb6d59bad0..6e98c2ed6ffc 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -439,12 +439,7 @@ nlm_bind_host(struct nlm_host *host)
 	 * RPC rebind is required
 	 */
 	if ((clnt = host->h_rpcclnt) != NULL) {
-		if (time_after_eq(jiffies, host->h_nextrebind)) {
-			rpc_force_rebind(clnt);
-			host->h_nextrebind = jiffies + NLM_HOST_REBIND;
-			dprintk("lockd: next rebind in %lu jiffies\n",
-					host->h_nextrebind - jiffies);
-		}
+		nlm_rebind_host(host);
 	} else {
 		unsigned long increment = nlmsvc_timeout;
 		struct rpc_timeout timeparms = {
@@ -495,15 +490,18 @@ nlm_bind_host(struct nlm_host *host)
 }
 
 /*
- * Force a portmap lookup of the remote lockd port
+ * Force a portmap lookup of the remote lockd port, unless we're using a
+ * TCP connection.
  */
 void
 nlm_rebind_host(struct nlm_host *host)
 {
-	dprintk("lockd: rebind host %s\n", host->h_name);
-	if (host->h_rpcclnt && time_after_eq(jiffies, host->h_nextrebind)) {
+	if (unlikely(host->h_proto == IPPROTO_UDP) && host->h_rpcclnt &&
+			time_after_eq(jiffies, host->h_nextrebind)) {
 		rpc_force_rebind(host->h_rpcclnt);
 		host->h_nextrebind = jiffies + NLM_HOST_REBIND;
+		dprintk("lockd: rebind host %s; next rebind in %lu jiffies\n",
+			host->h_name, host->h_nextrebind - jiffies);
 	}
 }
 
-- 
2.18.4

Re: [PATCH] lockd: don't use timed rebind with TCP

[Calum Mackay]

[-- Attachment #1.1.1: Type: text/plain, Size: 2880 bytes --]

Please hold off for now on this one; I think I need to adjust the 
reclaimer a little.

thanks,
calum.

On 02/10/2020 11:57 pm, Calum Mackay wrote:
> It is possible for nlm_bind_host() to clear XPRT_BOUND whilst a connection
> worker is in the middle of trying to reconnect. When the latter notices
> that XPRT_BOUND been cleared under it, in xs_tcp_finish_connecting(),
> that results in:
> 
> 	xs_tcp_setup_socket: connect returned unhandled error -107
> 
> Worse, it's possible that the two can get into lockstep, resulting in
> the same behaviour repeated indefinitely, with the above error every
> 300 seconds, without ever recovering, and the connection never being
> established. This is most likely to occur when there's a large number
> of NLM client tasks following a server reboot.
> 
> Since the timed rebind would seem not to be needed for TCP in any case,
> whilst the existing connection remains, restrict the timed rebinding to
> UDP only.
> 
> For TCP, we will still rebind when needed, e.g. on timeout, connection
> error (including closure), and in the reclaimer.
> 
> Whilst there, refactor some duplicate code.
> 
> Signed-off-by: Calum Mackay <calum.mackay@oracle.com>
> ---
>   fs/lockd/host.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 0afb6d59bad0..6e98c2ed6ffc 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -439,12 +439,7 @@ nlm_bind_host(struct nlm_host *host)
>   	 * RPC rebind is required
>   	 */
>   	if ((clnt = host->h_rpcclnt) != NULL) {
> -		if (time_after_eq(jiffies, host->h_nextrebind)) {
> -			rpc_force_rebind(clnt);
> -			host->h_nextrebind = jiffies + NLM_HOST_REBIND;
> -			dprintk("lockd: next rebind in %lu jiffies\n",
> -					host->h_nextrebind - jiffies);
> -		}
> +		nlm_rebind_host(host);
>   	} else {
>   		unsigned long increment = nlmsvc_timeout;
>   		struct rpc_timeout timeparms = {
> @@ -495,15 +490,18 @@ nlm_bind_host(struct nlm_host *host)
>   }
>   
>   /*
> - * Force a portmap lookup of the remote lockd port
> + * Force a portmap lookup of the remote lockd port, unless we're using a
> + * TCP connection.
>    */
>   void
>   nlm_rebind_host(struct nlm_host *host)
>   {
> -	dprintk("lockd: rebind host %s\n", host->h_name);
> -	if (host->h_rpcclnt && time_after_eq(jiffies, host->h_nextrebind)) {
> +	if (unlikely(host->h_proto == IPPROTO_UDP) && host->h_rpcclnt &&
> +			time_after_eq(jiffies, host->h_nextrebind)) {
>   		rpc_force_rebind(host->h_rpcclnt);
>   		host->h_nextrebind = jiffies + NLM_HOST_REBIND;
> +		dprintk("lockd: rebind host %s; next rebind in %lu jiffies\n",
> +			host->h_name, host->h_nextrebind - jiffies);
>   	}
>   }
>   
> 

-- 
Calum Mackay
Linux Kernel Engineering
Oracle Linux and Virtualisation

[-- Attachment #1.1.2: OpenPGP_0x8523EF006DC153E2.asc --]
[-- Type: application/pgp-keys, Size: 3183 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]