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

[NeilBrown <neilb@suse.com>]

[-- 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 --]