[RFC 0/4] nfs-utils: nfsd support for minor version

[RFC 0/4] nfs-utils: nfsd support for minor version

[Benny Halevy]

Steve, please review the following patch that add support
for controlling the nfsv4 minor version support via
/proc/fs/nfsd/versions.

[PATCH 1/4] utils/nfsd: fix -N optarg error printout
	This patch fixes an existing bug.

[RFC 2/4] utils/nfsd: add support for minorvers4
	Under-the-cover support for minorvers4

[RFC 3/4] utils/nfsd: add -n --nfs-version option
	Add a command line option to enable versions

[RFC 3/4] utils/nfsd: enable/disable minorvers4 via command line
	Extend -n/-N syntax to accept <version>.<minorversion>

Thanks,

Benny

[PATCH 1/4] utils/nfsd: fix -N optarg error printout

[Benny Halevy]

as currently printed c is the version number, not a string char,
therefore is should be printed as %d not %c.  That said, just print
optarg as %s since it might be non-numeric.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 utils/nfsd/nfsd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index aaf8d29..c97c81f 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -86,7 +86,7 @@ main(int argc, char **argv)
 				NFSCTL_VERUNSET(versbits, c);
 				break;
 			default:
-				fprintf(stderr, "%c: Unsupported version\n", c);
+				fprintf(stderr, "%s: Unsupported version\n", optarg);
 				exit(1);
 			}
 			break;
-- 
1.6.2.1

[RFC 2/4] utils/nfsd: add support for minorvers4

[Benny Halevy]

minorvers4 can be used to either enable or disable nfsv4.x.

If minorvers4 is a positive integer n, in the allowed range (only
minorversion 1 is supported for now), the string "+4.n" is appended
to the versions string written onto /proc/fs/nfsd/versions.

Correspondingly, if minorver4 is a negative integer -n, the string
"-4.n" is written.

With the default value, minorvers4==0, the minor version
setting is not changed.

Note that unlike the protocol versions 2, 3, or 4.  The minor version
setting controls the *maximum* minor version nfsd supports.  Particular
minor version cannot be controlled on their own.  With only minor version
1 supported at the moment the difference doesn't matter, but for future
minor versions greater than 1, enabling minor version X will enable support
for all minor versions 1 through X. Disabling minor version X will disable
support for minor versions X and up, enabling 1 through X-1.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 support/include/nfs/nfs.h |    3 +++
 support/include/nfslib.h  |    2 +-
 support/nfs/nfssvc.c      |   13 +++++++++----
 utils/nfsd/nfsd.c         |    3 ++-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index fc26f4e..00b0028 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -13,6 +13,9 @@
 #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/support/include/nfslib.h b/support/include/nfslib.h
index 9d0d39d..ae98650 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -130,7 +130,7 @@ int			wildmat(char *text, char *pattern);
  * nfsd library functions.
  */
 int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
-int			nfssvc(int port, int nrservs, unsigned int versbits, unsigned int portbits, char *haddr);
+int			nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4, unsigned int portbits, char *haddr);
 int			nfsaddclient(struct nfsctl_client *clp);
 int			nfsdelclient(struct nfsctl_client *clp);
 int			nfsexport(struct nfsctl_export *exp);
diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 9bbc9a5..33c15a7 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -116,7 +116,7 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
 	return;
 }
 static void
-nfssvc_versbits(unsigned int ctlbits)
+nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 {
 	int fd, n, off;
 	char buf[BUFSIZ], *ptr;
@@ -133,6 +133,11 @@ nfssvc_versbits(unsigned int ctlbits)
 		else
 		    off += snprintf(ptr+off, BUFSIZ - off, "-%d ", n);
 	}
+	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
+	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
+		    off += snprintf(ptr+off, BUFSIZ - off, "%c4.%d",
+				    minorvers4 > 0 ? '+' : '-',
+				    n);
 	snprintf(ptr+off, BUFSIZ - off, "\n");
 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
 		syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)", 
@@ -143,8 +148,8 @@ nfssvc_versbits(unsigned int ctlbits)
 	return;
 }
 int
-nfssvc(int port, int nrservs, unsigned int versbits, unsigned protobits,
-	char *haddr)
+nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
+	unsigned protobits, char *haddr)
 {
 	struct nfsctl_arg	arg;
 	int fd;
@@ -153,7 +158,7 @@ nfssvc(int port, int nrservs, unsigned int versbits, unsigned protobits,
 	 * the ports get registered with portmap against correct
 	 * versions
 	 */
-	nfssvc_versbits(versbits);
+	nfssvc_versbits(versbits, minorvers4);
 	nfssvc_setfds(port, protobits, haddr);
 
 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index c97c81f..ac264da 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -41,6 +41,7 @@ static struct option longopts[] =
 };
 unsigned int protobits = NFSCTL_ALLBITS;
 unsigned int versbits = NFSCTL_ALLBITS;
+int minorvers4;				/* nfsv4 minor version */
 char *haddr = NULL;
 
 int
@@ -158,7 +159,7 @@ main(int argc, char **argv)
 	closeall(3);
 
 	openlog("nfsd", LOG_PID, LOG_DAEMON);
-	if ((error = nfssvc(port, count, versbits, protobits, haddr)) < 0) {
+	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) {
 		int e = errno;
 		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
 		closelog();
-- 
1.6.2.1

[RFC 3/4] utils/nfsd: add -n --nfs-version option

[Benny Halevy]

Add the -n --nfs-version option to enable versions, in contrast
to -N --no-nfs-version.  This will be useful for enabling nfsv4
minor versions.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 support/include/nfs/nfs.h |    2 ++
 utils/nfsd/nfsd.c         |   17 +++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index 00b0028..2220305 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -42,6 +42,8 @@ struct nfs_fh_old {
 #define NFSCTL_GETFD		7	/* get an fh by path (used by mountd) */
 #define NFSCTL_GETFS		8	/* get an fh by path with max size (used by mountd) */
 
+#define NFSCTL_VERSET(_cltbits, _v)   ((_cltbits) |=  (1 << ((_v) - 1)))
+
 #define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) - 1))) 
 #define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (17 - 1))) 
 #define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (18 - 1))) 
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index ac264da..3d88b72 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -32,6 +32,7 @@ static struct option longopts[] =
 {
 	{ "host", 1, 0, 'H' },
 	{ "help", 0, 0, 'h' },
+	{ "nfs-version", 1, 0, 'n' },
 	{ "no-nfs-version", 1, 0, 'N' },
 	{ "no-tcp", 0, 0, 'T' },
 	{ "no-udp", 0, 0, 'U' },
@@ -57,7 +58,7 @@ main(int argc, char **argv)
 	else
 		port = 2049;
 
-	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts, NULL)) != EOF) {
+	while ((c = getopt_long(argc, argv, "H:hn:N:p:P:TU", longopts, NULL)) != EOF) {
 		switch(c) {
 		case 'H':
 			if (inet_addr(optarg) != INADDR_NONE) {
@@ -79,6 +80,18 @@ main(int argc, char **argv)
 				usage(argv [0]);
 			}
 			break;
+		case 'n':
+			switch((c = atoi(optarg))) {
+			case 4:
+			case 3:
+			case 2:
+				NFSCTL_VERSET(versbits, c);
+				break;
+			default:
+				fprintf(stderr, "%s: Unsupported version\n", optarg);
+				exit(1);
+			}
+			break;
 		case 'N':
 			switch((c = atoi(optarg))) {
 			case 2:
@@ -172,7 +185,7 @@ static void
 usage(const char *prog)
 {
 	fprintf(stderr, "Usage:\n"
-		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-T|--no-tcp] [-U|--no-udp] nrservs\n", 
+		"%s [-H hostname] [-p|-P|--port port] [-n|--nfs-version version] [-N|--no-nfs-version version] [-T|--no-tcp] [-U|--no-udp] nrservs\n", 
 		prog);
 	exit(2);
 }
-- 
1.6.2.1

[RFC 4/4] utils/nfsd: enable/disable minorvers4 via command line

[Benny Halevy]

Extend -n/-N command line option syntax to accept <version>.<minorversion>
Only 4.1 is currently supported.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 utils/nfsd/nfsd.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 3d88b72..ce88989 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -51,6 +51,7 @@ main(int argc, char **argv)
 	int	count = 1, c, error, port, fd, found_one;
 	struct servent *ent;
 	struct hostent *hp;
+	char *p;
 
 	ent = getservbyname ("nfs", "udp");
 	if (ent != NULL)
@@ -81,8 +82,12 @@ main(int argc, char **argv)
 			}
 			break;
 		case 'n':
-			switch((c = atoi(optarg))) {
+			switch((c = strtol(optarg, &p, 0))) {
 			case 4:
+				if (*p == '.') {
+					minorvers4 = atoi(p + 1);
+					break;
+				}
 			case 3:
 			case 2:
 				NFSCTL_VERSET(versbits, c);
@@ -93,10 +98,14 @@ main(int argc, char **argv)
 			}
 			break;
 		case 'N':
-			switch((c = atoi(optarg))) {
-			case 2:
-			case 3:
+			switch((c = strtol(optarg, &p, 0))) {
 			case 4:
+				if (*p == '.') {
+					minorvers4 = -atoi(p + 1);
+					break;
+				}
+			case 3:
+			case 2:
 				NFSCTL_VERUNSET(versbits, c);
 				break;
 			default:
-- 
1.6.2.1

Re: [RFC 0/4] nfs-utils: nfsd support for minor version

[Steve Dickson]

Hey Benny,

Benny Halevy wrote:
> Steve, please review the following patch that add support
> for controlling the nfsv4 minor version support via
> /proc/fs/nfsd/versions.
> 
> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
> 	This patch fixes an existing bug.
This is a bug... and has been committed...

> 
> [RFC 2/4] utils/nfsd: add support for minorvers4
> 	Under-the-cover support for minorvers4
I see you let minorvers4 default to zero, which means 4.1
support is off by default. Why? As long as we have away to
turn of 4.1 processing (i.e. your 4/4 patch), then I see
no reason we should have the support enabled by default. 

> 
> [RFC 3/4] utils/nfsd: add -n --nfs-version option
> 	Add a command line option to enable versions
I'm assuming you added this to be able to turn on 4.1 processing.
But 4.1 is on by default, the '-n' flag not need, correct?

> 
> [RFC 4/4] utils/nfsd: enable/disable minorvers4 via command line
> 	Extend -n/-N syntax to accept <version>.<minorversion>
look reasonable... 

steved.

Re: [RFC 0/4] nfs-utils: nfsd support for minor version

[Benny Halevy]

On Apr. 16, 2009, 20:24 +0300, Steve Dickson <SteveD@redhat.com> wrote:
> Hey Benny,
> 
> Benny Halevy wrote:
>> Steve, please review the following patch that add support
>> for controlling the nfsv4 minor version support via
>> /proc/fs/nfsd/versions.
>>
>> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
>> 	This patch fixes an existing bug.
> This is a bug... and has been committed...
> 
>> [RFC 2/4] utils/nfsd: add support for minorvers4
>> 	Under-the-cover support for minorvers4
> I see you let minorvers4 default to zero, which means 4.1
> support is off by default. Why? As long as we have away to
> turn of 4.1 processing (i.e. your 4/4 patch), then I see
> no reason we should have the support enabled by default. 

I was also thinking about using the new nfs-utils with old kernels.
Though, these should not puke on seeing [-+]4.1, they'll
just interpret it as enabling/disabling v4.
I'll test that...

> 
>> [RFC 3/4] utils/nfsd: add -n --nfs-version option
>> 	Add a command line option to enable versions
> I'm assuming you added this to be able to turn on 4.1 processing.
> But 4.1 is on by default, the '-n' flag not need, correct?

Right, and that would follow the existing design.

> 
>> [RFC 4/4] utils/nfsd: enable/disable minorvers4 via command line
>> 	Extend -n/-N syntax to accept <version>.<minorversion>
> look reasonable... 

Great.  Thanks for the review!

I think I'll have some time next week 
to revise and resend this patchset.

Benny

> 
> steved.

Re: [RFC 0/4] nfs-utils: nfsd support for minor version

[Steve Dickson]

Benny Halevy wrote:
>> Benny Halevy wrote:
>>> Steve, please review the following patch that add support
>>> for controlling the nfsv4 minor version support via
>>> /proc/fs/nfsd/versions.
>>>
>>> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
>>> 	This patch fixes an existing bug.
>> This is a bug... and has been committed...
>>
>>> [RFC 2/4] utils/nfsd: add support for minorvers4
>>> 	Under-the-cover support for minorvers4
>> I see you let minorvers4 default to zero, which means 4.1
>> support is off by default. Why? As long as we have away to
>> turn of 4.1 processing (i.e. your 4/4 patch), then I see
>> no reason we should have the support enabled by default. 
> 
> I was also thinking about using the new nfs-utils with old kernels.
> Though, these should not puke on seeing [-+]4.1, they'll
> just interpret it as enabling/disabling v4.
> I'll test that...
I just did... Using the '-N 4.1' flag to rpc.nfsd on an older 
kernel simply turns off all v4 processing... which is fine, IMHO...

steved.
 

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[J. Bruce Fields]

On Thu, Apr 16, 2009 at 02:13:16PM -0400, Steve Dickson wrote:
> Benny Halevy wrote:
> >> Benny Halevy wrote:
> >>> Steve, please review the following patch that add support
> >>> for controlling the nfsv4 minor version support via
> >>> /proc/fs/nfsd/versions.
> >>>
> >>> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
> >>> 	This patch fixes an existing bug.
> >> This is a bug... and has been committed...
> >>
> >>> [RFC 2/4] utils/nfsd: add support for minorvers4
> >>> 	Under-the-cover support for minorvers4
> >> I see you let minorvers4 default to zero, which means 4.1
> >> support is off by default. Why? As long as we have away to
> >> turn of 4.1 processing (i.e. your 4/4 patch), then I see
> >> no reason we should have the support enabled by default. 

Was there a typo there?  You ask why 4.1 support is off, then say you
see "no reason we should have the support enabled by default."  Those
two statements seem to agree?  Did you mean "no reason we should
not..."?

> > 
> > I was also thinking about using the new nfs-utils with old kernels.
> > Though, these should not puke on seeing [-+]4.1, they'll
> > just interpret it as enabling/disabling v4.
> > I'll test that...
> I just did... Using the '-N 4.1' flag to rpc.nfsd on an older 
> kernel simply turns off all v4 processing... which is fine, IMHO...

If you want v4 running, but don't trust v4.1 yet, and want to use a
mixture of new and old kernels--what configuration will you use?

--b.

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[Benny Halevy]

On Apr. 16, 2009, 21:23 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Thu, Apr 16, 2009 at 02:13:16PM -0400, Steve Dickson wrote:
>> Benny Halevy wrote:
>>>> Benny Halevy wrote:
>>>>> Steve, please review the following patch that add support
>>>>> for controlling the nfsv4 minor version support via
>>>>> /proc/fs/nfsd/versions.
>>>>>
>>>>> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
>>>>> 	This patch fixes an existing bug.
>>>> This is a bug... and has been committed...
>>>>
>>>>> [RFC 2/4] utils/nfsd: add support for minorvers4
>>>>> 	Under-the-cover support for minorvers4
>>>> I see you let minorvers4 default to zero, which means 4.1
>>>> support is off by default. Why? As long as we have away to
>>>> turn of 4.1 processing (i.e. your 4/4 patch), then I see
>>>> no reason we should have the support enabled by default. 
> 
> Was there a typo there?  You ask why 4.1 support is off, then say you
> see "no reason we should have the support enabled by default."  Those
> two statements seem to agree?  Did you mean "no reason we should
> not..."?

(that is what I assumed.)

> 
>>> I was also thinking about using the new nfs-utils with old kernels.
>>> Though, these should not puke on seeing [-+]4.1, they'll
>>> just interpret it as enabling/disabling v4.
>>> I'll test that...
>> I just did... Using the '-N 4.1' flag to rpc.nfsd on an older 
>> kernel simply turns off all v4 processing... which is fine, IMHO...
> 
> If you want v4 running, but don't trust v4.1 yet, and want to use a
> mixture of new and old kernels--what configuration will you use?

It's a bit hackish, but I think that prepending 4.1 to 4 should
do the trick.

Writing "+2 +3 -4.1 +4" will allow you to disable
4.1 while enabling 4.0 on all kernels, old and new.

"+2 +3 +4.1 +4" would enable 4.0, and 4.1 on kernels
that support it.

Benny

> 
> --b.

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[Steve Dickson]

J. Bruce Fields wrote:
> On Thu, Apr 16, 2009 at 02:13:16PM -0400, Steve Dickson wrote:
>> Benny Halevy wrote:
>>>> Benny Halevy wrote:
>>>>> Steve, please review the following patch that add support
>>>>> for controlling the nfsv4 minor version support via
>>>>> /proc/fs/nfsd/versions.
>>>>>
>>>>> [PATCH 1/4] utils/nfsd: fix -N optarg error printout
>>>>> 	This patch fixes an existing bug.
>>>> This is a bug... and has been committed...
>>>>
>>>>> [RFC 2/4] utils/nfsd: add support for minorvers4
>>>>> 	Under-the-cover support for minorvers4
>>>> I see you let minorvers4 default to zero, which means 4.1
>>>> support is off by default. Why? As long as we have away to
>>>> turn of 4.1 processing (i.e. your 4/4 patch), then I see
>>>> no reason we should have the support enabled by default. 
> 
> Was there a typo there?  You ask why 4.1 support is off, then say you
> see "no reason we should have the support enabled by default."  Those
> two statements seem to agree?  Did you mean "no reason we should
> not..."?
Yes... there was a typeo...  I meant to say I see no reason we 
should *not* have the v4.1 support enabled by default...

note to self... don't send email while in meetings... :-(

> 
>>> I was also thinking about using the new nfs-utils with old kernels.
>>> Though, these should not puke on seeing [-+]4.1, they'll
>>> just interpret it as enabling/disabling v4.
>>> I'll test that...
>> I just did... Using the '-N 4.1' flag to rpc.nfsd on an older 
>> kernel simply turns off all v4 processing... which is fine, IMHO...
> 
> If you want v4 running, but don't trust v4.1 yet, and want to use a
> mixture of new and old kernels--what configuration will you use?
I guess you're making the assumption that the client is v4.1 aware 
and is enabled by default... In that case I would suggest running
a 2.6.30+ kernel and use the '-N 4.1' flag... In all other cases
I think doing nothing (i.e. not specifying any flags) would work.

I do get your point, but as we did with the initial v4 support, 
having the support on by default and then having away to turn it
off is the correct approach... IMHO... 

steved.

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[J. Bruce Fields]

On Thu, Apr 16, 2009 at 03:01:47PM -0400, Steve Dickson wrote:
> I do get your point, but as we did with the initial v4 support, 
> having the support on by default and then having away to turn it
> off is the correct approach... IMHO... 

I'd prefer it be off by default, for the obvious safety reasons.  (It's
under rapid development and particularly likely to have bugs).  The only
reason we had it on by default before was that we didn't add the
switching mechanism early enough.  (Well, and because we could keep it
off in the config.  But I'd rather be able to ship users a kernel that
supports 4.1 and give them the option of turning it on at runtime, than
make them build a new kernel.)

--b.

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[Steve Dickson]

J. Bruce Fields wrote:
> On Thu, Apr 16, 2009 at 03:01:47PM -0400, Steve Dickson wrote:
>> I do get your point, but as we did with the initial v4 support, 
>> having the support on by default and then having away to turn it
>> off is the correct approach... IMHO... 
> 
> I'd prefer it be off by default, for the obvious safety reasons.  (It's
> under rapid development and particularly likely to have bugs).  The only
> reason we had it on by default before was that we didn't add the
> switching mechanism early enough.  (Well, and because we could keep it
> off in the config.  But I'd rather be able to ship users a kernel that
> supports 4.1 and give them the option of turning it on at runtime, than
> make them build a new kernel.)
I agree with not making people recompile kernels, which is the whole
purpose behind the Fedora repos, but do I think you might be a bit 
too cautious with exposing the technology. 

One, I've been running the kernels with everything enabled for a while
now with no problems whatsoever... A few scary looking warnings now and
then but nothing major. I also spent the majority of my time at Connectathon
this years testing with kernel that were fully enabled. Not one problem
WRT regression testing. Plus there is no better way to expose regression 
problems (early on) than to enable the code.. IMHO... 

Second, its my understanding that clients have to explicitly  ask
for 4.1 support. Are there any client out there that default to 
4.1 support? I would think not since there is only one client out there
that defaults to V4.0. If there is a client that defaults to 4.1, then we 
will a knob to turn that support off.

Finally, with rpc.nfsd the precedence has already been set that we disable
versions, not enabled them. For us to start enabling versions with rpc.nfsd
we would have to come up with another command line flags (similar to Benny's
patch). This means we would have one flag that enables versions and 
another that would disable them... talk about confusion... and that's just
not right... IMHO.... 

So I say, lets stay with the precedence that was already set, enable
the support by default but have a way to disable it... 

steved.



   

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[J. Bruce Fields]

On Fri, Apr 17, 2009 at 08:35:36AM -0400, Steve Dickson wrote:
> J. Bruce Fields wrote:
> > On Thu, Apr 16, 2009 at 03:01:47PM -0400, Steve Dickson wrote:
> >> I do get your point, but as we did with the initial v4 support, 
> >> having the support on by default and then having away to turn it
> >> off is the correct approach... IMHO... 
> > 
> > I'd prefer it be off by default, for the obvious safety reasons.  (It's
> > under rapid development and particularly likely to have bugs).  The only
> > reason we had it on by default before was that we didn't add the
> > switching mechanism early enough.  (Well, and because we could keep it
> > off in the config.  But I'd rather be able to ship users a kernel that
> > supports 4.1 and give them the option of turning it on at runtime, than
> > make them build a new kernel.)
> I agree with not making people recompile kernels, which is the whole
> purpose behind the Fedora repos, but do I think you might be a bit 
> too cautious with exposing the technology. 
> 
> One, I've been running the kernels with everything enabled for a while
> now with no problems whatsoever... A few scary looking warnings now and
> then but nothing major. I also spent the majority of my time at Connectathon
> this years testing with kernel that were fully enabled. Not one problem
> WRT regression testing. Plus there is no better way to expose regression 
> problems (early on) than to enable the code.. IMHO... 
> 
> Second, its my understanding that clients have to explicitly  ask
> for 4.1 support. Are there any client out there that default to 
> 4.1 support? I would think not since there is only one client out there
> that defaults to V4.0. If there is a client that defaults to 4.1, then we 
> will a knob to turn that support off.

We can't really control what all (especially, all future) clients will
do.  Only the server can know how mature its own code is, so it's the
server's default that matters here.

There's also a greater risk of security problems with younger code, and
a vulnerable 4.1 server can be exploited without the need for a 4.1
client.

If rpc.nfsd itself is going to default to turning this on, then at the
very least I would strongly advise distributions for now to ship an
/etc/syconfig/ or /etc/defaults/ file that defaults to turning it off.

> Finally, with rpc.nfsd the precedence has already been set that we disable
> versions, not enabled them. For us to start enabling versions with rpc.nfsd
> we would have to come up with another command line flags (similar to Benny's
> patch). This means we would have one flag that enables versions and 
> another that would disable them... talk about confusion... and that's just
> not right... IMHO.... 

I agree that it's annoying to need another flag, but if that's the only
way to keep 4.1 defaulted off then on balance it's the right thing to
do.  I think it'd be OK to change the default later and remove the need
for the flag.  So only early adopters will need to know about it.

--b.

> So I say, lets stay with the precedence that was already set, enable
> the support by default but have a way to disable it... 

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[Chuck Lever]

On Apr 17, 2009, at 8:35 AM, Steve Dickson wrote:
> J. Bruce Fields wrote:
>> On Thu, Apr 16, 2009 at 03:01:47PM -0400, Steve Dickson wrote:
>>> I do get your point, but as we did with the initial v4 support,
>>> having the support on by default and then having away to turn it
>>> off is the correct approach... IMHO...
>>
>> I'd prefer it be off by default, for the obvious safety reasons.   
>> (It's
>> under rapid development and particularly likely to have bugs).  The  
>> only
>> reason we had it on by default before was that we didn't add the
>> switching mechanism early enough.  (Well, and because we could keep  
>> it
>> off in the config.  But I'd rather be able to ship users a kernel  
>> that
>> supports 4.1 and give them the option of turning it on at runtime,  
>> than
>> make them build a new kernel.)
> I agree with not making people recompile kernels, which is the whole
> purpose behind the Fedora repos, but do I think you might be a bit
> too cautious with exposing the technology.
>
> One, I've been running the kernels with everything enabled for a while
> now with no problems whatsoever... A few scary looking warnings now  
> and
> then but nothing major. I also spent the majority of my time at  
> Connectathon
> this years testing with kernel that were fully enabled. Not one  
> problem
> WRT regression testing. Plus there is no better way to expose  
> regression
> problems (early on) than to enable the code.. IMHO...
>
> Second, its my understanding that clients have to explicitly  ask
> for 4.1 support. Are there any client out there that default to
> 4.1 support? I would think not since there is only one client out  
> there
> that defaults to V4.0. If there is a client that defaults to 4.1,  
> then we
> will a knob to turn that support off.

That might even be OK for Fedora-based NFS servers.  I think what you  
are driving at is erring on the side of increasing the testing base.

For an enterprise distribution, however, I suggest following the  
experience of proprietary storage vendors who enabled their NFSv4.0  
implementation early, and were bitten hard by that decision.  They are  
still dealing with the bad press.

4.1 support should be left turned off by default, or disabled  
entirely, in enterprise distributions until we have a high degree of  
confidence that 4.1 doesn't open security or data integrity exposures,  
and that the feature set is stable.  Perhaps you could provide a  
"technology preview" release of RHEL 6 with NFSv4.1 enabled, just as  
was done with FScache in earlier RHEL releases?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [pnfs] [RFC 0/4] nfs-utils: nfsd support for minor version

[Benny Halevy]

On Apr. 17, 2009, 19:18 +0300, Chuck Lever <chuck.lever@oracle.com> wrote:
> On Apr 17, 2009, at 8:35 AM, Steve Dickson wrote:
>> J. Bruce Fields wrote:
>>> On Thu, Apr 16, 2009 at 03:01:47PM -0400, Steve Dickson wrote:
>>>> I do get your point, but as we did with the initial v4 support,
>>>> having the support on by default and then having away to turn it
>>>> off is the correct approach... IMHO...
>>> I'd prefer it be off by default, for the obvious safety reasons.   
>>> (It's
>>> under rapid development and particularly likely to have bugs).  The  
>>> only
>>> reason we had it on by default before was that we didn't add the
>>> switching mechanism early enough.  (Well, and because we could keep  
>>> it
>>> off in the config.  But I'd rather be able to ship users a kernel  
>>> that
>>> supports 4.1 and give them the option of turning it on at runtime,  
>>> than
>>> make them build a new kernel.)
>> I agree with not making people recompile kernels, which is the whole
>> purpose behind the Fedora repos, but do I think you might be a bit
>> too cautious with exposing the technology.
>>
>> One, I've been running the kernels with everything enabled for a while
>> now with no problems whatsoever... A few scary looking warnings now  
>> and
>> then but nothing major. I also spent the majority of my time at  
>> Connectathon
>> this years testing with kernel that were fully enabled. Not one  
>> problem
>> WRT regression testing. Plus there is no better way to expose  
>> regression
>> problems (early on) than to enable the code.. IMHO...
>>
>> Second, its my understanding that clients have to explicitly  ask
>> for 4.1 support. Are there any client out there that default to
>> 4.1 support? I would think not since there is only one client out  
>> there
>> that defaults to V4.0. If there is a client that defaults to 4.1,  
>> then we
>> will a knob to turn that support off.
> 
> That might even be OK for Fedora-based NFS servers.  I think what you  
> are driving at is erring on the side of increasing the testing base.
> 
> For an enterprise distribution, however, I suggest following the  
> experience of proprietary storage vendors who enabled their NFSv4.0  
> implementation early, and were bitten hard by that decision.  They are  
> still dealing with the bad press.
> 
> 4.1 support should be left turned off by default, or disabled  
> entirely, in enterprise distributions until we have a high degree of  
> confidence that 4.1 doesn't open security or data integrity exposures,  
> and that the feature set is stable.  Perhaps you could provide a  
> "technology preview" release of RHEL 6 with NFSv4.1 enabled, just as  
> was done with FScache in earlier RHEL releases?

That sounds like a good compromise to me.

Benny

> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> --
> 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 0/3] nfs-utils: nfsd support for minor version, take 2

[Benny Halevy]

Following the discussion we had last week
(see http://linux-nfs.org/pipermail/pnfs/2009-April/007283.html)

I made the following changes:
* minorvers4 enabled by default.
* No new -n option.

This makes minorvers control essentially the same as
the major protocol version.  minorvers4 is enabled by default
and can be disabled using -N 4.1.

On Fedora, /etc/sysconfig/nfs can be changed as follows
to configure the service startup script to disable 4.1:

--- /etc/sysconfig/nfs.orig	2009-04-22 14:57:15.000000000 +0300
+++ /etc/sysconfig/nfs	2009-04-22 14:46:52.000000000 +0300
@@ -26,6 +26,8 @@
 #RPCNFSDARGS="-N 2 -N 3"
 # Turn off v4 protocol support
 #RPCNFSDARGS="-N 4"
+# Turn off v4.1 minorversion support
+RPCNFSDARGS="-N 4.1"
 # Number of nfs server processes to be started.
 # The default is 8. 
 #RPCNFSDCOUNT=8

The patches in this set are:
[PATCH 1/3] utils/nfsd: add support for minorvers4
[PATCH 2/3] utils/nfsd: disable minorvers4 via command line
[PATCH 3/3] utils/nfsd: enable nfs minorvers4 by default

Benny

[PATCH 1/3] utils/nfsd: add support for minorvers4

[Benny Halevy]

minorvers4 can be used to either enable or disable nfsv4.x.

If minorvers4 is a positive integer n, in the allowed range (only
minorversion 1 is supported for now), the string "+4.n" is appended
to the versions string written onto /proc/fs/nfsd/versions.

Correspondingly, if minorver4 is a negative integer -n, the string
"-4.n" is written.

With the default value, minorvers4==0, the minor version
setting is not changed.

Note that unlike the protocol versions 2, 3, or 4.  The minor version
setting controls the *maximum* minor version nfsd supports.  Particular
minor version cannot be controlled on their own.  With only minor version
1 supported at the moment the difference doesn't matter, but for future
minor versions greater than 1, enabling minor version X will enable support
for all minor versions 1 through X. Disabling minor version X will disable
support for minor versions X and up, enabling 1 through X-1.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 support/include/nfs/nfs.h |    3 +++
 support/include/nfslib.h  |    2 +-
 support/nfs/nfssvc.c      |   13 +++++++++----
 utils/nfsd/nfsd.c         |    3 ++-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index fc26f4e..00b0028 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -13,6 +13,9 @@
 #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/support/include/nfslib.h b/support/include/nfslib.h
index 9d0d39d..ae98650 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -130,7 +130,7 @@ int			wildmat(char *text, char *pattern);
  * nfsd library functions.
  */
 int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
-int			nfssvc(int port, int nrservs, unsigned int versbits, unsigned int portbits, char *haddr);
+int			nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4, unsigned int portbits, char *haddr);
 int			nfsaddclient(struct nfsctl_client *clp);
 int			nfsdelclient(struct nfsctl_client *clp);
 int			nfsexport(struct nfsctl_export *exp);
diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 9bbc9a5..33c15a7 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -116,7 +116,7 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
 	return;
 }
 static void
-nfssvc_versbits(unsigned int ctlbits)
+nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 {
 	int fd, n, off;
 	char buf[BUFSIZ], *ptr;
@@ -133,6 +133,11 @@ nfssvc_versbits(unsigned int ctlbits)
 		else
 		    off += snprintf(ptr+off, BUFSIZ - off, "-%d ", n);
 	}
+	n = minorvers4 >= 0 ? minorvers4 : -minorvers4;
+	if (n >= NFSD_MINMINORVERS4 && n <= NFSD_MAXMINORVERS4)
+		    off += snprintf(ptr+off, BUFSIZ - off, "%c4.%d",
+				    minorvers4 > 0 ? '+' : '-',
+				    n);
 	snprintf(ptr+off, BUFSIZ - off, "\n");
 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
 		syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)", 
@@ -143,8 +148,8 @@ nfssvc_versbits(unsigned int ctlbits)
 	return;
 }
 int
-nfssvc(int port, int nrservs, unsigned int versbits, unsigned protobits,
-	char *haddr)
+nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
+	unsigned protobits, char *haddr)
 {
 	struct nfsctl_arg	arg;
 	int fd;
@@ -153,7 +158,7 @@ nfssvc(int port, int nrservs, unsigned int versbits, unsigned protobits,
 	 * the ports get registered with portmap against correct
 	 * versions
 	 */
-	nfssvc_versbits(versbits);
+	nfssvc_versbits(versbits, minorvers4);
 	nfssvc_setfds(port, protobits, haddr);
 
 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index c97c81f..ac264da 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -41,6 +41,7 @@ static struct option longopts[] =
 };
 unsigned int protobits = NFSCTL_ALLBITS;
 unsigned int versbits = NFSCTL_ALLBITS;
+int minorvers4;				/* nfsv4 minor version */
 char *haddr = NULL;
 
 int
@@ -158,7 +159,7 @@ main(int argc, char **argv)
 	closeall(3);
 
 	openlog("nfsd", LOG_PID, LOG_DAEMON);
-	if ((error = nfssvc(port, count, versbits, protobits, haddr)) < 0) {
+	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) {
 		int e = errno;
 		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
 		closelog();
-- 
1.6.2.1

[PATCH 2/3] utils/nfsd: disable minorvers4 via command line

[Benny Halevy]

Extend -N command line option syntax to accept <version>.<minorversion>
to disable support for <minorversion>.
Only 4.1 is currently supported.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 utils/nfsd/nfsd.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index ac264da..bd23d9d 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -50,6 +50,7 @@ main(int argc, char **argv)
 	int	count = 1, c, error, port, fd, found_one;
 	struct servent *ent;
 	struct hostent *hp;
+	char *p;
 
 	ent = getservbyname ("nfs", "udp");
 	if (ent != NULL)
@@ -80,10 +81,14 @@ main(int argc, char **argv)
 			}
 			break;
 		case 'N':
-			switch((c = atoi(optarg))) {
-			case 2:
-			case 3:
+			switch((c = strtol(optarg, &p, 0))) {
 			case 4:
+				if (*p == '.') {
+					minorvers4 = -atoi(p + 1);
+					break;
+				}
+			case 3:
+			case 2:
 				NFSCTL_VERUNSET(versbits, c);
 				break;
 			default:
-- 
1.6.2.1

[PATCH 3/3] utils/nfsd: enable nfs minorvers4 by default

[Benny Halevy]

Enable support for the maximum minor version (4.1 at the moment)
by default.  It can be disabled using the -N command line
option.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 utils/nfsd/nfsd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index bd23d9d..e3c0094 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -41,7 +41,7 @@ static struct option longopts[] =
 };
 unsigned int protobits = NFSCTL_ALLBITS;
 unsigned int versbits = NFSCTL_ALLBITS;
-int minorvers4;				/* nfsv4 minor version */
+int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
 char *haddr = NULL;
 
 int
-- 
1.6.2.1

Re: [PATCH 0/3] nfs-utils: nfsd support for minor version, take 2

[J. Bruce Fields]

On Wed, Apr 22, 2009 at 03:06:30PM +0300, Benny Halevy wrote:
> Following the discussion we had last week
> (see http://linux-nfs.org/pipermail/pnfs/2009-April/007283.html)
> 
> I made the following changes:
> * minorvers4 enabled by default.
> * No new -n option.
> 
> This makes minorvers control essentially the same as
> the major protocol version.  minorvers4 is enabled by default
> and can be disabled using -N 4.1.
> 
> On Fedora, /etc/sysconfig/nfs can be changed as follows
> to configure the service startup script to disable 4.1:

Why couldn't nfs-utils just respect the kernel's default and make no
attempt to set the minor version?  A (possibly undocumented) -P 4.1
option (or some other name) could be used by testers to specify that
they want 4.1.  After things have settled down a little we'd change the
kernel's default, and then only -N 4.1 would be needed.

If I'd known we'd be enabling 4.1 by default in nfs-utils, I wouldn't
have been so happy about removing the 4.1 config option--how confident
are we that the 4.1 code to be in 2.6.30 has no security holes?

--b.

> 
> --- /etc/sysconfig/nfs.orig	2009-04-22 14:57:15.000000000 +0300
> +++ /etc/sysconfig/nfs	2009-04-22 14:46:52.000000000 +0300
> @@ -26,6 +26,8 @@
>  #RPCNFSDARGS="-N 2 -N 3"
>  # Turn off v4 protocol support
>  #RPCNFSDARGS="-N 4"
> +# Turn off v4.1 minorversion support
> +RPCNFSDARGS="-N 4.1"
>  # Number of nfs server processes to be started.
>  # The default is 8. 
>  #RPCNFSDCOUNT=8
> 
> The patches in this set are:
> [PATCH 1/3] utils/nfsd: add support for minorvers4
> [PATCH 2/3] utils/nfsd: disable minorvers4 via command line
> [PATCH 3/3] utils/nfsd: enable nfs minorvers4 by default
> 
> Benny

Re: [PATCH 0/3] nfs-utils: nfsd support for minor version, take 2

[Benny Halevy]

On Apr. 23, 2009, 0:54 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Wed, Apr 22, 2009 at 03:06:30PM +0300, Benny Halevy wrote:
>> Following the discussion we had last week
>> (see http://linux-nfs.org/pipermail/pnfs/2009-April/007283.html)
>>
>> I made the following changes:
>> * minorvers4 enabled by default.
>> * No new -n option.
>>
>> This makes minorvers control essentially the same as
>> the major protocol version.  minorvers4 is enabled by default
>> and can be disabled using -N 4.1.
>>
>> On Fedora, /etc/sysconfig/nfs can be changed as follows
>> to configure the service startup script to disable 4.1:
> 
> Why couldn't nfs-utils just respect the kernel's default and make no
> attempt to set the minor version?  A (possibly undocumented) -P 4.1
> option (or some other name) could be used by testers to specify that
> they want 4.1.  After things have settled down a little we'd change the
> kernel's default, and then only -N 4.1 would be needed.

It could, and that was pretty much my original intent, which was
criticized by Steve and others for being overly cautious.  I agree with
Chuck that the distributions using nfs-utils should decide about the
default and that can be done either way.  The difference is for people
upgrading nfs-utils while keeping their old /etc/sysconfig/nfs.

I've no problem with having an option to enable 4.1 (and I don't care
much if it's -n or -P either).  Should a distribution that want this option
enabled by default use an undocumented option?  I'm not sure.
I'd prefer it to be documented if that's the case.

> 
> If I'd known we'd be enabling 4.1 by default in nfs-utils, I wouldn't
> have been so happy about removing the 4.1 config option--how confident
> are we that the 4.1 code to be in 2.6.30 has no security holes?

The more it will be tested and used, the better my confidence will be.
I don't think that disabling it by default will help us find any security
holes before the code will be ready for prime time.  Not without a
full blown QA effort.

Benny

> 
> --b.
> 
>> --- /etc/sysconfig/nfs.orig	2009-04-22 14:57:15.000000000 +0300
>> +++ /etc/sysconfig/nfs	2009-04-22 14:46:52.000000000 +0300
>> @@ -26,6 +26,8 @@
>>  #RPCNFSDARGS="-N 2 -N 3"
>>  # Turn off v4 protocol support
>>  #RPCNFSDARGS="-N 4"
>> +# Turn off v4.1 minorversion support
>> +RPCNFSDARGS="-N 4.1"
>>  # Number of nfs server processes to be started.
>>  # The default is 8. 
>>  #RPCNFSDCOUNT=8
>>
>> The patches in this set are:
>> [PATCH 1/3] utils/nfsd: add support for minorvers4
>> [PATCH 2/3] utils/nfsd: disable minorvers4 via command line
>> [PATCH 3/3] utils/nfsd: enable nfs minorvers4 by default
>>
>> Benny

Re: [PATCH 0/3] nfs-utils: nfsd support for minor version, take 2

[Steve Dickson]

Sorry it took so long...  

Benny Halevy wrote:
> Following the discussion we had last week
> (see http://linux-nfs.org/pipermail/pnfs/2009-April/007283.html)
> 
> I made the following changes:
> * minorvers4 enabled by default.
> * No new -n option.
> 
> This makes minorvers control essentially the same as
> the major protocol version.  minorvers4 is enabled by default
> and can be disabled using -N 4.1.
> 
> On Fedora, /etc/sysconfig/nfs can be changed as follows
> to configure the service startup script to disable 4.1:
Committed... 

steved.