[linux-cifs] Re: [PATCH 04/11] CIFS: Delete echo_retries module parm


2012/2/22 Jeff Layton :
> On Wed, 22 Feb 2012 10:32:57 +0300
> Pavel Shilovsky  wrote:
>
>> It's the essential step before respecting MaxMpxCount value during
>> negotiating because we will keep only one extra slot for sending
>> echo requests. If there is no response for echo message - reconnect
>> the tcp session.
>>
>> Signed-off-by: Pavel Shilovsky 
>> ---
>> fs/cifs/README   |  6 +-----
>> fs/cifs/cifsfs.c  |  5 -----
>> fs/cifs/cifsglob.h |  3 ---
>> fs/cifs/connect.c |  7 +++----
>> 4 files changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/cifs/README b/fs/cifs/README
>> index 895da1d..b7d782b 100644
>> --- a/fs/cifs/README
>> +++ b/fs/cifs/README
>> @@ -753,10 +753,6 @@ module loading or during the runtime by using the interface
>>
>> i.e. echo "value" > /sys/module/cifs/parameters/
>>
>> -1. echo_retries - The number of echo attempts before giving up and
>> -        reconnecting to the server. The default is 5. The value 0
>> -        means never reconnect.
>> -
>> -2. enable_oplocks - Enable or disable oplocks. Oplocks are enabled by default.
>> +1. enable_oplocks - Enable or disable oplocks. Oplocks are enabled by default.
>>          [Y/y/1]. To disable use any of [N/n/0].
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index b1fd382..e1ce1ca 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -77,11 +77,6 @@ unsigned int cifs_max_pending = CIFS_MAX_REQ;
>> module_param(cifs_max_pending, int, 0444);
>> MODULE_PARM_DESC(cifs_max_pending, "Simultaneous requests to server. "
>>                 "Default: 50 Range: 2 to 256");
>> -unsigned short echo_retries = 5;
>> -module_param(echo_retries, ushort, 0644);
>> -MODULE_PARM_DESC(echo_retries, "Number of echo attempts before giving up and "
>> -              "reconnecting server. Default: 5. 0 means "
>> -              "never reconnect.");
>> module_param(enable_oplocks, bool, 0644);
>> MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks (bool). Default:"
>>                "y/Y/1");
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 600d61c..674792e 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -1049,9 +1049,6 @@ GLOBAL_EXTERN unsigned int cifs_min_rcv;  /* min size of big ntwrk buf pool */
>> GLOBAL_EXTERN unsigned int cifs_min_small; /* min size of small buf pool */
>> GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/
>>
>> -/* reconnect after this many failed echo attempts */
>> -GLOBAL_EXTERN unsigned short echo_retries;
>> -
>> #ifdef CONFIG_CIFS_ACL
>> GLOBAL_EXTERN struct rb_root uidtree;
>> GLOBAL_EXTERN struct rb_root gidtree;
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 673813f..86d7a40 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -373,12 +373,11 @@ allocate_buffers(struct TCP_Server_Info *server)
>> static bool
>> server_unresponsive(struct TCP_Server_Info *server)
>> {
>> -   if (echo_retries > 0 && server->tcpStatus == CifsGood &&
>> -     time_after(jiffies, server->lstrp +
>> -               (echo_retries * SMB_ECHO_INTERVAL))) {
>> +   if (server->tcpStatus == CifsGood &&
>> +     time_after(jiffies, server->lstrp + SMB_ECHO_INTERVAL)) {
>>        cERROR(1, "Server %s has not responded in %d seconds. "
>>             "Reconnecting...", server->hostname,
>> -            (echo_retries * SMB_ECHO_INTERVAL / HZ));
>> +            SMB_ECHO_INTERVAL / HZ);
>
> This will be a problem since we skip sending an echo when the client
> has gotten a response recently (within the last echo interval).
>
> Consider:
>
> 1s   client sends a normal smb request
> 2s   client gets a response
> 30s   echo workqueue job pops, and decides we got a response
>    recently and don't need to send another
> ...
> 65s   kernel_recvmsg times out, and we see that we haven't gotten
>    a response in >60s
>
> ...I think you'll probably want to only reconnect when we haven't
> gotten a response in 2 echo intervals? Or maybe there's a better way?
>

Seems like 2 intervals is good for now. We can change this later, if
we find a better way.

-- 
Best regards,
Pavel Shilovsky.

This message from: http://www.mailbrowse.com/linux-cifs/5509.html
Previous message: Re: [linux-cifs-client] [PATCH] cifs: hard mount option behaviour implementation
Next message:Re: [PATCH 05/11] CIFS: Make wait_for_free_request interruptible