All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chuck Lever <chuck.lever@oracle.com>
To: "Batsakis, Alexandros" <Alexandros.Batsakis@netapp.com>
Cc: Alexandros Batsakis <batsakis@netapp.com>,
	linux-nfs@vger.kernel.org, trond@netapp.com
Subject: Re: [PATCH 6/6] RPC: adjust timeout for connect, bind, restablish so that they sensitive to the major time out value
Date: Fri, 05 Feb 2010 19:11:51 -0500	[thread overview]
Message-ID: <4B6CB3C7.8070001@oracle.com> (raw)
In-Reply-To: <77EBFB14-A6B6-41DC-90DC-7A00548DFAEA@netapp.com>

On 02/05/2010 06:04 PM, Batsakis, Alexandros wrote:
>
>
> On Feb 5, 2010, at 14:47, "Chuck Lever" <chuck.lever@oracle.com> wrote:
>
>> On 02/05/2010 05:14 PM, Batsakis, Alexandros wrote:
>>> Yeah sure,
>>>
>>> So imagine that for a specific connection the remaining major timeo
>>> value is 30secs. Xs_connect has a default timeout before attempting to
>>> reconnect of 60secs. The user (NFS) expects to "hear back" from the rpc
>>> layer within the timeout as in often cases e.g. lease renewal, it's of
>>> no benefit for an operation to reach the server at a later time and miss
>>> the critical time because it was sleeping for an arbitrary amount of
>>> time.

Maybe you want RPC_TASK_SOFTCONN for NFSv4 renewals instead of 
RPC_TASK_SOFT.  This would cause the RENEW request to fail immediately 
if the transport can't connect.

>>>
>>> My patch ensures that the reconnection attempts happen within the
>>> user-specified limits by replacing the 60secs default timeout with a
>>> value that is less than the time to expire. If this is not ideal the
>>> user should readjust the timeout value.
>>>
>>> Does it make sense?
>>
>> That's better, but this explanation needs to accompany the patch, as
>> the long description. It's hard for reviewers who are not familiar
>> with the constraints on NFSv4 RENEW to understand what you're trying
>> to fix. Same comment applies to the other patches in your series, I
>> think. (And see further comments in the code below).
>>
>
> Yeah you are right about that
>
>> You are changing a generic part of the RPC client to fix an issue with
>> one specific NFSv4 procedure, it seems to me. Did you actually observe
>> a situation where the reconnect delay caused the server to miss a
>> RENEW request?
>>
>
> Yeah, it's very easy to reproduce too

Can you describe the scenario more precisely?  If I wanted to reproduce 
this here, how would I do it?

>> There are good reasons why there is a timeout before reestablishing a
>> connection. Have you tested this patch with NFSv3 servers that are
>> going up and down repeatedly, for example? I think skipping the
>> reconnect delay could have consequences for the cases which the
>> original xs_connect logic is supposed to address, and it's not
>> something we want in many other cases.
>>
>
> I am not skipping the reconnect delay. What i am saying is that it seems
> wrong to me to hard-code the reconnection delay. Why 60secs for example
> ? To me it seems that this value should be related to the timeout. Do
> you disagree ?

Hrm.  It looks like the TCP re-establish timeout starts at 3 seconds, 
and then backs off to 5 minutes.  So, it's not fixed, it's related to 
how quickly the server responds to the client's connect() attempt.

The reestablish timer is meant to prevent a client with pending requests 
from hammering an overloaded (or rebooting) server with connection 
requests.  It's purpose is markedly different than the purpose of an 
individual RPC retransmit timer.

>> Perhaps a better idea would be to mark these particular RPCs with some
>> kind of indication that the RPC client has to connect immediately for
>> this request, if possible. Similar to RPC_TASK_SOFTCONN.
>
>> In general, sunrpc.ko has a problem with this kind of "urgent" RPC
>> request. If the RPC backlog queue is large for a particular rpc_clnt,
>> it can often take many seconds (like longer than the major timeout)
>> for a request to actually get down to the transport and get sent. I
>> don't see that these timeout changes necessarily address that at all.
>>
>
> Bu if the timeout has expired rpc_execute will quit the task anyways.
> What is the downside of instead of sleeping for a long, arbitrary period
> to wake up and poll the server at intervals that actually make some
> sense to the client?

If the 5 minute backoff maximum is too long, then you can easily reduce 
that maximum (which is probably not unreasonable).  We originally had 
the client retrying to connect every 3 seconds, but it was thought that 
would put unnecessary load on servers.

-- 
chuck[dot]lever[at]oracle[dot]com

  reply	other threads:[~2010-02-06  0:13 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-02-03  0:06 [PATCH 0/6] nfs: renewd fixes Alexandros Batsakis
2010-02-03  0:06 ` [PATCH 1/6] nfs: kill renewd before clearing client minor version Alexandros Batsakis
2010-02-03  0:06   ` [PATCH 2/6] nfs: prevent backlogging of renewd requests Alexandros Batsakis
2010-02-03  0:06     ` [PATCH 3/6] nfs41: fix race between umount and renewd sequence operations Alexandros Batsakis
2010-02-03  0:06       ` [PATCH 4/6] nfs4: fix race between umount and renewd renew operations Alexandros Batsakis
2010-02-03  0:06         ` [PATCH 5/6] nfs4: adjust rpc timeout for nfs_client rpc client based on the lease_time Alexandros Batsakis
2010-02-03  0:06           ` [PATCH 6/6] RPC: adjust timeout for connect, bind, restablish so that they sensitive to the major time out value Alexandros Batsakis
2010-02-05 20:12             ` Chuck Lever
2010-02-05 22:14               ` Batsakis, Alexandros
2010-02-05 22:45                 ` Chuck Lever
2010-02-05 23:04                   ` Batsakis, Alexandros
2010-02-06  0:11                     ` Chuck Lever [this message]
     [not found]                       ` <B9364369CA66BF45806C2CD86EAB8BA60259D23D@SACMVEXC3-PRD.hq.netapp.com>
2010-02-07  0:53                         ` Chuck Lever
     [not found]                           ` <2CDC4373-10AD-4F84-BA44-3C2106D590BE@netapp.com>
2010-02-08 18:43                             ` Chuck Lever
2010-02-08 23:13                               ` Batsakis, Alexandros

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4B6CB3C7.8070001@oracle.com \
    --to=chuck.lever@oracle.com \
    --cc=Alexandros.Batsakis@netapp.com \
    --cc=batsakis@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trond@netapp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.