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;
Message-ID: <c9ae2eec-6db9-d4de-fb29-e4dcc6be2dac@oracle.com>
Date:   Thu, 12 Nov 2020 21:04:07 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0)
 Gecko/20100101 Thunderbird/84.0a1
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH] lockd: don't use interval-based rebinding over TCP
Content-Language: en-GB
From:   Calum Mackay <calum.mackay@oracle.com>
To:     trondmy@hammerspace.com, anna.schumaker@netapp.com
References: <20201028201627.23625-1-calum.mackay@oracle.com>
Organization: Oracle
In-Reply-To: <20201028201627.23625-1-calum.mackay@oracle.com>
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature";
 boundary="SUdd9BMVNyphArlP5YbbX1iV3z9flDo6O"
Precedence: bulk

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--SUdd9BMVNyphArlP5YbbX1iV3z9flDo6O
Content-Type: multipart/mixed; boundary="ssv343dyU2kvrWTh71joEsIrIUvftDujy";
 protected-headers="v1"
From: Calum Mackay <calum.mackay@oracle.com>
To: trondmy@hammerspace.com, anna.schumaker@netapp.com
Cc: linux-nfs@vger.kernel.org
Message-ID: <c9ae2eec-6db9-d4de-fb29-e4dcc6be2dac@oracle.com>
Subject: Re: [PATCH] lockd: don't use interval-based rebinding over TCP
References: <20201028201627.23625-1-calum.mackay@oracle.com>
In-Reply-To: <20201028201627.23625-1-calum.mackay@oracle.com>

--ssv343dyU2kvrWTh71joEsIrIUvftDujy
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable

hi Anna, Trond,

was there any comment on this, please?

thanks,
calum.

On 28/10/2020 8:16 pm, Calum Mackay wrote:
> NLM uses an interval-based rebinding, i.e. it clears the transport's
> binding under certain conditions if more than 60 seconds have elapsed
> since the connection was last bound.
>=20
> This rebinding is not necessary for an autobind RPC client over a
> connection-oriented protocol like TCP.
>=20
> It can also cause problems: it is possible for nlm_bind_host() to clear=

> XPRT_BOUND whilst a connection worker is in the middle of trying to
> reconnect, after it had already been checked in xprt_connect().
>=20
> When the connection worker notices that XPRT_BOUND has been cleared
> under it, in xs_tcp_finish_connecting(), that results in:
>=20
> 	xs_tcp_setup_socket: connect returned unhandled error -107
>=20
> 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 has been seen in practice, with a large number of NLM=

> client tasks, following a server restart.
>=20
> The existing callers of nlm_bind_host & nlm_rebind_host should not need=

> to force the rebind, for TCP, so restrict the interval-based rebinding
> to UDP only.
>=20
> For TCP, we will still rebind when needed, e.g. on timeout, and connect=
ion
> error (including closure), since connection-related errors on an existi=
ng
> connection, ECONNREFUSED when trying to connect, and rpc_check_timeout(=
),
> already unconditionally clear XPRT_BOUND.
>=20
> To avoid having to add the fix, and explanation, to both nlm_bind_host(=
)
> and nlm_rebind_host(), remove the duplicate code from the former, and
> have it call the latter.
>=20
> Drop the dprintk, which adds no value over a trace.
>=20
> Signed-off-by: Calum Mackay <calum.mackay@oracle.com>
> ---
>   fs/lockd/host.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>=20
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 0afb6d59bad0..771c289f6df7 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 =3D host->h_rpcclnt) !=3D NULL) {
> -		if (time_after_eq(jiffies, host->h_nextrebind)) {
> -			rpc_force_rebind(clnt);
> -			host->h_nextrebind =3D jiffies + NLM_HOST_REBIND;
> -			dprintk("lockd: next rebind in %lu jiffies\n",
> -					host->h_nextrebind - jiffies);
> -		}
> +		nlm_rebind_host(host);
>   	} else {
>   		unsigned long increment =3D nlmsvc_timeout;
>   		struct rpc_timeout timeparms =3D {
> @@ -494,13 +489,20 @@ nlm_bind_host(struct nlm_host *host)
>   	return clnt;
>   }
>  =20
> -/*
> - * Force a portmap lookup of the remote lockd port
> +/**
> + * nlm_rebind_host - If needed, force a portmap lookup of the peer's l=
ockd port
> + * @host: NLM host handle for peer
> + *
> + * This is not needed when using a connection-oriented protocol, such =
as TCP.
> + * The existing autobind mechanism is sufficient to force a rebind whe=
n
> + * required, e.g. on connection state transitions.
>    */
>   void
>   nlm_rebind_host(struct nlm_host *host)
>   {
> -	dprintk("lockd: rebind host %s\n", host->h_name);
> +	if (host->h_proto !=3D IPPROTO_UDP)
> +		return;
> +
>   	if (host->h_rpcclnt && time_after_eq(jiffies, host->h_nextrebind)) {=

>   		rpc_force_rebind(host->h_rpcclnt);
>   		host->h_nextrebind =3D jiffies + NLM_HOST_REBIND;
>=20

--=20
Calum Mackay
Linux Kernel Engineering
Oracle Linux and Virtualisation

--ssv343dyU2kvrWTh71joEsIrIUvftDujy--

--SUdd9BMVNyphArlP5YbbX1iV3z9flDo6O
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature"

-----BEGIN PGP SIGNATURE-----

wsF5BAABCAAjFiEE1GBbrQYgx8o9Vdw+hSPvAG3BU+IFAl+to0cFAwAAAAAACgkQhSPvAG3BU+KF
Gw/+PqU+ZAgxsnKOkGsD/SHCQmSmdS8GP/UQ/QR+0TMURe5r99k1/g3/EiZfUqOZ80+eqcM+hi3c
k/sc/8cRrHLIz+7K0qtPwi+YniTiElM+anOzQCDJ8Ydd6sYtRYDXXvG76FlCYDEq9t7N455tiPDn
3sg/GuyzAbrcvICwNaVmOb5V47RtP3Ug582AK0Fe7sDmvnQfaiHt5V7yt3snLOLFiRsNv3iORtxD
HlfWXjk08dkWpJI8mTXGANmf6Vrz4G6LDUz95J4YROkUHg8oD2lBjAcB//5ipVoWj1osAF5RHrC3
420uYKN8XxhW/XFv3ga22WzDNkyYpQhkE+e64wFIkU9yt81qY55qhIWCF8iptY5rUBj7FTQpyQDr
zFPZPnJRYDiJ1bKqdySKPwkVvNTBoG8RWhGxT8pXZXbWDNkGrwV7QrKtQ0u3b4/vPbdduDIlIUCO
MhlKQHhOaZ+F1kKqIP/k/e1KTcwddeAsotrt+Nt99kmFMkUqkqw5CPhA9TKL2lsTMjdZ7J48qVTP
jkF2IW3tt/UZl1GN/a6YdI+cZNLcDN/ki8gjVZP8SRpVEYEwz/S8g+kl2AULuo3LxFqHLPGmkuyw
6LQ6xwp87thddSAQLZVWnTLAvuO94pdZGZC8fKVrzWEfBMKcHU5Oyt1pmjuwDqqVv4B/NgFF7w36
wIQ=
=OWrw
-----END PGP SIGNATURE-----

--SUdd9BMVNyphArlP5YbbX1iV3z9flDo6O--