Received: by 2002:a05:6902:102b:0:0:0:0 with SMTP id x11csp721126ybt; Fri, 10 Jul 2020 10:37:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwnIxQH+34QE7w4tD4u7NqAJnasa3qYTELsNrlVMsf/Ps3pn8IeB700p0pGSYF1SFvY/FxN X-Received: by 2002:a17:906:f2c4:: with SMTP id gz4mr61052441ejb.484.1594402623803; Fri, 10 Jul 2020 10:37:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1594402623; cv=none; d=google.com; s=arc-20160816; b=Qmvd63j6L47GbOC9wv36qh7nJtd70G4LZP0CUyyh0XtR+XhmBoqOjgKsca+kOnbZ/w R3Yro2yGktP7dLccqG88sr6jQ9Cxk4BFNLKO2FLbfxMePJSFXrwYQeJdy0mqvneegPMy A5mmlGnjmzrQiuJ5n3N/VjP53HtSlZnv3SWLtIYtHLwM7zvIoJq81Q8L62R0JReVqYt2 J5VULu2nGtvgLAxx67CHevBbuyuuLKqRDN6ExsgdNH7RkLu06xHbTVuv8YKjk4ju59vS M9q9VQJV29+DNOa4Lg4FwDY9yL1/2e3tapmHeaxp8pnFqQ5/OGqz1ipEPPBpkAh/JiIk LHYQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=E3z4mb9I0JzaoTH4m+ecKp4hGqOt1eKV4Y931PVR9VM=; b=kE40cGAIqgal9/rF257wzct/pVMtHQrF4uVnpovruzw8PZSHjqfGUO9fPadZwq+9NF up2kknnrC8j1pgODlVWV9inZ2WeJFB9gLo7Dv3nHe/sBsgionfanOeCuP7fTNJyaqNfJ glXW+4SozRhwry5W/EgEde9sY4b/KLDKeVd1YozrZSGERd4Afkyl/CslmN8PW96uKcaD m6J5S37p3H1NXRDDvdB6OPZMu5m06HNnZFe+BzrtRJ4BagfE7wp6NpHJfkXHjaL3v6HC g6SuolWKmc9DZd09Jm/4eCbq+tVetXPetBl1KSctBj3SS4AGvScGQkCD1TgLU0WOl4bv qhrw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=uluJoM1v; spf=pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id p7si4154665edm.514.2020.07.10.10.36.29; Fri, 10 Jul 2020 10:37:03 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=uluJoM1v; spf=pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727078AbgGJRgJ (ORCPT + 99 others); Fri, 10 Jul 2020 13:36:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44596 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726977AbgGJRgJ (ORCPT ); Fri, 10 Jul 2020 13:36:09 -0400 Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C1D44C08C5DC for ; Fri, 10 Jul 2020 10:36:08 -0700 (PDT) Received: by mail-wm1-x342.google.com with SMTP id g75so6956112wme.5 for ; Fri, 10 Jul 2020 10:36:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=E3z4mb9I0JzaoTH4m+ecKp4hGqOt1eKV4Y931PVR9VM=; b=uluJoM1v3d49sv0LLtmfS+R2uZ76IzUhUmEN2c07H0RWz6HHsZf6uUgD18QFE1/KB1 SVYUx7uAeTCKCvfKY344ol4icHw7gIGLhM5lx7ubkwtfJkasIIulsf3N8X8ReRyrzBoM ZGelb/fyTvpBQh4fhudy3iBOtYdGxPyPhZR+dWpc+h7RPdWOveihR0s4CawUXhEN43Lj /zQ9bhnWGBKkPPvhaVOFIIqWzdcwbzqviMJSYA04saVD3O3II+0qLq0FqYJRFVWMyPd+ 7MqIFo4PTISkPAMje+lKWoUj+ehxk7XEB2bcIJRHSE7ljP/Dd49MebiszJKSp0SdM7M6 XQuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=E3z4mb9I0JzaoTH4m+ecKp4hGqOt1eKV4Y931PVR9VM=; b=NlWFiNkevT8a6NmRud/l1SGhNmAWLBgiYiLCY84P7JfUx2CZLAmw8fQliY4wCkEliF eQZDoc86IqY3xjeynkcuMIBZxTQI9ZFKYCTdoI0txaj2p/kLrf5WbQGPu+2ks4fpD2gY 6Xv++1U40AvafutGsXDjsqr0oqR+QckG/d00AfGlQB8aUJfikjIBUweoMxZOIDQaeOCW Jcd8s7sa3NfT3MlZHciw/80hXVcHzm4aVeeKY+sUdA+sA+vys0nXgJgrcz4t9EIYFrKf ohlWbPEEUWBPed+k3hRfgaxb1a+ZJX2MMjNu+y3TxEmCZwRVjG9we0tCZxNyhI+Zwmuq 0A1A== X-Gm-Message-State: AOAM5321JagrXii536y6Mzz18o6Ne3GtaWtAI3DjShOECRLvP1PcvyP/ xUtMZTem12iMyf6Udhb5c5nBKnYV5o1YtN0HBiF+VFPa X-Received: by 2002:a05:600c:2249:: with SMTP id a9mr5894047wmm.163.1594402567345; Fri, 10 Jul 2020 10:36:07 -0700 (PDT) MIME-Version: 1.0 References: <20200708210514.84671-1-olga.kornievskaia@gmail.com> <41873966ea839cca97332df3c56612441f840e0d.camel@hammerspace.com> <3fe49121d027eaa3aa2263f24d76d72e750d8592.camel@hammerspace.com> In-Reply-To: From: Olga Kornievskaia Date: Fri, 10 Jul 2020 13:35:55 -0400 Message-ID: Subject: Re: [PATCH 1/1] SUNRPC dont update timeout value on connection reset To: Trond Myklebust Cc: "linux-nfs@vger.kernel.org" , "anna.schumaker@netapp.com" Content-Type: text/plain; charset="UTF-8" Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org On Thu, Jul 9, 2020 at 5:07 PM Olga Kornievskaia wrote: > > On Thu, Jul 9, 2020 at 1:19 PM Trond Myklebust 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 > > > > > --- > > > > > 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 > > > >