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

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

On Thu, Jul 9, 2020 at 5:07 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> 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?

Do you feel that the following approach is incorrect? Sry it's just
cut-and-paste but the logic is there. Thank you.

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..66d412b 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,11 @@ 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)) {
+ req->rq_majortimeo = jiffies + xprt_calc_majortimeo(req);
+ req->rq_minortimeo = jiffies + req->rq_timeout;
+ return status;
+ }
  if (time_before(jiffies, req->rq_majortimeo)) {
  if (to->to_exponential)
  req->rq_timeout <<= 1;
@@ -649,6 +660,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
  spin_unlock(&xprt->transport_lock);
  status = -ETIMEDOUT;
  }
+ xprt_reset_minortimeo(req);

  if (req->rq_timeout == 0) {
  printk(KERN_WARNING "xprt_adjust_timeout: rq_timeout = 0!\n");
-- 

> > > > > +     }
> > > > >       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
> >
> >