On Tue, 11 Oct 2011 11:38:30 -0400 Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Tue, 11 Oct 2011 10:35:07 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > > > On Tue, Oct 11, 2011 at 10:24 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > On Tue, 11 Oct 2011 10:21:21 -0500 > > > Steve French <smfrench@xxxxxxxxx> wrote: > > > > > >> On Tue, Oct 11, 2011 at 10:12 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > >> > On Tue, 11 Oct 2011 10:03:31 -0500 > > >> > Steve French <smfrench@xxxxxxxxx> wrote: > > >> > > > >> >> On Tue, Oct 11, 2011 at 6:08 AM, Suresh Jayaraman <sjayaraman@xxxxxxx> wrote: > > >> >> > On 10/11/2011 04:02 PM, Jeff Layton wrote: > > >> >> >> On Tue, 11 Oct 2011 15:06:43 +0530 > > >> >> >> Suresh Jayaraman <sjayaraman@xxxxxxx> 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@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
