Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1: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 1/3] SUNRPC: Fix socket waits for write buffer space
Thread-Topic: [PATCH 1/3] SUNRPC: Fix socket waits for write buffer space
Thread-Index: AQHYOIqROoDoMfMo0kabDTVmKfkU+KzCTZ+A
Date:   Wed, 16 Mar 2022 17:57:52 +0000
Message-ID: <D9030997-0BB7-435B-B7AD-4896BEF95B03@oracle.com>
References: <20220315162805.570850-1-trondmy@kernel.org>
In-Reply-To: <20220315162805.570850-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?07BjyL5wb6RQmXNqPp6GEitOKqKhx+KcLdIoYryun9W3KoqMHC43JT9k5wfF?=
 =?us-ascii?Q?byWCmarbWgiDjk33Fe8Kklq5aMj6zEGZzz7NlvvHHTYenvoofq1EVssymWME?=
 =?us-ascii?Q?PEEERijCj/H3+tDb+C2kL8Jd8FBnvHU+hmBum/qDBJGdxJ8cqlTsxGAppAgT?=
 =?us-ascii?Q?FagjXedhf/SUQYXKl2hJ+NgtHV973pnkuKQhVubRtjAyilloGw5BPouNwycx?=
 =?us-ascii?Q?IN1LIM0XxpEmkWwpcoVVnCvY7HwyeqGZEbgQZvJ/nag6DtP+tzwZBj3do0gd?=
 =?us-ascii?Q?hZhtzXZRF2H9kRy2S/WAQzO9vXQPK0Fo11KoyEcsgiiFvq2FGdPVCImWDgDH?=
 =?us-ascii?Q?FQ+0e47IdBdYH61xoyuB7PMHwnRt0k9xn3ZPoCxhTaCR9n+XcNf2sd28Xkke?=
 =?us-ascii?Q?kgcfMldF6yHi6EeWzm+UsbiqJPvKN1INSAzS6IM1bBnLJqKWSgKbqccxl7fj?=
 =?us-ascii?Q?WmWaBY9kiis+4w+xpiRBKwSwkOxf8cuduhEWHU6UHlki0mJvKLTAQaSFWXlK?=
 =?us-ascii?Q?Irh4z7yv8l0tN1JoaKfR7kpsrogZgb1dye6yf9N5MpZ13jABhXSLAFUoj7f2?=
 =?us-ascii?Q?AZJmNNjTYiZB+s2kAZGvqh/s0kPex4p640bjP8JeMO1eK+Co3zXUDtKVre2B?=
 =?us-ascii?Q?jNNEWqG0gJm7PxqOdc/gi1fL/hDSV/m+zGfLcHRd7qc1ivppMAVciM+jL3Nc?=
 =?us-ascii?Q?D8ntHFLDaOBTAYSSO0nmPXNEVBy02F5dje8JGUzDots4z3jYH2UF7UBFhL3s?=
 =?us-ascii?Q?vo0d/C21iTl4AoIiGwUXaED0baoROvSOsa6Lhz9/gznHsSutkJmuupQRxtnt?=
 =?us-ascii?Q?9UUfm8zHgOrs1uBMKJ94ZczRwdCGHMceE2opxjhf3VABMElkqAV2xNfj86HB?=
 =?us-ascii?Q?lOsaWECpgTLECyZLH3mzOiyDb5ItPwmyRPJShNcJYV0yq55Nk/71PED8CHCf?=
 =?us-ascii?Q?JtILCQj+ShjZMHN5jeTuQ7M6Lww4a6BBYuiJT27/+KncnHFk/t6q/0hM9+mW?=
 =?us-ascii?Q?fcMBN2/gqFe4FyyJ/6H/pZ6DU6PWfMo+5dUAWGaZZNnUlAW15VnoD/OHk0vR?=
 =?us-ascii?Q?u5W84Wsw3LiIl+x9gN/NWREPndUECeLUGN/8ItQyFx9BstjZIeavEc566HE5?=
 =?us-ascii?Q?MYqGTM3z5LTx2ELqUBl2oj8uWyDTl9xJh76iVN9hL9N0yZQt1l8tLsBnh0jw?=
 =?us-ascii?Q?NnQJTAgKtwHZe9F4ekJAfW/ADFfd5O5Y69FaFBpK0/Ib5wGd5BNG3dXjcL2O?=
 =?us-ascii?Q?XnaEZmAWV6aWSaCbMdpnPLZ8YI2N5OOzF069boM9rzCBWT2mf5tI+u9maeH+?=
 =?us-ascii?Q?NUOLSS4vFOKHysxQaqE8C6OHOGrMe5MZIFdAsN+3nBkyTyTMPacNdVUCrHRR?=
 =?us-ascii?Q?XQzNwVda4syetbUlAzHZ4scjDjyk3eMnsLxnECSiPT68c4xSlJDjLCLEWqsh?=
 =?us-ascii?Q?H9Hn/rGO1fY6LqJAosnIwGdSinDYU1JpCCVdV9+8M/JXrcJrRlm3yg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3FBC0E243931464FA516AFF33A79F603@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: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9eab7252-103d-472f-e8cd-08da07767da8
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Mar 2022 17:57:52.1478
 (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: BlJnE8fsbGSM/IdDHPrzWJBjfzJr27R0xCRwcNZBUVCRULtBDrnqBk6N1vVxIN/5N8mx4hh90W1nrxUmrl+Pyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1001MB2052
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10288 signatures=693715
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 suspectscore=0 adultscore=0 spamscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2203160106
X-Proofpoint-GUID: w93qH4lKZnFyKUg2ppjmfvJBwAYpKaPs
X-Proofpoint-ORIG-GUID: w93qH4lKZnFyKUg2ppjmfvJBwAYpKaPs
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 15, 2022, at 12:28 PM, trondmy@kernel.org wrote:
>=20
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>=20
> The socket layer requires that we use the socket lock to protect changes
> to the sock->sk_write_pending field and others.
>=20
> Reported-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

Applied the three patches in this series on top of my RPC-with-TLS
prototype. Works nicely!


> ---
> net/sunrpc/xprtsock.c | 54 +++++++++++++++++++++++++++++++------------
> 1 file changed, 39 insertions(+), 15 deletions(-)
>=20
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7e39f87cde2d..786df8c0cda3 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -763,12 +763,12 @@ xs_stream_start_connect(struct sock_xprt *transport=
)
> /**
>  * xs_nospace - handle transmit was incomplete
>  * @req: pointer to RPC request
> + * @transport: pointer to struct sock_xprt
>  *
>  */
> -static int xs_nospace(struct rpc_rqst *req)
> +static int xs_nospace(struct rpc_rqst *req, struct sock_xprt *transport)
> {
> -	struct rpc_xprt *xprt =3D req->rq_xprt;
> -	struct sock_xprt *transport =3D container_of(xprt, struct sock_xprt, xp=
rt);
> +	struct rpc_xprt *xprt =3D &transport->xprt;
> 	struct sock *sk =3D transport->inet;
> 	int ret =3D -EAGAIN;
>=20
> @@ -779,25 +779,49 @@ static int xs_nospace(struct rpc_rqst *req)
>=20
> 	/* Don't race with disconnect */
> 	if (xprt_connected(xprt)) {
> +		struct socket_wq *wq;
> +
> +		rcu_read_lock();
> +		wq =3D rcu_dereference(sk->sk_wq);
> +		set_bit(SOCKWQ_ASYNC_NOSPACE, &wq->flags);
> +		rcu_read_unlock();
> +
> 		/* wait for more buffer space */
> +		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> 		sk->sk_write_pending++;
> 		xprt_wait_for_buffer_space(xprt);
> 	} else
> 		ret =3D -ENOTCONN;
>=20
> 	spin_unlock(&xprt->transport_lock);
> +	return ret;
> +}
>=20
> -	/* Race breaker in case memory is freed before above code is called */
> -	if (ret =3D=3D -EAGAIN) {
> -		struct socket_wq *wq;
> +static int xs_sock_nospace(struct rpc_rqst *req)
> +{
> +	struct sock_xprt *transport =3D
> +		container_of(req->rq_xprt, struct sock_xprt, xprt);
> +	struct sock *sk =3D transport->inet;
> +	int ret =3D -EAGAIN;
>=20
> -		rcu_read_lock();
> -		wq =3D rcu_dereference(sk->sk_wq);
> -		set_bit(SOCKWQ_ASYNC_NOSPACE, &wq->flags);
> -		rcu_read_unlock();
> +	lock_sock(sk);
> +	if (!sock_writeable(sk))
> +		ret =3D xs_nospace(req, transport);
> +	release_sock(sk);
> +	return ret;
> +}
>=20
> -		sk->sk_write_space(sk);
> -	}
> +static int xs_stream_nospace(struct rpc_rqst *req)
> +{
> +	struct sock_xprt *transport =3D
> +		container_of(req->rq_xprt, struct sock_xprt, xprt);
> +	struct sock *sk =3D transport->inet;
> +	int ret =3D -EAGAIN;
> +
> +	lock_sock(sk);
> +	if (!sk_stream_memory_free(sk))
> +		ret =3D xs_nospace(req, transport);
> +	release_sock(sk);
> 	return ret;
> }
>=20
> @@ -887,7 +911,7 @@ static int xs_local_send_request(struct rpc_rqst *req=
)
> 	case -ENOBUFS:
> 		break;
> 	case -EAGAIN:
> -		status =3D xs_nospace(req);
> +		status =3D xs_stream_nospace(req);
> 		break;
> 	default:
> 		dprintk("RPC:       sendmsg returned unrecognized error %d\n",
> @@ -963,7 +987,7 @@ static int xs_udp_send_request(struct rpc_rqst *req)
> 		/* Should we call xs_close() here? */
> 		break;
> 	case -EAGAIN:
> -		status =3D xs_nospace(req);
> +		status =3D xs_sock_nospace(req);
> 		break;
> 	case -ENETUNREACH:
> 	case -ENOBUFS:
> @@ -1083,7 +1107,7 @@ static int xs_tcp_send_request(struct rpc_rqst *req=
)
> 		/* Should we call xs_close() here? */
> 		break;
> 	case -EAGAIN:
> -		status =3D xs_nospace(req);
> +		status =3D xs_stream_nospace(req);
> 		break;
> 	case -ECONNRESET:
> 	case -ECONNREFUSED:
> --=20
> 2.35.1
>=20

--
Chuck Lever