[PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[Trond Myklebust]

This patch series was intended as a standalone series. However it now
aims to fix up a few issues with Neil's initial patch:

1) When the user turns off all minor versions of NFSv4, then that should
   be equivalent to turning off NFSv4 support, and so when someone tries
   to mount, we should return RPC_PROG_MISMATCH.
2) Allow the user to use either '4.0' or '4' in order to enable or disable
   minor version 0. Other minor versions remain enabled/disabled using the
   '4.x' format.
3) Fix up the display of the minor versions so that we don't add '4.0'.
   Previous kernels have always assumed the '4' means 'minor version 0',
   so there should be no need to have both.

v2: Make incremental to Neil's patch
    Fix up display of the version string

Trond Myklebust (2):
  nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0
  nfsd: Fix display of the version string

 fs/nfsd/nfsctl.c | 72 ++++++++++++++++++++++++++------------------------------
 fs/nfsd/nfssvc.c | 14 +++++++++++
 2 files changed, 48 insertions(+), 38 deletions(-)

-- 
2.9.3

[PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0

[Trond Myklebust]

For some setups, we may want to allow clients to fall back to NFSv3, or
even to fail, if a specific minor version of NFSv4 is not enabled.
For instance, an application that relies on NFSv4.2 CLONE functionality
is simply not going to work when it falls back to NFSv4.1 or NFSv4.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfsctl.c | 16 ++++++++--------
 fs/nfsd/nfssvc.c | 14 ++++++++++++++
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d54fb0e3f30e..4bbba88416dc 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -561,6 +561,7 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 		len = qword_get(&mesg, vers, size);
 		if (len <= 0) return -EINVAL;
 		do {
+			enum vers_op cmd;
 			sign = *vers;
 			if (sign == '+' || sign == '-')
 				num = simple_strtol((vers+1), &minorp, 0);
@@ -571,21 +572,20 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 					return -EINVAL;
 				if (kstrtouint(minorp+1, 0, &minor) < 0)
 					return -EINVAL;
-				if (nfsd_minorversion(minor, sign == '-' ?
-						     NFSD_CLEAR : NFSD_SET) < 0)
-					return -EINVAL;
-				goto next;
-			}
+			} else
+				minor = 0;
+			cmd = sign == '-' ? NFSD_CLEAR : NFSD_SET;
 			switch(num) {
 			case 2:
 			case 3:
-			case 4:
-				nfsd_vers(num, sign == '-' ? NFSD_CLEAR : NFSD_SET);
+				nfsd_vers(num, cmd);
 				break;
+			case 4:
+				if (nfsd_minorversion(minor, cmd) >= 0)
+					break;
 			default:
 				return -EINVAL;
 			}
-		next:
 			vers += len + 1;
 		} while ((len = qword_get(&mesg, vers, size)) > 0);
 		/* If all get turned off, turn them back on, as
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index e6bfd96734c0..07bf3bd9687b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -153,6 +153,18 @@ int nfsd_vers(int vers, enum vers_op change)
 	return 0;
 }
 
+static void
+nfsd_adjust_nfsd_versions4(void)
+{
+	unsigned i;
+
+	for (i = 0; i <= NFSD_SUPPORTED_MINOR_VERSION; i++) {
+		if (nfsd_supported_minorversions[i])
+			return;
+	}
+	nfsd_vers(4, NFSD_CLEAR);
+}
+
 int nfsd_minorversion(u32 minorversion, enum vers_op change)
 {
 	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION)
@@ -160,9 +172,11 @@ int nfsd_minorversion(u32 minorversion, enum vers_op change)
 	switch(change) {
 	case NFSD_SET:
 		nfsd_supported_minorversions[minorversion] = true;
+		nfsd_vers(4, NFSD_SET);
 		break;
 	case NFSD_CLEAR:
 		nfsd_supported_minorversions[minorversion] = false;
+		nfsd_adjust_nfsd_versions4();
 		break;
 	case NFSD_TEST:
 		return nfsd_supported_minorversions[minorversion];
-- 
2.9.3

[PATCH v2 2/2] nfsd: Fix display of the version string

[Trond Myklebust]

The current display code assumes that v4 minor version 0 is tracked by
the call to nfsd_vers(). Now it is tracked by nfsd_minorversion(), and
so we need to adjust the display code.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfsctl.c | 56 ++++++++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 4bbba88416dc..73e75ac90525 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -536,6 +536,19 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 	return rv;
 }
 
+static ssize_t
+nfsd_print_version_support(char *buf, int remaining, const char *sep,
+		unsigned vers, unsigned minor)
+{
+	const char *format = (minor == 0) ? "%s%c%u" : "%s%c%u.%u";
+	bool supported = !!nfsd_vers(vers, NFSD_TEST);
+
+	if (vers == 4 && !nfsd_minorversion(minor, NFSD_TEST))
+		supported = false;
+	return snprintf(buf, remaining, format, sep,
+			supported ? '+' : '-', vers, minor);
+}
+
 static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 {
 	char *mesg = buf;
@@ -598,40 +611,23 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 	len = 0;
 	sep = "";
 	remaining = SIMPLE_TRANSACTION_LIMIT;
-	for (num=2 ; num <= 4 ; num++)
-		if (nfsd_vers(num, NFSD_AVAIL)) {
-			len = snprintf(buf, remaining, "%s%c%d", sep,
-				       nfsd_vers(num, NFSD_TEST)?'+':'-',
-				       num);
-			sep = " ";
-
-			if (len >= remaining)
-				break;
-			remaining -= len;
-			buf += len;
-			tlen += len;
-		}
-	if (nfsd_vers(4, NFSD_AVAIL))
-		for (minor = 0; minor <= NFSD_SUPPORTED_MINOR_VERSION;
-		     minor++) {
-			if (minor == 0 && nfsd_minorversion(minor, NFSD_TEST))
-				/* for backward compatibility, don't report
-				 * +4.0
-				 */
-				continue;
-			len = snprintf(buf, remaining, " %c4.%u",
-					(nfsd_vers(4, NFSD_TEST) &&
-					 nfsd_minorversion(minor, NFSD_TEST)) ?
-						'+' : '-',
-					minor);
-
+	for (num=2 ; num <= 4 ; num++) {
+		if (!nfsd_vers(num, NFSD_AVAIL))
+			continue;
+		minor = 0;
+		do {
+			len = nfsd_print_version_support(buf, remaining,
+					sep, num, minor);
 			if (len >= remaining)
-				break;
+				goto out;
 			remaining -= len;
 			buf += len;
 			tlen += len;
-		}
-
+			minor++;
+			sep = " ";
+		} while (num == 4 && minor <= NFSD_SUPPORTED_MINOR_VERSION);
+	}
+out:
 	len = snprintf(buf, remaining, "\n");
 	if (len >= remaining)
 		return -EINVAL;
-- 
2.9.3

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[NeilBrown]

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

On Wed, Feb 22 2017, Trond Myklebust wrote:

> This patch series was intended as a standalone series. However it now
> aims to fix up a few issues with Neil's initial patch:
>
> 1) When the user turns off all minor versions of NFSv4, then that should
>    be equivalent to turning off NFSv4 support, and so when someone tries
>    to mount, we should return RPC_PROG_MISMATCH.
> 2) Allow the user to use either '4.0' or '4' in order to enable or disable
>    minor version 0. Other minor versions remain enabled/disabled using the
>    '4.x' format.

If I understand you and the code correctly, then this change means that
if I run

   rpc.nfsd -N 4

the nfs server will continue to server v4.1 and v4.2 requests, only v4.0
will be disabled (Assuming a kernel which defaults to serving v4.0, v4.1
and v4.0).

Is that really what we want?

The text in the man page for rpc.nfsd could be read as allowing this
interpretation, though it isn't explicit.
I've always used "NFSv4" to mean any or all for 4.0, 4.1, 4.2, ...

Thanks,
NeilBrown


> 3) Fix up the display of the minor versions so that we don't add '4.0'.
>    Previous kernels have always assumed the '4' means 'minor version 0',
>    so there should be no need to have both.
>
> v2: Make incremental to Neil's patch
>     Fix up display of the version string
>
> Trond Myklebust (2):
>   nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0
>   nfsd: Fix display of the version string
>
>  fs/nfsd/nfsctl.c | 72 ++++++++++++++++++++++++++------------------------------
>  fs/nfsd/nfssvc.c | 14 +++++++++++
>  2 files changed, 48 insertions(+), 38 deletions(-)
>
> -- 
> 2.9.3

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

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[Trond Myklebust]

T24gVGh1LCAyMDE3LTAyLTIzIGF0IDExOjEzICsxMTAwLCBOZWlsQnJvd24gd3JvdGU6DQo+IE9u
IFdlZCwgRmViIDIyIDIwMTcsIFRyb25kIE15a2xlYnVzdCB3cm90ZToNCj4gDQo+ID4gVGhpcyBw
YXRjaCBzZXJpZXMgd2FzIGludGVuZGVkIGFzIGEgc3RhbmRhbG9uZSBzZXJpZXMuIEhvd2V2ZXIg
aXQNCj4gPiBub3cNCj4gPiBhaW1zIHRvIGZpeCB1cCBhIGZldyBpc3N1ZXMgd2l0aCBOZWlsJ3Mg
aW5pdGlhbCBwYXRjaDoNCj4gPiANCj4gPiAxKSBXaGVuIHRoZSB1c2VyIHR1cm5zIG9mZiBhbGwg
bWlub3IgdmVyc2lvbnMgb2YgTkZTdjQsIHRoZW4gdGhhdA0KPiA+IHNob3VsZA0KPiA+IMKgwqDC
oGJlIGVxdWl2YWxlbnQgdG8gdHVybmluZyBvZmYgTkZTdjQgc3VwcG9ydCwgYW5kIHNvIHdoZW4g
c29tZW9uZQ0KPiA+IHRyaWVzDQo+ID4gwqDCoMKgdG8gbW91bnQsIHdlIHNob3VsZCByZXR1cm4g
UlBDX1BST0dfTUlTTUFUQ0guDQo+ID4gMikgQWxsb3cgdGhlIHVzZXIgdG8gdXNlIGVpdGhlciAn
NC4wJyBvciAnNCcgaW4gb3JkZXIgdG8gZW5hYmxlIG9yDQo+ID4gZGlzYWJsZQ0KPiA+IMKgwqDC
oG1pbm9yIHZlcnNpb24gMC4gT3RoZXIgbWlub3IgdmVyc2lvbnMgcmVtYWluIGVuYWJsZWQvZGlz
YWJsZWQNCj4gPiB1c2luZyB0aGUNCj4gPiDCoMKgwqAnNC54JyBmb3JtYXQuDQo+IA0KPiBJZiBJ
IHVuZGVyc3RhbmQgeW91IGFuZCB0aGUgY29kZSBjb3JyZWN0bHksIHRoZW4gdGhpcyBjaGFuZ2Ug
bWVhbnMNCj4gdGhhdA0KPiBpZiBJIHJ1bg0KPiANCj4gwqDCoMKgcnBjLm5mc2QgLU4gNA0KPiAN
Cj4gdGhlIG5mcyBzZXJ2ZXIgd2lsbCBjb250aW51ZSB0byBzZXJ2ZXIgdjQuMSBhbmQgdjQuMiBy
ZXF1ZXN0cywgb25seQ0KPiB2NC4wDQo+IHdpbGwgYmUgZGlzYWJsZWQgKEFzc3VtaW5nIGEga2Vy
bmVsIHdoaWNoIGRlZmF1bHRzIHRvIHNlcnZpbmcgdjQuMCwNCj4gdjQuMQ0KPiBhbmQgdjQuMCku
DQoNClVuZm9ydHVuYXRlbHkgbm90Li4uIFRoZSBwcm9ibGVtIGlzIHRoYXQgcnBjLm5mc2QgaGFz
IGEgYm9ya2VuIHBhcnNlcg0KdGhhdCByZWZ1c2VzIHRoZSAnLU40IC1WNC4yJyBzeW50YXgsIGFu
ZCB0aGF0IHRocm93cyBoaXNzeSBmaXQgaWYgeW91DQp0cnkgdG8gZmVlZCBpdCAnLU40LjAnIG9y
ICctVjQuMCcuDQoNClRoZSBrZXJuZWwgY2hhbmdlIG5lZWRzIHRvIGJlIGFjY29tcGFuaWVkIGJ5
IGEgZml4IHRvIG5mcy11dGlscy4NCg0KPiBJcyB0aGF0IHJlYWxseSB3aGF0IHdlIHdhbnQ/DQo+
IA0KPiBUaGUgdGV4dCBpbiB0aGUgbWFuIHBhZ2UgZm9yIHJwYy5uZnNkIGNvdWxkIGJlIHJlYWQg
YXMgYWxsb3dpbmcgdGhpcw0KPiBpbnRlcnByZXRhdGlvbiwgdGhvdWdoIGl0IGlzbid0IGV4cGxp
Y2l0Lg0KPiBJJ3ZlIGFsd2F5cyB1c2VkICJORlN2NCIgdG8gbWVhbiBhbnkgb3IgYWxsIGZvciA0
LjAsIDQuMSwgNC4yLCAuLi4NCg0KV2UgX2NvdWxkXyBkbyB0aGF0LiBBcyBJIHNhaWQsIHRoYXQn
cyBhIHF1ZXN0aW9uIG9mIGZpeGluZyBycGMubmZzZC4NCg0KVGhlIHF1ZXN0aW9uIGhlcmUsIHRo
b3VnaCwgaXMgd2hhdCBzaG91bGQgdGhlIGtlcm5lbCBkbyB3aGVuIHlvdSBzdGFydA0KcG9raW5n
IGludG8gL3Byb2MvZnMvbmZzZC92ZXJzaW9ucy4gSSB0aGluayBhZGRpbmcgYSBuZXcgZmllbGQg
YmV0d2Vlbg0KJzQnIGFuZCAnNC4wJyBtaWdodCBiZSBtb3JlIGRpc3J1cHRpdmUgdGhhbiBjaGFu
Z2luZyB0aGUgbWVhbmluZyBvZiAnNCcNCnRvIG1lYW4gJzQuMCcgYXMgdGhlcmUgbWlnaHQgYmUg
b3RoZXIgcGFyc2VycyBvZiB0aGF0IHBzZXVkb2ZpbGUgb3V0DQp0aGVyZSB0b2RheS4NCg0KPiBU
aGFua3MsDQo+IE5laWxCcm93bg0KPiANCj4gDQo+ID4gMykgRml4IHVwIHRoZSBkaXNwbGF5IG9m
IHRoZSBtaW5vciB2ZXJzaW9ucyBzbyB0aGF0IHdlIGRvbid0IGFkZA0KPiA+ICc0LjAnLg0KPiA+
IMKgwqDCoFByZXZpb3VzIGtlcm5lbHMgaGF2ZSBhbHdheXMgYXNzdW1lZCB0aGUgJzQnIG1lYW5z
ICdtaW5vcg0KPiA+IHZlcnNpb24gMCcsDQo+ID4gwqDCoMKgc28gdGhlcmUgc2hvdWxkIGJlIG5v
IG5lZWQgdG8gaGF2ZSBib3RoLg0KPiA+IA0KPiA+IHYyOiBNYWtlIGluY3JlbWVudGFsIHRvIE5l
aWwncyBwYXRjaA0KPiA+IMKgwqDCoMKgRml4IHVwIGRpc3BsYXkgb2YgdGhlIHZlcnNpb24gc3Ry
aW5nDQo+ID4gDQo+ID4gVHJvbmQgTXlrbGVidXN0ICgyKToNCj4gPiDCoCBuZnNkOiBBbGxvdyBl
bmFibGluZyBORlN2NC54IHdpdGhvdXQgYWxzbyByZXF1aXJpbmcgTkZTdjQuMA0KPiA+IMKgIG5m
c2Q6IEZpeCBkaXNwbGF5IG9mIHRoZSB2ZXJzaW9uIHN0cmluZw0KPiA+IA0KPiA+IMKgZnMvbmZz
ZC9uZnNjdGwuYyB8IDcyICsrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0t
LQ0KPiA+IC0tLS0tLS0tLS0tLS0tDQo+ID4gwqBmcy9uZnNkL25mc3N2Yy5jIHwgMTQgKysrKysr
KysrKysNCj4gPiDCoDIgZmlsZXMgY2hhbmdlZCwgNDggaW5zZXJ0aW9ucygrKSwgMzggZGVsZXRp
b25zKC0pDQo+ID4gDQo+ID4gLS3CoA0KPiA+IDIuOS4zDQotLSANClRyb25kIE15a2xlYnVzdA0K
TGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0
QHByaW1hcnlkYXRhLmNvbQ0K

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[bfields]

On Thu, Feb 23, 2017 at 01:00:12AM +0000, Trond Myklebust wrote:
> On Thu, 2017-02-23 at 11:13 +1100, NeilBrown wrote:
> > On Wed, Feb 22 2017, Trond Myklebust wrote:
> > 
> > > This patch series was intended as a standalone series. However it
> > > now
> > > aims to fix up a few issues with Neil's initial patch:
> > > 
> > > 1) When the user turns off all minor versions of NFSv4, then that
> > > should
> > >    be equivalent to turning off NFSv4 support, and so when someone
> > > tries
> > >    to mount, we should return RPC_PROG_MISMATCH.
> > > 2) Allow the user to use either '4.0' or '4' in order to enable or
> > > disable
> > >    minor version 0. Other minor versions remain enabled/disabled
> > > using the
> > >    '4.x' format.
> > 
> > If I understand you and the code correctly, then this change means
> > that
> > if I run
> > 
> >    rpc.nfsd -N 4
> > 
> > the nfs server will continue to server v4.1 and v4.2 requests, only
> > v4.0
> > will be disabled (Assuming a kernel which defaults to serving v4.0,
> > v4.1
> > and v4.0).
> 
> Unfortunately not... The problem is that rpc.nfsd has a borken parser
> that refuses the '-N4 -V4.2' syntax, and that throws hissy fit if you
> try to feed it '-N4.0' or '-V4.0'.
> 
> The kernel change needs to be accompanied by a fix to nfs-utils.
> 
> > Is that really what we want?
> > 
> > The text in the man page for rpc.nfsd could be read as allowing this
> > interpretation, though it isn't explicit.
> > I've always used "NFSv4" to mean any or all for 4.0, 4.1, 4.2, ...
> 
> We _could_ do that. As I said, that's a question of fixing rpc.nfsd.
> 
> The question here, though, is what should the kernel do when you start
> poking into /proc/fs/nfsd/versions. I think adding a new field between
> '4' and '4.0' might be more disruptive than changing the meaning of '4'
> to mean '4.0' as there might be other parsers of that pseudofile out
> there today.

I'm OK with that, unless Neil sees a problem.

The changelog on the first patch doesn't seem to describe the v2 version
very well; using the below cribbed from your cover letter instead.

    nfsd: fix configuration of supported minor versions
    
    When the user turns off all minor versions of NFSv4, that should be
    equivalent to turning off NFSv4 support, so a mount attempt using NFSv4
    should get RPC_PROG_MISMATCH, not NFSERR_MINOR_VERS_MISMATCH.
    
    Allow the user to use either '4.0' or '4' to enable or disable minor
    version 0.  Other minor versions are still enabled or disabled using the
    '4.x' format.

--b.

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[NeilBrown]

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

On Mon, Feb 27 2017, bfields@fieldses.org wrote:

> On Thu, Feb 23, 2017 at 01:00:12AM +0000, Trond Myklebust wrote:
>> On Thu, 2017-02-23 at 11:13 +1100, NeilBrown wrote:
>> > On Wed, Feb 22 2017, Trond Myklebust wrote:
>> > 
>> > > This patch series was intended as a standalone series. However it
>> > > now
>> > > aims to fix up a few issues with Neil's initial patch:
>> > > 
>> > > 1) When the user turns off all minor versions of NFSv4, then that
>> > > should
>> > >    be equivalent to turning off NFSv4 support, and so when someone
>> > > tries
>> > >    to mount, we should return RPC_PROG_MISMATCH.
>> > > 2) Allow the user to use either '4.0' or '4' in order to enable or
>> > > disable
>> > >    minor version 0. Other minor versions remain enabled/disabled
>> > > using the
>> > >    '4.x' format.
>> > 
>> > If I understand you and the code correctly, then this change means
>> > that
>> > if I run
>> > 
>> >    rpc.nfsd -N 4
>> > 
>> > the nfs server will continue to server v4.1 and v4.2 requests, only
>> > v4.0
>> > will be disabled (Assuming a kernel which defaults to serving v4.0,
>> > v4.1
>> > and v4.0).
>> 
>> Unfortunately not... The problem is that rpc.nfsd has a borken parser
>> that refuses the '-N4 -V4.2' syntax, and that throws hissy fit if you
>> try to feed it '-N4.0' or '-V4.0'.
>> 
>> The kernel change needs to be accompanied by a fix to nfs-utils.
>> 
>> > Is that really what we want?
>> > 
>> > The text in the man page for rpc.nfsd could be read as allowing this
>> > interpretation, though it isn't explicit.
>> > I've always used "NFSv4" to mean any or all for 4.0, 4.1, 4.2, ...
>> 
>> We _could_ do that. As I said, that's a question of fixing rpc.nfsd.
>> 
>> The question here, though, is what should the kernel do when you start
>> poking into /proc/fs/nfsd/versions. I think adding a new field between
>> '4' and '4.0' might be more disruptive than changing the meaning of '4'
>> to mean '4.0' as there might be other parsers of that pseudofile out
>> there today.
>
> I'm OK with that, unless Neil sees a problem.

I don't know ... the more I think about it, the less certain I seem to
be.

Prior to Trond's updates for nfs-utils, "rpc.nfsd -N 4" would write
 "+3 -4"
to the 'versions' file, which will currently disable NFSv4 completely.
With this patch it will disable 4.0, but leave 4.1 active.

This behavior is consistent with the documentation, which says:

   The  current version of rpc.nfsd can support major NFS versions 2,3,4
   and the minor versions 4.1 and 4.2.

which suggests that "4.1" is a different thing to "4", not part of it.
And the current nfs-utils doesn't support disabling just 4.0 at all.
You cannot say "rpc.nfsd -N 4.0".
So this patch gives more control to the old nfs-utils, by changing the
semantics of a particular operation.

Technically, it is a regression though.  It does result in a more
coherent interface, which is a good thing in the long term.

When you read from "versions" you get the major version and minor
version listed separately, with a special case that "+4.0" is never
reported.
So if just 4.1 is enabled, you might get
   -3 +4 -4.0 +4.1 -4.2

after Trond's change "4" means "4.0" and "4.0" is never displayed,
so the above case would become
   -3 -4 +4.1 -4.2

which is very different.  Probably another regression.

I guess I'm not entirely sure what Trond is trying to fix.
This bit:

1) When the user turns off all minor versions of NFSv4, then that should
   be equivalent to turning off NFSv4 support, and so when someone tries
   to mount, we should return RPC_PROG_MISMATCH.

Makes perfect sense.  But I'm not clear on why the format of the
versions file needs to change.
Trond - what am I missing?

Thanks,
NeilBrown



>
> The changelog on the first patch doesn't seem to describe the v2 version
> very well; using the below cribbed from your cover letter instead.
>
>     nfsd: fix configuration of supported minor versions
>     
>     When the user turns off all minor versions of NFSv4, that should be
>     equivalent to turning off NFSv4 support, so a mount attempt using NFSv4
>     should get RPC_PROG_MISMATCH, not NFSERR_MINOR_VERS_MISMATCH.
>     
>     Allow the user to use either '4.0' or '4' to enable or disable minor
>     version 0.  Other minor versions are still enabled or disabled using the
>     '4.x' format.
>
> --b.

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

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[Trond Myklebust]

T24gRnJpLCAyMDE3LTAzLTAzIGF0IDA5OjU3ICsxMTAwLCBOZWlsQnJvd24gd3JvdGU6DQo+IE9u
IE1vbiwgRmViIDI3IDIwMTcsIGJmaWVsZHNAZmllbGRzZXMub3JnIHdyb3RlOg0KPiANCj4gPiBP
biBUaHUsIEZlYiAyMywgMjAxNyBhdCAwMTowMDoxMkFNICswMDAwLCBUcm9uZCBNeWtsZWJ1c3Qg
d3JvdGU6DQo+ID4gPiBPbiBUaHUsIDIwMTctMDItMjMgYXQgMTE6MTMgKzExMDAsIE5laWxCcm93
biB3cm90ZToNCj4gPiA+ID4gT24gV2VkLCBGZWIgMjIgMjAxNywgVHJvbmQgTXlrbGVidXN0IHdy
b3RlOg0KPiA+ID4gPiANCj4gPiA+ID4gPiBUaGlzIHBhdGNoIHNlcmllcyB3YXMgaW50ZW5kZWQg
YXMgYSBzdGFuZGFsb25lIHNlcmllcy4NCj4gPiA+ID4gPiBIb3dldmVyIGl0DQo+ID4gPiA+ID4g
bm93DQo+ID4gPiA+ID4gYWltcyB0byBmaXggdXAgYSBmZXcgaXNzdWVzIHdpdGggTmVpbCdzIGlu
aXRpYWwgcGF0Y2g6DQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gMSkgV2hlbiB0aGUgdXNlciB0dXJu
cyBvZmYgYWxsIG1pbm9yIHZlcnNpb25zIG9mIE5GU3Y0LCB0aGVuDQo+ID4gPiA+ID4gdGhhdA0K
PiA+ID4gPiA+IHNob3VsZA0KPiA+ID4gPiA+IMKgwqDCoGJlIGVxdWl2YWxlbnQgdG8gdHVybmlu
ZyBvZmYgTkZTdjQgc3VwcG9ydCwgYW5kIHNvIHdoZW4NCj4gPiA+ID4gPiBzb21lb25lDQo+ID4g
PiA+ID4gdHJpZXMNCj4gPiA+ID4gPiDCoMKgwqB0byBtb3VudCwgd2Ugc2hvdWxkIHJldHVybiBS
UENfUFJPR19NSVNNQVRDSC4NCj4gPiA+ID4gPiAyKSBBbGxvdyB0aGUgdXNlciB0byB1c2UgZWl0
aGVyICc0LjAnIG9yICc0JyBpbiBvcmRlciB0bw0KPiA+ID4gPiA+IGVuYWJsZSBvcg0KPiA+ID4g
PiA+IGRpc2FibGUNCj4gPiA+ID4gPiDCoMKgwqBtaW5vciB2ZXJzaW9uIDAuIE90aGVyIG1pbm9y
IHZlcnNpb25zIHJlbWFpbg0KPiA+ID4gPiA+IGVuYWJsZWQvZGlzYWJsZWQNCj4gPiA+ID4gPiB1
c2luZyB0aGUNCj4gPiA+ID4gPiDCoMKgwqAnNC54JyBmb3JtYXQuDQo+ID4gPiA+IA0KPiA+ID4g
PiBJZiBJIHVuZGVyc3RhbmQgeW91IGFuZCB0aGUgY29kZSBjb3JyZWN0bHksIHRoZW4gdGhpcyBj
aGFuZ2UNCj4gPiA+ID4gbWVhbnMNCj4gPiA+ID4gdGhhdA0KPiA+ID4gPiBpZiBJIHJ1bg0KPiA+
ID4gPiANCj4gPiA+ID4gwqDCoMKgcnBjLm5mc2QgLU4gNA0KPiA+ID4gPiANCj4gPiA+ID4gdGhl
IG5mcyBzZXJ2ZXIgd2lsbCBjb250aW51ZSB0byBzZXJ2ZXIgdjQuMSBhbmQgdjQuMiByZXF1ZXN0
cywNCj4gPiA+ID4gb25seQ0KPiA+ID4gPiB2NC4wDQo+ID4gPiA+IHdpbGwgYmUgZGlzYWJsZWQg
KEFzc3VtaW5nIGEga2VybmVsIHdoaWNoIGRlZmF1bHRzIHRvIHNlcnZpbmcNCj4gPiA+ID4gdjQu
MCwNCj4gPiA+ID4gdjQuMQ0KPiA+ID4gPiBhbmQgdjQuMCkuDQo+ID4gPiANCj4gPiA+IFVuZm9y
dHVuYXRlbHkgbm90Li4uIFRoZSBwcm9ibGVtIGlzIHRoYXQgcnBjLm5mc2QgaGFzIGEgYm9ya2Vu
DQo+ID4gPiBwYXJzZXINCj4gPiA+IHRoYXQgcmVmdXNlcyB0aGUgJy1ONCAtVjQuMicgc3ludGF4
LCBhbmQgdGhhdCB0aHJvd3MgaGlzc3kgZml0IGlmDQo+ID4gPiB5b3UNCj4gPiA+IHRyeSB0byBm
ZWVkIGl0ICctTjQuMCcgb3IgJy1WNC4wJy4NCj4gPiA+IA0KPiA+ID4gVGhlIGtlcm5lbCBjaGFu
Z2UgbmVlZHMgdG8gYmUgYWNjb21wYW5pZWQgYnkgYSBmaXggdG8gbmZzLXV0aWxzLg0KPiA+ID4g
DQo+ID4gPiA+IElzIHRoYXQgcmVhbGx5IHdoYXQgd2Ugd2FudD8NCj4gPiA+ID4gDQo+ID4gPiA+
IFRoZSB0ZXh0IGluIHRoZSBtYW4gcGFnZSBmb3IgcnBjLm5mc2QgY291bGQgYmUgcmVhZCBhcyBh
bGxvd2luZw0KPiA+ID4gPiB0aGlzDQo+ID4gPiA+IGludGVycHJldGF0aW9uLCB0aG91Z2ggaXQg
aXNuJ3QgZXhwbGljaXQuDQo+ID4gPiA+IEkndmUgYWx3YXlzIHVzZWQgIk5GU3Y0IiB0byBtZWFu
IGFueSBvciBhbGwgZm9yIDQuMCwgNC4xLCA0LjIsDQo+ID4gPiA+IC4uLg0KPiA+ID4gDQo+ID4g
PiBXZSBfY291bGRfIGRvIHRoYXQuIEFzIEkgc2FpZCwgdGhhdCdzIGEgcXVlc3Rpb24gb2YgZml4
aW5nDQo+ID4gPiBycGMubmZzZC4NCj4gPiA+IA0KPiA+ID4gVGhlIHF1ZXN0aW9uIGhlcmUsIHRo
b3VnaCwgaXMgd2hhdCBzaG91bGQgdGhlIGtlcm5lbCBkbyB3aGVuIHlvdQ0KPiA+ID4gc3RhcnQN
Cj4gPiA+IHBva2luZyBpbnRvIC9wcm9jL2ZzL25mc2QvdmVyc2lvbnMuIEkgdGhpbmsgYWRkaW5n
IGEgbmV3IGZpZWxkDQo+ID4gPiBiZXR3ZWVuDQo+ID4gPiAnNCcgYW5kICc0LjAnIG1pZ2h0IGJl
IG1vcmUgZGlzcnVwdGl2ZSB0aGFuIGNoYW5naW5nIHRoZSBtZWFuaW5nDQo+ID4gPiBvZiAnNCcN
Cj4gPiA+IHRvIG1lYW4gJzQuMCcgYXMgdGhlcmUgbWlnaHQgYmUgb3RoZXIgcGFyc2VycyBvZiB0
aGF0IHBzZXVkb2ZpbGUNCj4gPiA+IG91dA0KPiA+ID4gdGhlcmUgdG9kYXkuDQo+ID4gDQo+ID4g
SSdtIE9LIHdpdGggdGhhdCwgdW5sZXNzIE5laWwgc2VlcyBhIHByb2JsZW0uDQo+IA0KPiBJIGRv
bid0IGtub3cgLi4uIHRoZSBtb3JlIEkgdGhpbmsgYWJvdXQgaXQsIHRoZSBsZXNzIGNlcnRhaW4g
SSBzZWVtDQo+IHRvDQo+IGJlLg0KPiANCj4gUHJpb3IgdG8gVHJvbmQncyB1cGRhdGVzIGZvciBu
ZnMtdXRpbHMsICJycGMubmZzZCAtTiA0IiB3b3VsZCB3cml0ZQ0KPiDCoCIrMyAtNCINCj4gdG8g
dGhlICd2ZXJzaW9ucycgZmlsZSwgd2hpY2ggd2lsbCBjdXJyZW50bHkgZGlzYWJsZSBORlN2NA0K
PiBjb21wbGV0ZWx5Lg0KPiBXaXRoIHRoaXMgcGF0Y2ggaXQgd2lsbCBkaXNhYmxlIDQuMCwgYnV0
IGxlYXZlIDQuMSBhY3RpdmUuDQo+IA0KPiBUaGlzIGJlaGF2aW9yIGlzIGNvbnNpc3RlbnQgd2l0
aCB0aGUgZG9jdW1lbnRhdGlvbiwgd2hpY2ggc2F5czoNCj4gDQo+IMKgwqDCoFRoZcKgwqBjdXJy
ZW50IHZlcnNpb24gb2YgcnBjLm5mc2QgY2FuIHN1cHBvcnQgbWFqb3IgTkZTIHZlcnNpb25zDQo+
IDIsMyw0DQo+IMKgwqDCoGFuZCB0aGUgbWlub3IgdmVyc2lvbnMgNC4xIGFuZCA0LjIuDQo+IA0K
PiB3aGljaCBzdWdnZXN0cyB0aGF0ICI0LjEiIGlzIGEgZGlmZmVyZW50IHRoaW5nIHRvICI0Iiwg
bm90IHBhcnQgb2YNCj4gaXQuDQo+IEFuZCB0aGUgY3VycmVudCBuZnMtdXRpbHMgZG9lc24ndCBz
dXBwb3J0IGRpc2FibGluZyBqdXN0IDQuMCBhdCBhbGwuDQo+IFlvdSBjYW5ub3Qgc2F5ICJycGMu
bmZzZCAtTiA0LjAiLg0KPiBTbyB0aGlzIHBhdGNoIGdpdmVzIG1vcmUgY29udHJvbCB0byB0aGUg
b2xkIG5mcy11dGlscywgYnkgY2hhbmdpbmcNCj4gdGhlDQo+IHNlbWFudGljcyBvZiBhIHBhcnRp
Y3VsYXIgb3BlcmF0aW9uLg0KPiANCj4gVGVjaG5pY2FsbHksIGl0IGlzIGEgcmVncmVzc2lvbiB0
aG91Z2guwqDCoEl0IGRvZXMgcmVzdWx0IGluIGEgbW9yZQ0KPiBjb2hlcmVudCBpbnRlcmZhY2Us
IHdoaWNoIGlzIGEgZ29vZCB0aGluZyBpbiB0aGUgbG9uZyB0ZXJtLg0KPiANCj4gV2hlbiB5b3Ug
cmVhZCBmcm9tICJ2ZXJzaW9ucyIgeW91IGdldCB0aGUgbWFqb3IgdmVyc2lvbiBhbmQgbWlub3IN
Cj4gdmVyc2lvbiBsaXN0ZWQgc2VwYXJhdGVseSwgd2l0aCBhIHNwZWNpYWwgY2FzZSB0aGF0ICIr
NC4wIiBpcyBuZXZlcg0KPiByZXBvcnRlZC4NCj4gU28gaWYganVzdCA0LjEgaXMgZW5hYmxlZCwg
eW91IG1pZ2h0IGdldA0KPiDCoMKgwqAtMyArNCAtNC4wICs0LjEgLTQuMg0KPiANCj4gYWZ0ZXIg
VHJvbmQncyBjaGFuZ2UgIjQiIG1lYW5zICI0LjAiIGFuZCAiNC4wIiBpcyBuZXZlciBkaXNwbGF5
ZWQsDQo+IHNvIHRoZSBhYm92ZSBjYXNlIHdvdWxkIGJlY29tZQ0KPiDCoMKgwqAtMyAtNCArNC4x
IC00LjINCj4gDQo+IHdoaWNoIGlzIHZlcnkgZGlmZmVyZW50LsKgwqBQcm9iYWJseSBhbm90aGVy
IHJlZ3Jlc3Npb24uDQo+IA0KPiBJIGd1ZXNzIEknbSBub3QgZW50aXJlbHkgc3VyZSB3aGF0IFRy
b25kIGlzIHRyeWluZyB0byBmaXguDQo+IFRoaXMgYml0Og0KPiANCj4gMSkgV2hlbiB0aGUgdXNl
ciB0dXJucyBvZmYgYWxsIG1pbm9yIHZlcnNpb25zIG9mIE5GU3Y0LCB0aGVuIHRoYXQNCj4gc2hv
dWxkDQo+IMKgwqDCoGJlIGVxdWl2YWxlbnQgdG8gdHVybmluZyBvZmYgTkZTdjQgc3VwcG9ydCwg
YW5kIHNvIHdoZW4gc29tZW9uZQ0KPiB0cmllcw0KPiDCoMKgwqB0byBtb3VudCwgd2Ugc2hvdWxk
IHJldHVybiBSUENfUFJPR19NSVNNQVRDSC4NCj4gDQo+IE1ha2VzIHBlcmZlY3Qgc2Vuc2UuwqDC
oEJ1dCBJJ20gbm90IGNsZWFyIG9uIHdoeSB0aGUgZm9ybWF0IG9mIHRoZQ0KPiB2ZXJzaW9ucyBm
aWxlIG5lZWRzIHRvIGNoYW5nZS4NCj4gVHJvbmQgLSB3aGF0IGFtIEkgbWlzc2luZz8NCj4gDQoN
ClRoZSBtYWluIGlzc3VlIHRoYXQgd29ycmllZCBtZSB3YXMgb25lIG9mIHByZWRpY3RhYmlsaXR5
LiBXaGF0IGlzDQpzdXBwb3NlZCB0byBoYXBwZW4gd2hlbiB5b3UgdHlwZSAiZWNobyArNCI/IE9u
ZSB0aGluZyB0aGF0IEkgY29uc2lkZXJlZA0KdG8gYmUgYSByZWdyZXNzaW9uLCB3YXMgdGhhdCBw
cmV2aW91c2x5LCBJIGNvdWxkIGV4cGVjdCB0aGF0ICJlY2hvICs0Ig0Kd291bGQgYXQgdGhlIHZl
cnkgbGVhc3QgdHVybiBvbiBORlN2NCBtaW5vciB2ZXJzaW9uIDAsIGJ1dCB3aXRoIHRoZQ0KY2hh
bmdlIHRvIHNlbWFudGljcywgaXQgd291bGQgb25seSBkbyBzbyBpZiBub2JvZHkgaGFkIHR5cGVk
ICJlY2hvDQotNC4wIi4NCg0KQW4gYW5hbG9neSB3b3VsZCBiZSBwdXR0aW5nIDIgbGlnaHQgc3dp
dGNoZXMgaW4gZnJvbnQgb2YgYSBsaWdodCBidWxiLA0Kc28gdGhhdCB1bmxlc3MgYm90aCBzd2l0
Y2hlcyBhcmUgb24sIHRoZSBidWxiIHdpbGwgbm90IHR1cm4gb24uDQpBY3R1YWxseSwgaXQgaXMg
d29yc2UgdGhhbiB0aGF0LCBiZWNhdXNlIG5vbmUgb2YgdGhlIGJ1bGJzIHR1cm4gb24NCnVudGls
IHlvdSBzdGFydCB1cCBrbmZzZCAoc28geW91IGNhbiBhcmd1ZSB0aGF0IHRoZXJlIGlzIGEgdGhp
cmQgc3dpdGNoDQppbiBmcm9udCBvZiB0aGUgb3RoZXIgdHdvKS4NCldoeSBkbyB3ZSBuZWVkIHRo
aXMgbWFueSBsZXZlbHMgb2Ygc3dpdGNoZXMgaW4gYSBrZXJuZWwgaW50ZXJmYWNlPyBZb3UNCnNo
b3VsZCBiZSBhYmxlIHRvIGFjaGlldmUgdGhlIHNhbWUgZnVuY3Rpb25hbGl0eSBieSBqdXN0IHR1
cm5pbmcgb24gYW5kDQpvZmYgdGhlIGluZGl2aWR1YWwgbWlub3IgdmVyc2lvbnMuIFRoZSByaWdo
dCBwbGFjZSBmb3IgZGVzaWduaW5nIG1vcmUNCmNvbXBsZXggaW50ZXJmYWNlcyB3b3VsZCBiZSB1
c2Vyc3BhY2UsIGFuZCBpcyBleGFjdGx5IHdoYXQgdGhlIHJwYy5uZnNkDQp1dGlsaXR5IHNob3Vs
ZCBiZSB0YWtpbmcgY2FyZSBvZi4NCg0KRmluYWxseSwgdGhlcmUgaXMgdGhlIGlzc3VlIHRoYXQg
dGhlIGludGVyZmFjZSBhbGxvd2VkIHNpdHVhdGlvbnMgd2hlcmUNCmtuZnNkIHdhcyBhZHZlcnRp
c2luZyBzdXBwb3J0IGZvciBORlN2NCB2aWEgcnBjYmluZCwgYnV0IHRoZXJlIHdlcmUgbm8NCm1p
bm9yIHZlcnNpb25zIGVuYWJsZWQsIGFuZCBzbyB5b3UnZCBqdXN0IGdldCBhIGNvbmZ1c2luZyBz
ZXJpZXMgb2YNCk5GUzRFUlJfTUlOT1JfVkVSU0lPTl9NSVNNQVRDSCByZXBsaWVzIHdoZW4gYXR0
ZW1wdGluZyB0byBtb3VudC4gV2h5DQpldmVuIGFkdmVydGlzZSBzdXBwb3J0IGluIHRoYXQgY2Fz
ZT8NCg0KPiBUaGFua3MsDQo+IE5laWxCcm93bg0KPiANCj4gDQo+IA0KPiA+IA0KPiA+IFRoZSBj
aGFuZ2Vsb2cgb24gdGhlIGZpcnN0IHBhdGNoIGRvZXNuJ3Qgc2VlbSB0byBkZXNjcmliZSB0aGUg
djINCj4gPiB2ZXJzaW9uDQo+ID4gdmVyeSB3ZWxsOyB1c2luZyB0aGUgYmVsb3cgY3JpYmJlZCBm
cm9tIHlvdXIgY292ZXIgbGV0dGVyIGluc3RlYWQuDQo+ID4gDQo+ID4gwqDCoMKgwqBuZnNkOiBm
aXggY29uZmlndXJhdGlvbiBvZiBzdXBwb3J0ZWQgbWlub3IgdmVyc2lvbnMNCj4gPiDCoMKgwqDC
oA0KPiA+IMKgwqDCoMKgV2hlbiB0aGUgdXNlciB0dXJucyBvZmYgYWxsIG1pbm9yIHZlcnNpb25z
IG9mIE5GU3Y0LCB0aGF0DQo+ID4gc2hvdWxkIGJlDQo+ID4gwqDCoMKgwqBlcXVpdmFsZW50IHRv
IHR1cm5pbmcgb2ZmIE5GU3Y0IHN1cHBvcnQsIHNvIGEgbW91bnQgYXR0ZW1wdA0KPiA+IHVzaW5n
IE5GU3Y0DQo+ID4gwqDCoMKgwqBzaG91bGQgZ2V0IFJQQ19QUk9HX01JU01BVENILCBub3QgTkZT
RVJSX01JTk9SX1ZFUlNfTUlTTUFUQ0guDQo+ID4gwqDCoMKgwqANCj4gPiDCoMKgwqDCoEFsbG93
IHRoZSB1c2VyIHRvIHVzZSBlaXRoZXIgJzQuMCcgb3IgJzQnIHRvIGVuYWJsZSBvciBkaXNhYmxl
DQo+ID4gbWlub3INCj4gPiDCoMKgwqDCoHZlcnNpb24gMC7CoMKgT3RoZXIgbWlub3IgdmVyc2lv
bnMgYXJlIHN0aWxsIGVuYWJsZWQgb3IgZGlzYWJsZWQNCj4gPiB1c2luZyB0aGUNCj4gPiDCoMKg
wqDCoCc0LngnIGZvcm1hdC4NCj4gPiANCj4gPiAtLWIuDQotLSANClRyb25kIE15a2xlYnVzdA0K
TGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0
QHByaW1hcnlkYXRhLmNvbQ0K

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[NeilBrown]

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

On Sat, Mar 04 2017, Trond Myklebust wrote:

> On Fri, 2017-03-03 at 09:57 +1100, NeilBrown wrote:
>> I guess I'm not entirely sure what Trond is trying to fix.
>> This bit:
>> 
>> 1) When the user turns off all minor versions of NFSv4, then that
>> should
>>    be equivalent to turning off NFSv4 support, and so when someone
>> tries
>>    to mount, we should return RPC_PROG_MISMATCH.
>> 
>> Makes perfect sense.  But I'm not clear on why the format of the
>> versions file needs to change.
>> Trond - what am I missing?
>>

Thanks for the details!!


>
> The main issue that worried me was one of predictability. What is
> supposed to happen when you type "echo +4"? One thing that I considered
> to be a regression, was that previously, I could expect that "echo +4"
> would at the very least turn on NFSv4 minor version 0, but with the
> change to semantics, it would only do so if nobody had typed "echo
> -4.0".

I don't think I would consider this as a regression.
Prior to

Commit: e35659f1b03c ("NFSD: correctly range-check v4.x minor version when setting versions.")

"echo -4.0" would result in an error.  After that patch it will result in
behaviour that you think is inconsistent.  While that might be a poor
design choice, I don't think it is a regression because it is not
(holistically) something that worked before and now works differently.

I agree that "echo +4" should do something sensible and predictable.  I
would like to suggest that it should enable all "supported" NFSv4.x minor
versions.  That is consistent with how rpc.nfsd uses it, and makes sense
to me. "echo -4" should disable all minor versions.

>
> An analogy would be putting 2 light switches in front of a light bulb,
> so that unless both switches are on, the bulb will not turn on.
> Actually, it is worse than that, because none of the bulbs turn on
> until you start up knfsd (so you can argue that there is a third switch
> in front of the other two).
> Why do we need this many levels of switches in a kernel interface? You
> should be able to achieve the same functionality by just turning on and
> off the individual minor versions. The right place for designing more
> complex interfaces would be userspace, and is exactly what the rpc.nfsd
> utility should be taking care of.

The "no regressions" rule can often lead to clunky interfaces.  It
certainly isn't ideal, but sometimes we need to live with it.
The right place to hide that clunkiness is in rpc.nfsd :-)

>
> Finally, there is the issue that the interface allowed situations where
> knfsd was advertising support for NFSv4 via rpcbind, but there were no
> minor versions enabled, and so you'd just get a confusing series of
> NFS4ERR_MINOR_VERSION_MISMATCH replies when attempting to mount. Why
> even advertise support in that case?

I agree with this.
If all minor versions are disabled, the major version should be disabled
as well.  If any minor versions are enabled, the major version must be
enabled too.

Thanks,
NeilBrown

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

Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0

[bfields]

Sorry for not paying close attention here.  Catching up:

On Mon, Mar 06, 2017 at 03:26:40PM +1100, NeilBrown wrote:
> On Sat, Mar 04 2017, Trond Myklebust wrote:
> 
> > The main issue that worried me was one of predictability. What is
> > supposed to happen when you type "echo +4"? One thing that I considered
> > to be a regression, was that previously, I could expect that "echo +4"
> > would at the very least turn on NFSv4 minor version 0, but with the
> > change to semantics, it would only do so if nobody had typed "echo
> > -4.0".
> 
> I don't think I would consider this as a regression. Prior to
> 
> Commit: e35659f1b03c ("NFSD: correctly range-check v4.x minor version when setting versions.")
> 
> "echo -4.0" would result in an error.  After that patch it will result in
> behaviour that you think is inconsistent.  While that might be a poor
> design choice, I don't think it is a regression because it is not
> (holistically) something that worked before and now works differently.
>
> I agree that "echo +4" should do something sensible and predictable.  I
> would like to suggest that it should enable all "supported" NFSv4.x minor
> versions.  That is consistent with how rpc.nfsd uses it, and makes sense
> to me. "echo -4" should disable all minor versions.
> 
> >
> > An analogy would be putting 2 light switches in front of a light bulb,
> > so that unless both switches are on, the bulb will not turn on.
> > Actually, it is worse than that, because none of the bulbs turn on
> > until you start up knfsd (so you can argue that there is a third switch
> > in front of the other two).
> > Why do we need this many levels of switches in a kernel interface? You
> > should be able to achieve the same functionality by just turning on and
> > off the individual minor versions. The right place for designing more
> > complex interfaces would be userspace, and is exactly what the rpc.nfsd
> > utility should be taking care of.
> 
> The "no regressions" rule can often lead to clunky interfaces.  It
> certainly isn't ideal, but sometimes we need to live with it.
> The right place to hide that clunkiness is in rpc.nfsd :-)

That sounds right to me.

> > Finally, there is the issue that the interface allowed situations where
> > knfsd was advertising support for NFSv4 via rpcbind, but there were no
> > minor versions enabled, and so you'd just get a confusing series of
> > NFS4ERR_MINOR_VERSION_MISMATCH replies when attempting to mount. Why
> > even advertise support in that case?
> 
> I agree with this.
> If all minor versions are disabled, the major version should be disabled
> as well.  If any minor versions are enabled, the major version must be
> enabled too.

So, if you have a patch that keeps the agreed-on change while reverting
to a (clunkier, but more backwards-compatible) interface, and if you can
do it while we're still early in 4.11, then I'd take that.

--b.