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

[Chris J Arges <chris.j.arges-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> 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-cpG9vB2psbIb1SvskN2V4Q@public.gmane.org>
>> Signed-off-by: Chris J Arges <chris.j.arges-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>>  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