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: AQHXghY54y0I0AjDkU2IFWZmCYAtk6tVTa+A
Date:   Mon, 26 Jul 2021 14:17:14 +0000
Message-ID: <412EF4F6-6CD6-4188-B3BD-F8BD5CDCF859@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?ITrAFImudGc1y3ATegeLcwIrAYXkAEU7viSgXEkBxKPZb60W0PzQKXxCsCNB?=
 =?us-ascii?Q?VVMU2taEpgYG0thvxP9cTqi0FyAFrsMRyn81wskqhmf533OFkvjDTd76I1qO?=
 =?us-ascii?Q?53g1obd6PiMk3E9UVwvtlPqD3cbqzy//C9j2WJmItgs38/ket1iJa1S/QD2R?=
 =?us-ascii?Q?B9zOiRl5HzEuigjR6Pl5ZmaF1SHtRP1LW5x8lpjyLIs+/S92AK82WpXYmPlu?=
 =?us-ascii?Q?m6kve4B5jWkk9e8HHISDmJSnYq46wBm+beIFUvnru3Ee8oNaz2I9H4ehH/2D?=
 =?us-ascii?Q?vFt5MgjJB+I1JSL15rAuQm6E6p9cfW/WBtKF1/BzPB+7yQOruCboWuAe7bZi?=
 =?us-ascii?Q?MS+DUaq2gFCrnaGiyuywrR4kktxOmlhrgVsUir76csDgUAWDQM8D2KvOm/wb?=
 =?us-ascii?Q?PsEYb0OOLhWAKRFpuyjefB12AuNl8rHuP5mjFvg6B8hTHs+kdiJbaXSzsids?=
 =?us-ascii?Q?tfNfsoQOV2/rNVYRUzhY1mPx0/zcu96OJnwTnXRIpz3pQz6mPcdLRcRLwuNs?=
 =?us-ascii?Q?u3x+wQiI/yUNJ+AkcNgKnBrLHFeQaOl3NrpiKkQhbcf08kp3R/2kia+e4+/W?=
 =?us-ascii?Q?VJ+yXx8NLxEa1J2aBCUZqS2c38DASqdq1lWCvo62Nivt0NzKcoKYy6W+tAD6?=
 =?us-ascii?Q?d+hwGWX8GRI+j+DG+VBa1hos5A5vcuNyT/JqBxWuKeY13y5B01rTnyrfriDe?=
 =?us-ascii?Q?KiOYCz2WszHUSzzBo/jbdy6urYbwIjNwRMTd/WNLrvoziK4KZw6n1RNbk0ob?=
 =?us-ascii?Q?bMcJR+XPjuQp117pzOfI9cowf0Lmi0S4P1leCeXh9OTfB2TKZtMH3ZrO5S+r?=
 =?us-ascii?Q?716q295VuGpy9Kco/JFGk5k65CF4zxhlR/WAdjYgiG8/+gMEIGAzYzMPHm6x?=
 =?us-ascii?Q?3OrG2CwVHsRJUtl5cr3ixMCGVzEyU59dtldAvZwyc9GiS6MfrxwlpRds0WHJ?=
 =?us-ascii?Q?5P2JpnR2hYq1PejDLK8i7lfdY+8et9tOq1mQiai0AVi8AuxTqj9SOVPPinp/?=
 =?us-ascii?Q?mHvKu/qt0oUHdrBa+qBjQZbyXvik19ZqNYCD7Ywu0uE1r6riIWl7617q8u8+?=
 =?us-ascii?Q?INxhsi/DbhwT87nUCskvVu0UOjUxTzAraFSyXvWs2VfmgI/V+wguIU8cOQXl?=
 =?us-ascii?Q?jOF2f378SLGPRl+eKPE5QQ4oDsiAM78iMeMdXuBimuEDGyWAPtpNTF/mpW8u?=
 =?us-ascii?Q?RJZrELyxnSj/OgHE4FE7fMVwonjMxuKVVnjBG/oDfmvjVQG1Ockk5fe6iCYv?=
 =?us-ascii?Q?o3i550csN7yK8fw0zHeBZpvknw2rQujzLGi0QT5/vy/HvVcwRKsoRN1MsSWF?=
 =?us-ascii?Q?Xwa5F0phnAd6AketyHM5fTr/?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <FB06256EC6F3A14CB05B6505010F26A6@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: af842fe6-26d5-4146-2004-08d9504010ec
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2021 14:17:14.1057
 (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: +3wPOTkOsqiOZG/CQGN110WA4odf3xNQpix0jeY0I5ZLis+5Jla98PIvFbr7GLSVgGYBTo7+SeJFBApHuXarig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3221
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10056 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0 phishscore=0
 malwarescore=0 spamscore=0 mlxlogscore=999 mlxscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2107260081
X-Proofpoint-GUID: eV5yE_cXv2j78OpeqrMSLJXekJy4u7gu
X-Proofpoint-ORIG-GUID: eV5yE_cXv2j78OpeqrMSLJXekJy4u7gu
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>

I would like to test this change with disconnect injection
before it is merged.


> ---
> 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