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

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

[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

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

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

[Jeff Layton]

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.

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

>  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 <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Suresh Jayaraman]

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'll repost the patch.


Thanks
Suresh

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

[Steve French]

On Tue, Oct 11, 2011 at 6:08 AM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> 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.

-- 
Thanks,

Steve

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

[Jeff Layton]

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-l3A5Bk7waGM@public.gmane.org> 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).

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

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

[Steve French]

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


-- 
Thanks,

Steve

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

[Steve French]

On Tue, Oct 11, 2011 at 10:21 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 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,

I meant the reverse (obviously oplock is enabled by default)

 "echo 0 > /sys/module/cifs/parameters/oplock_enabled"
-- 
Thanks,

Steve

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

[Jeff Layton]

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>

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

[Steve French]

On Tue, Oct 11, 2011 at 10:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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.
>> >> 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.

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)
-- 
Thanks,

Steve

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

[Jeff Layton]

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

> On Tue, Oct 11, 2011 at 10:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > 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.
> >> >> 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.
> 
> 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)

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.

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

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

[Jeff Layton]

On Tue, 11 Oct 2011 11:38:30 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, 11 Oct 2011 10:35:07 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Tue, Oct 11, 2011 at 10:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > 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+wXaHxf7aJhl2p70BpVqQ@public.gmane.orgm> 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.
> > >> >> 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.
> > 
> > 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)
> 
> 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.
> 

Some of the files under there too have really been superceded by mount
options. It might make sense to just get rid of some of them altogether
in favor of those. In particular, I'm thinking of
LinuxExtensionsEnabled (same as -o nounix). Maybe LookupCacheEnabled
can go too?

Either way, you probably need to do this on a case-by-case basis...

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>