[PATCH cifs-next] fs: cifs: make smb_echo_interval tunable

[PATCH cifs-next] fs: cifs: make smb_echo_interval tunable

[Chris J Arges]

Change SMB_ECHO_INTERVAL to make it a module parameter.

BugLink: http://bugs.launchpad.net/bugs/1017622
BugLink: https://bugzilla.samba.org/show_bug.cgi?id=9006

Reported-by: Oliver Dumschat-Hoette <dumschat-hoette@trisinus.de>
Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5e62f44..25748b3 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -82,6 +82,11 @@ MODULE_PARM_DESC(cifs_max_pending, "Simultaneous requests to server. "
 module_param(enable_oplocks, bool, 0644);
 MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks. Default: y/Y/1");
 
+unsigned short smb_echo_timeout = 60;
+module_param(smb_echo_timeout, ushort, 0644);
+MODULE_PARM_DESC(smb_echo_timeout, "Timeout between two echo requests. "
+				   "Default: 60. Timeout in seconds ");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f5af252..d64dcd3 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -78,8 +78,9 @@
 /*           (max path length + 1 for null) * 2 for unicode    */
 #define MAX_NAME 514
 
-/* SMB echo "timeout" -- FIXME: tunable? */
-#define SMB_ECHO_INTERVAL (60 * HZ)
+/* SMB echo "timeout" */
+extern unsigned short smb_echo_timeout;
+#define SMB_ECHO_INTERVAL (smb_echo_timeout * HZ)
 
 #include "cifspdu.h"
 
-- 
1.7.10.4

Re: [PATCH cifs-next] fs: cifs: make smb_echo_interval tunable

[Jeff Layton]

On Thu,  8 Nov 2012 14:50:28 -0600
Chris J Arges <chris.j.arges@canonical.com> wrote:

> Change SMB_ECHO_INTERVAL to make it a module parameter.
> 
> BugLink: http://bugs.launchpad.net/bugs/1017622
> BugLink: https://bugzilla.samba.org/show_bug.cgi?id=9006
> 

It's customary to write up the reason for a change in the bug
description. A pointer to a bug tracker is nice as a reference, but
it's not reasonable to expect someone to chase down a bunch of bug
tracker links when reading the git logs. It's also possible that
these bug reports could go away or be unavailable when someone needs
to track down the reason for a change.

> Reported-by: Oliver Dumschat-Hoette <dumschat-hoette@trisinus.de>
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  fs/cifs/cifsfs.c   |    5 +++++
>  fs/cifs/cifsglob.h |    5 +++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 5e62f44..25748b3 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -82,6 +82,11 @@ MODULE_PARM_DESC(cifs_max_pending, "Simultaneous requests to server. "
>  module_param(enable_oplocks, bool, 0644);
>  MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks. Default: y/Y/1");
>  
> +unsigned short smb_echo_timeout = 60;
> +module_param(smb_echo_timeout, ushort, 0644);
> +MODULE_PARM_DESC(smb_echo_timeout, "Timeout between two echo requests. "
> +				   "Default: 60. Timeout in seconds ");
> +
>  extern mempool_t *cifs_sm_req_poolp;
>  extern mempool_t *cifs_req_poolp;
>  extern mempool_t *cifs_mid_poolp;
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f5af252..d64dcd3 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -78,8 +78,9 @@
>  /*           (max path length + 1 for null) * 2 for unicode    */
>  #define MAX_NAME 514
>  
> -/* SMB echo "timeout" -- FIXME: tunable? */
> -#define SMB_ECHO_INTERVAL (60 * HZ)
> +/* SMB echo "timeout" */
> +extern unsigned short smb_echo_timeout;
> +#define SMB_ECHO_INTERVAL (smb_echo_timeout * HZ)
>  
>  #include "cifspdu.h"
>  

I'm not so sure I like making this tunable...

What would really be better is fixing the code to only echo when there
are outstanding calls to the server.

This also seems like a bit of a kludgy workaround for the real problem.
>From looking over the bug reports, it sounds like the real issue is
that the server is timing out the connection while the client is
suspended. It then has to wait until the next echo comes around to
figure that out. Is that the case?

If so, then I think what you really want here is a way to tell if the
connection is still valid after resume. Perhaps there's some way to
force an immediate SMB echo on these connections after resume? With
that, there'd be little delay at all in contacting the server after a
resume. The server would presumably send back a RST immediately in such
a case and we could get on with the business of reconnecting.

The cifs_demultiplex_thread does make some calls to try_to_freeze().
Perhaps checking the return value on those and kicking off an immediate
echo if it returns true would be a better solution?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH cifs-next] fs: cifs: make smb_echo_interval tunable

[Chris J Arges]

On 11/09/2012 05:00 AM, Jeff Layton wrote:
> On Thu,  8 Nov 2012 14:50:28 -0600
> Chris J Arges <chris.j.arges@canonical.com> wrote:
> 
>> Change SMB_ECHO_INTERVAL to make it a module parameter.
>>
>> BugLink: http://bugs.launchpad.net/bugs/1017622
>> BugLink: https://bugzilla.samba.org/show_bug.cgi?id=9006
>>
> 
> It's customary to write up the reason for a change in the bug
> description. A pointer to a bug tracker is nice as a reference, but
> it's not reasonable to expect someone to chase down a bunch of bug
> tracker links when reading the git logs. It's also possible that
> these bug reports could go away or be unavailable when someone needs
> to track down the reason for a change.
> 
Hi,
Ok I'll fix this in the next version to include a summary of the bug.

>> Reported-by: Oliver Dumschat-Hoette <dumschat-hoette@trisinus.de>
>> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
>> ---
>>  fs/cifs/cifsfs.c   |    5 +++++
>>  fs/cifs/cifsglob.h |    5 +++--
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index 5e62f44..25748b3 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -82,6 +82,11 @@ MODULE_PARM_DESC(cifs_max_pending, "Simultaneous requests to server. "
>>  module_param(enable_oplocks, bool, 0644);
>>  MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks. Default: y/Y/1");
>>  
>> +unsigned short smb_echo_timeout = 60;
>> +module_param(smb_echo_timeout, ushort, 0644);
>> +MODULE_PARM_DESC(smb_echo_timeout, "Timeout between two echo requests. "
>> +				   "Default: 60. Timeout in seconds ");
>> +
>>  extern mempool_t *cifs_sm_req_poolp;
>>  extern mempool_t *cifs_req_poolp;
>>  extern mempool_t *cifs_mid_poolp;
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index f5af252..d64dcd3 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -78,8 +78,9 @@
>>  /*           (max path length + 1 for null) * 2 for unicode    */
>>  #define MAX_NAME 514
>>  
>> -/* SMB echo "timeout" -- FIXME: tunable? */
>> -#define SMB_ECHO_INTERVAL (60 * HZ)
>> +/* SMB echo "timeout" */
>> +extern unsigned short smb_echo_timeout;
>> +#define SMB_ECHO_INTERVAL (smb_echo_timeout * HZ)
>>  
>>  #include "cifspdu.h"
>>  
> 
> I'm not so sure I like making this tunable...
> 
Ok, I saw the 'FIXME: tunable?', and thought this was something that
could be exposed as a parameter in the future.

> What would really be better is fixing the code to only echo when there
> are outstanding calls to the server.
> 
> This also seems like a bit of a kludgy workaround for the real problem.
> From looking over the bug reports, it sounds like the real issue is
> that the server is timing out the connection while the client is
> suspended. It then has to wait until the next echo comes around to
> figure that out. Is that the case?
> 
> If so, then I think what you really want here is a way to tell if the
> connection is still valid after resume. Perhaps there's some way to
> force an immediate SMB echo on these connections after resume? With
> that, there'd be little delay at all in contacting the server after a
> resume. The server would presumably send back a RST immediately in such
> a case and we could get on with the business of reconnecting.
> 
> The cifs_demultiplex_thread does make some calls to try_to_freeze().
> Perhaps checking the return value on those and kicking off an immediate
> echo if it returns true would be a better solution?
> 
Great, I'll set up the test environment again and attempt a patch that
does this.

Thanks for the feedback.
--chris j arges

Re: [PATCH cifs-next] fs: cifs: make smb_echo_interval tunable

[Jeff Layton]

On Fri, 09 Nov 2012 09:40:37 -0600
Chris J Arges <chris.j.arges@canonical.com> wrote:

> > 
> > I'm not so sure I like making this tunable...
> > 
> Ok, I saw the 'FIXME: tunable?', and thought this was something that
> could be exposed as a parameter in the future.
> 

Yeah, when I originally wrote that code I put that comment in. Since
then I think we've figured out more about how the echo facility in CIFS
should work. Making it tunable is not really a great solution. Having
the echoes better adapt to the situation would be preferable.

-- 
Jeff Layton <jlayton@redhat.com>