Re: [PATCH 1/3] cifs: add new module parameter 'enable_oplocks'

[Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>]

On 10/12/2011 12:20 AM, Steve French wrote:
> On Tue, Oct 11, 2011 at 11:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Tue, 11 Oct 2011 21:28:00 +0530
>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>>
>>> Thus spake Jeff Layton:
>>>
>>> "Making that a module parm would allow you to set that parameter at boot
>>> time without needing to add special startup scripts. IMO, all of the
>>> procfile "switches" under /proc/fs/cifs should be module parms
>>> instead."
>>>
>>> This patch doesn't alter the default behavior (Oplocks are enabled by
>>> default).
>>>
>>> To disable oplocks when loading the module, use
>>>
>>>    modprobe cifs enable_oplocks=0
>>>
>>> (any of '0' or 'n' or 'N' conventions can be used).
>>>
>>> To disable oplocks at runtime using the new interface, use
>>>
>>>    echo 0 > /sys/module/cifs/parameters/enable_oplocks
>>>
>>> The older /proc/fs/cifs/OplockEnabled interface will be deprecated
>>> after two releases. A subsequent patch will add an warning message
>>> about the deprecation.
>>>
>>> Reported-by: Alexander Swen <alex-UE4+9DRHXtc@public.gmane.org>
>>> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>>> ---
>>>  fs/cifs/cifsfs.c   |    5 +++++
>>>  fs/cifs/cifsglob.h |    3 +++
>>>  fs/cifs/dir.c      |    2 +-
>>>  fs/cifs/file.c     |    4 ++--
>>>  4 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>>> index 3e29899..675ab40 100644
>>> --- a/fs/cifs/cifsfs.c
>>> +++ b/fs/cifs/cifsfs.c
>>> @@ -81,6 +81,11 @@ 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.");
>>> +unsigned int enable_oplocks = 1;
>>> +module_param(enable_oplocks, bool, 0644);
>>> +MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks (bool). Default:"
>>> +                              "y/Y/1");
>>> +
>>>  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 6255fa8..b3e229c 100644
>>> --- a/fs/cifs/cifsglob.h
>>> +++ b/fs/cifs/cifsglob.h
>>> @@ -936,6 +936,9 @@ 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;
>>>
>>> +/* enable or disable oplocks */
>>> +GLOBAL_EXTERN unsigned int enable_oplocks;
>>> +
>>
>> Let's not add an extra variable for this. I'd just rename oplockEnabled
>> to enable_oplocks and fix up all the references to it. It should
>> probably also be a bool too...

Will make this change.

> Right, leave as one variable - as I mentioned in the earlier note, it
> also looks like adding
> another variable broke the logic in Suresh's patch.
> 

Bah, indeed it did. Will repost the patches.


-Suresh