lkml.org 
[lkml]   [2012]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH cifs-next] fs: cifs: make smb_echo_interval tunable
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




\
 
 \ /
  Last update: 2012-11-09 17:21    [W:0.052 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site