On Mon, Jun 19 2017, Steve Dickson wrote:
> On 06/15/2017 11:20 PM, NeilBrown wrote:
>> On Wed, Jun 14 2017, Steve Dickson wrote:
>>
>>> On 06/13/2017 10:09 PM, NeilBrown wrote:
>>>> On Tue, Jun 13 2017, Steve Dickson wrote:
>>>>
>>>>> Sorry I didn't see your entire response
>>>>> the first time around...
>>>>>
>>>>> On 06/12/2017 09:19 PM, NeilBrown wrote:
>>>>>> On Fri, Jun 09 2017, Steve Dickson wrote:
>>>>>>
>>>>>>> When v4 is specified on the command line the
>>>>>>> default minor version needs to be used.
>>>>>>>
>>>>>>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> utils/mount/stropts.c | 27 ++++++++++++++++++++-------
>>>>>>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>>>>> index 81fb945..d2d303f 100644
>>>>>>> --- a/utils/mount/stropts.c
>>>>>>> +++ b/utils/mount/stropts.c
>>>>>>> @@ -119,14 +119,22 @@ static void nfs_default_version(struct nfsmount_info *mi)
>>>>>>> if (mi->version.v_mode == V_DEFAULT &&
>>>>>>> config_default_vers.v_mode != V_DEFAULT) {
>>>>>>> mi->version.major = config_default_vers.major;
>>>>>>> - mi->version.minor = config_default_vers.minor;
>>>>>>> + if (config_default_vers.minor)
>>>>>>> + mi->version.minor = config_default_vers.minor;
>>>>>>> + else if (!mi->version.minor)
>>>>>>> + mi->version.minor = NFS_DEFAULT_MINOR;
>>>>>>
>>>>>> No, this looks wrong. A minor number of '0' can be perfectly valid.
>>>>>> Deciding to turn a minor of '0' into '2' is simply wrong.
>>>>> Please see my first response. Basically if the minor version
>>>>> is not set, the set it to the default version.
>>>>
>>>> You are testing config_default_vers.minor without first confirming that
>>>> config_default_vers.v_mode is V_SPECIFIC. I don't see how that makes
>>>> sense.
>>> Well at this point we know the config_default v_mode is either
>>> V_SPECIFIC or V_GENERAL.
>>
>> Yes.
>> If v_mode==V_GENERAL then minor==0, but that doesn't mean anything.
> This means v4 and use the NFS_DEFAULT_MINOR as the minor version
No, before nfs_default_version() is called, it really really doesn't mean
anything.
nfs_default_version() sees v_mode==V_GENERAL and needs to set a default
minor version, which might be NFS_DEFAULT_MINOR, or might be something
from config_default_vers.
>
>> If v_mode==V_SPECIFIC and minor==0, it means that v4.0 was explicitly requested.
>>
>>>>
>>>> We should use major/minor values out of config_default_vers based on its
>>>> v_mode, not on whether the values are zero or not.
>>>>
>>>> If v_mode == V_DEFAULT, don't use anything
>>>> If v_mode == V_GENERAL, use the major, not the minor
>>> How can't we use the minor if its v4... The whole point of
>>> these patches is to used the default minor when v4 is used.
>>
>> If v_mode==V_GENERAL, then the minor doesn't mean anything. It will
>> happen to be zero, but it is safer to think of it as undefined.
> When v_mode==V_GENERAL we know it is a v4 mount so the minor does
> mean something... and since we want to used the latest minor
> version, in the case minor should be set to NFS_DEFAULT_MINOR.
Before nfs_default_version() is called, the minor number hasn't been
set, so it cannot mean anything.
In this (V_GENERAL) case, nfs_default_version() sets the minor version to
a default.
After nfs_default_version ()is called, if v_mode=V_GENERAL, then minor
does mean something. It means 'try this minor number first'.
>
>>
>> Just to check what you want:
>> If someone mounts with "-t nfs4" or "-o v4" or "-o vers=4" and
>> nfsmount.conf contains "vers=4.1" then you want it to default to
>> try 4.1, then fallback to 4.0
>> But if nfsmount.conf contains "vers=4.0", you want to try
>> 4.0, and not fall back to anything.
>> If nfsmount.conf contains "vers=4", you want to try 4.2 first (because
>> that is NFS_DEFAULT_MINOR), then fall back to 4.1, then 4.0
>>
>> Is that correct (seems reasonable to me)?
> Yes.
>
>>
>> We know that the user requested one of those because mi->version.v_mode ==
>> V_GENERAL.
>> Then if config_default_vers.v_mode == V_SPECIFIC and major==4, we
>> want to copy .minor in. Otherwise we use NFS_DEFAULT_MINOR.
> I believe this is what the code is doing...
Your belief is ill founded.
Look at the code:
if (mi->version.v_mode == V_GENERAL) {
if (config_default_vers.v_mode != V_DEFAULT &&
- mi->version.major == config_default_vers.major)
- mi->version.minor = config_default_vers.minor;
+ mi->version.major == config_default_vers.major) {
+ if (mi->version.minor)
+ mi->version.minor = config_default_vers.minor;
+ else if (!mi->version.minor)
+ mi->version.minor = NFS_DEFAULT_MINOR;
+ } else if (!mi->version.minor)
+ mi->version.minor = NFS_DEFAULT_MINOR;
return;
}
if config_default_vers.v_mode == V_SPECIFIC and
config_default_vers.minor == 0 (meaning that the config file requested
vers=4.0)
then what is the result of this code?
Is v4.0 used as requested?
>
>>
>>
>>>
>>>> If v_mode == V_SPECIFIC, use major and minor (though minor is irrelevant
>>>> if major != 4)
>>>>
>>>>>
>>>>>>
>>>>>> You can only tell if .minor is valid by looking at .v_mode.
>>>>>> If .v_mode is V_GENERAL or V_DEFAULT, then .minor is not valid
>>>>>> and a default should be used. If it is V_SPECIFIC, then .minor is
>>>>>> either valid or irrelevant, depending on .major.
>>>>>>
>>>>>>
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> if (mi->version.v_mode == V_GENERAL) {
>>>>>>> if (config_default_vers.v_mode != V_DEFAULT &&
>>>>>>> - mi->version.major == config_default_vers.major)
>>>>>>> - mi->version.minor = config_default_vers.minor;
>>>>>>> + mi->version.major == config_default_vers.major) {
>>>>>>> + if (mi->version.minor)
>>>>>>> + mi->version.minor = config_default_vers.minor;
>>>>>>
>>>>>> Again, don't test version.minor like this.
>>>>> You have to... For the -o minorversion=1 cast.
>>>>> If the minor is already set, don't reset it.
>>>>
>>>> "-o minorversion=1" causes "v_mode = V_SPECIFIC" (in nfs_nfs_version()).
>>>> This isn't a case that is of any concern in nfs_default_version().
>>> True..
>>>>>
>>>>>>
>>>>>>> + else if (!mi->version.minor)
>>>>>>> + mi->version.minor = NFS_DEFAULT_MINOR;
>>>>>>> + } else if (!mi->version.minor)
>>>>>>> + mi->version.minor = NFS_DEFAULT_MINOR;
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -740,10 +748,15 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>>>>>>> }
>>>>>>>
>>>>>>> if (mi->version.v_mode != V_SPECIFIC) {
>>>>>>> - if (mi->version.v_mode == V_GENERAL)
>>>>>>> - snprintf(version_opt, sizeof(version_opt) - 1,
>>>>>>> - "vers=%lu", mi->version.major);
>>>>>>> - else
>>>>>>> + if (mi->version.v_mode == V_GENERAL) {
>>>>>>> + if (mi->version.major > 3)
>>>>>>
>>>>>> This test is pointless as .v_mode is only ever V_GENERAL when
>>>>>> .major == 4
>>>>>>
>>>>>> And given that this is nfs_do_mount_v4(), we can be certain that
>>>>>> version.major == 4.
>>>>>>
>>>>>> Prior to
>>>>>> Commit: 0d71b058092f ("NFS: Extend the -overs= mount option to allow 4.x minorversions")
>>>>>>
>>>>>> in Linux 3.4, writing "vers=4.1" wasn't supported.
>>>>>> So I think it would be safest to use
>>>>>> vers=4 if minor == 0
>>>>> I'm thinking if you don't specify the minor version
>>>>> the default minor version should be used.
>>>>
>>>> Yes, but I'm talking about how you tell the kernel that.
>>>> At this point in the code a choice has been made about what major/minor
>>>> to use. Negotiation is happening at a higher level and here we just
>>>> need to tell the kernel the current choice.
>>>> If v_mode == V_SPECIFIC, we don't do anything because the options passed
>>>> to mount.nfs are pass through to the kernel unchanged.
>>> Fine...
>>>
>>>> In other cases we need to create a kernel option to request
>>>> a specific version.
>>>> vers=4
>>>> will always choose v4.0, in any kernel that supports it. On some
>>>> kernels, "vers=4.0" won't work.
>>> which ones don't support v4.0?
>>>
>>
>> Everything prior to
>> Commit: 0d71b058092f ("NFS: Extend the -overs= mount option to allow 4.x minorversions")
>>
>> in Linux 3.4.
>>
>>>> vers=4,minorversion=1
>>>> will always choose v4.1 in any kernel that supports it.
>>>> vers=4.2
>>>> will always choose v4.2 if supported.
>>>>
>>>> So I think those are the options we should use.
>>> I guess I don't see how we can test what a kernel can
>>> or can not support... It has to be an assumption.
>>> Does it have to be dynamic? Meaning and the mount
>>> command be tailored to a particular kernel?
>>
>> The mount command doesn't need to be tailored because the
>> kernel is, fortunately, backward compatible.
>> Each of
>> vers=4
>> vers=4,minorversion=1
>> vers=4.2
>>
>> will work correctly on all kernels which support that particular minor
>> version.
> And it does... see below.
>
>>
>>
>>>>
>>>>>
>>>>> Again, I'm thinking there should be no difference
>>>>> between -t nfs4, -o vers=4, -o nfsvers=4 and -o v4.
>>>>> They all should become v4.2 mounts
>>>>
>>>> They should on current kernels if the server supports it. On older
>>>> kernels, or with older servers, they might become v4.1 or v4.0.
>>>> We need to make sure the way we tell the kernel what version to use,
>>>> works on the widest possible range of kernels.
>>> So you are talking about using a new nfs-utils on a older kernel?
>>
>> Correct.
>>
>>> Is that something that is done?
>>
>> We have to assume that it is. We don't necessarily need to test all the
>> combinations, but we need to make a reasonable effort to support
>> all kernels which have the functionality requested.
>> The only alternative is to put some sort of rule in the configure
>> script so that it refuses to compile on old kernels, but I think that
>> should be a last-resort.
> I tested the latest patch version (V3) on a 2.6 kernel. It ends
> up falling back to v3 since nfs_autonegotiate() retries with
> lower minor version then ends up doing a v3 mount which succeeds.
>
> mount: no type was given - I'll assume nfs because of the colon
> mount.nfs: timeout set for Sun Dec 11 23:16:15 2016
> mount.nfs: trying text-based options 'vers=4.2,addr=172.31.1.50,clientaddr=172.31.1.28'
> mount.nfs: mount(2): Invalid argument
> mount.nfs: trying text-based options 'vers=4.1,addr=172.31.1.50,clientaddr=172.31.1.28'
> mount.nfs: mount(2): Invalid argument
> mount.nfs: trying text-based options 'vers=4.0,addr=172.31.1.50,clientaddr=172.31.1.28'
> mount.nfs: mount(2): Invalid argument
> mount.nfs: trying text-based options 'addr=172.31.1.50'
> mount.nfs: prog 100003, trying vers=3, prot=6
> mount.nfs: trying 172.31.1.50 prog 100003 vers 3 prot TCP port 2049
> mount.nfs: prog 100005, trying vers=3, prot=17
> mount.nfs: trying 172.31.1.50 prog 100005 vers 3 prot UDP port 20048
>
> So I think we are good with older kernels.
Given that all 2.6 kernels support NFSv4, and given that current
nfs-utils will correctly use NFSv4 on 2.6.x, it isn't clear to me how
you can say that having the new nfs-utils fall back to NFSv3 is "good".
Thanks,
NeilBrown
>>
>> Thanks,
>> NeilBrown
>>
Attachment:
signature.asc
Description: PGP signature
