On Tue, Oct 11, 2011 at 10:12 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Tue, 11 Oct 2011 10:03:31 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > >> On Tue, Oct 11, 2011 at 6:08 AM, Suresh Jayaraman <sjayaraman@xxxxxxx> wrote: >> > On 10/11/2011 04:02 PM, Jeff Layton wrote: >> >> On Tue, 11 Oct 2011 15:06:43 +0530 >> >> Suresh Jayaraman <sjayaraman@xxxxxxx> 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@xxxxxxx> >> >>> Cc: Jeff Layton <jlayton@xxxxxxxxxx> >> >>> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx> >> >>> --- >> >>> 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)?? -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html