| Message ID | 1bcb93ab5feefca853b95a1759da5b008c204092.1702063105.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] SUNRPC: Fixup v4.1 backchannel request timeouts | expand |
On Fri, 2023-12-08 at 14:19 -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. > 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 <bcodding@redhat.com> > --- > include/linux/sunrpc/xprt.h | 1 + > net/sunrpc/clnt.c | 3 +++ > net/sunrpc/xprt.c | 23 ++++++++++++++--------- > 3 files changed, 18 insertions(+), 9 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 Hmm... The xprt can and will be shared among a number of rpc_clnt instances. I therefore think we're better off doing this when we're setting up the back channel. i.e. probably doing it in nfs41_init_clientid() after we picked up the lease time (but before we mark the client as ready), and then doing it in nfs4_proc_bind_conn_to_session() if ever that gets called. Note that we have to set the bc_timeout on all xprts that could act as back channels, so you might want to use rpc_clnt_iterate_for_each_xprt(). It might also be worth to look at Olga's trunking code, since I suspect we might need to do something there when adding a new xprt to the existing set. > 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..7dce780869fc 100644 > --- a/net/sunrpc/xprt.c > +++ b/net/sunrpc/xprt.c > @@ -653,9 +653,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) > @@ -667,9 +667,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) > @@ -677,7 +678,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; > @@ -686,8 +688,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; > } > > @@ -715,7 +718,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); > @@ -1888,7 +1891,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); > } > @@ -1998,6 +2001,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->bc_timeout); > } > #endif > Otherwise, looks good.
On 9 Dec 2023, at 4:55, Trond Myklebust wrote: > On Fri, 2023-12-08 at 14:19 -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. >> 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 <bcodding@redhat.com> >> --- >> include/linux/sunrpc/xprt.h | 1 + >> net/sunrpc/clnt.c | 3 +++ >> net/sunrpc/xprt.c | 23 ++++++++++++++--------- >> 3 files changed, 18 insertions(+), 9 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 > > Hmm... The xprt can and will be shared among a number of rpc_clnt > instances. I therefore think we're better off doing this when we're > setting up the back channel. .. and it seems the timeouts could be different for each, so now I think keeping a copy of the last rpc_clnt's timeouts on the xprt is wrong. We could use the current timeouts from the nfs_client side after we figure out which nfs_client that would be in nfs4_callback_compound(). Trouble is if we have to make a reply before getting that far there's still no timeout set, but that's probably a rare case and could use the xprt type defaults. > i.e. probably doing it in nfs41_init_clientid() after we picked up the > lease time (but before we mark the client as ready), and then doing it > in nfs4_proc_bind_conn_to_session() if ever that gets called. > > Note that we have to set the bc_timeout on all xprts that could act as > back channels, so you might want to use > rpc_clnt_iterate_for_each_xprt(). > It might also be worth to look at Olga's trunking code, since I suspect > we might need to do something there when adding a new xprt to the > existing set. Ben
On Mon, 2023-12-11 at 06:53 -0500, Benjamin Coddington wrote: > On 9 Dec 2023, at 4:55, Trond Myklebust wrote: > > > On Fri, 2023-12-08 at 14:19 -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. > > > 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 <bcodding@redhat.com> > > > --- > > > include/linux/sunrpc/xprt.h | 1 + > > > net/sunrpc/clnt.c | 3 +++ > > > net/sunrpc/xprt.c | 23 ++++++++++++++--------- > > > 3 files changed, 18 insertions(+), 9 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 > > > > Hmm... The xprt can and will be shared among a number of rpc_clnt > > instances. I therefore think we're better off doing this when we're > > setting up the back channel. > > .. and it seems the timeouts could be different for each, so now I > think > keeping a copy of the last rpc_clnt's timeouts on the xprt is wrong. > > We could use the current timeouts from the nfs_client side after we > figure out which nfs_client that would be in > nfs4_callback_compound(). > Trouble is if we have to make a reply before getting that far there's > still no timeout set, but that's probably a rare case and could use > the xprt > type defaults. The suggestion is that we consider the callback channel to be associated with the 'service rpc_client', i.e. the one that is assigned to the nfs_client and that is used for lease recovery, etc. If you set it up after the negotiation of the session, and after picking up the lease time, then you should have a useful value. There will be no other client activity anyway until the client is marked as being ready, so you can shoehorn it in there (in nfs41_init_clientid() as suggested below) to address the common case. Then there is the issue of bind_conn_to_session (when we're setting up the callback channel anew because the server lost it) and the cases where we're adding new xprts. > > > i.e. probably doing it in nfs41_init_clientid() after we picked up > > the > > lease time (but before we mark the client as ready), and then doing > > it > > in nfs4_proc_bind_conn_to_session() if ever that gets called. > > > > Note that we have to set the bc_timeout on all xprts that could act > > as > > back channels, so you might want to use > > rpc_clnt_iterate_for_each_xprt(). > > It might also be worth to look at Olga's trunking code, since I > > suspect > > we might need to do something there when adding a new xprt to the > > existing set. > > Ben >
On 11 Dec 2023, at 8:54, Trond Myklebust wrote: > On Mon, 2023-12-11 at 06:53 -0500, Benjamin Coddington wrote: >> On 9 Dec 2023, at 4:55, Trond Myklebust wrote: >> >>> On Fri, 2023-12-08 at 14:19 -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. >>>> 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 <bcodding@redhat.com> >>>> --- >>>> include/linux/sunrpc/xprt.h | 1 + >>>> net/sunrpc/clnt.c | 3 +++ >>>> net/sunrpc/xprt.c | 23 ++++++++++++++--------- >>>> 3 files changed, 18 insertions(+), 9 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 >>> >>> Hmm... The xprt can and will be shared among a number of rpc_clnt >>> instances. I therefore think we're better off doing this when we're >>> setting up the back channel. >> >> .. and it seems the timeouts could be different for each, so now I >> think >> keeping a copy of the last rpc_clnt's timeouts on the xprt is wrong. >> >> We could use the current timeouts from the nfs_client side after we >> figure out which nfs_client that would be in >> nfs4_callback_compound(). >> Trouble is if we have to make a reply before getting that far there's >> still no timeout set, but that's probably a rare case and could use >> the xprt >> type defaults. > > The suggestion is that we consider the callback channel to be > associated with the 'service rpc_client', i.e. the one that is assigned > to the nfs_client and that is used for lease recovery, etc. Right, I'm on the same page - there's just no place to keep the timeout currently. We can't just carry it on the xprt since the xprt can be used for various clients, and there's multiple xprts to handle too. > If you set it up after the negotiation of the session, and after > picking up the lease time, then you should have a useful value. There > will be no other client activity anyway until the client is marked as > being ready, so you can shoehorn it in there (in nfs41_init_clientid() > as suggested below) to address the common case. > > Then there is the issue of bind_conn_to_session (when we're setting up > the callback channel anew because the server lost it) and the cases > where we're adding new xprts. I think all these cases can be avoided by setting the callback responses' req timeout values during callback processing after we've looked up the appropriate nfs_client for the response. Then we can just use the cl_timeout, no need to touch the xprts. Otherwise, I'm looking at every xprt having a list of bc_timeouts for every rpc_client or some mess like that. I really appreciate your suggestions, Ben
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..7dce780869fc 100644 --- a/net/sunrpc/xprt.c +++ b/net/sunrpc/xprt.c @@ -653,9 +653,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) @@ -667,9 +667,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) @@ -677,7 +678,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; @@ -686,8 +688,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; } @@ -715,7 +718,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); @@ -1888,7 +1891,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); } @@ -1998,6 +2001,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->bc_timeout); } #endif
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 <bcodding@redhat.com> --- include/linux/sunrpc/xprt.h | 1 + net/sunrpc/clnt.c | 3 +++ net/sunrpc/xprt.c | 23 ++++++++++++++--------- 3 files changed, 18 insertions(+), 9 deletions(-)