After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
the sending list"), any 4.1 backchannel tasks placed on the sending queue
would immediately return with -ETIMEDOUT since their req timers are zero.
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 the sending list")
Signed-off-by: Benjamin Coddington <[email protected]>
---
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(ktime_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 = req->rq_task->tk_client->cl_timeout;
unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
+ req->rq_majortimeo += 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 += req->rq_timeout;
}
-static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
+static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
+ const struct rpc_timeout *to)
{
unsigned long time_init;
struct rpc_xprt *xprt = req->rq_xprt;
@@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
time_init = jiffies;
else
time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
- req->rq_timeout = task->tk_client->cl_timeout->to_initval;
- req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
+
+ req->rq_timeout = to->to_initval;
+ req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
req->rq_minortimeo = time_init + req->rq_timeout;
}
@@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
} else {
req->rq_timeout = to->to_initval;
req->rq_retries = 0;
- xprt_reset_majortimeo(req);
+ xprt_reset_majortimeo(req, to);
/* Reset the RTT counters == "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 = NULL;
req->rq_rcv_buf.bvec = NULL;
req->rq_release_snd_buf = 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, struct rpc_task *task)
*/
xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
xbufp->tail[0].iov_len;
+
+ xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
}
#endif
--
2.43.0
For backchannel requests that lookup the appropriate nfs_client, use the
state-management rpc_clnt's rpc_timeout parameters for the backchannel's
response. When the nfs_client cannot be found, fall back to using the
xprt's default timeout parameters.
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/callback_xdr.c | 5 +++++
include/linux/sunrpc/bc_xprt.h | 3 ++-
include/linux/sunrpc/sched.h | 14 +++++++++++++-
include/linux/sunrpc/svc.h | 2 ++
include/linux/sunrpc/xprt.h | 11 -----------
net/sunrpc/clnt.c | 6 ++++--
net/sunrpc/svc.c | 11 ++++++++++-
net/sunrpc/xprt.c | 12 +++++++++---
8 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 321af81c456e..9369488f2ed4 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -967,6 +967,11 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
nops--;
}
+ if (svc_is_backchannel(rqstp) && cps.clp) {
+ rqstp->bc_to_initval = cps.clp->cl_rpcclient->cl_timeout->to_initval;
+ rqstp->bc_to_retries = cps.clp->cl_rpcclient->cl_timeout->to_retries;
+ }
+
*hdr_res.status = status;
*hdr_res.nops = htonl(nops);
nfs4_cb_free_slot(&cps);
diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
index db30a159f9d5..f22bf915dcf6 100644
--- a/include/linux/sunrpc/bc_xprt.h
+++ b/include/linux/sunrpc/bc_xprt.h
@@ -20,7 +20,8 @@
#ifdef CONFIG_SUNRPC_BACKCHANNEL
struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid);
void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
-void xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task);
+void xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task,
+ const struct rpc_timeout *to);
void xprt_free_bc_request(struct rpc_rqst *req);
int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 8ada7dc802d3..2d61987b3545 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -37,6 +37,17 @@ struct rpc_wait {
struct list_head timer_list; /* Timer list */
};
+/*
+ * This describes a timeout strategy
+ */
+struct rpc_timeout {
+ unsigned long to_initval, /* initial timeout */
+ to_maxval, /* max timeout */
+ to_increment; /* if !exponential */
+ unsigned int to_retries; /* max # of retries */
+ unsigned char to_exponential;
+};
+
/*
* This is the RPC task struct
*/
@@ -205,7 +216,8 @@ struct rpc_wait_queue {
*/
struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
-struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req);
+struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
+ struct rpc_timeout *timeout);
void rpc_put_task(struct rpc_task *);
void rpc_put_task_async(struct rpc_task *);
bool rpc_task_set_rpc_status(struct rpc_task *task, int rpc_status);
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index b10f987509cc..3331a1c2b47e 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -250,6 +250,8 @@ struct svc_rqst {
struct net *rq_bc_net; /* pointer to backchannel's
* net namespace
*/
+ unsigned long bc_to_initval;
+ unsigned int bc_to_retries;
void ** rq_lease_breaker; /* The v4 client breaking a lease */
unsigned int rq_status_counter; /* RPC processing counter */
};
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index f85d3a0daca2..464f6a9492ab 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -30,17 +30,6 @@
#define RPC_MAXCWND(xprt) ((xprt)->max_reqs << RPC_CWNDSHIFT)
#define RPCXPRT_CONGESTED(xprt) ((xprt)->cong >= (xprt)->cwnd)
-/*
- * This describes a timeout strategy
- */
-struct rpc_timeout {
- unsigned long to_initval, /* initial timeout */
- to_maxval, /* max timeout */
- to_increment; /* if !exponential */
- unsigned int to_retries; /* max # of retries */
- unsigned char to_exponential;
-};
-
enum rpc_display_format_t {
RPC_DISPLAY_ADDR = 0,
RPC_DISPLAY_PORT,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index daa9582ec861..886fc4c76558 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1302,8 +1302,10 @@ static void call_bc_encode(struct rpc_task *task);
* rpc_run_bc_task - Allocate a new RPC task for backchannel use, then run
* rpc_execute against it
* @req: RPC request
+ * @timeout: timeout values to use for this task
*/
-struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
+struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
+ struct rpc_timeout *timeout)
{
struct rpc_task *task;
struct rpc_task_setup task_setup_data = {
@@ -1322,7 +1324,7 @@ struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
return task;
}
- xprt_init_bc_request(req, task);
+ xprt_init_bc_request(req, task, timeout);
task->tk_action = call_bc_encode;
atomic_inc(&task->tk_count);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 3f2ea7a0496f..3f714d33624b 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1557,6 +1557,7 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
{
struct rpc_task *task;
int proc_error;
+ struct rpc_timeout timeout;
/* Build the svc_rqst used by the common processing routine */
rqstp->rq_xid = req->rq_xid;
@@ -1602,8 +1603,16 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
return;
}
/* Finally, send the reply synchronously */
+ if (rqstp->bc_to_initval > 0) {
+ timeout.to_initval = rqstp->bc_to_initval;
+ timeout.to_retries = rqstp->bc_to_initval;
+ } else {
+ timeout.to_initval = req->rq_xprt->timeout->to_initval;
+ timeout.to_initval = req->rq_xprt->timeout->to_retries;
+ }
memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
- task = rpc_run_bc_task(req);
+ task = rpc_run_bc_task(req, &timeout);
+
if (IS_ERR(task))
return;
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 6cc9ffac962d..af13fdfa6672 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1986,7 +1986,8 @@ void xprt_release(struct rpc_task *task)
#ifdef CONFIG_SUNRPC_BACKCHANNEL
void
-xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task)
+xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task,
+ const struct rpc_timeout *to)
{
struct xdr_buf *xbufp = &req->rq_snd_buf;
@@ -1999,8 +2000,13 @@ xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task)
*/
xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
xbufp->tail[0].iov_len;
-
- xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
+ /*
+ * 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.
+ */
+ xprt_init_majortimeo(task, req, to);
}
#endif
--
2.43.0
On Thu, Jan 04, 2024 at 09:58:45AM -0500, Benjamin Coddington wrote:
> After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
> the sending list"), any 4.1 backchannel tasks placed on the sending queue
^^^
"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.
One more comment below.
> would immediately return with -ETIMEDOUT since their req timers are zero.
>
> 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 the sending list")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> 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(ktime_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 = req->rq_task->tk_client->cl_timeout;
> unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
> + req->rq_majortimeo += 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 += req->rq_timeout;
> }
>
> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
> + const struct rpc_timeout *to)
> {
> unsigned long time_init;
> struct rpc_xprt *xprt = req->rq_xprt;
> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> time_init = jiffies;
> else
> time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
> - req->rq_timeout = task->tk_client->cl_timeout->to_initval;
> - req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
> +
> + req->rq_timeout = to->to_initval;
> + req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
> req->rq_minortimeo = time_init + req->rq_timeout;
> }
>
> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> } else {
> req->rq_timeout = to->to_initval;
> req->rq_retries = 0;
> - xprt_reset_majortimeo(req);
> + xprt_reset_majortimeo(req, to);
> /* Reset the RTT counters == "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 = NULL;
> req->rq_rcv_buf.bvec = NULL;
> req->rq_release_snd_buf = 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, struct rpc_task *task)
> */
> xbufp->len = 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.
+ */
> + xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
> }
> #endif
>
> --
> 2.43.0
>
>
--
Chuck Lever
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 when on
>> the sending list"), any 4.1 backchannel tasks placed on the sending queue
> ^^^
>
> "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 zero.
>>
>> 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 the sending list")
>> Signed-off-by: Benjamin Coddington <[email protected]>
>> ---
>> 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(ktime_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 = req->rq_task->tk_client->cl_timeout;
>> unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
>> + req->rq_majortimeo += 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 += req->rq_timeout;
>> }
>>
>> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
>> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
>> + const struct rpc_timeout *to)
>> {
>> unsigned long time_init;
>> struct rpc_xprt *xprt = req->rq_xprt;
>> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
>> time_init = jiffies;
>> else
>> time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
>> - req->rq_timeout = task->tk_client->cl_timeout->to_initval;
>> - req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
>> +
>> + req->rq_timeout = to->to_initval;
>> + req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
>> req->rq_minortimeo = time_init + req->rq_timeout;
>> }
>>
>> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
>> } else {
>> req->rq_timeout = to->to_initval;
>> req->rq_retries = 0;
>> - xprt_reset_majortimeo(req);
>> + xprt_reset_majortimeo(req, to);
>> /* Reset the RTT counters == "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 = NULL;
>> req->rq_rcv_buf.bvec = NULL;
>> req->rq_release_snd_buf = 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, struct rpc_task *task)
>> */
>> xbufp->len = 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 not
to make a delta on 1/2 (yes, even though its just a comment) because there'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 2nd
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
On Thu, Jan 04, 2024 at 10:20:55AM -0500, Benjamin Coddington wrote:
> 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 when on
> >> the sending list"), any 4.1 backchannel tasks placed on the sending queue
> > ^^^
> >
> > "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_.
Ah, I misremembered: I thought all to-be-sent tasks were placed on
the sending queue. But no, only the ones that are put to sleep are.
> > One more comment below.
> >
> >
> >> would immediately return with -ETIMEDOUT since their req timers are zero.
> >>
> >> 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 the sending list")
> >> Signed-off-by: Benjamin Coddington <[email protected]>
> >> ---
> >> 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(ktime_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 = req->rq_task->tk_client->cl_timeout;
> >> unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
> >> + req->rq_majortimeo += 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 += req->rq_timeout;
> >> }
> >>
> >> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> >> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
> >> + const struct rpc_timeout *to)
> >> {
> >> unsigned long time_init;
> >> struct rpc_xprt *xprt = req->rq_xprt;
> >> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> >> time_init = jiffies;
> >> else
> >> time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
> >> - req->rq_timeout = task->tk_client->cl_timeout->to_initval;
> >> - req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
> >> +
> >> + req->rq_timeout = to->to_initval;
> >> + req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
> >> req->rq_minortimeo = time_init + req->rq_timeout;
> >> }
> >>
> >> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> >> } else {
> >> req->rq_timeout = to->to_initval;
> >> req->rq_retries = 0;
> >> - xprt_reset_majortimeo(req);
> >> + xprt_reset_majortimeo(req, to);
> >> /* Reset the RTT counters == "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 = NULL;
> >> req->rq_rcv_buf.bvec = NULL;
> >> req->rq_release_snd_buf = 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, struct rpc_task *task)
> >> */
> >> xbufp->len = 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 not
> to make a delta on 1/2 (yes, even though its just a comment) because there'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 2nd
> depends on the 1st.
>
> If you definitely want it here instead, I will send a v5.
Got it, I didn't realize 1/2 was immutable at this point.
> I think we're probably going to be stuck with a broken 6.7 at this
> point.
Well, 6.7.0 might have the bug, but unless I've missed something,
1/2 will get backported to 6.7.y pretty quickly, even if it goes
in during the 6.8 merge window.
--
Chuck Lever
Hi Ben,
On Thu, Jan 4, 2024 at 9:58 AM Benjamin Coddington <[email protected]> wrote:
>
> For backchannel requests that lookup the appropriate nfs_client, use the
> state-management rpc_clnt's rpc_timeout parameters for the backchannel's
> response. When the nfs_client cannot be found, fall back to using the
> xprt's default timeout parameters.
Thanks for sending the v4, it fixes the problem I was seeing yesterday!
Anna
>
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> fs/nfs/callback_xdr.c | 5 +++++
> include/linux/sunrpc/bc_xprt.h | 3 ++-
> include/linux/sunrpc/sched.h | 14 +++++++++++++-
> include/linux/sunrpc/svc.h | 2 ++
> include/linux/sunrpc/xprt.h | 11 -----------
> net/sunrpc/clnt.c | 6 ++++--
> net/sunrpc/svc.c | 11 ++++++++++-
> net/sunrpc/xprt.c | 12 +++++++++---
> 8 files changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 321af81c456e..9369488f2ed4 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -967,6 +967,11 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
> nops--;
> }
>
> + if (svc_is_backchannel(rqstp) && cps.clp) {
> + rqstp->bc_to_initval = cps.clp->cl_rpcclient->cl_timeout->to_initval;
> + rqstp->bc_to_retries = cps.clp->cl_rpcclient->cl_timeout->to_retries;
> + }
> +
> *hdr_res.status = status;
> *hdr_res.nops = htonl(nops);
> nfs4_cb_free_slot(&cps);
> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
> index db30a159f9d5..f22bf915dcf6 100644
> --- a/include/linux/sunrpc/bc_xprt.h
> +++ b/include/linux/sunrpc/bc_xprt.h
> @@ -20,7 +20,8 @@
> #ifdef CONFIG_SUNRPC_BACKCHANNEL
> struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid);
> void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
> -void xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task);
> +void xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task,
> + const struct rpc_timeout *to);
> void xprt_free_bc_request(struct rpc_rqst *req);
> int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
> void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index 8ada7dc802d3..2d61987b3545 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -37,6 +37,17 @@ struct rpc_wait {
> struct list_head timer_list; /* Timer list */
> };
>
> +/*
> + * This describes a timeout strategy
> + */
> +struct rpc_timeout {
> + unsigned long to_initval, /* initial timeout */
> + to_maxval, /* max timeout */
> + to_increment; /* if !exponential */
> + unsigned int to_retries; /* max # of retries */
> + unsigned char to_exponential;
> +};
> +
> /*
> * This is the RPC task struct
> */
> @@ -205,7 +216,8 @@ struct rpc_wait_queue {
> */
> struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
> struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
> -struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req);
> +struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
> + struct rpc_timeout *timeout);
> void rpc_put_task(struct rpc_task *);
> void rpc_put_task_async(struct rpc_task *);
> bool rpc_task_set_rpc_status(struct rpc_task *task, int rpc_status);
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index b10f987509cc..3331a1c2b47e 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -250,6 +250,8 @@ struct svc_rqst {
> struct net *rq_bc_net; /* pointer to backchannel's
> * net namespace
> */
> + unsigned long bc_to_initval;
> + unsigned int bc_to_retries;
> void ** rq_lease_breaker; /* The v4 client breaking a lease */
> unsigned int rq_status_counter; /* RPC processing counter */
> };
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index f85d3a0daca2..464f6a9492ab 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -30,17 +30,6 @@
> #define RPC_MAXCWND(xprt) ((xprt)->max_reqs << RPC_CWNDSHIFT)
> #define RPCXPRT_CONGESTED(xprt) ((xprt)->cong >= (xprt)->cwnd)
>
> -/*
> - * This describes a timeout strategy
> - */
> -struct rpc_timeout {
> - unsigned long to_initval, /* initial timeout */
> - to_maxval, /* max timeout */
> - to_increment; /* if !exponential */
> - unsigned int to_retries; /* max # of retries */
> - unsigned char to_exponential;
> -};
> -
> enum rpc_display_format_t {
> RPC_DISPLAY_ADDR = 0,
> RPC_DISPLAY_PORT,
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index daa9582ec861..886fc4c76558 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1302,8 +1302,10 @@ static void call_bc_encode(struct rpc_task *task);
> * rpc_run_bc_task - Allocate a new RPC task for backchannel use, then run
> * rpc_execute against it
> * @req: RPC request
> + * @timeout: timeout values to use for this task
> */
> -struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
> +struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
> + struct rpc_timeout *timeout)
> {
> struct rpc_task *task;
> struct rpc_task_setup task_setup_data = {
> @@ -1322,7 +1324,7 @@ struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
> return task;
> }
>
> - xprt_init_bc_request(req, task);
> + xprt_init_bc_request(req, task, timeout);
>
> task->tk_action = call_bc_encode;
> atomic_inc(&task->tk_count);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 3f2ea7a0496f..3f714d33624b 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1557,6 +1557,7 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
> {
> struct rpc_task *task;
> int proc_error;
> + struct rpc_timeout timeout;
>
> /* Build the svc_rqst used by the common processing routine */
> rqstp->rq_xid = req->rq_xid;
> @@ -1602,8 +1603,16 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
> return;
> }
> /* Finally, send the reply synchronously */
> + if (rqstp->bc_to_initval > 0) {
> + timeout.to_initval = rqstp->bc_to_initval;
> + timeout.to_retries = rqstp->bc_to_initval;
> + } else {
> + timeout.to_initval = req->rq_xprt->timeout->to_initval;
> + timeout.to_initval = req->rq_xprt->timeout->to_retries;
> + }
> memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
> - task = rpc_run_bc_task(req);
> + task = rpc_run_bc_task(req, &timeout);
> +
> if (IS_ERR(task))
> return;
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 6cc9ffac962d..af13fdfa6672 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1986,7 +1986,8 @@ void xprt_release(struct rpc_task *task)
>
> #ifdef CONFIG_SUNRPC_BACKCHANNEL
> void
> -xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task)
> +xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task,
> + const struct rpc_timeout *to)
> {
> struct xdr_buf *xbufp = &req->rq_snd_buf;
>
> @@ -1999,8 +2000,13 @@ xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task)
> */
> xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
> xbufp->tail[0].iov_len;
> -
> - xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
> + /*
> + * 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.
> + */
> + xprt_init_majortimeo(task, req, to);
> }
> #endif
>
> --
> 2.43.0
>
On Thu, Jan 04, 2024 at 09:58:45AM -0500, Benjamin Coddington wrote:
> After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
> the sending list"), any 4.1 backchannel tasks placed on the sending queue
> would immediately return with -ETIMEDOUT since their req timers are zero.
>
> 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 the sending list")
> Signed-off-by: Benjamin Coddington <[email protected]>
For both 1/2 and 2/2:
Tested-by: Chuck Lever <[email protected]>
> ---
> 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(ktime_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 = req->rq_task->tk_client->cl_timeout;
> unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
> + req->rq_majortimeo += 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 += req->rq_timeout;
> }
>
> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
> + const struct rpc_timeout *to)
> {
> unsigned long time_init;
> struct rpc_xprt *xprt = req->rq_xprt;
> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> time_init = jiffies;
> else
> time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
> - req->rq_timeout = task->tk_client->cl_timeout->to_initval;
> - req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
> +
> + req->rq_timeout = to->to_initval;
> + req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
> req->rq_minortimeo = time_init + req->rq_timeout;
> }
>
> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> } else {
> req->rq_timeout = to->to_initval;
> req->rq_retries = 0;
> - xprt_reset_majortimeo(req);
> + xprt_reset_majortimeo(req, to);
> /* Reset the RTT counters == "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 = NULL;
> req->rq_rcv_buf.bvec = NULL;
> req->rq_release_snd_buf = 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, struct rpc_task *task)
> */
> xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
> xbufp->tail[0].iov_len;
> +
> + xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
> }
> #endif
>
> --
> 2.43.0
>
>
--
Chuck Lever
On Thu, 2024-01-04 at 09:58 -0500, Benjamin Coddington wrote:
> After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
> the sending list"), any 4.1 backchannel tasks placed on the sending queue
> would immediately return with -ETIMEDOUT since their req timers are zero.
>
> 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 the sending list")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> 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(ktime_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 = req->rq_task->tk_client->cl_timeout;
> unsigned long majortimeo = 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 += xprt_calc_majortimeo(req);
> + req->rq_majortimeo += 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 += req->rq_timeout;
> }
>
> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
> + const struct rpc_timeout *to)
> {
> unsigned long time_init;
> struct rpc_xprt *xprt = req->rq_xprt;
> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> time_init = jiffies;
> else
> time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
> - req->rq_timeout = task->tk_client->cl_timeout->to_initval;
> - req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
> +
> + req->rq_timeout = to->to_initval;
> + req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
> req->rq_minortimeo = time_init + req->rq_timeout;
> }
>
> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> } else {
> req->rq_timeout = to->to_initval;
> req->rq_retries = 0;
> - xprt_reset_majortimeo(req);
> + xprt_reset_majortimeo(req, to);
> /* Reset the RTT counters == "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 = NULL;
> req->rq_rcv_buf.bvec = NULL;
> req->rq_release_snd_buf = 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, struct rpc_task *task)
> */
> xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
> xbufp->tail[0].iov_len;
> +
> + xprt_init_majortimeo(task, req, req->rq_xprt->timeout);
> }
> #endif
>
This seems to fix the problems I was seeing with TLS.
For both patches:
Tested-by: Jeff Layton <[email protected]>