Re: [PATCH 1/1] SUNRPC dont update timeout value on connection reset

[Olga Kornievskaia <olga.kornievskaia@gmail.com>]

On Thu, Jul 9, 2020 at 1:19 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Thu, 2020-07-09 at 11:43 -0400, Olga Kornievskaia wrote:
> > On Thu, Jul 9, 2020 at 8:08 AM Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > > Hi Olga
> > >
> > > On Wed, 2020-07-08 at 17:05 -0400, Olga Kornievskaia wrote:
> > > > Current behaviour: every time a v3 operation is re-sent to the
> > > > server
> > > > we update (double) the timeout. There is no distinction between
> > > > whether
> > > > or not the previous timer had expired before the re-sent
> > > > happened.
> > > >
> > > > Here's the scenario:
> > > > 1. Client sends a v3 operation
> > > > 2. Server RST-s the connection (prior to the timeout) (eg.,
> > > > connection
> > > > is immediately reset)
> > > > 3. Client re-sends a v3 operation but the timeout is now 120sec.
> > > >
> > > > As a result, an application sees 2mins pause before a retry in
> > > > case
> > > > server again does not reply.
> > > >
> > > > Instead, this patch proposes to keep track off when the minor
> > > > timeout
> > > > should happen and if it didn't, then don't update the new
> > > > timeout.
> > > >
> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > ---
> > > >  include/linux/sunrpc/xprt.h |  1 +
> > > >  net/sunrpc/xprt.c           | 11 +++++++++++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/include/linux/sunrpc/xprt.h
> > > > b/include/linux/sunrpc/xprt.h
> > > > index e64bd82..a603d48 100644
> > > > --- a/include/linux/sunrpc/xprt.h
> > > > +++ b/include/linux/sunrpc/xprt.h
> > > > @@ -101,6 +101,7 @@ struct rpc_rqst {
> > > >                                                        * used in
> > > > the
> > > > softirq.
> > > >                                                        */
> > > >       unsigned long           rq_majortimeo;  /* major timeout
> > > > alarm */
> > > > +     unsigned long           rq_minortimeo;  /* minor timeout
> > > > alarm */
> > > >       unsigned long           rq_timeout;     /* Current timeout
> > > > value */
> > > >       ktime_t                 rq_rtt;         /* round-trip time
> > > > */
> > > >       unsigned int            rq_retries;     /* # of retries */
> > > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > > index d5cc5db..c0ce232 100644
> > > > --- a/net/sunrpc/xprt.c
> > > > +++ b/net/sunrpc/xprt.c
> > > > @@ -607,6 +607,11 @@ static void xprt_reset_majortimeo(struct
> > > > rpc_rqst *req)
> > > >       req->rq_majortimeo += xprt_calc_majortimeo(req);
> > > >  }
> > > >
> > > > +static void xprt_reset_minortimeo(struct rpc_rqst *req)
> > > > +{
> > > > +     req->rq_minortimeo = jiffies + req->rq_timeout;
> > > > +}
> > > > +
> > > >  static void xprt_init_majortimeo(struct rpc_task *task, struct
> > > > rpc_rqst *req)
> > > >  {
> > > >       unsigned long time_init;
> > > > @@ -618,6 +623,7 @@ static void xprt_init_majortimeo(struct
> > > > rpc_task
> > > > *task, struct rpc_rqst *req)
> > > >               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_minortimeo = time_init + req->rq_timeout;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -631,6 +637,10 @@ int xprt_adjust_timeout(struct rpc_rqst
> > > > *req)
> > > >       const struct rpc_timeout *to = req->rq_task->tk_client-
> > > > > cl_timeout;
> > > >       int status = 0;
> > > >
> > > > +     if (time_before(jiffies, req->rq_minortimeo)) {
> > > > +             xprt_reset_minortimeo(req);
> > > > +             return status;
> > >
> > > Shouldn't this case be just returning without updating the timeout?
> > > After all, this is the case where nothing has expired yet.
> >
> > I think we perhaps should readjust the minor timeout every here but I
> > can't figure out what the desired behaviour should be. When should we
> > consider it's appropriate to double the timer. Consider the
> > following:
> >
> > time1: v3 op sent
> > time1+50s: server RSTs
> > We check that it's not yet the minor timeout (time1+60s)
> > time1+50s: v3 op re-sent  (say we don't reset the minor timeout to be
> > current time+60s)
> > time1+60s: server RSTs
> > Client will resend the op but now it's past the initial minor timeout
> > so the timeout will be doubled. Is that what we really want? Maybe it
> > is.
> > Say now the server RSTs the connection again (shortly after or in
> > less
> > than 60s), since we are not updating the minor timeout value, then
> > the
> > client will again modify the timeout before resending. Is that Ok?
> >
> > That's why my reasoning was that at every re-evaluation of the
> > timeout
> > value, we have the minor timeout set for current time+60s and we get
> > an RST within it then we don't modify the timeout value.
>
> So a couple of issues with that:
>
> The first is that a series of RST calls could cause the timeout to get
> pushed to the max value fairly quickly (btw, xprt_reset_minortimeo()
> does not enforce a limit right now).
>
> The second is that we end up pushing out the major timeout value, since
> the major timeout cannot occur unless the value of jiffies is after the
> minor timeout (which keeps changing on each pass).

But dont we want to push out the major timeout?

Actually i think, back in my example of getting the RST, at
(time1+50s). shouldn't minor_timeo and majortimeo be reset to
currenttime+appropriate value of minor/major?  If we are evaluating
the timer and the time difference between when the operation was sent
and now is less than 60s, we shouldn't say a timeout has occurried
(it's a pre-mature timeout) and thus its value shouldn't be modified.

Thoughts?


>
> >
> >
> > > > +     }
> > > >       if (time_before(jiffies, req->rq_majortimeo)) {
> > > >               if (to->to_exponential)
> > > >                       req->rq_timeout <<= 1;
> > > > @@ -638,6 +648,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> > > >                       req->rq_timeout += to->to_increment;
> > > >               if (to->to_maxval && req->rq_timeout >= to-
> > > > >to_maxval)
> > > >                       req->rq_timeout = to->to_maxval;
> > > > +             xprt_reset_minortimeo(req);
> > >
> > > ...and then perhaps this can just be moved out of the time_before()
> > > condition, since it looks to me as if we also want to reset req-
> > > > rq_minortimeo when a major timeout occurs.
> > > >               req->rq_retries++;
> > > >       } else {
> > > >               req->rq_timeout = to->to_initval;
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>