[PATCH 0/4] Assorted nfs-utils patches.

[PATCH 0/4] Assorted nfs-utils patches.

[NeilBrown]

First two relate to /etc/nfs.conf and nfsd.
Second to help make sure that nfs things don't cause
problem when they are started early because various
other services are available (most of that is already
fixed, these are just dribs and drabs).

At some stage I want to convert everything to use the same logging
infrastructure, but that doesn't need to happen before the pending
release.

Thanks,
NeilBrown


---

NeilBrown (4):
      nfsd: fix setting of minor version from config file.
      nfsd: Do not permit manipulation of NFSv4.0, e.g. "-N 4.0"
      nfs-server-generator: avoid using syslog
      mountd: delay reading etab until first request arrives.


 systemd/nfs-server-generator.c |    3 +++
 utils/mountd/mountd.c          |    2 --
 utils/nfsd/nfsd.c              |   20 ++++++++++++++++----
 utils/nfsd/nfsd.man            |   10 +++++-----
 4 files changed, 24 insertions(+), 11 deletions(-)

--
Signature

[PATCH 1/4] nfsd: fix setting of minor version from config file.

[NeilBrown]

Several problem here:
- code didn't actually work, as it cleared a bit from minorversset
  when it should have cleared from minorvers
- code did not allow minor versions to be enabled, which is useful
  when a new minor version is partially implemented in the kernel
  but not yet enabled by default
- code allowed version 4.0 to be enabled/disabled, which the kernel
  does not support (as for 4.9 at least).

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/nfsd/nfsd.c   |   16 ++++++++++++++--
 utils/nfsd/nfsd.man |    6 +++---
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 3c451aa46be1..eb346f67f9e4 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -107,12 +107,24 @@ main(int argc, char **argv)
 	/* We assume the kernel will default all minor versions to 'on',
 	 * and allow the config file to disable some.
 	 */
-	for (i = 0; i <= NFS4_MAXMINOR; i++) {
+	for (i = NFS4_MINMINOR; i <= NFS4_MAXMINOR; i++) {
 		char tag[20];
 		sprintf(tag, "vers4.%d", i);
+		/* The default for minor version support is to let the
+		 * kernel decide.  We could ask the kernel what that choice
+		 * will be, but that is needlessly complex.
+		 * Instead, perform a config-file lookup using each of the
+		 * two possible default.  If the result is different from the
+		 * default, then impose that value, else don't make a change
+		 * (i.e. don't set the bit in minorversset).
+		 */
 		if (!conf_get_bool("nfsd", tag, 1)) {
 			NFSCTL_VERSET(minorversset, i);
-			NFSCTL_VERUNSET(minorversset, i);
+			NFSCTL_VERUNSET(minorvers, i);
+		}
+		if (conf_get_bool("nfsd", tag, 0)) {
+			NFSCTL_VERSET(minorversset, i);
+			NFSCTL_VERSET(minorvers, i);
 		}
 	}
 
diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
index 9381cf9d30c3..8d198e25685e 100644
--- a/utils/nfsd/nfsd.man
+++ b/utils/nfsd/nfsd.man
@@ -161,10 +161,10 @@ by default.
 .B vers4.1
 .TP
 .B vers4.2
-.TP
-.B vers4.3
 Setting these to "off" or similar will disable the selected minor
-versions.  All are enabled by default.
+versions.  Setting to "on" will enable them.  The default values
+are determined by the kernel, and usually minor versions default to
+being enabled once the implementation is sufficiently complete.
 
 .SH NOTES
 If the program is built with TI-RPC support, it will enable any protocol and

[PATCH 2/4] nfsd: Do not permit manipulation of NFSv4.0, e.g. "-N 4.0"

[NeilBrown]

The code maps this into "-4.32", which the kernel rejects.
The kernel also rejects "-4.0" (when written to the 'versions'
file).
So require the minor number to be at least NFS4_MINMINOR, which is '1'.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/nfsd/nfsd.c   |    4 ++--
 utils/nfsd/nfsd.man |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index eb346f67f9e4..20f4b7952203 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -179,7 +179,7 @@ main(int argc, char **argv)
 			case 4:
 				if (*p == '.') {
 					int i = atoi(p+1);
-					if (i > NFS4_MAXMINOR) {
+					if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {
 						fprintf(stderr, "%s: unsupported minor version\n", optarg);
 						exit(1);
 					}
@@ -201,7 +201,7 @@ main(int argc, char **argv)
 			case 4:
 				if (*p == '.') {
 					int i = atoi(p+1);
-					if (i > NFS4_MAXMINOR) {
+					if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {
 						fprintf(stderr, "%s: unsupported minor version\n", optarg);
 						exit(1);
 					}
diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
index 8d198e25685e..8901fb6c6872 100644
--- a/utils/nfsd/nfsd.man
+++ b/utils/nfsd/nfsd.man
@@ -57,7 +57,7 @@ This option can be used to request that
 .B rpc.nfsd
 does not offer certain versions of NFS. The current version of
 .B rpc.nfsd
-can support NFS versions 2,3,4 and the newer version 4.1.
+can support major NFS versions 2,3,4 and the minor versions 4.1 and 4.2.
 .TP
 .B \-s " or " \-\-syslog
 By default,
@@ -82,7 +82,7 @@ This option can be used to request that
 .B rpc.nfsd
 offer certain versions of NFS. The current version of
 .B rpc.nfsd
-can support NFS versions 2,3,4 and the newer version 4.1.
+can support major NFS versions 2,3,4 and the minor versions 4.1 and 4.2.
 .TP
 .B \-L " or " \-\-lease-time seconds
 Set the lease-time used for NFSv4.  This corresponds to how often

[PATCH 3/4] nfs-server-generator: avoid using syslog

[NeilBrown]

nfs-server-generator is run very early when a lot of services
are not yet started, so it mustn't depend on them.
It already avoids using DNS, but it should avoid syslog too.

If it tries to log error to syslog, it can deadlock.  So just let
messages go to stderr.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/nfs-server-generator.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/systemd/nfs-server-generator.c b/systemd/nfs-server-generator.c
index 7c40b3f29b99..cc99969e9922 100644
--- a/systemd/nfs-server-generator.c
+++ b/systemd/nfs-server-generator.c
@@ -95,6 +95,9 @@ int main(int argc, char *argv[])
 	FILE		*f, *fstab;
 	struct mntent	*mnt;
 
+	/* Avoid using any external services */
+	xlog_syslog(0);
+
 	if (argc != 4 || argv[1][0] != '/') {
 		fprintf(stderr, "nfs-server-generator: create systemd dependencies for nfs-server\n");
 		fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");

[PATCH 4/4] mountd: delay reading etab until first request arrives.

[NeilBrown]

Reading etab may require hostname lookup, so it is not reliable
until the network is active.
But we want mountd to start before that so that it is ready
when the very first NFS request arrives.
So delay reading etab until that request arrives, by which time
the network must be online so hopefully hostname look will be reliable.

An alternate would be to delay starting mountd and nfsd until the
network is on-line, but that will often be an unnecessary delay.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mountd/mountd.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 5d9466f5c651..61699e62a6f0 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -852,8 +852,6 @@ main(int argc, char **argv)
 	sa.sa_handler = sig_hup;
 	sigaction(SIGHUP, &sa, NULL);
 
-	auth_init();
-
 	if (!foreground) {
 		/* We first fork off a child. */
 		if ((c = fork()) > 0)

Re: [PATCH 4/4] mountd: delay reading etab until first request arrives.

[J. Bruce Fields]

On Wed, Dec 21, 2016 at 11:19:14AM +1100, NeilBrown wrote:
> Reading etab may require hostname lookup, so it is not reliable
> until the network is active.
> But we want mountd to start before that so that it is ready
> when the very first NFS request arrives.
> So delay reading etab until that request arrives, by which time
> the network must be online so hopefully hostname look will be reliable.
> 
> An alternate would be to delay starting mountd and nfsd until the
> network is on-line, but that will often be an unnecessary delay.

One argument for that delay would be to get the grace period right: it's
not really correct to start timing the grace period before you start
accepting requests.

In practice, with grace periods by default a minute and (I'm guessing)
the delay here not even a few seconds, maybe it's a minor point.

And in theory I guess knfsd could account for that by waiting to start
the grace period timer until it receives its first rpc.

Anyway, that's not an objection to applying this patch.

--b.

> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  utils/mountd/mountd.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
> index 5d9466f5c651..61699e62a6f0 100644
> --- a/utils/mountd/mountd.c
> +++ b/utils/mountd/mountd.c
> @@ -852,8 +852,6 @@ main(int argc, char **argv)
>  	sa.sa_handler = sig_hup;
>  	sigaction(SIGHUP, &sa, NULL);
>  
> -	auth_init();
> -
>  	if (!foreground) {
>  		/* We first fork off a child. */
>  		if ((c = fork()) > 0)
> 

Re: [PATCH 4/4] mountd: delay reading etab until first request arrives.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Fri, Dec 23 2016, J. Bruce Fields wrote:

> On Wed, Dec 21, 2016 at 11:19:14AM +1100, NeilBrown wrote:
>> Reading etab may require hostname lookup, so it is not reliable
>> until the network is active.
>> But we want mountd to start before that so that it is ready
>> when the very first NFS request arrives.
>> So delay reading etab until that request arrives, by which time
>> the network must be online so hopefully hostname look will be reliable.
>> 
>> An alternate would be to delay starting mountd and nfsd until the
>> network is on-line, but that will often be an unnecessary delay.
>
> One argument for that delay would be to get the grace period right: it's
> not really correct to start timing the grace period before you start
> accepting requests.
>
> In practice, with grace periods by default a minute and (I'm guessing)
> the delay here not even a few seconds, maybe it's a minor point.
>
> And in theory I guess knfsd could account for that by waiting to start
> the grace period timer until it receives its first rpc.

Interesting idea.  One would need to be careful about the case where
there are no clients wanting to recover state, so some timeout without
seeing any RPC would be needed.

It's times like this that I wish the grace period was a lot shorter, but
was auto-extended whenever a state-recovery request arrived (maybe with
some limit).  But that isn't what the spec say :-(

Thanks,
NeilBrown


>
> Anyway, that's not an objection to applying this patch.
>
> --b.
>
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  utils/mountd/mountd.c |    2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
>> index 5d9466f5c651..61699e62a6f0 100644
>> --- a/utils/mountd/mountd.c
>> +++ b/utils/mountd/mountd.c
>> @@ -852,8 +852,6 @@ main(int argc, char **argv)
>>  	sa.sa_handler = sig_hup;
>>  	sigaction(SIGHUP, &sa, NULL);
>>  
>> -	auth_init();
>> -
>>  	if (!foreground) {
>>  		/* We first fork off a child. */
>>  		if ((c = fork()) > 0)
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/4] mountd: delay reading etab until first request arrives.

[J. Bruce Fields]

On Fri, Dec 23, 2016 at 10:16:57AM +1100, NeilBrown wrote:
> On Fri, Dec 23 2016, J. Bruce Fields wrote:
> 
> > On Wed, Dec 21, 2016 at 11:19:14AM +1100, NeilBrown wrote:
> >> Reading etab may require hostname lookup, so it is not reliable
> >> until the network is active.
> >> But we want mountd to start before that so that it is ready
> >> when the very first NFS request arrives.
> >> So delay reading etab until that request arrives, by which time
> >> the network must be online so hopefully hostname look will be reliable.
> >> 
> >> An alternate would be to delay starting mountd and nfsd until the
> >> network is on-line, but that will often be an unnecessary delay.
> >
> > One argument for that delay would be to get the grace period right: it's
> > not really correct to start timing the grace period before you start
> > accepting requests.
> >
> > In practice, with grace periods by default a minute and (I'm guessing)
> > the delay here not even a few seconds, maybe it's a minor point.
> >
> > And in theory I guess knfsd could account for that by waiting to start
> > the grace period timer until it receives its first rpc.
> 
> Interesting idea.  One would need to be careful about the case where
> there are no clients wanting to recover state, so some timeout without
> seeing any RPC would be needed.

In theory the server should have enough information to skip the grace
period in that case, I'm not sure if it always does.

But there might be rare cases where a client that was expected to cover
state doesn't, in which case we could impose an unnecessary wait on the
first new client.....

> It's times like this that I wish the grace period was a lot shorter, but
> was auto-extended whenever a state-recovery request arrived (maybe with
> some limit).  But that isn't what the spec say :-(

Actually I think it does permit that, but I don't have a citation to the
exact text.--b.


> 
> Thanks,
> NeilBrown
> 
> 
> >
> > Anyway, that's not an objection to applying this patch.
> >
> > --b.
> >
> >> 
> >> Signed-off-by: NeilBrown <neilb@suse.com>
> >> ---
> >>  utils/mountd/mountd.c |    2 --
> >>  1 file changed, 2 deletions(-)
> >> 
> >> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
> >> index 5d9466f5c651..61699e62a6f0 100644
> >> --- a/utils/mountd/mountd.c
> >> +++ b/utils/mountd/mountd.c
> >> @@ -852,8 +852,6 @@ main(int argc, char **argv)
> >>  	sa.sa_handler = sig_hup;
> >>  	sigaction(SIGHUP, &sa, NULL);
> >>  
> >> -	auth_init();
> >> -
> >>  	if (!foreground) {
> >>  		/* We first fork off a child. */
> >>  		if ((c = fork()) > 0)
> >> 

Re: [PATCH 0/4] Assorted nfs-utils patches.

[Steve Dickson]

On 12/20/2016 07:19 PM, NeilBrown wrote:
> First two relate to /etc/nfs.conf and nfsd.
> Second to help make sure that nfs things don't cause
> problem when they are started early because various
> other services are available (most of that is already
> fixed, these are just dribs and drabs).
> 
> At some stage I want to convert everything to use the same logging
> infrastructure, but that doesn't need to happen before the pending
> release.
> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (4):
>       nfsd: fix setting of minor version from config file.
>       nfsd: Do not permit manipulation of NFSv4.0, e.g. "-N 4.0"
>       nfs-server-generator: avoid using syslog
>       mountd: delay reading etab until first request arrives.
> 
> 
>  systemd/nfs-server-generator.c |    3 +++
>  utils/mountd/mountd.c          |    2 --
>  utils/nfsd/nfsd.c              |   20 ++++++++++++++++----
>  utils/nfsd/nfsd.man            |   10 +++++-----
>  4 files changed, 24 insertions(+), 11 deletions(-)
All four committed! Thank you!

steved.

> 
> --
> Signature
>