On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman <[email protected]> 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. > Reported-by: Alexander Swen <[email protected]> > Cc: Jeff Layton <[email protected]> > Signed-off-by: Suresh Jayaraman <[email protected]> > --- > 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? > 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..9c33727 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -922,7 +922,6 @@ GLOBAL_EXTERN unsigned int multiuser_mount; /* if enabled > allows new sessions > to be established on existing mount if we > have the uid/password or Kerberos credential > or equivalent for current user */ > -GLOBAL_EXTERN unsigned int oplockEnabled; > GLOBAL_EXTERN unsigned int lookupCacheEnabled; > GLOBAL_EXTERN unsigned int global_secflags; /* if on, session setup sent > with more secure ntlmssp2 challenge/resp */ > @@ -936,6 +935,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; > + > GLOBAL_EXTERN struct rb_root uidtree; > GLOBAL_EXTERN struct rb_root gidtree; > GLOBAL_EXTERN spinlock_t siduidlock; > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 81914df..4d83659 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -165,7 +165,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, > int mode, > } > tcon = tlink_tcon(tlink); > > - if (oplockEnabled) > + if (enable_oplocks) > oplock = REQ_OPLOCK; > > if (nd && (nd->flags & LOOKUP_OPEN)) > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index bb71471..41149a0 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -369,7 +369,7 @@ int cifs_open(struct inode *inode, struct file *file) > cFYI(1, "inode = 0x%p file flags are 0x%x for %s", > inode, file->f_flags, full_path); > > - if (oplockEnabled) > + if (enable_oplocks) > oplock = REQ_OPLOCK; > else > oplock = 0; > @@ -493,7 +493,7 @@ static int cifs_reopen_file(struct cifsFileInfo > *pCifsFile, bool can_flush) > cFYI(1, "inode = 0x%p file flags 0x%x for %s", > inode, pCifsFile->f_flags, full_path); > > - if (oplockEnabled) > + if (enable_oplocks) > oplock = REQ_OPLOCK; > else > oplock = 0; -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
