[PATCH 1/2] nfsd: fix version-setting regression on old kernels

[PATCH 1/2] nfsd: fix version-setting regression on old kernels

[J. Bruce Fields]

/proc/fs/nfsd/versions was extended to allow turning on/off minor
versions by echoing "+4.1" or "-4.1" to /proc/fs/nsfd/versions.

Unfortunately, pre-2.6.30 kernels just stop parsing at first non-digit,
so "-4.1" is interpreted as "-4".  If new nfs-utils (on old kernel)
writes "+2", "+3", "+4", then "-4.1", result therefore is to turn off
4.1.

Given that historical behavior, it may have been a mistake to extend the
interface the way we did; but at this point we're probably stuck with
it.  So, just reverse the order we write versions in.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 utils/nfsd/nfssvc.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
index b8028bb..7bbbaba 100644
--- a/utils/nfsd/nfssvc.c
+++ b/utils/nfsd/nfssvc.c
@@ -238,17 +238,17 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
 	if (fd < 0)
 		return;
 
+	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
+	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
+		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
+				    minorvers4 > 0 ? '+' : '-',
+				    n);
 	for (n = NFSD_MINVERS; n <= NFSD_MAXVERS; n++) {
 		if (NFSCTL_VERISSET(ctlbits, n))
 		    off += snprintf(ptr+off, sizeof(buf) - off, "+%d ", n);
 		else
 		    off += snprintf(ptr+off, sizeof(buf) - off, "-%d ", n);
 	}
-	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
-	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
-		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
-				    minorvers4 > 0 ? '+' : '-',
-				    n);
 	xlog(D_GENERAL, "Writing version string to kernel: %s", buf);
 	snprintf(ptr+off, sizeof(buf) - off, "\n");
 	if (write(fd, buf, strlen(buf)) != strlen(buf))
-- 
1.6.3.3

[PATCH 2/2] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

The current kernel code should not be enabled by default, because it
does not yet attempt to be a conform completely to the rfc; for example,
some required pieces of protocol are missing.

Therefore the kernel defaults to leaving minorversion1 off.  When the
code matures sufficiently, that default will change.

That kernel default becomes meaningless if nfs-utils always explicitly
turns 4.1 on or off.  So, nfs-utils should by default do nothing.

Provide a --enable-experimental-v41-support option to turn it on
explicitly.  The option is intentionally spelled out (and has no short
equivalent), to help ensure that users know what they're getting into.

Once 4.1 defaults to on, that option will become unnecessary (and can
probably just be dropped from nfs-utils), and only the -N 4.1 option
will be necessary.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 support/include/nfs/nfs.h |    3 ---
 utils/nfsd/nfsd.c         |   17 +++++++++++++----
 utils/nfsd/nfssvc.c       |   10 ++++------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index a64eb0a..824f604 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -13,9 +13,6 @@
 #define NFSD_MINVERS 2
 #define NFSD_MAXVERS 4
 
-#define NFSD_MINMINORVERS4 1
-#define NFSD_MAXMINORVERS4 1
-
 struct nfs_fh_len {
 	int		fh_size;
 	u_int8_t	fh_handle[NFS3_FHSIZE];
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 1cda1e5..c72f73e 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -49,6 +49,7 @@ static struct option longopts[] =
 	{ "port", 1, 0, 'p' },
 	{ "debug", 0, 0, 'd' },
 	{ "syslog", 0, 0, 's' },
+	{ "enable-experimental-v41-support", 0, 0, 'X' },
 	{ NULL, 0, 0, 0 }
 };
 
@@ -103,7 +104,7 @@ main(int argc, char **argv)
 	char *p, *progname, *port;
 	char *haddr = NULL;
 	int	socket_up = 0;
-	int minorvers4 = NFSD_MAXMINORVERS4;	/* nfsv4 minor version */
+	int minorvers41 = 0;	/* nfsv4 minor version */
 	unsigned int versbits = NFSCTL_ALLBITS;
 	unsigned int protobits = NFSCTL_ALLBITS;
 	unsigned int proto4 = 0;
@@ -163,7 +164,12 @@ main(int argc, char **argv)
 			switch((c = strtol(optarg, &p, 0))) {
 			case 4:
 				if (*p == '.') {
-					minorvers4 = -atoi(p + 1);
+					int i = atoi(p+1);
+					if (i != 1) {
+						fprintf(stderr, "%s: unsupported minor version\n", optarg);
+						exit(1);
+					}
+					minorvers41 = -1;
 					break;
 				}
 			case 3:
@@ -185,6 +191,9 @@ main(int argc, char **argv)
 		case 'U':
 			NFSCTL_UDPUNSET(protobits);
 			break;
+		case 'X':
+			minorvers41 = 1;
+			break;
 		default:
 			fprintf(stderr, "Invalid argument: '%c'\n", c);
 		case 'h':
@@ -257,7 +266,7 @@ main(int argc, char **argv)
 	 * registered with rpcbind. Note that on older kernels w/o the right
 	 * interfaces, these are a no-op.
 	 */
-	nfssvc_setvers(versbits, minorvers4);
+	nfssvc_setvers(versbits, minorvers41);
  
 	error = nfssvc_set_sockets(AF_INET, proto4, haddr, port);
 	if (!error)
@@ -309,7 +318,7 @@ static void
 usage(const char *prog)
 {
 	fprintf(stderr, "Usage:\n"
-		"%s [-d|--debug] [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-s|--syslog] [-T|--no-tcp] [-U|--no-udp] nrservs\n", 
+		"%s [-d|--debug] [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-s|--syslog] [-T|--no-tcp] [-U|--no-udp] [--enable-experimental-v41-support] nrservs\n", 
 		prog);
 	exit(2);
 }
diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
index 7bbbaba..ec9446a 100644
--- a/utils/nfsd/nfssvc.c
+++ b/utils/nfsd/nfssvc.c
@@ -227,7 +227,7 @@ nfssvc_set_sockets(const int family, const unsigned int protobits,
 }
 
 void
-nfssvc_setvers(unsigned int ctlbits, int minorvers4)
+nfssvc_setvers(unsigned int ctlbits, int minorvers41)
 {
 	int fd, n, off;
 	char *ptr;
@@ -238,11 +238,9 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
 	if (fd < 0)
 		return;
 
-	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
-	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
-		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
-				    minorvers4 > 0 ? '+' : '-',
-				    n);
+	if (minorvers41)
+		off += snprintf(ptr+off, sizeof(buf) - off, "%c4.1",
+				minorvers41 > 0 ? '+' : '-');
 	for (n = NFSD_MINVERS; n <= NFSD_MAXVERS; n++) {
 		if (NFSCTL_VERISSET(ctlbits, n))
 		    off += snprintf(ptr+off, sizeof(buf) - off, "+%d ", n);
-- 
1.6.3.3

Re: [PATCH 2/2] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Wed, Jan 27, 2010 at 05:26:06PM -0500, J. Bruce Fields wrote:
> The current kernel code should not be enabled by default, because it
> does not yet attempt to be a conform completely to the rfc; for example,
> some required pieces of protocol are missing.
> 
> Therefore the kernel defaults to leaving minorversion1 off.  When the
> code matures sufficiently, that default will change.
> 
> That kernel default becomes meaningless if nfs-utils always explicitly
> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> 
> Provide a --enable-experimental-v41-support option to turn it on
> explicitly.  The option is intentionally spelled out (and has no short
> equivalent), to help ensure that users know what they're getting into.
> 
> Once 4.1 defaults to on, that option will become unnecessary (and can
> probably just be dropped from nfs-utils), and only the -N 4.1 option
> will be necessary.

We need to figure out how we're going to handle this.  The current
situation (ignoring the kernel's default) isn't acceptable.

We need to figure out something for v4 as well.  If every distro has run
nfsd without -N4, depending on the lack of fsid=0 to keep nfsv4 off by
default, then we're effectively turning v4 on by default with the
pseudoroot changes.  But there are good reasons why the v4 server code
is still marked experimental.

--b.

> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  support/include/nfs/nfs.h |    3 ---
>  utils/nfsd/nfsd.c         |   17 +++++++++++++----
>  utils/nfsd/nfssvc.c       |   10 ++++------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
> index a64eb0a..824f604 100644
> --- a/support/include/nfs/nfs.h
> +++ b/support/include/nfs/nfs.h
> @@ -13,9 +13,6 @@
>  #define NFSD_MINVERS 2
>  #define NFSD_MAXVERS 4
>  
> -#define NFSD_MINMINORVERS4 1
> -#define NFSD_MAXMINORVERS4 1
> -
>  struct nfs_fh_len {
>  	int		fh_size;
>  	u_int8_t	fh_handle[NFS3_FHSIZE];
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..c72f73e 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -49,6 +49,7 @@ static struct option longopts[] =
>  	{ "port", 1, 0, 'p' },
>  	{ "debug", 0, 0, 'd' },
>  	{ "syslog", 0, 0, 's' },
> +	{ "enable-experimental-v41-support", 0, 0, 'X' },
>  	{ NULL, 0, 0, 0 }
>  };
>  
> @@ -103,7 +104,7 @@ main(int argc, char **argv)
>  	char *p, *progname, *port;
>  	char *haddr = NULL;
>  	int	socket_up = 0;
> -	int minorvers4 = NFSD_MAXMINORVERS4;	/* nfsv4 minor version */
> +	int minorvers41 = 0;	/* nfsv4 minor version */
>  	unsigned int versbits = NFSCTL_ALLBITS;
>  	unsigned int protobits = NFSCTL_ALLBITS;
>  	unsigned int proto4 = 0;
> @@ -163,7 +164,12 @@ main(int argc, char **argv)
>  			switch((c = strtol(optarg, &p, 0))) {
>  			case 4:
>  				if (*p == '.') {
> -					minorvers4 = -atoi(p + 1);
> +					int i = atoi(p+1);
> +					if (i != 1) {
> +						fprintf(stderr, "%s: unsupported minor version\n", optarg);
> +						exit(1);
> +					}
> +					minorvers41 = -1;
>  					break;
>  				}
>  			case 3:
> @@ -185,6 +191,9 @@ main(int argc, char **argv)
>  		case 'U':
>  			NFSCTL_UDPUNSET(protobits);
>  			break;
> +		case 'X':
> +			minorvers41 = 1;
> +			break;
>  		default:
>  			fprintf(stderr, "Invalid argument: '%c'\n", c);
>  		case 'h':
> @@ -257,7 +266,7 @@ main(int argc, char **argv)
>  	 * registered with rpcbind. Note that on older kernels w/o the right
>  	 * interfaces, these are a no-op.
>  	 */
> -	nfssvc_setvers(versbits, minorvers4);
> +	nfssvc_setvers(versbits, minorvers41);
>   
>  	error = nfssvc_set_sockets(AF_INET, proto4, haddr, port);
>  	if (!error)
> @@ -309,7 +318,7 @@ static void
>  usage(const char *prog)
>  {
>  	fprintf(stderr, "Usage:\n"
> -		"%s [-d|--debug] [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-s|--syslog] [-T|--no-tcp] [-U|--no-udp] nrservs\n", 
> +		"%s [-d|--debug] [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-s|--syslog] [-T|--no-tcp] [-U|--no-udp] [--enable-experimental-v41-support] nrservs\n", 
>  		prog);
>  	exit(2);
>  }
> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> index 7bbbaba..ec9446a 100644
> --- a/utils/nfsd/nfssvc.c
> +++ b/utils/nfsd/nfssvc.c
> @@ -227,7 +227,7 @@ nfssvc_set_sockets(const int family, const unsigned int protobits,
>  }
>  
>  void
> -nfssvc_setvers(unsigned int ctlbits, int minorvers4)
> +nfssvc_setvers(unsigned int ctlbits, int minorvers41)
>  {
>  	int fd, n, off;
>  	char *ptr;
> @@ -238,11 +238,9 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
>  	if (fd < 0)
>  		return;
>  
> -	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
> -	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
> -		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
> -				    minorvers4 > 0 ? '+' : '-',
> -				    n);
> +	if (minorvers41)
> +		off += snprintf(ptr+off, sizeof(buf) - off, "%c4.1",
> +				minorvers41 > 0 ? '+' : '-');
>  	for (n = NFSD_MINVERS; n <= NFSD_MAXVERS; n++) {
>  		if (NFSCTL_VERISSET(ctlbits, n))
>  		    off += snprintf(ptr+off, sizeof(buf) - off, "+%d ", n);
> -- 
> 1.6.3.3
> 

Re: [PATCH 1/2] nfsd: fix version-setting regression on old kernels

[Steve Dickson]

On 01/27/2010 05:26 PM, J. Bruce Fields wrote:
> /proc/fs/nfsd/versions was extended to allow turning on/off minor
> versions by echoing "+4.1" or "-4.1" to /proc/fs/nsfd/versions.
> 
> Unfortunately, pre-2.6.30 kernels just stop parsing at first non-digit,
> so "-4.1" is interpreted as "-4".  If new nfs-utils (on old kernel)
> writes "+2", "+3", "+4", then "-4.1", result therefore is to turn off
> 4.1.
> 
> Given that historical behavior, it may have been a mistake to extend the
> interface the way we did; but at this point we're probably stuck with
> it.  So, just reverse the order we write versions in.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  utils/nfsd/nfssvc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> index b8028bb..7bbbaba 100644
> --- a/utils/nfsd/nfssvc.c
> +++ b/utils/nfsd/nfssvc.c
> @@ -238,17 +238,17 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
>  	if (fd < 0)
>  		return;
>  
> +	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
> +	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
> +		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
There needs to be a space between the %d and ".

But more importantly what problem is the patch fixing?? I've tested this
on a 2.6.29 and 2.6.31 as well as a 2.6.33 kernel and I see no different
in how the versions are ordered.
 
Note, as you probably already know, the kernel will always display
the versions in descending order...

steved.
 

Re: [PATCH 1/2] nfsd: fix version-setting regression on old kernels

[J. Bruce Fields]

On Thu, Feb 04, 2010 at 04:37:52PM -0500, Steve Dickson wrote:
> 
> 
> On 01/27/2010 05:26 PM, J. Bruce Fields wrote:
> > /proc/fs/nfsd/versions was extended to allow turning on/off minor
> > versions by echoing "+4.1" or "-4.1" to /proc/fs/nsfd/versions.
> > 
> > Unfortunately, pre-2.6.30 kernels just stop parsing at first non-digit,
> > so "-4.1" is interpreted as "-4".  If new nfs-utils (on old kernel)
> > writes "+2", "+3", "+4", then "-4.1", result therefore is to turn off
> > 4.1.
> > 
> > Given that historical behavior, it may have been a mistake to extend the
> > interface the way we did; but at this point we're probably stuck with
> > it.  So, just reverse the order we write versions in.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  utils/nfsd/nfssvc.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> > index b8028bb..7bbbaba 100644
> > --- a/utils/nfsd/nfssvc.c
> > +++ b/utils/nfsd/nfssvc.c
> > @@ -238,17 +238,17 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
> >  	if (fd < 0)
> >  		return;
> >  
> > +	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
> > +	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
> > +		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
> There needs to be a space between the %d and ".
> 
> But more importantly what problem is the patch fixing?? I've tested this
> on a 2.6.29 and 2.6.31 as well as a 2.6.33 kernel and I see no different
> in how the versions are ordered.

See the patch description.  A test case:

	- boot 2.6.29
	- run 'nfsd -N4.1'
	- try to mount v4.0.

See also https://bugzilla.redhat.com/show_bug.cgi?id=512377.

--b.

> Note, as you probably already know, the kernel will always display
> the versions in descending order...
> 
> steved.
>  
> 

Re: [PATCH 1/2] nfsd: fix version-setting regression on old kernels

[Steve Dickson]

On 02/04/2010 04:47 PM, J. Bruce Fields wrote:
> On Thu, Feb 04, 2010 at 04:37:52PM -0500, Steve Dickson wrote:
>>
>>
>> On 01/27/2010 05:26 PM, J. Bruce Fields wrote:
>>> /proc/fs/nfsd/versions was extended to allow turning on/off minor
>>> versions by echoing "+4.1" or "-4.1" to /proc/fs/nsfd/versions.
>>>
>>> Unfortunately, pre-2.6.30 kernels just stop parsing at first non-digit,
>>> so "-4.1" is interpreted as "-4".  If new nfs-utils (on old kernel)
>>> writes "+2", "+3", "+4", then "-4.1", result therefore is to turn off
>>> 4.1.
>>>
>>> Given that historical behavior, it may have been a mistake to extend the
>>> interface the way we did; but at this point we're probably stuck with
>>> it.  So, just reverse the order we write versions in.
>>>
>>> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>> ---
>>>  utils/nfsd/nfssvc.c |   10 +++++-----
>>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
>>> index b8028bb..7bbbaba 100644
>>> --- a/utils/nfsd/nfssvc.c
>>> +++ b/utils/nfsd/nfssvc.c
>>> @@ -238,17 +238,17 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
>>>  	if (fd < 0)
>>>  		return;
>>>  
>>> +	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
>>> +	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
>>> +		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
>> There needs to be a space between the %d and ".
>>
>> But more importantly what problem is the patch fixing?? I've tested this
>> on a 2.6.29 and 2.6.31 as well as a 2.6.33 kernel and I see no different
>> in how the versions are ordered.
> 
> See the patch description.  A test case:
> 
> 	- boot 2.6.29
> 	- run 'nfsd -N4.1'
> 	- try to mount v4.0.
> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=512377.
Ah... I see it now... for some reason I thought this was fixed already..

Committed...

steved.

Re: [PATCH 1/2] nfsd: fix version-setting regression on old kernels

[J. Bruce Fields]

On Thu, Feb 04, 2010 at 05:06:34PM -0500, Steve Dickson wrote:
> 
> 
> On 02/04/2010 04:47 PM, J. Bruce Fields wrote:
> > On Thu, Feb 04, 2010 at 04:37:52PM -0500, Steve Dickson wrote:
> >>
> >>
> >> On 01/27/2010 05:26 PM, J. Bruce Fields wrote:
> >>> /proc/fs/nfsd/versions was extended to allow turning on/off minor
> >>> versions by echoing "+4.1" or "-4.1" to /proc/fs/nsfd/versions.
> >>>
> >>> Unfortunately, pre-2.6.30 kernels just stop parsing at first non-digit,
> >>> so "-4.1" is interpreted as "-4".  If new nfs-utils (on old kernel)
> >>> writes "+2", "+3", "+4", then "-4.1", result therefore is to turn off
> >>> 4.1.
> >>>
> >>> Given that historical behavior, it may have been a mistake to extend the
> >>> interface the way we did; but at this point we're probably stuck with
> >>> it.  So, just reverse the order we write versions in.
> >>>
> >>> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> >>> ---
> >>>  utils/nfsd/nfssvc.c |   10 +++++-----
> >>>  1 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> >>> index b8028bb..7bbbaba 100644
> >>> --- a/utils/nfsd/nfssvc.c
> >>> +++ b/utils/nfsd/nfssvc.c
> >>> @@ -238,17 +238,17 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
> >>>  	if (fd < 0)
> >>>  		return;
> >>>  
> >>> +	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
> >>> +	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
> >>> +		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
> >> There needs to be a space between the %d and ".
> >>
> >> But more importantly what problem is the patch fixing?? I've tested this
> >> on a 2.6.29 and 2.6.31 as well as a 2.6.33 kernel and I see no different
> >> in how the versions are ordered.
> > 
> > See the patch description.  A test case:
> > 
> > 	- boot 2.6.29
> > 	- run 'nfsd -N4.1'
> > 	- try to mount v4.0.
> > 
> > See also https://bugzilla.redhat.com/show_bug.cgi?id=512377.
> Ah... I see it now... for some reason I thought this was fixed already..

I think you worked around it in fedora by dropping the -N4.1 from the
initscripts temporarily?

Thanks for catching the missing space, obviously I hadn't tested this
version properly!

--b.

Re: [PATCH 2/2] nfsd: default to kernel default for minorversion 1

[Steve Dickson]

On 02/01/2010 02:58 PM, J. Bruce Fields wrote:
> On Wed, Jan 27, 2010 at 05:26:06PM -0500, J. Bruce Fields wrote:
>> The current kernel code should not be enabled by default, because it
>> does not yet attempt to be a conform completely to the rfc; for example,
>> some required pieces of protocol are missing.
>>
>> Therefore the kernel defaults to leaving minorversion1 off.  When the
>> code matures sufficiently, that default will change.
>>
>> That kernel default becomes meaningless if nfs-utils always explicitly
>> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
>>
>> Provide a --enable-experimental-v41-support option to turn it on
>> explicitly.  The option is intentionally spelled out (and has no short
>> equivalent), to help ensure that users know what they're getting into.
Command options like this are so hard to get rid of.... We just can't introduce
an option one release and then have it go away a few releases down the road.
That's sure fire way to breaking existing configurations which something that is, 
has been and will continue to be unacceptable... 
 
>>
>> Once 4.1 defaults to on, that option will become unnecessary (and can
>> probably just be dropped from nfs-utils), and only the -N 4.1 option
>> will be necessary.
> 
> We need to figure out how we're going to handle this.  The current
> situation (ignoring the kernel's default) isn't acceptable.
Why can't each distro simple turn it off with there init scripts?

> 
> We need to figure out something for v4 as well.  If every distro has run
> nfsd without -N4, depending on the lack of fsid=0 to keep nfsv4 off by
> default, then we're effectively turning v4 on by default with the
> pseudoroot changes.  But there are good reasons why the v4 server code
> is still marked experimental.
I agree that with the latest kernels, v4 will be the default version. But
there are ways to override this on both the server and client. So why let
the users decided what they want?

steved.

Re: [PATCH 2/2] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Thu, Feb 04, 2010 at 05:19:56PM -0500, Steve Dickson wrote:
> On 02/01/2010 02:58 PM, J. Bruce Fields wrote:
> > On Wed, Jan 27, 2010 at 05:26:06PM -0500, J. Bruce Fields wrote:
> >> The current kernel code should not be enabled by default, because it
> >> does not yet attempt to be a conform completely to the rfc; for example,
> >> some required pieces of protocol are missing.
> >>
> >> Therefore the kernel defaults to leaving minorversion1 off.  When the
> >> code matures sufficiently, that default will change.
> >>
> >> That kernel default becomes meaningless if nfs-utils always explicitly
> >> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> >>
> >> Provide a --enable-experimental-v41-support option to turn it on
> >> explicitly.  The option is intentionally spelled out (and has no short
> >> equivalent), to help ensure that users know what they're getting into.
> Command options like this are so hard to get rid of.... We just can't introduce
> an option one release and then have it go away a few releases down the road.
> That's sure fire way to breaking existing configurations which something that is, 
> has been and will continue to be unacceptable... 

OK, fair enough.  It shouldn't be a problem to keep it indefinitely,
though.

> >> Once 4.1 defaults to on, that option will become unnecessary (and can
> >> probably just be dropped from nfs-utils), and only the -N 4.1 option
> >> will be necessary.
> > 
> > We need to figure out how we're going to handle this.  The current
> > situation (ignoring the kernel's default) isn't acceptable.
> Why can't each distro simple turn it off with there init scripts?

On the server-side the kernel doesn't have a separate config option for
V4.1; the only way for the kernel to indicate whether it has a mature
version of V4.1 is by its choice of default.

> > We need to figure out something for v4 as well.  If every distro has run
> > nfsd without -N4, depending on the lack of fsid=0 to keep nfsv4 off by
> > default, then we're effectively turning v4 on by default with the
> > pseudoroot changes.  But there are good reasons why the v4 server code
> > is still marked experimental.
> I agree that with the latest kernels, v4 will be the default version. But
> there are ways to override this on both the server and client. So why let
> the users decided what they want?

I don't understand.

--b.

Re: [PATCH 2/2] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Fri, Feb 05, 2010 at 11:10:02AM -0500, J. Bruce Fields wrote:
> On Thu, Feb 04, 2010 at 05:19:56PM -0500, Steve Dickson wrote:
> > On 02/01/2010 02:58 PM, J. Bruce Fields wrote:
> > > On Wed, Jan 27, 2010 at 05:26:06PM -0500, J. Bruce Fields wrote:
> > >> The current kernel code should not be enabled by default, because it
> > >> does not yet attempt to be a conform completely to the rfc; for example,
> > >> some required pieces of protocol are missing.
> > >>
> > >> Therefore the kernel defaults to leaving minorversion1 off.  When the
> > >> code matures sufficiently, that default will change.
> > >>
> > >> That kernel default becomes meaningless if nfs-utils always explicitly
> > >> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> > >>
> > >> Provide a --enable-experimental-v41-support option to turn it on
> > >> explicitly.  The option is intentionally spelled out (and has no short
> > >> equivalent), to help ensure that users know what they're getting into.
> > Command options like this are so hard to get rid of.... We just can't introduce
> > an option one release and then have it go away a few releases down the road.
> > That's sure fire way to breaking existing configurations which something that is, 
> > has been and will continue to be unacceptable... 
> 
> OK, fair enough.  It shouldn't be a problem to keep it indefinitely,
> though.

Actually, an even simpler option: instead of adding a new option, just
modify the code so that the *absence* of -N4.1 isn't taken to mean
"please turn on 4.1".  People can always just do their own

	echo +4.1 >/proc/fs/nfsd/version

if that's what they want.

--b.

[PATCH] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

The current kernel code should not be enabled by default, because it
does not yet attempt to be a conform completely to the rfc; for example,
some required pieces of protocol are missing.

Therefore the kernel defaults to leaving minorversion1 off.  When the
code matures sufficiently, that default will change.

That kernel default becomes meaningless if nfs-utils always explicitly
turns 4.1 on or off.  So, nfs-utils should by default do nothing.

Early adopters that want to turn on NFSv4.1 explicitly can still do so
using

	echo "+4.1" >/proc/fs/nfsd/versions

Signed-off-by: J.  Bruce Fields <bfields@citi.umich.edu>
---
 utils/nfsd/nfssvc.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
index 7bbbaba..1fb420d 100644
--- a/utils/nfsd/nfssvc.c
+++ b/utils/nfsd/nfssvc.c
@@ -239,10 +239,9 @@ nfssvc_setvers(unsigned int ctlbits, int minorvers4)
 		return;
 
 	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
-	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
-		    off += snprintf(ptr+off, sizeof(buf) - off, "%c4.%d",
-				    minorvers4 > 0 ? '+' : '-',
-				    n);
+	if (minorvers4 < 0 && n >= NFSD_MINMINORVERS4
+			   && n <= NFSD_MAXMINORVERS4)
+		    off += snprintf(ptr+off, sizeof(buf) - off, "-4.%d", n);
 	for (n = NFSD_MINVERS; n <= NFSD_MAXVERS; n++) {
 		if (NFSCTL_VERISSET(ctlbits, n))
 		    off += snprintf(ptr+off, sizeof(buf) - off, "+%d ", n);
-- 
1.6.3.3

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[Steve Dickson]

On 02/05/2010 03:05 PM, J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> The current kernel code should not be enabled by default, because it
> does not yet attempt to be a conform completely to the rfc; for example,
> some required pieces of protocol are missing.
> 
> Therefore the kernel defaults to leaving minorversion1 off.  When the
> code matures sufficiently, that default will change.
> 
> That kernel default becomes meaningless if nfs-utils always explicitly
> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> 
> Early adopters that want to turn on NFSv4.1 explicitly can still do so
> using
> 
> 	echo "+4.1" >/proc/fs/nfsd/versions
> 
When I write to /proc/fs/nfsd/versions I'm getting 
   write error: Device or resource busy

What did you do to make the file writeable?

steved.

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Fri, Feb 12, 2010 at 02:58:23PM -0500, Steve Dickson wrote:
> 
> 
> On 02/05/2010 03:05 PM, J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > The current kernel code should not be enabled by default, because it
> > does not yet attempt to be a conform completely to the rfc; for example,
> > some required pieces of protocol are missing.
> > 
> > Therefore the kernel defaults to leaving minorversion1 off.  When the
> > code matures sufficiently, that default will change.
> > 
> > That kernel default becomes meaningless if nfs-utils always explicitly
> > turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> > 
> > Early adopters that want to turn on NFSv4.1 explicitly can still do so
> > using
> > 
> > 	echo "+4.1" >/proc/fs/nfsd/versions
> > 
> When I write to /proc/fs/nfsd/versions I'm getting 
>    write error: Device or resource busy
> 
> What did you do to make the file writeable?

You just need to do it before starting nfsd.

So if it's just a one-off experiment you could

	/etc/init.d/nfs-server stop
	echo "+4.1" >/proc/fs/nsfd/versions
	/etc/init.d/nfs-server start

On machines where I was using 4.1 regularly I'd probably at a line to
the init script, or to a local init script that ran before it.

--b.

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[Steve Dickson]

On 02/12/2010 03:05 PM, J. Bruce Fields wrote:
> On Fri, Feb 12, 2010 at 02:58:23PM -0500, Steve Dickson wrote:
>>
>>
>> On 02/05/2010 03:05 PM, J. Bruce Fields wrote:
>>> From: J. Bruce Fields <bfields@citi.umich.edu>
>>>
>>> The current kernel code should not be enabled by default, because it
>>> does not yet attempt to be a conform completely to the rfc; for example,
>>> some required pieces of protocol are missing.
>>>
>>> Therefore the kernel defaults to leaving minorversion1 off.  When the
>>> code matures sufficiently, that default will change.
>>>
>>> That kernel default becomes meaningless if nfs-utils always explicitly
>>> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
>>>
>>> Early adopters that want to turn on NFSv4.1 explicitly can still do so
>>> using
>>>
>>> 	echo "+4.1" >/proc/fs/nfsd/versions
>>>
>> When I write to /proc/fs/nfsd/versions I'm getting 
>>    write error: Device or resource busy
>>
>> What did you do to make the file writeable?
> 
> You just need to do it before starting nfsd.
Well before nfsd starts but after the nfsd module is loaded
(assuming nfsd is a module)

> 
> So if it's just a one-off experiment you could
> 
> 	/etc/init.d/nfs-server stop
> 	echo "+4.1" >/proc/fs/nsfd/versions
> 	/etc/init.d/nfs-server start
> 
> On machines where I was using 4.1 regularly I'd probably at a line to
> the init script, or to a local init script that ran before it.
Yea.. I'm looking into do something of this nature...

The odd thing about this patch is 4.1 can only be turned off. There
is no way to enabled except from doing the above echo... which 
seems to beg the question, why have the 4.1 code in nfsd at all?

steved.
 

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Fri, Feb 12, 2010 at 04:44:03PM -0500, Steve Dickson wrote:
> 
> 
> On 02/12/2010 03:05 PM, J. Bruce Fields wrote:
> > On Fri, Feb 12, 2010 at 02:58:23PM -0500, Steve Dickson wrote:
> >>
> >>
> >> On 02/05/2010 03:05 PM, J. Bruce Fields wrote:
> >>> From: J. Bruce Fields <bfields@citi.umich.edu>
> >>>
> >>> The current kernel code should not be enabled by default, because it
> >>> does not yet attempt to be a conform completely to the rfc; for example,
> >>> some required pieces of protocol are missing.
> >>>
> >>> Therefore the kernel defaults to leaving minorversion1 off.  When the
> >>> code matures sufficiently, that default will change.
> >>>
> >>> That kernel default becomes meaningless if nfs-utils always explicitly
> >>> turns 4.1 on or off.  So, nfs-utils should by default do nothing.
> >>>
> >>> Early adopters that want to turn on NFSv4.1 explicitly can still do so
> >>> using
> >>>
> >>> 	echo "+4.1" >/proc/fs/nfsd/versions
> >>>
> >> When I write to /proc/fs/nfsd/versions I'm getting 
> >>    write error: Device or resource busy
> >>
> >> What did you do to make the file writeable?
> > 
> > You just need to do it before starting nfsd.
> Well before nfsd starts but after the nfsd module is loaded
> (assuming nfsd is a module)

Right.

> > So if it's just a one-off experiment you could
> > 
> > 	/etc/init.d/nfs-server stop
> > 	echo "+4.1" >/proc/fs/nsfd/versions
> > 	/etc/init.d/nfs-server start
> > 
> > On machines where I was using 4.1 regularly I'd probably at a line to
> > the init script, or to a local init script that ran before it.
> Yea.. I'm looking into do something of this nature...
> 
> The odd thing about this patch is 4.1 can only be turned off. There
> is no way to enabled except from doing the above echo... which 
> seems to beg the question, why have the 4.1 code in nfsd at all?

So people can test it.

We should not recommend it for production use at this point, at least
not by people who are not fully aware of what they're doing.

If you'd rather we added an option to rpc.nfsd to do the equivalent of
the echo, that's fine.  But I think for testers/early adopters, adding a
"echo" to their init script isn't a big deal.

Of course, once it's mature we can flip the kernel default to on, and
nobody will have to do anything.

--b.

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[Steve Dickson]

The fact there needed to be yet another code change to
re-enabled the 4.1 functionality bother me... So This
patch basically does the same as your patch, does not
write "+4.1" to the versions file. But it also introduces
a configuration variable that will allow us to re-enabled
the functionality w/out changing any code...

BTW, there was precedence with adding this type of 
configuration variable since there has been
NFS3_SUPPORTED and NFS4_SUPPORTED variables in the 
past.

steved.

commit 6d5ac3fa75024be569b458f4d9b6ce05be47f601
Author: Steve Dickson <steved@redhat.com>
Date:   Wed Feb 17 14:38:19 2010 -0500

    nfsd: Disble NFS 4.1 functionality by default
    
    Due to the fact the current kernel code do not completely
    conform to the NFS 4.1 RFC, this patch disable the 4.1 support
    on the server.
    
    To control this 41 functionality, the NFS41_SUPPORTED
    configuration variable now exist that will allow us to
    re enable the functionality  without any code changes.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/configure.ac b/configure.ac
index 1dc4249..f6b1189 100644
--- a/configure.ac
+++ b/configure.ac
@@ -72,6 +72,20 @@ AC_ARG_ENABLE(nfsv4,
 	AC_SUBST(IDMAPD)
 	AC_SUBST(enable_nfsv4)
 	AM_CONDITIONAL(CONFIG_NFSV4, [test "$enable_nfsv4" = "yes"])
+
+AC_ARG_ENABLE(nfsv41,
+	[AC_HELP_STRING([--enable-nfsv41],
+                        [enable support for NFSv41 @<:@default=no@:>@])],
+	enable_nfsv41=$enableval,
+	enable_nfsv41=no)
+	if test "$enable_nfsv41" = yes; then
+		AC_DEFINE(NFS41_SUPPORTED, 1, [Define this if you want NFSv41 support compiled in])
+	else
+		enable_nfsv4=
+	fi
+	AC_SUBST(enable_nfsv41)
+	AM_CONDITIONAL(CONFIG_NFSV41, [test "$enable_nfsv41" = "yes"])
+
 AC_ARG_ENABLE(gss,
 	[AC_HELP_STRING([--enable-gss],
                         [enable support for rpcsec_gss @<:@default=yes@:>@])],
diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index a64eb0a..c939d78 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -1,6 +1,8 @@
 #ifndef _NFS_NFS_H
 #define _NFS_NFS_H
 
+#include <config.h>
+
 #include <linux/posix_types.h>
 #include <sys/types.h>
 #include <netinet/in.h>
@@ -14,7 +16,11 @@
 #define NFSD_MAXVERS 4
 
 #define NFSD_MINMINORVERS4 1
+#ifdef  NFS41_SUPPORTED
 #define NFSD_MAXMINORVERS4 1
+#else
+#define NFSD_MAXMINORVERS4 0
+#endif
 
 struct nfs_fh_len {
 	int		fh_size;

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[Steve Dickson]

On 02/17/2010 02:46 PM, Steve Dickson wrote:
> The fact there needed to be yet another code change to
> re-enabled the 4.1 functionality bother me... So This
> patch basically does the same as your patch, does not
> write "+4.1" to the versions file. But it also introduces
> a configuration variable that will allow us to re-enabled
> the functionality w/out changing any code...
> 
> BTW, there was precedence with adding this type of 
> configuration variable since there has been
> NFS3_SUPPORTED and NFS4_SUPPORTED variables in the 
> past.
> 
> steved.
> 
> commit 6d5ac3fa75024be569b458f4d9b6ce05be47f601
> Author: Steve Dickson <steved@redhat.com>
> Date:   Wed Feb 17 14:38:19 2010 -0500
> 
>     nfsd: Disble NFS 4.1 functionality by default
>     
>     Due to the fact the current kernel code do not completely
>     conform to the NFS 4.1 RFC, this patch disable the 4.1 support
>     on the server.
>     
>     To control this 41 functionality, the NFS41_SUPPORTED
>     configuration variable now exist that will allow us to
>     re enable the functionality  without any code changes.
Committed...

steved.

Re: [PATCH] nfsd: default to kernel default for minorversion 1

[J. Bruce Fields]

On Wed, Feb 17, 2010 at 02:46:42PM -0500, Steve Dickson wrote:
> The fact there needed to be yet another code change to
> re-enabled the 4.1 functionality bother me... So This
> patch basically does the same as your patch, does not
> write "+4.1" to the versions file. But it also introduces
> a configuration variable that will allow us to re-enabled
> the functionality w/out changing any code...

The correct way to enable 4.1 functionality is with a patch to the
kernel that changes 4.1 to default to on.  We'll do that once a minimal
4.1 implementation is finished.

What this patch does is allow a distribution to force 4.1 on by default
even for kernel which have not reached that level of maturity.  I
would strongly advise *against* any distribution building with such an
option.

In addition, this patch disables -N4 in the disable_nfsv4 case, which is
incorrect; a user that doesn't wish nfs-utils to override the kernel's
default should still be able to turn off 4.1 on kernels that do default
4.1 to on.

Please don't do this.

--b.

> 
> BTW, there was precedence with adding this type of 
> configuration variable since there has been
> NFS3_SUPPORTED and NFS4_SUPPORTED variables in the 
> past.
> 
> steved.
> 
> commit 6d5ac3fa75024be569b458f4d9b6ce05be47f601
> Author: Steve Dickson <steved@redhat.com>
> Date:   Wed Feb 17 14:38:19 2010 -0500
> 
>     nfsd: Disble NFS 4.1 functionality by default
>     
>     Due to the fact the current kernel code do not completely
>     conform to the NFS 4.1 RFC, this patch disable the 4.1 support
>     on the server.
>     
>     To control this 41 functionality, the NFS41_SUPPORTED
>     configuration variable now exist that will allow us to
>     re enable the functionality  without any code changes.
>     
>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/configure.ac b/configure.ac
> index 1dc4249..f6b1189 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -72,6 +72,20 @@ AC_ARG_ENABLE(nfsv4,
>  	AC_SUBST(IDMAPD)
>  	AC_SUBST(enable_nfsv4)
>  	AM_CONDITIONAL(CONFIG_NFSV4, [test "$enable_nfsv4" = "yes"])
> +
> +AC_ARG_ENABLE(nfsv41,
> +	[AC_HELP_STRING([--enable-nfsv41],
> +                        [enable support for NFSv41 @<:@default=no@:>@])],
> +	enable_nfsv41=$enableval,
> +	enable_nfsv41=no)
> +	if test "$enable_nfsv41" = yes; then
> +		AC_DEFINE(NFS41_SUPPORTED, 1, [Define this if you want NFSv41 support compiled in])
> +	else
> +		enable_nfsv4=
> +	fi
> +	AC_SUBST(enable_nfsv41)
> +	AM_CONDITIONAL(CONFIG_NFSV41, [test "$enable_nfsv41" = "yes"])
> +
>  AC_ARG_ENABLE(gss,
>  	[AC_HELP_STRING([--enable-gss],
>                          [enable support for rpcsec_gss @<:@default=yes@:>@])],
> diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
> index a64eb0a..c939d78 100644
> --- a/support/include/nfs/nfs.h
> +++ b/support/include/nfs/nfs.h
> @@ -1,6 +1,8 @@
>  #ifndef _NFS_NFS_H
>  #define _NFS_NFS_H
>  
> +#include <config.h>
> +
>  #include <linux/posix_types.h>
>  #include <sys/types.h>
>  #include <netinet/in.h>
> @@ -14,7 +16,11 @@
>  #define NFSD_MAXVERS 4
>  
>  #define NFSD_MINMINORVERS4 1
> +#ifdef  NFS41_SUPPORTED
>  #define NFSD_MAXMINORVERS4 1
> +#else
> +#define NFSD_MAXMINORVERS4 0
> +#endif
>  
>  struct nfs_fh_len {
>  	int		fh_size;
>