Skip to contentSkip to conversation

cifs: add new module parameter 'enable_oplocks'

PATCH In review Suresh Jayaraman 3 participants 7 messages Oct 11, 2011 – Oct 12, 2011 4 messages not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 cifs: add new module parameter 'enable_oplocks'

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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--
 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +5 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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:"
+				 "y/Y/1");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
fs/cifs/cifsglob.h +3 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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..6529b69 100644
@@ -165,7 +165,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	}
 	tcon = tlink_tcon(tlink);
 
-	if (oplockEnabled)
+	if (oplockEnabled || enable_oplocks)
 		oplock = REQ_OPLOCK;
 
 	if (nd && (nd->flags & LOOKUP_OPEN))
fs/cifs/file.c +2 -2
index bb71471..b76c31d 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 (oplockEnabled || 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 (oplockEnabled || enable_oplocks)
 		oplock = REQ_OPLOCK;
 	else
 		oplock = 0;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 55 lines · On Tue, 11 Oct 2011 21:28:00 +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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--
 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +5 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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:"
+				 "y/Y/1");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
fs/cifs/cifsglob.h +0 -0
index 6255fa8..b3e229c 100644

fs/cifs/cifsglob.h GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ · line 941

Quoted 6 lines
fs/cifs/cifsglob.h +3 -0
@@ -936,6 +936,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;
+

Let's not add an extra variable for this. I'd just rename oplockEnabled to enable_oplocks and fix up all the references to it. It should probably also be a bool too...

Quoted 38 lines
fs/cifs/cifsglob.h +0 -0
 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..6529b69 100644
@@ -165,7 +165,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	}
 	tcon = tlink_tcon(tlink);
 
-	if (oplockEnabled)
+	if (oplockEnabled || enable_oplocks)
 		oplock = REQ_OPLOCK;
 
 	if (nd && (nd->flags & LOOKUP_OPEN))
fs/cifs/file.c +2 -2
index bb71471..b76c31d 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 (oplockEnabled || 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 (oplockEnabled || enable_oplocks)
 		oplock = REQ_OPLOCK;
 	else
 		oplock = 0;
Signature · 1 line
Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20111011124405.37e09487-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org

#4 lore raw
Quoted 61 lines · Jeff Layton
Quoted 58 lines · On Tue, 11 Oct 2011 21:28:00 +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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

   modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

   echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--

 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +0 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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.");

fs/cifs/cifsfs.c +5 -0
+unsigned int enable_oplocks = 1;
+module_param(enable_oplocks, bool, 0644);
+MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks (bool). Default:"
+                              "y/Y/1");
+

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

fs/cifs/cifsglob.h +0 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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;
 
Quoted 7 lines

fs/cifs/cifsglob.h GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ · line 941

Quoted 3 lines
fs/cifs/cifsglob.h +3 -0
+/* enable or disable oplocks */
+GLOBAL_EXTERN unsigned int enable_oplocks;
+

Let's not add an extra variable for this. I'd just rename oplockEnabled to enable_oplocks and fix up all the references to it. It should probably also be a bool too...

Right, leave as one variable - as I mentioned in the earlier note, it also looks like adding
another variable broke the logic in Suresh's patch.

Signature · 3 lines
Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mv=SruLndtjRjPkpF27n5ekokk7SwMgY+4eOpehbJBpHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#6 lore raw
Quoted 62 lines · Steve French
Quoted 61 lines · Jeff Layton
Quoted 58 lines · On Tue, 11 Oct 2011 21:28:00 +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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--
 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +5 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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:"
+                              "y/Y/1");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
fs/cifs/cifsglob.h +0 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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;
 
Quoted 7 lines
Quoted 7 lines

fs/cifs/cifsglob.h GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ · line 941

Quoted 3 lines
fs/cifs/cifsglob.h +3 -0
+/* enable or disable oplocks */
+GLOBAL_EXTERN unsigned int enable_oplocks;
+

Let's not add an extra variable for this. I'd just rename oplockEnabled to enable_oplocks and fix up all the references to it. It should probably also be a bool too...

Will make this change.

Quoted 4 lines

Right, leave as one variable - as I mentioned in the earlier note, it also looks like adding
another variable broke the logic in Suresh's patch.

Bah, indeed it did. Will repost the patches.

-Suresh

#7 lore raw
In reply to a message not in the archive
Quoted 61 lines · Jeff Layton
Quoted 58 lines · On Tue, 11 Oct 2011 21:28:00 +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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--
 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +5 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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:"
+				 "y/Y/1");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
fs/cifs/cifsglob.h +0 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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;
 
Quoted 8 lines

fs/cifs/cifsglob.h GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ · line 941

Quoted 3 lines
fs/cifs/cifsglob.h +3 -0
+/* enable or disable oplocks */
+GLOBAL_EXTERN unsigned int enable_oplocks;
+

Let's not add an extra variable for this. I'd just rename oplockEnabled to enable_oplocks and fix up all the references to it. It should probably also be a bool too...

Need not be I think. Looking in to it further it looks like 'bool' type of module parameter are stored in variable of type 'int'. Looking into a few examples comfirms this too (e.g. enable_ino64 and printk_time etc.). So, I'll make it an int.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw
Quoted 71 lines · On Wed, 12 Oct 2011 11:29:25 +0530 Suresh Jayaraman…
Quoted 61 lines · Jeff Layton
Quoted 58 lines · On Tue, 11 Oct 2011 21:28:00 +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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--
 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +5 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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:"
+				 "y/Y/1");
+
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
fs/cifs/cifsglob.h +0 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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;
 
Quoted 8 lines

fs/cifs/cifsglob.h GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ · line 941

Quoted 3 lines
fs/cifs/cifsglob.h +3 -0
+/* enable or disable oplocks */
+GLOBAL_EXTERN unsigned int enable_oplocks;
+

Let's not add an extra variable for this. I'd just rename oplockEnabled to enable_oplocks and fix up all the references to it. It should probably also be a bool too...

Quoted 4 lines

Need not be I think. Looking in to it further it looks like 'bool' type of module parameter are stored in variable of type 'int'. Looking into a few examples comfirms this too (e.g. enable_ino64 and printk_time etc.). So, I'll make it an int.

I don't think so...

types.h has this:

typedef _Bool bool;

...and _Bool is an internal type in gcc (and other C9X compilers). I think gcc actually uses a char for this internally, but that's not always guaranteed to be the case. In any event, why ask for more memory than you need here? This is just a single-bit flag.

Signature · 1 line
Jeff Layton <[email protected]>
#10 lore raw
In reply to a message not in the archive

Does that really work - to disable oplocks you would need to reset both the old (/proc/fs/cifs/OplockEnabled) and new module parm (oplock_enabled) to zero.

We should be using one global for this, not two (the /proc/fs/cifs one could be changed to affect "enable_oplocks")

Quoted 100 lines · 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 patch doesn't alter the default behavior (Oplocks are enabled by default).

To disable oplocks when loading the module, use

  modprobe cifs enable_oplocks=0

(any of '0' or 'n' or 'N' conventions can be used).

To disable oplocks at runtime using the new interface, use

  echo 0 > /sys/module/cifs/parameters/enable_oplocks

The older /proc/fs/cifs/OplockEnabled interface will be deprecated after two releases. A subsequent patch will add an warning message about the deprecation.

---
 fs/cifs/cifsfs.c   |    5 +++++
 fs/cifs/cifsglob.h |    3 +++
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    4 ++--

 4 files changed, 11 insertions(+), 3 deletions(-)
fs/cifs/cifsfs.c +0 -0
index 3e29899..675ab40 100644
@@ -81,6 +81,11 @@ 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.");

fs/cifs/cifsfs.c +5 -0
+unsigned int enable_oplocks = 1;
+module_param(enable_oplocks, bool, 0644);
+MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks (bool). Default:"
+                                "y/Y/1");
+

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

fs/cifs/cifsglob.h +3 -0
index 6255fa8..b3e229c 100644
@@ -936,6 +936,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..6529b69 100644
@@ -165,7 +165,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
        }
        tcon = tlink_tcon(tlink);
 
-       if (oplockEnabled)
+       if (oplockEnabled || enable_oplocks)
                oplock = REQ_OPLOCK;
 
        if (nd && (nd->flags & LOOKUP_OPEN))
fs/cifs/file.c +1 -1
index bb71471..b76c31d 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 (oplockEnabled || enable_oplocks)
                oplock = REQ_OPLOCK;

       else
               oplock = 0;

fs/cifs/file.c +1 -1
@@ -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 (oplockEnabled || enable_oplocks)
                oplock = REQ_OPLOCK;

       else
               oplock = 0;

Signature · 3 lines
Thanks,

Steve