Received-SPF: pass (google.com: domain of linux-nfs+bounces-937-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Benjamin Coddington <bcodding@redhat.com>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
 Anna Schumaker <anna@kernel.org>, Jeff Layton <jlayton@kernel.org>,
 linux-nfs@vger.kernel.org
Subject: Re: [PATCH v4 1/2] SUNRPC: Fixup v4.1 backchannel request timeouts
Date: Thu, 04 Jan 2024 10:20:55 -0500
Message-ID: <FBF6D528-DD7C-40C3-85DC-8D48B98990A8@redhat.com>
In-Reply-To: <ZZbKRp3oe1lptDvf@tissot.1015granger.net>
References: <e28038fba1243f00b0dd66b7c5296a1e181645ea.1704379780.git.bcodding@redhat.com>
 <ZZbKRp3oe1lptDvf@tissot.1015granger.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 4 Jan 2024, at 10:09, Chuck Lever wrote:

> On Thu, Jan 04, 2024 at 09:58:45AM -0500, Benjamin Coddington wrote:
>> After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out whe=
n on
>> the sending list"), any 4.1 backchannel tasks placed on the sending qu=
eue
>                       ^^^
>
> "any" ? I found that this problem occurs only when the transport
> write lock is held (ie, when the forechannel is sending a Call).
> If the transport is idle, things work as expected. But OK, maybe
> your reproducer is different than mine.

Any that are _placed on the sending queue_.

> One more comment below.
>
>
>> would immediately return with -ETIMEDOUT since their req timers are ze=
ro.
>>
>> Initialize the backchannel's rpc_rqst timeout parameters from the xprt=
's
>> default timeout settings.
>>
>> Fixes: 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on t=
he sending list")
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  net/sunrpc/xprt.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>> index 2364c485540c..6cc9ffac962d 100644
>> --- a/net/sunrpc/xprt.c
>> +++ b/net/sunrpc/xprt.c
>> @@ -651,9 +651,9 @@ static unsigned long xprt_abs_ktime_to_jiffies(kti=
me_t abstime)
>>  		jiffies + nsecs_to_jiffies(-delta);
>>  }
>>
>> -static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req)
>> +static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req,
>> +		const struct rpc_timeout *to)
>>  {
>> -	const struct rpc_timeout *to =3D req->rq_task->tk_client->cl_timeout=
;
>>  	unsigned long majortimeo =3D req->rq_timeout;
>>
>>  	if (to->to_exponential)
>> @@ -665,9 +665,10 @@ static unsigned long xprt_calc_majortimeo(struct =
rpc_rqst *req)
>>  	return majortimeo;
>>  }
>>
>> -static void xprt_reset_majortimeo(struct rpc_rqst *req)
>> +static void xprt_reset_majortimeo(struct rpc_rqst *req,
>> +		const struct rpc_timeout *to)
>>  {
>> -	req->rq_majortimeo +=3D xprt_calc_majortimeo(req);
>> +	req->rq_majortimeo +=3D xprt_calc_majortimeo(req, to);
>>  }
>>
>>  static void xprt_reset_minortimeo(struct rpc_rqst *req)
>> @@ -675,7 +676,8 @@ static void xprt_reset_minortimeo(struct rpc_rqst =
*req)
>>  	req->rq_minortimeo +=3D req->rq_timeout;
>>  }
>>
>> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rq=
st *req)
>> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rq=
st *req,
>> +		const struct rpc_timeout *to)
>>  {
>>  	unsigned long time_init;
>>  	struct rpc_xprt *xprt =3D req->rq_xprt;
>> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *=
task, struct rpc_rqst *req)
>>  		time_init =3D jiffies;
>>  	else
>>  		time_init =3D xprt_abs_ktime_to_jiffies(task->tk_start);
>> -	req->rq_timeout =3D task->tk_client->cl_timeout->to_initval;
>> -	req->rq_majortimeo =3D time_init + xprt_calc_majortimeo(req);
>> +
>> +	req->rq_timeout =3D to->to_initval;
>> +	req->rq_majortimeo =3D time_init + xprt_calc_majortimeo(req, to);
>>  	req->rq_minortimeo =3D time_init + req->rq_timeout;
>>  }
>>
>> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
>>  	} else {
>>  		req->rq_timeout =3D to->to_initval;
>>  		req->rq_retries =3D 0;
>> -		xprt_reset_majortimeo(req);
>> +		xprt_reset_majortimeo(req, to);
>>  		/* Reset the RTT counters =3D=3D "slow start" */
>>  		spin_lock(&xprt->transport_lock);
>>  		rpc_init_rtt(req->rq_task->tk_client->cl_rtt, to->to_initval);
>> @@ -1886,7 +1889,7 @@ xprt_request_init(struct rpc_task *task)
>>  	req->rq_snd_buf.bvec =3D NULL;
>>  	req->rq_rcv_buf.bvec =3D NULL;
>>  	req->rq_release_snd_buf =3D NULL;
>> -	xprt_init_majortimeo(task, req);
>> +	xprt_init_majortimeo(task, req, task->tk_client->cl_timeout);
>>
>>  	trace_xprt_reserve(req);
>>  }
>> @@ -1996,6 +1999,8 @@ xprt_init_bc_request(struct rpc_rqst *req, struc=
t rpc_task *task)
>>  	 */
>>  	xbufp->len =3D xbufp->head[0].iov_len + xbufp->page_len +
>>  		xbufp->tail[0].iov_len;
>> +
>
> +	/*
> +	 * Backchannel Replies are sent with !RPC_TASK_SOFT and
> +	 * RPC_TASK_NO_RETRANS_TIMEOUT. The major timeout setting
> +	 * affects only how long each Reply waits to be sent when
> +	 * a transport connection cannot be established.
> +	 */

I put this on 2/2 like I said in my earlier response.  I've been trying n=
ot
to make a delta on 1/2 (yes, even though its just a comment) because ther=
e's
a nonzero chance a maintainer is currently testing it to fix 6.7.  I
probably should not have made these two into a series, except that the 2n=
d
depends on the 1st.

If you definitely want it here instead, I will send a v5.  I think we're
probably going to be stuck with a broken 6.7 at this point.

Ben