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

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

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>