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 v2] SUNRPC: More fixes for backlog congestion
Thread-Topic: [PATCH v2] SUNRPC: More fixes for backlog congestion
Thread-Index: AQHXUh6fW/1jodFH5kq6aV1RuDdf36r1z4UA
Date:   Wed, 26 May 2021 14:17:39 +0000
Message-ID: <DCFAF511-B2F0-458F-9F3F-CE870F40EE9D@oracle.com>
References: <20210526110221.338870-1-trondmy@kernel.org>
In-Reply-To: <20210526110221.338870-1-trondmy@kernel.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <BCB4084B11A3E64BB5E32FC5919CBD84@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a7f9a06-4b16-4efc-ab3a-08d9205104d5
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 May 2021 14:17:39.3823
 (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: ooL1FstulJ8Xgwg4iUABBeidUr2sCvrur9+TMaEBYTkxVY6q/gr9rIfyUNW5pg1eeYYWXRLTYJQIr8m+oCzpHQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4194
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9996 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 adultscore=0
 mlxscore=0 mlxlogscore=999 malwarescore=0 phishscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2105260097
X-Proofpoint-GUID: VaDqoigI8I47ieF9UqdwTkZI6PIpVc31
X-Proofpoint-ORIG-GUID: VaDqoigI8I47ieF9UqdwTkZI6PIpVc31
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9996 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 clxscore=1015
 malwarescore=0 bulkscore=0 impostorscore=0 phishscore=0 spamscore=0
 adultscore=0 priorityscore=1501 mlxlogscore=999 lowpriorityscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2105260097
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On May 26, 2021, at 7:02 AM, trondmy@kernel.org wrote:
>=20
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>=20
> Ensure that we fix the XPRT_CONGESTED starvation issue for RDMA as well
> as socket based transports.
> Ensure we always initialise the request after waking up from the backlog
> list.

Out of interest, what prompted this commit? Code audit,
or misbehavior?


> Fixes: e877a88d1f06 ("SUNRPC in case of backlog, hand free slots directly=
 to waiting task")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> v2: Ensure we release the RDMA reply buffer
>=20
> include/linux/sunrpc/xprt.h     |  2 ++
> net/sunrpc/xprt.c               | 58 ++++++++++++++++-----------------
> net/sunrpc/xprtrdma/transport.c | 12 +++----
> net/sunrpc/xprtrdma/verbs.c     | 18 ++++++++--
> net/sunrpc/xprtrdma/xprt_rdma.h |  1 +
> 5 files changed, 52 insertions(+), 39 deletions(-)
>=20
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index d81fe8b364d0..61b622e334ee 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -368,6 +368,8 @@ struct rpc_xprt *	xprt_alloc(struct net *net, size_t =
size,
> 				unsigned int num_prealloc,
> 				unsigned int max_req);
> void			xprt_free(struct rpc_xprt *);
> +void			xprt_add_backlog(struct rpc_xprt *xprt, struct rpc_task *task);
> +bool			xprt_wake_up_backlog(struct rpc_xprt *xprt, struct rpc_rqst *req)=
;
>=20
> static inline int
> xprt_enable_swap(struct rpc_xprt *xprt)
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 5b3981fd3783..3509a7f139b9 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1607,11 +1607,18 @@ xprt_transmit(struct rpc_task *task)
> 	spin_unlock(&xprt->queue_lock);
> }
>=20
> -static void xprt_add_backlog(struct rpc_xprt *xprt, struct rpc_task *tas=
k)
> +static void xprt_complete_request_init(struct rpc_task *task)
> +{
> +	if (task->tk_rqstp)
> +		xprt_request_init(task);
> +}
> +
> +void xprt_add_backlog(struct rpc_xprt *xprt, struct rpc_task *task)
> {
> 	set_bit(XPRT_CONGESTED, &xprt->state);
> -	rpc_sleep_on(&xprt->backlog, task, NULL);
> +	rpc_sleep_on(&xprt->backlog, task, xprt_complete_request_init);
> }
> +EXPORT_SYMBOL_GPL(xprt_add_backlog);
>=20
> static bool __xprt_set_rq(struct rpc_task *task, void *data)
> {
> @@ -1619,14 +1626,13 @@ static bool __xprt_set_rq(struct rpc_task *task, =
void *data)
>=20
> 	if (task->tk_rqstp =3D=3D NULL) {
> 		memset(req, 0, sizeof(*req));	/* mark unused */
> -		task->tk_status =3D -EAGAIN;
> 		task->tk_rqstp =3D req;
> 		return true;
> 	}
> 	return false;
> }
>=20
> -static bool xprt_wake_up_backlog(struct rpc_xprt *xprt, struct rpc_rqst =
*req)
> +bool xprt_wake_up_backlog(struct rpc_xprt *xprt, struct rpc_rqst *req)
> {
> 	if (rpc_wake_up_first(&xprt->backlog, __xprt_set_rq, req) =3D=3D NULL) {
> 		clear_bit(XPRT_CONGESTED, &xprt->state);
> @@ -1634,6 +1640,7 @@ static bool xprt_wake_up_backlog(struct rpc_xprt *x=
prt, struct rpc_rqst *req)
> 	}
> 	return true;
> }
> +EXPORT_SYMBOL_GPL(xprt_wake_up_backlog);
>=20
> static bool xprt_throttle_congested(struct rpc_xprt *xprt, struct rpc_tas=
k *task)
> {
> @@ -1643,7 +1650,7 @@ static bool xprt_throttle_congested(struct rpc_xprt=
 *xprt, struct rpc_task *task
> 		goto out;
> 	spin_lock(&xprt->reserve_lock);
> 	if (test_bit(XPRT_CONGESTED, &xprt->state)) {
> -		rpc_sleep_on(&xprt->backlog, task, NULL);
> +		xprt_add_backlog(xprt, task);
> 		ret =3D true;
> 	}
> 	spin_unlock(&xprt->reserve_lock);
> @@ -1812,10 +1819,6 @@ xprt_request_init(struct rpc_task *task)
> 	struct rpc_xprt *xprt =3D task->tk_xprt;
> 	struct rpc_rqst	*req =3D task->tk_rqstp;
>=20
> -	if (req->rq_task)
> -		/* Already initialized */
> -		return;
> -
> 	req->rq_task	=3D task;
> 	req->rq_xprt    =3D xprt;
> 	req->rq_buffer  =3D NULL;
> @@ -1876,10 +1879,8 @@ void xprt_retry_reserve(struct rpc_task *task)
> 	struct rpc_xprt *xprt =3D task->tk_xprt;
>=20
> 	task->tk_status =3D 0;
> -	if (task->tk_rqstp !=3D NULL) {
> -		xprt_request_init(task);
> +	if (task->tk_rqstp !=3D NULL)
> 		return;
> -	}
>=20
> 	task->tk_status =3D -EAGAIN;
> 	xprt_do_reserve(xprt, task);
> @@ -1904,24 +1905,21 @@ void xprt_release(struct rpc_task *task)
> 	}
>=20
> 	xprt =3D req->rq_xprt;
> -	if (xprt) {
> -		xprt_request_dequeue_xprt(task);
> -		spin_lock(&xprt->transport_lock);
> -		xprt->ops->release_xprt(xprt, task);
> -		if (xprt->ops->release_request)
> -			xprt->ops->release_request(task);
> -		xprt_schedule_autodisconnect(xprt);
> -		spin_unlock(&xprt->transport_lock);
> -		if (req->rq_buffer)
> -			xprt->ops->buf_free(task);
> -		xdr_free_bvec(&req->rq_rcv_buf);
> -		xdr_free_bvec(&req->rq_snd_buf);
> -		if (req->rq_cred !=3D NULL)
> -			put_rpccred(req->rq_cred);
> -		if (req->rq_release_snd_buf)
> -			req->rq_release_snd_buf(req);
> -	} else
> -		xprt =3D task->tk_xprt;
> +	xprt_request_dequeue_xprt(task);
> +	spin_lock(&xprt->transport_lock);
> +	xprt->ops->release_xprt(xprt, task);
> +	if (xprt->ops->release_request)
> +		xprt->ops->release_request(task);
> +	xprt_schedule_autodisconnect(xprt);
> +	spin_unlock(&xprt->transport_lock);
> +	if (req->rq_buffer)
> +		xprt->ops->buf_free(task);
> +	xdr_free_bvec(&req->rq_rcv_buf);
> +	xdr_free_bvec(&req->rq_snd_buf);
> +	if (req->rq_cred !=3D NULL)
> +		put_rpccred(req->rq_cred);
> +	if (req->rq_release_snd_buf)
> +		req->rq_release_snd_buf(req);
>=20
> 	task->tk_rqstp =3D NULL;
> 	if (likely(!bc_prealloc(req)))
> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transp=
ort.c
> index 09953597d055..19a49d26b1e4 100644
> --- a/net/sunrpc/xprtrdma/transport.c
> +++ b/net/sunrpc/xprtrdma/transport.c
> @@ -520,9 +520,8 @@ xprt_rdma_alloc_slot(struct rpc_xprt *xprt, struct rp=
c_task *task)
> 	return;
>=20
> out_sleep:
> -	set_bit(XPRT_CONGESTED, &xprt->state);
> -	rpc_sleep_on(&xprt->backlog, task, NULL);
> 	task->tk_status =3D -EAGAIN;
> +	xprt_add_backlog(xprt, task);
> }
>=20
> /**
> @@ -537,10 +536,11 @@ xprt_rdma_free_slot(struct rpc_xprt *xprt, struct r=
pc_rqst *rqst)
> 	struct rpcrdma_xprt *r_xprt =3D
> 		container_of(xprt, struct rpcrdma_xprt, rx_xprt);
>=20
> -	memset(rqst, 0, sizeof(*rqst));
> -	rpcrdma_buffer_put(&r_xprt->rx_buf, rpcr_to_rdmar(rqst));
> -	if (unlikely(!rpc_wake_up_next(&xprt->backlog)))
> -		clear_bit(XPRT_CONGESTED, &xprt->state);
> +	rpcrdma_reply_put(&r_xprt->rx_buf, rpcr_to_rdmar(rqst));
> +	if (!xprt_wake_up_backlog(xprt, rqst)) {
> +		memset(rqst, 0, sizeof(*rqst));
> +		rpcrdma_buffer_put(&r_xprt->rx_buf, rpcr_to_rdmar(rqst));
> +	}
> }
>=20
> static bool rpcrdma_check_regbuf(struct rpcrdma_xprt *r_xprt,
> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 1e965a380896..649c23518ec0 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -1200,6 +1200,20 @@ rpcrdma_mr_get(struct rpcrdma_xprt *r_xprt)
> 	return mr;
> }
>=20
> +/**
> + * rpcrdma_reply_put - Put reply buffers back into pool
> + * @buffers: buffer pool
> + * @req: object to return
> + *
> + */
> +void rpcrdma_reply_put(struct rpcrdma_buffer *buffers, struct rpcrdma_re=
q *req)
> +{
> +	if (req->rl_reply) {
> +		rpcrdma_rep_put(buffers, req->rl_reply);
> +		req->rl_reply =3D NULL;
> +	}
> +}
> +
> /**
>  * rpcrdma_buffer_get - Get a request buffer
>  * @buffers: Buffer pool from which to obtain a buffer
> @@ -1228,9 +1242,7 @@ rpcrdma_buffer_get(struct rpcrdma_buffer *buffers)
>  */
> void rpcrdma_buffer_put(struct rpcrdma_buffer *buffers, struct rpcrdma_re=
q *req)
> {
> -	if (req->rl_reply)
> -		rpcrdma_rep_put(buffers, req->rl_reply);
> -	req->rl_reply =3D NULL;
> +	rpcrdma_reply_put(buffers, req);
>=20
> 	spin_lock(&buffers->rb_lock);
> 	list_add(&req->rl_list, &buffers->rb_send_bufs);
> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_r=
dma.h
> index 436ad7312614..5d231d94e944 100644
> --- a/net/sunrpc/xprtrdma/xprt_rdma.h
> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h
> @@ -479,6 +479,7 @@ struct rpcrdma_req *rpcrdma_buffer_get(struct rpcrdma=
_buffer *);
> void rpcrdma_buffer_put(struct rpcrdma_buffer *buffers,
> 			struct rpcrdma_req *req);
> void rpcrdma_rep_put(struct rpcrdma_buffer *buf, struct rpcrdma_rep *rep)=
;
> +void rpcrdma_reply_put(struct rpcrdma_buffer *buffers, struct rpcrdma_re=
q *req);
>=20
> bool rpcrdma_regbuf_realloc(struct rpcrdma_regbuf *rb, size_t size,
> 			    gfp_t flags);
> --=20
> 2.31.1
>=20

--
Chuck Lever