| Message ID | 20170224003344.113724-3-trond.myklebust@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 23 2017, Trond Myklebust wrote: > Instead of letting the kernel decide, default to enabling all versions, > and let the user be more specifc in /etc/nfs.conf or on the command line. What is your rationale for this? I think there is value in allowing the kernel to support a version while disabling it by default. This allows it to be used for experimentation, without much risk of it being used in production until it is deemed to be really ready. I like the rest of the patch set, and withdraw my comments about the earlier patch set which were due to a hasty misunderstanding. Thanks, NeilBrown > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > utils/nfsd/nfsd.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c > index 1708521ab286..9735e65466fa 100644 > --- a/utils/nfsd/nfsd.c > +++ b/utils/nfsd/nfsd.c > @@ -107,13 +107,12 @@ main(int argc, char **argv) > } > > nfssvc_get_minormask(&minormask); > - /* We assume the kernel will default all minor versions to 'on', > - * and allow the config file to disable some. > + /* We default all minor versions to 'on', and allow the config > + * file to disable some. > */ > - if (NFSCTL_VERISSET(versbits, 4)) { > - NFSCTL_MINORSET(minorversset, 0); > - NFSCTL_MINORSET(minorvers, 0); > - } > + if (NFSCTL_VERISSET(versbits, 4)) > + minorversset = minorvers = minormask; > + > for (i = 1; i <= NFS4_MAXMINOR; i++) { > char tag[20]; > sprintf(tag, "vers4.%d", i); > -- > 2.9.3
Hi Neil, On Fri, 2017-02-24 at 12:17 +1100, NeilBrown wrote: > On Thu, Feb 23 2017, Trond Myklebust wrote: > > > Instead of letting the kernel decide, default to enabling all > > versions, > > and let the user be more specifc in /etc/nfs.conf or on the command > > line. > > What is your rationale for this? > I think there is value in allowing the kernel to support a version > while > disabling it by default. This allows it to be used for > experimentation, > without much risk of it being used in production until it is deemed > to > be really ready. I think we can still do that by having the kernel simply not report that version. You'll note that -V4.x and -N4.x are allowed whether or not the kernel is reporting a version 'x' in /proc/fs/nfsd/versions. IOW: if you want to make the version not appear by default because it is unstable, then you probably don't want it to appear when the user does '+V4' either so you might as well hide it in /proc/fs/nfsd/versions too. For the record, this patch is not one that I'm too attached to, so it can be dropped if deemed too controversial. > I like the rest of the patch set, and withdraw my comments about the > earlier patch set which were due to a hasty misunderstanding. > Thanks! > > > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > > --- > > utils/nfsd/nfsd.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c > > index 1708521ab286..9735e65466fa 100644 > > --- a/utils/nfsd/nfsd.c > > +++ b/utils/nfsd/nfsd.c > > @@ -107,13 +107,12 @@ main(int argc, char **argv) > > } > > > > nfssvc_get_minormask(&minormask); > > - /* We assume the kernel will default all minor versions to > > 'on', > > - * and allow the config file to disable some. > > + /* We default all minor versions to 'on', and allow the > > config > > + * file to disable some. > > */ > > - if (NFSCTL_VERISSET(versbits, 4)) { > > - NFSCTL_MINORSET(minorversset, 0); > > - NFSCTL_MINORSET(minorvers, 0); > > - } > > + if (NFSCTL_VERISSET(versbits, 4)) > > + minorversset = minorvers = minormask; > > + > > for (i = 1; i <= NFS4_MAXMINOR; i++) { > > char tag[20]; > > sprintf(tag, "vers4.%d", i); > > -- > > 2.9.3 -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com
On Fri, Feb 24 2017, Trond Myklebust wrote: > Hi Neil, > > On Fri, 2017-02-24 at 12:17 +1100, NeilBrown wrote: >> On Thu, Feb 23 2017, Trond Myklebust wrote: >> >> > Instead of letting the kernel decide, default to enabling all >> > versions, >> > and let the user be more specifc in /etc/nfs.conf or on the command >> > line. >> >> What is your rationale for this? >> I think there is value in allowing the kernel to support a version >> while >> disabling it by default. This allows it to be used for >> experimentation, >> without much risk of it being used in production until it is deemed >> to >> be really ready. > > I think we can still do that by having the kernel simply not report > that version. You'll note that -V4.x and -N4.x are allowed whether or > not the kernel is reporting a version 'x' in /proc/fs/nfsd/versions. > > IOW: if you want to make the version not appear by default because it > is unstable, then you probably don't want it to appear when the user > does '+V4' either so you might as well hide it in > /proc/fs/nfsd/versions too. That seems reasonable - possibly even better than the current approach, though it is hard to be sure without actually trying it out for a while. I haven't poured over the patches enough for a reviewed-by, but Acked-by: NeilBrown <neilb@suse.com> thanks! NeilBrown
On Fri, Feb 24, 2017 at 01:27:57PM +1100, NeilBrown wrote: > On Fri, Feb 24 2017, Trond Myklebust wrote: > > > Hi Neil, > > > > On Fri, 2017-02-24 at 12:17 +1100, NeilBrown wrote: > >> On Thu, Feb 23 2017, Trond Myklebust wrote: > >> > >> > Instead of letting the kernel decide, default to enabling all > >> > versions, > >> > and let the user be more specifc in /etc/nfs.conf or on the command > >> > line. > >> > >> What is your rationale for this? > >> I think there is value in allowing the kernel to support a version > >> while > >> disabling it by default. This allows it to be used for > >> experimentation, > >> without much risk of it being used in production until it is deemed > >> to > >> be really ready. > > > > I think we can still do that by having the kernel simply not report > > that version. You'll note that -V4.x and -N4.x are allowed whether or > > not the kernel is reporting a version 'x' in /proc/fs/nfsd/versions. > > > > IOW: if you want to make the version not appear by default because it > > is unstable, then you probably don't want it to appear when the user > > does '+V4' either so you might as well hide it in > > /proc/fs/nfsd/versions too. > > That seems reasonable - possibly even better than the current approach, > though it is hard to be sure without actually trying it out for a while. > I haven't poured over the patches enough for a reviewed-by, but > Acked-by: NeilBrown <neilb@suse.com> It's too late to tell old kernels to hide experimental versions, though. And it's 4.1's experimental stage that's most likely to be a problem--though maybe that's getting long enough go that not too many people are bisecting back to that era of kernel. I dunno, I'm inclined to drop as long as this part isn't necessary to solve Trond's immediate problem. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2017 07:33 PM, Trond Myklebust wrote: > Instead of letting the kernel decide, default to enabling all versions, > and let the user be more specifc in /etc/nfs.conf or on the command line. > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> This one was NACKed by Bruce... steved. > --- > utils/nfsd/nfsd.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c > index 1708521ab286..9735e65466fa 100644 > --- a/utils/nfsd/nfsd.c > +++ b/utils/nfsd/nfsd.c > @@ -107,13 +107,12 @@ main(int argc, char **argv) > } > > nfssvc_get_minormask(&minormask); > - /* We assume the kernel will default all minor versions to 'on', > - * and allow the config file to disable some. > + /* We default all minor versions to 'on', and allow the config > + * file to disable some. > */ > - if (NFSCTL_VERISSET(versbits, 4)) { > - NFSCTL_MINORSET(minorversset, 0); > - NFSCTL_MINORSET(minorvers, 0); > - } > + if (NFSCTL_VERISSET(versbits, 4)) > + minorversset = minorvers = minormask; > + > for (i = 1; i <= NFS4_MAXMINOR; i++) { > char tag[20]; > sprintf(tag, "vers4.%d", i); > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c index 1708521ab286..9735e65466fa 100644 --- a/utils/nfsd/nfsd.c +++ b/utils/nfsd/nfsd.c @@ -107,13 +107,12 @@ main(int argc, char **argv) } nfssvc_get_minormask(&minormask); - /* We assume the kernel will default all minor versions to 'on', - * and allow the config file to disable some. + /* We default all minor versions to 'on', and allow the config + * file to disable some. */ - if (NFSCTL_VERISSET(versbits, 4)) { - NFSCTL_MINORSET(minorversset, 0); - NFSCTL_MINORSET(minorvers, 0); - } + if (NFSCTL_VERISSET(versbits, 4)) + minorversset = minorvers = minormask; + for (i = 1; i <= NFS4_MAXMINOR; i++) { char tag[20]; sprintf(tag, "vers4.%d", i);
Instead of letting the kernel decide, default to enabling all versions, and let the user be more specifc in /etc/nfs.conf or on the command line. Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> --- utils/nfsd/nfsd.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)