cifs: make OplockEnabled a module parameter

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.

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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;

fs/cifs/cifsfs.c +0 -0

 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;

fs/cifs/cifsglob.h +3 -1

index 6255fa8..9c33727 100644
@@ -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;

fs/cifs/dir.c +1 -1

index 81914df..4d83659 100644
@@ -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))

fs/cifs/file.c +2 -2

index bb71471..41149a0 100644
@@ -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;

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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.

Quoted 96 lines

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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);

fs/cifs/cifsfs.c module_param(echo_retries, ushort, 0644); · line 85

Quoted 6 lines

fs/cifs/cifsfs.c +3 -0

 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?

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

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.

Quoted 96 lines

Quoted 96 lines

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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);

Quoted 9 lines

fs/cifs/cifsfs.c module_param(echo_retries, ushort, 0644); · line 85

Quoted 6 lines

 MODULE_PARM_DESC(echo_retries, "Number of echo attempts before giving up and " "reconnecting server. Default: 5. 0 means " "never reconnect.");

fs/cifs/cifsfs.c +3 -0

+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.

Quoted 164 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

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.

Quoted 96 lines

Quoted 96 lines

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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);

Quoted 9 lines

fs/cifs/cifsfs.c

Quoted 6 lines

 MODULE_PARM_DESC(echo_retries, "Number of echo attempts before giving up and " "reconnecting server. Default: 5. 0 means " "never reconnect.");

fs/cifs/cifsfs.c +3 -0

+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)

Quoted 1 line

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.

Quoted 167 lines · On Tue, 11 Oct 2011 10:03:31 -0500 Steve French…

Quoted 164 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

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.

Quoted 96 lines

Quoted 96 lines

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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);

Quoted 9 lines

fs/cifs/cifsfs.c

Quoted 6 lines

 MODULE_PARM_DESC(echo_retries, "Number of echo attempts before giving up and " "reconnecting server. Default: 5. 0 means " "never reconnect.");

fs/cifs/cifsfs.c +3 -0

+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)

Quoted 1 line

I should perhaps add y/Y.

Quoted 11 lines

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).

Quoted 12 lines

Quoted 11 lines

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.

Quoted 3 lines

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,

Quoted 182 lines · Jeff Layton

Quoted 178 lines · On Tue, 11 Oct 2011 10:03:31 -0500 Steve French…

Quoted 164 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

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.

Quoted 96 lines

Quoted 96 lines

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(-)

fs/cifs/cifs_debug.c +0 -40

index 2fe3cf1..393b37b 100644
@@ -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);

fs/cifs/cifsfs.c +0 -1

index 3e29899..37c2fbb 100644
@@ -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);

Quoted 9 lines

fs/cifs/cifsfs.c

Quoted 6 lines

 MODULE_PARM_DESC(echo_retries, "Number of echo attempts before giving up and " "reconnecting server. Default: 5. 0 means " "never reconnect.");

fs/cifs/cifsfs.c +3 -0

+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)

Quoted 1 line

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.

Quoted 3 lines

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)??

Quoted 61 lines · On Tue, 11 Oct 2011 10:21:21 -0500 Steve French…

Quoted 60 lines · Jeff Layton

Quoted 56 lines · On Tue, 11 Oct 2011 10:03:31 -0500 Steve French…

Quoted 36 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

Quoted 8 lines

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.

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.

Quoted 10 lines

Quoted 3 lines

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.

Quoted 75 lines · Jeff Layton

Quoted 61 lines · On Tue, 11 Oct 2011 10:21:21 -0500 Steve French…

Quoted 60 lines · Jeff Layton

Quoted 56 lines · On Tue, 11 Oct 2011 10:03:31 -0500 Steve French…

Quoted 36 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

Quoted 8 lines

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.

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.

Quoted 10 lines

Quoted 3 lines

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)??

Quoted 5 lines

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.

Next obvious question - do we have other module parameters that need to be changed to make config possible runtime. Today only echo_retries is configurable that way. The only other possible one I see is cifs_max_pending (whose meaning will change in any case when the maxmpx patch is merged - with the patch the upper limit will be the minimum of cifs_max_pending and the server's returned maxmpx rather than simply using cifs_max_pending).

There are six other parms (besides OplockEnabled) in /proc/fs/cifs but I doubt that it is worth the trouble and confusion to change any of them (although security flags is one that is more likely to want to be changed at module install time)

Quoted 90 lines · On Tue, 11 Oct 2011 10:35:07 -0500 Steve French…

Quoted 75 lines · Jeff Layton

Quoted 61 lines · On Tue, 11 Oct 2011 10:21:21 -0500 Steve French…

Quoted 60 lines · Jeff Layton

Quoted 56 lines · On Tue, 11 Oct 2011 10:03:31 -0500 Steve French…

Quoted 36 lines · Suresh Jayaraman

Quoted 35 lines · Jeff Layton

Quoted 18 lines · On Tue, 11 Oct 2011 15:06:43 +0530 Suresh Jayaraman…

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

Quoted 10 lines

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:

Quoted 8 lines

Quoted 8 lines

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.

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.

Quoted 10 lines

Quoted 3 lines

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)??

Quoted 5 lines

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.

Next obvious question - do we have other module parameters that need to be changed to make config possible runtime. Today only echo_retries is configurable that way. The only other possible one I see is cifs_max_pending (whose meaning will change in any case when the maxmpx patch is merged - with the patch the upper limit will be the minimum of cifs_max_pending and the server's returned maxmpx rather than simply using cifs_max_pending).

Quoted 4 lines

There are six other parms (besides OplockEnabled) in /proc/fs/cifs but I doubt that it is worth the trouble and confusion to change any of them (although security flags is one that is more likely to want to be changed at module install time)

I think it would be best to move most or all of the controls under /proc/fs/cifs to module parms. It really doesn't make much sense to users for us to present these controls in different places. Module parms have distinct advantages over files in /proc/fs/cifs so that seems like the best place for them.