[2/2] nfsd: Change the default to enable all minor versions unless told otherwise
diff mbox

Message ID 20170224003344.113724-3-trond.myklebust@primarydata.com
State New
Headers show

Commit Message

Trond Myklebust Feb. 24, 2017, 12:33 a.m. UTC
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(-)

Comments

NeilBrown Feb. 24, 2017, 1:17 a.m. UTC | #1
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
Trond Myklebust Feb. 24, 2017, 1:42 a.m. UTC | #2
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
NeilBrown Feb. 24, 2017, 2:27 a.m. UTC | #3
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
J. Bruce Fields Feb. 24, 2017, 8:32 p.m. UTC | #4
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
Steve Dickson April 4, 2017, 8:08 p.m. UTC | #5
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

Patch
diff mbox

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);