On 8 Dec 2023, at 13:16, Trond Myklebust wrote:

On Fri, 2023-12-08 at 11:33 -0500, Benjamin Coddington wrote:

quoted

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.
We can fix this by keeping a copy of the rpc_clnt's timeout params on
the
transport and using them to properly setup the timeouts on the v4.1
backchannel tasks' req.

Fixes: 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
the sending list")
Signed-off-by: Benjamin Coddington <redacted>
---
 include/linux/sunrpc/xprt.h |  1 +
 net/sunrpc/clnt.c           |  3 +++
 net/sunrpc/xprt.c           | 15 +++++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/sunrpc/xprt.h

b/include/linux/sunrpc/xprt.h
index f85d3a0daca2..7565902053f3 100644

--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h

@@ -285,6 +285,7 @@ struct rpc_xprt {

 						 * items */
 	struct list_head	bc_pa_list;	/* List of
preallocated
 						 * backchannel
rpc_rqst's */
+	struct rpc_timeout	bc_timeout;	/* backchannel
timeout params */
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 
 	struct rb_root		recv_queue;	/* Receive queue */

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d6805c1268a7..5891757c88b1 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c

@@ -279,6 +279,9 @@ static struct rpc_xprt

*rpc_clnt_set_transport(struct rpc_clnt *clnt,
 		clnt->cl_autobind = 1;
 
 	clnt->cl_timeout = timeout;
+#if defined(CONFIG_SUNRPC_BACKCHANNEL)
+	memcpy(&xprt->bc_timeout, timeout, sizeof(struct
rpc_timeout));
+#endif
 	rcu_assign_pointer(clnt->cl_xprt, xprt);
 	spin_unlock(&clnt->cl_lock);
 

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 92301e32cda4..d9cbe0814fd8 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c

@@ -655,9 +655,14 @@ static unsigned long

xprt_abs_ktime_to_jiffies(ktime_t abstime)
 
 static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req)
 {
-	const struct rpc_timeout *to = req->rq_task->tk_client-

quoted

cl_timeout;

+	const struct rpc_timeout *to;
 	unsigned long majortimeo = req->rq_timeout;
 
+	if (req->rq_task->tk_client)
+		to = req->rq_task->tk_client->cl_timeout;
+	else
+		to = &req->rq_xprt->bc_timeout;

If you're going to convert this function for generic use, then please
pass the timeout 'to' as a function parameter rather than making
assumptions here.

No problem, I'll send it, but we'll end needing to make the same assumption
calling xprt_reset_majortimeo() from xprt_adjust_timeout().

.. actually it looks like backchannel tasks never currently call
rpc_check_timeout(), so we could just send the rpc_client's rpc_timeout
there, but that looks like a potential future problem.  I'll send a v2 that
way and kick off my testing again.

I always thought that NULL tk_client was definitively backchannel.  Is there
a case you're worried about?

We can fix this another way, probably.  Looks like this fix won't actually
end up implementing normal timeout processing without adding
rpc_check_timeout() calls to the backchannel's tk_actions.

Ben