* [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 @ 2017-02-22 23:35 Trond Myklebust 2017-02-22 23:35 ` [PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0 Trond Myklebust 2017-02-23 0:13 ` [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 NeilBrown 0 siblings, 2 replies; 10+ messages in thread From: Trond Myklebust @ 2017-02-22 23:35 UTC (permalink / raw) To: bfields, neilb; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0 2017-02-22 23:35 [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 Trond Myklebust @ 2017-02-22 23:35 ` Trond Myklebust 2017-02-22 23:35 ` [PATCH v2 2/2] nfsd: Fix display of the version string Trond Myklebust 2017-02-23 0:13 ` [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 NeilBrown 1 sibling, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2017-02-22 23:35 UTC (permalink / raw) To: bfields, neilb; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] nfsd: Fix display of the version string 2017-02-22 23:35 ` [PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0 Trond Myklebust @ 2017-02-22 23:35 ` Trond Myklebust 0 siblings, 0 replies; 10+ messages in thread From: Trond Myklebust @ 2017-02-22 23:35 UTC (permalink / raw) To: bfields, neilb; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-02-22 23:35 [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 Trond Myklebust 2017-02-22 23:35 ` [PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0 Trond Myklebust @ 2017-02-23 0:13 ` NeilBrown 2017-02-23 1:00 ` Trond Myklebust 1 sibling, 1 reply; 10+ messages in thread From: NeilBrown @ 2017-02-23 0:13 UTC (permalink / raw) To: Trond Myklebust, bfields; +Cc: linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-02-23 0:13 ` [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 NeilBrown @ 2017-02-23 1:00 ` Trond Myklebust 2017-02-27 23:03 ` bfields 0 siblings, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2017-02-23 1:00 UTC (permalink / raw) To: bfields, neilb; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-02-23 1:00 ` Trond Myklebust @ 2017-02-27 23:03 ` bfields 2017-03-02 22:57 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: bfields @ 2017-02-27 23:03 UTC (permalink / raw) To: Trond Myklebust; +Cc: neilb, linux-nfs 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-02-27 23:03 ` bfields @ 2017-03-02 22:57 ` NeilBrown 2017-03-04 18:55 ` Trond Myklebust 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2017-03-02 22:57 UTC (permalink / raw) To: bfields, Trond Myklebust; +Cc: linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-03-02 22:57 ` NeilBrown @ 2017-03-04 18:55 ` Trond Myklebust 2017-03-06 4:26 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2017-03-04 18:55 UTC (permalink / raw) To: bfields, neilb; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-03-04 18:55 ` Trond Myklebust @ 2017-03-06 4:26 ` NeilBrown 2017-03-09 20:51 ` bfields 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2017-03-06 4:26 UTC (permalink / raw) To: Trond Myklebust, bfields; +Cc: linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 2017-03-06 4:26 ` NeilBrown @ 2017-03-09 20:51 ` bfields 0 siblings, 0 replies; 10+ messages in thread From: bfields @ 2017-03-09 20:51 UTC (permalink / raw) To: NeilBrown; +Cc: Trond Myklebust, linux-nfs 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-09 20:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-22 23:35 [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 Trond Myklebust 2017-02-22 23:35 ` [PATCH v2 1/2] nfsd: Allow enabling NFSv4.x without also requiring NFSv4.0 Trond Myklebust 2017-02-22 23:35 ` [PATCH v2 2/2] nfsd: Fix display of the version string Trond Myklebust 2017-02-23 0:13 ` [PATCH v2 0/2] Fix up nfsd to enable NFSv4.x without NFSv4.0 NeilBrown 2017-02-23 1:00 ` Trond Myklebust 2017-02-27 23:03 ` bfields 2017-03-02 22:57 ` NeilBrown 2017-03-04 18:55 ` Trond Myklebust 2017-03-06 4:26 ` NeilBrown 2017-03-09 20:51 ` bfields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.