On Tue, 11 Oct 2011 10:35:07 -0500 Steve French <[email protected]> wrote:
> On Tue, Oct 11, 2011 at 10:24 AM, Jeff Layton <[email protected]> wrote: > > On Tue, 11 Oct 2011 10:21:21 -0500 > > Steve French <[email protected]> wrote: > > > >> On Tue, Oct 11, 2011 at 10:12 AM, Jeff Layton <[email protected]> wrote: > >> > On Tue, 11 Oct 2011 10:03:31 -0500 > >> > Steve French <[email protected]> wrote: > >> > > >> >> On Tue, Oct 11, 2011 at 6:08 AM, Suresh Jayaraman <[email protected]> > >> >> wrote: > >> >> > On 10/11/2011 04:02 PM, Jeff Layton wrote: > >> >> >> On Tue, 11 Oct 2011 15:06:43 +0530 > >> >> >> Suresh Jayaraman <[email protected]> wrote: > >> >> >> > >> >> >>> Thus spake Jeff Layton: > >> >> >>> > >> >> >>> "Making that a module parm would allow you to set that parameter at > >> >> >>> boot > >> >> >>> time without needing to add special startup scripts. IMO, all of the > >> >> >>> procfile "switches" under /proc/fs/cifs should be module parms > >> >> >>> instead." > >> >> >>> > >> >> >>> This seems reasonable so this patch makes OplockEnabled a module > >> >> >>> parameter and rename it to enable_oplocks to comply with the coding > >> >> >>> conventions. This patch removes the proc file handling pertaining to > >> >> >>> /proc/fs/cifs/OplockEnabled which would no longer be required if > >> >> >>> this > >> >> >>> patch gets accepted. > >> >> >>> > >> >> >>> This patch doesn't alter the default behavior (Oplocks enabled by > >> >> >>> default). To disable oplocks when loading the module, use > >> >> >>> > >> >> >>> modprobe cifs enable_oplocks=0 > >> >> >>> > >> >> >>> Note: > >> >> >>> (a) I'm little worried about eliminating an already known interace > >> >> >>> to > >> >> >>> enable/disable Oplocks. An update to README file mentioning > >> >> >>> this info > >> >> >>> is planned. Do we need to warn users before pulling it off? Any > >> >> >>> suggestions on how we could do this? > >> >> >>> (b) Most of the /proc/fs/cifs switches could be converted to a > >> >> >>> module > >> >> >>> param for e.g. LookupCacheEnabled, LinuxExtensionsEnabled, > >> >> >>> MultiuserMount etc. I'll post further patches once this gets > >> >> >>> accepted. > >> >> >>> > >> >> >> > >> >> >> Yeah, I don't think we ought to just rip out these interfaces > >> >> >> unannounced. What should probably happen is this: > >> >> >> > >> >> >> Add the new module parms, and a patch that makes a printk pop when > >> >> >> the > >> >> >> old interfaces are used. The printk should announce something like: > >> >> >> > >> >> >> "The /proc/fs/cifs/foo interface will be removed in kernel version > >> >> >> 3.x. > >> >> >> Please migrate to using the 'enable_foo' module parameter in > >> >> >> cifs.ko." > >> >> >> > >> >> >> Make the 3.x version be 2 releases out. Then have patches ready to go > >> >> >> to remove those interfaces when the 3.x merge window opens. > >> >> 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 <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
