Re: [RFC][PATCH] cifs: make OplockEnabled a module parameter

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Tue, 11 Oct 2011 10:21:21 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Oct 11, 2011 at 10:12 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 11 Oct 2011 10:03:31 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Tue, Oct 11, 2011 at 6:08 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> > On 10/11/2011 04:02 PM, Jeff Layton wrote:
> >> >> On Tue, 11 Oct 2011 15:06:43 +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 seems reasonable so this patch makes OplockEnabled a module
> >> >>> parameter and rename it to enable_oplocks to comply with the coding
> >> >>> conventions. This patch removes the proc file handling pertaining to
> >> >>> /proc/fs/cifs/OplockEnabled which would no longer be required if this
> >> >>> patch gets accepted.
> >> >>>
> >> >>> This patch doesn't alter the default behavior (Oplocks enabled by
> >> >>> default). To disable oplocks when loading the module, use
> >> >>>
> >> >>>    modprobe cifs enable_oplocks=0
> >> >>>
> >> >>> Note:
> >> >>> (a) I'm little worried about eliminating an already known interace to
> >> >>>     enable/disable Oplocks. An update to README file mentioning this info
> >> >>>     is planned. Do we need to warn users before pulling it off? Any
> >> >>>     suggestions on how we could do this?
> >> >>> (b) Most of the /proc/fs/cifs switches could be converted to a module
> >> >>>     param for e.g. LookupCacheEnabled, LinuxExtensionsEnabled,
> >> >>>     MultiuserMount etc. I'll post further patches once this gets
> >> >>>     accepted.
> >> >>>
> >> >>
> >> >> Yeah, I don't think we ought to just rip out these interfaces
> >> >> unannounced. What should probably happen is this:
> >> >>
> >> >> Add the new module parms, and a patch that makes a printk pop when the
> >> >> old interfaces are used. The printk should announce something like:
> >> >>
> >> >> "The /proc/fs/cifs/foo interface will be removed in kernel version 3.x.
> >> >> Please migrate to using the 'enable_foo' module parameter in cifs.ko."
> >> >>
> >> >> Make the 3.x version be 2 releases out. Then have patches ready to go
> >> >> to remove those interfaces when the 3.x merge window opens.
> >> >
> >> > Makes sense. Thanks.
> >> >
> >> >>> 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/cifs_debug.c |   40 ----------------------------------------
> >> >>>  fs/cifs/cifsfs.c     |    4 +++-
> >> >>>  fs/cifs/cifsglob.h   |    4 +++-
> >> >>>  fs/cifs/dir.c        |    2 +-
> >> >>>  fs/cifs/file.c       |    4 ++--
> >> >>>  5 files changed, 9 insertions(+), 45 deletions(-)
> >> >>>
> >> >>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> >> >>> index 2fe3cf1..393b37b 100644
> >> >>> --- a/fs/cifs/cifs_debug.c
> >> >>> +++ b/fs/cifs/cifs_debug.c
> >> >>> @@ -418,7 +418,6 @@ static const struct file_operations cifs_stats_proc_fops = {
> >> >>>
> >> >>>  static struct proc_dir_entry *proc_fs_cifs;
> >> >>>  static const struct file_operations cifsFYI_proc_fops;
> >> >>> -static const struct file_operations cifs_oplock_proc_fops;
> >> >>>  static const struct file_operations cifs_lookup_cache_proc_fops;
> >> >>>  static const struct file_operations traceSMB_proc_fops;
> >> >>>  static const struct file_operations cifs_multiuser_mount_proc_fops;
> >> >>> @@ -439,7 +438,6 @@ cifs_proc_init(void)
> >> >>>  #endif /* STATS */
> >> >>>      proc_create("cifsFYI", 0, proc_fs_cifs, &cifsFYI_proc_fops);
> >> >>>      proc_create("traceSMB", 0, proc_fs_cifs, &traceSMB_proc_fops);
> >> >>> -    proc_create("OplockEnabled", 0, proc_fs_cifs, &cifs_oplock_proc_fops);
> >> >>>      proc_create("LinuxExtensionsEnabled", 0, proc_fs_cifs,
> >> >>>                  &cifs_linux_ext_proc_fops);
> >> >>>      proc_create("MultiuserMount", 0, proc_fs_cifs,
> >> >>> @@ -463,7 +461,6 @@ cifs_proc_clean(void)
> >> >>>      remove_proc_entry("Stats", proc_fs_cifs);
> >> >>>  #endif
> >> >>>      remove_proc_entry("MultiuserMount", proc_fs_cifs);
> >> >>> -    remove_proc_entry("OplockEnabled", proc_fs_cifs);
> >> >>>      remove_proc_entry("SecurityFlags", proc_fs_cifs);
> >> >>>      remove_proc_entry("LinuxExtensionsEnabled", proc_fs_cifs);
> >> >>>      remove_proc_entry("LookupCacheEnabled", proc_fs_cifs);
> >> >>> @@ -509,43 +506,6 @@ static const struct file_operations cifsFYI_proc_fops = {
> >> >>>      .write          = cifsFYI_proc_write,
> >> >>>  };
> >> >>>
> >> >>> -static int cifs_oplock_proc_show(struct seq_file *m, void *v)
> >> >>> -{
> >> >>> -    seq_printf(m, "%d\n", oplockEnabled);
> >> >>> -    return 0;
> >> >>> -}
> >> >>> -
> >> >>> -static int cifs_oplock_proc_open(struct inode *inode, struct file *file)
> >> >>> -{
> >> >>> -    return single_open(file, cifs_oplock_proc_show, NULL);
> >> >>> -}
> >> >>> -
> >> >>> -static ssize_t cifs_oplock_proc_write(struct file *file,
> >> >>> -            const char __user *buffer, size_t count, loff_t *ppos)
> >> >>> -{
> >> >>> -    char c;
> >> >>> -    int rc;
> >> >>> -
> >> >>> -    rc = get_user(c, buffer);
> >> >>> -    if (rc)
> >> >>> -            return rc;
> >> >>> -    if (c == '0' || c == 'n' || c == 'N')
> >> >>> -            oplockEnabled = 0;
> >> >>> -    else if (c == '1' || c == 'y' || c == 'Y')
> >> >>> -            oplockEnabled = 1;
> >> >>> -
> >> >>> -    return count;
> >> >>> -}
> >> >>> -
> >> >>> -static const struct file_operations cifs_oplock_proc_fops = {
> >> >>> -    .owner          = THIS_MODULE,
> >> >>> -    .open           = cifs_oplock_proc_open,
> >> >>> -    .read           = seq_read,
> >> >>> -    .llseek         = seq_lseek,
> >> >>> -    .release        = single_release,
> >> >>> -    .write          = cifs_oplock_proc_write,
> >> >>> -};
> >> >>> -
> >> >>>  static int cifs_linux_ext_proc_show(struct seq_file *m, void *v)
> >> >>>  {
> >> >>>      seq_printf(m, "%d\n", linuxExtEnabled);
> >> >>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> >>> index 3e29899..37c2fbb 100644
> >> >>> --- a/fs/cifs/cifsfs.c
> >> >>> +++ b/fs/cifs/cifsfs.c
> >> >>> @@ -52,7 +52,6 @@
> >> >>>  int cifsFYI = 0;
> >> >>>  int cifsERROR = 1;
> >> >>>  int traceSMB = 0;
> >> >>> -unsigned int oplockEnabled = 1;
> >> >>>  unsigned int linuxExtEnabled = 1;
> >> >>>  unsigned int lookupCacheEnabled = 1;
> >> >>>  unsigned int multiuser_mount = 0;
> >> >>> @@ -81,6 +80,9 @@ 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: 1.");
> >> >>
> >> >> I think you want this as "Default: true" since this is a bool?
> >> >>
> >> >
> >> > No. From the definition of module_param:
> >> >
> >> > (snip)
> >> >  * Standard types are:
> >> >  *      byte, short, ushort, int, uint, long, ulong
> >> >  *      charp: a character pointer
> >> >  *      bool: a bool, values 0/1, y/n, Y/N.
> >> >  */
> >> > #define module_param(name, type, perm)                          \
> >> >        module_param_named(name, name, type, perm)
> >> >
> >> > I should perhaps add y/Y.
> >>
> >> I don't mind adding a module parm to change the default but it seems
> >> odd, and removing the ability to temporarily turn off oplock seems to
> >> make things worse not better.  But I am not convinced of a good use
> >> case for disabling oplocks on module load (rather than the more
> >> granular "forcedirectio" on mount, or the temporary ie at runtime via
> >> /proc).   If oplock/caching on the client were broken, then we would
> >> fix the bug rather than ask users to load with oplock off, if oplock
> >> were broken on a server, we would not want to disable it for mounts to
> >> all servers (as would a module parm) but just to workaround the broken
> >> server.
> >>
> >
> > This doesn't prevent you from changing this setting after the module is
> > loaded. It just moves the control to a more standard location
> > (/sys/module/cifs/parameters).
> 
> If "echo 1 > /sys/module/cifs/parameters/oplock_enabled" would work at runtime,
> then I fine with the change.   We could leave them both in for one release,
> and throw a onetime syslog message if you use the one in /proc/fs/cifs/ (so
> that users know that the old interface is going away)??
> 
> 

Yes, that will work at runtime.

I suggest leaving the old control in for 2 releases since that's the
kernel "standard" for user-visible changes. There's no rush to
deprecate the old code, so we should err on the side of caution.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>