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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     "trondmy@kernel.org" <trondmy@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] SUNRPC/xprtrdma: Fix reconnection locking
Thread-Topic: [PATCH] SUNRPC/xprtrdma: Fix reconnection locking
Thread-Index: AQHXghY54y0I0AjDkU2IFWZmCYAtk6tW+/oA
Date:   Tue, 27 Jul 2021 15:57:18 +0000
Message-ID: <7DF7706B-CFA1-4AF1-8329-B60ED92B6C6C@oracle.com>
References: <20210726120312.8856-1-trondmy@kernel.org>
In-Reply-To: <20210726120312.8856-1-trondmy@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Fcc0J3nr8iK5C+lGckCRVAIaRHsmlvZU5OPN9PlKJAmkUgg5x7f6IoykHLIe?=
 =?us-ascii?Q?iQwYZWwlpnoA04W5XgDO/SIbdMpCanmq6qoTKQshpOkEFYOnBAS817cL1BHE?=
 =?us-ascii?Q?pvD04gRGCawNduFj+CXXu+Rf/gRkiuVfopbWodtDdaQDLKcf02pt8itVXq3j?=
 =?us-ascii?Q?a4Aqa8pzQykNxU84SiIUfgaE9VJTZx1+HemEIlbq250GQRNNM1ZjkgiXE/6B?=
 =?us-ascii?Q?3fEWWhKZYfktnTaFEkmAiuBQLtNXYBRNypKv3c97efPshg9DxPyzdpDUVoDU?=
 =?us-ascii?Q?5YEH+sJSQVJfoovOkiX+IkaCvdJHbV+b88W2y3P2eHOqhUoPvz+LBBOXI+Ud?=
 =?us-ascii?Q?k9/K+TgcDbE+1s9A6MCsrF01E7yVUyelIZpr8WDHlooDkrKcDsCKuicn8E/9?=
 =?us-ascii?Q?ExkYaIxrg+H9AvpnIeSN0e0Hk83Tk20F/skggfKQmERbiGY2jB8r6xtrFLMZ?=
 =?us-ascii?Q?frQZhmzfJKeHZrpzufOR8pErR0jvcr3RTeqjtqvalYZDZjw4MKSDuhjgRZXB?=
 =?us-ascii?Q?2Nk3aupV7Ojn3WiNUx2pdOSCJ7Zl6eVt/g570vRjWljmTxSAu/UguJIrb4lN?=
 =?us-ascii?Q?5qlVi5BuSELzpWbMXd13Fcw+K0KhmzCGj0M3BGCmd8GjcW0E3DzV7KgsMt4z?=
 =?us-ascii?Q?Z/Q/cUdimni1AZFtdI167CT82INYQCgkHVHkNRDq3/b4af/38UTDe8/LsebV?=
 =?us-ascii?Q?3xLzb0D4dGwd+aCVQC/XHPurfw8RF2Jk7+96zNPqBgztM/shpongQaR1uuaV?=
 =?us-ascii?Q?djdI7JTzmMSDw77VreIITwZuPNKnVRhrueUN6cnE7s1l2DsoS1/jaNcC+NW+?=
 =?us-ascii?Q?7MkVpwHzcuaALnAPiYEEFbpeILuFBANOIJ3Uxcn2H/MzR5UOIh2iPhXZ4sqg?=
 =?us-ascii?Q?a39KGGe3DyUV5fyDZLYirt+2D4wa+8jCguEVW8x+ArVx5+1LHCh6nxqgxY28?=
 =?us-ascii?Q?/Vcbg7tWnMxxSNRZ8eC5lc+q4atAXbcLM5Gg1djLJCT1FrjoFcxCOboPi61D?=
 =?us-ascii?Q?M3/DmhS1qsTcNPgbFRoI4e1gi/D3jygUpxJeLoXVnPU9uiWiYv6HEZa3Ms0O?=
 =?us-ascii?Q?E2VDpYe8rzFzobESszGEpzSgDGYReP+DBbtHFKDDjcwm4XWQPqqPb0Kv/RXW?=
 =?us-ascii?Q?N86o49OlopuP9cCDhDCTfcjUsc039x0CAMzVAXE7p9KatNe5sgX7l/f3WMyq?=
 =?us-ascii?Q?Wvpu1lVksd2UW3+LcPtLrEdvOlQYVSY97LsAw/P8gfayhMKhM3Ow+7Hdp1ug?=
 =?us-ascii?Q?cgyaiJUsne/zOxV5VSlanPEtCEMW7idcePrtlEQO4DfRggSz/TBlq1Ex22NN?=
 =?us-ascii?Q?5pqefaiu2nU8nafcpE/FOAoGtqyuO5sloCoaeUT65xQa0w=3D=3D?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <21F267B795E1274B8030FFA98A12AFD5@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e32e5200-96df-46dd-f553-08d951173698
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jul 2021 15:57:18.9995
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: juS3u94F6wI5W39qdynXk6JN3otixCO5duVVIn1pmDSqa3b9Psj0rHqK1XfKfgFEyUX4NZruQUeysYA0lERvqA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4162
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10057 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 suspectscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2107140000 definitions=main-2107270096
X-Proofpoint-ORIG-GUID: TIDhuLx1xWaRv2DtsxXVJQfxX91CdrFx
X-Proofpoint-GUID: TIDhuLx1xWaRv2DtsxXVJQfxX91CdrFx
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 26, 2021, at 8:03 AM, trondmy@kernel.org wrote:
>=20
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>=20
> The xprtrdma client code currently relies on the task that initiated the
> connect to hold the XPRT_LOCK for the duration of the connection
> attempt. If the task is woken early, due to some other event, then that
> lock could get released early.
> Avoid races by using the same mechanism that the socket code uses of
> transferring lock ownership to the RDMA connect worker itself. That
> frees us to call rpcrdma_xprt_disconnect() directly since we're now
> guaranteed exclusion w.r.t. other callers.
>=20
> Fixes: 4cf44be6f1e8 ("xprtrdma: Fix recursion into rpcrdma_xprt_disconnec=
t()")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

First, thanks for the clean up. I had been wondering about that,
but I haven't encountered any problems so far with the code as
it was.

Second, after applying this patch and enabling KASAN, I ran
several disconnect injection passes with an intensive software
development workload. About 500 disconnects were injected over
the test runs. I did not encounter any issues.

I inspected a trace capture of a couple injected disconnects
and did not see anything unexpected.

I don't have a server-side disconnect injection rig. I probably
should build one.

Tested-by: Chuck Lever <chuck.lever@oracle.com>

> ---
> net/sunrpc/xprt.c               |  2 ++
> net/sunrpc/xprtrdma/transport.c | 11 +++++------
> 2 files changed, 7 insertions(+), 6 deletions(-)
>=20
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index aae5a328b15b..b88ac8132054 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -877,6 +877,7 @@ bool xprt_lock_connect(struct rpc_xprt *xprt,
> 	spin_unlock(&xprt->transport_lock);
> 	return ret;
> }
> +EXPORT_SYMBOL_GPL(xprt_lock_connect);
>=20
> void xprt_unlock_connect(struct rpc_xprt *xprt, void *cookie)
> {
> @@ -893,6 +894,7 @@ void xprt_unlock_connect(struct rpc_xprt *xprt, void =
*cookie)
> 	spin_unlock(&xprt->transport_lock);
> 	wake_up_bit(&xprt->state, XPRT_LOCKED);
> }
> +EXPORT_SYMBOL_GPL(xprt_unlock_connect);
>=20
> /**
>  * xprt_connect - schedule a transport connect operation
> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transp=
ort.c
> index 9c2ffc67c0fd..975aef16ad34 100644
> --- a/net/sunrpc/xprtrdma/transport.c
> +++ b/net/sunrpc/xprtrdma/transport.c
> @@ -250,12 +250,9 @@ xprt_rdma_connect_worker(struct work_struct *work)
> 					   xprt->stat.connect_start;
> 		xprt_set_connected(xprt);
> 		rc =3D -EAGAIN;
> -	} else {
> -		/* Force a call to xprt_rdma_close to clean up */
> -		spin_lock(&xprt->transport_lock);
> -		set_bit(XPRT_CLOSE_WAIT, &xprt->state);
> -		spin_unlock(&xprt->transport_lock);
> -	}
> +	} else
> +		rpcrdma_xprt_disconnect(r_xprt);
> +	xprt_unlock_connect(xprt, r_xprt);
> 	xprt_wake_pending_tasks(xprt, rc);
> }
>=20
> @@ -489,6 +486,8 @@ xprt_rdma_connect(struct rpc_xprt *xprt, struct rpc_t=
ask *task)
> 	struct rpcrdma_ep *ep =3D r_xprt->rx_ep;
> 	unsigned long delay;
>=20
> +	WARN_ON_ONCE(!xprt_lock_connect(xprt, task, r_xprt));
> +
> 	delay =3D 0;
> 	if (ep && ep->re_connect_status !=3D 0) {
> 		delay =3D xprt_reconnect_delay(xprt);
> --=20
> 2.31.1
>=20

--
Chuck Lever