[PATCH 1/2 V2] mount.nfs: Use default minor version when -t nfs4 is specified

[PATCH 1/2 V2] mount.nfs: Use default minor version when -t nfs4 is specified

[Steve Dickson]

When the nfs4 filesystem specified, the default major
and minor versions should be used.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/stropts.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c0266e5..81fb945 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -73,6 +73,13 @@
 #define NFS_DEF_BG_TIMEOUT_MINUTES	(10000u)
 #endif
 
+#ifndef NFS_DEFAULT_MAJOR
+#define NFS_DEFAULT_MAJOR	4
+#endif
+#ifndef NFS_DEFAULT_MINOR
+#define NFS_DEFAULT_MINOR	2
+#endif
+
 extern int nfs_mount_data_version;
 extern char *progname;
 extern int verbose;
@@ -124,8 +131,8 @@ static void nfs_default_version(struct nfsmount_info *mi)
 	}
 
 #endif /* MOUNT_CONFIG */
-	mi->version.major = 4;
-	mi->version.minor = 2;
+	mi->version.major = NFS_DEFAULT_MAJOR;
+	mi->version.minor = NFS_DEFAULT_MINOR;
 }
 
 /*
@@ -316,8 +323,9 @@ static int nfs_set_version(struct nfsmount_info *mi)
 		return 0;
 
 	if (strncmp(mi->type, "nfs4", 4) == 0) {
-		mi->version.major = 4;
-		mi->version.v_mode = V_GENERAL;
+		/* Set to default values */
+		mi->version.major = NFS_DEFAULT_MAJOR;
+		mi->version.minor = NFS_DEFAULT_MINOR;
 	}
 	/*
 	 * Before 2.6.32, the kernel NFS client didn't
-- 
2.9.4

[PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Steve Dickson]

When v4 is specified on the command line the
default minor version needs to be used.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 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;
 		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;
+			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)
+				snprintf(version_opt, sizeof(version_opt) - 1,
+					"vers=%lu.%lu", mi->version.major,
+					mi->version.minor);
+			else 
+				snprintf(version_opt, sizeof(version_opt) - 1,
+					"vers=%lu", mi->version.major);
+		} else
 			snprintf(version_opt, sizeof(version_opt) - 1,
 				"vers=%lu.%lu", mi->version.major,
 				mi->version.minor);
-- 
2.9.4

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[NeilBrown]

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

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@redhat.com>
> ---
>  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.

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.

> +			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
  vers=4,minorversion=1    if minor == 1
  vers=4.2                 if minor == 2

This is despite the fact that the comment in the kernel says
                                                  In future,
                 * the mount program should always supply
                 * a NFSv4 minor version number.

I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
versions.

Alternately we could test the kernel version and behave differently, but
I think that is worse.

Thanks,
NeilBrown

> +				snprintf(version_opt, sizeof(version_opt) - 1,
> +					"vers=%lu.%lu", mi->version.major,
> +					mi->version.minor);
> +			else 
> +				snprintf(version_opt, sizeof(version_opt) - 1,
> +					"vers=%lu", mi->version.major);
> +		} else
>  			snprintf(version_opt, sizeof(version_opt) - 1,
>  				"vers=%lu.%lu", mi->version.major,
>  				mi->version.minor);
> -- 
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Steve Dickson]

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@redhat.com>
>> ---
>>  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.
The only way to have a minor version of '0' is to use 
the -o v4.0 flag. That would causes v_mode to be V_SPECIFIC.
Also note, in nfs_nfs_version() when major is < 4 
the v_mode is also set to V_SPECIFIC (aka -o v3)
For both those case nfs_default_version() is not called. 

So the only way we can get into this code is if v_mode 
is V_GENERAL or V_DEFAULT. So it is a v4 mount and the
minor version needs to be set. So a minor == 0 means
the minor version has not be set so set it to the
default minor version. 

So I think we are good with this...

steved.

> 
> 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.
> 
>> +			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
>   vers=4,minorversion=1    if minor == 1
>   vers=4.2                 if minor == 2
> 
> This is despite the fact that the comment in the kernel says
>                                                   In future,
>                  * the mount program should always supply
>                  * a NFSv4 minor version number.
> 
> I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
> versions.
> 
> Alternately we could test the kernel version and behave differently, but
> I think that is worse.
> 
> Thanks,
> NeilBrown
> 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu.%lu", mi->version.major,
>> +					mi->version.minor);
>> +			else 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu", mi->version.major);
>> +		} else
>>  			snprintf(version_opt, sizeof(version_opt) - 1,
>>  				"vers=%lu.%lu", mi->version.major,
>>  				mi->version.minor);
>> -- 
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Steve Dickson]

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@redhat.com>
>> ---
>>  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 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.

> 
>> +			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. 

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

This mean the only way to not used the default version
is to specify it.

>   vers=4,minorversion=1    if minor == 1
>   vers=4.2                 if minor == 2
> 
> This is despite the fact that the comment in the kernel says
>                                                   In future,
>                  * the mount program should always supply
>                  * a NFSv4 minor version number.
I agree... 

> 
> I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
> versions.
> 
> Alternately we could test the kernel version and behave differently, but
> I think that is worse.
Yeah we have a few MAKE_VERSION() sprinkle though the legacy code 
> 
> Thanks,
> NeilBrown
> 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu.%lu", mi->version.major,
>> +					mi->version.minor);
>> +			else 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu", mi->version.major);
>> +		} else
>>  			snprintf(version_opt, sizeof(version_opt) - 1,
>>  				"vers=%lu.%lu", mi->version.major,
>>  				mi->version.minor);
>> -- 
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[NeilBrown]

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

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@redhat.com>
>>> ---
>>>  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.

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
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().

>
>> 
>>> +			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.
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.
  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.

>
> 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.

Thanks,
NeilBrown


>
> This mean the only way to not used the default version
> is to specify it.
>
>>   vers=4,minorversion=1    if minor == 1
>>   vers=4.2                 if minor == 2
>> 
>> This is despite the fact that the comment in the kernel says
>>                                                   In future,
>>                  * the mount program should always supply
>>                  * a NFSv4 minor version number.
> I agree... 
>
>> 
>> I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
>> versions.
>> 
>> Alternately we could test the kernel version and behave differently, but
>> I think that is worse.
> Yeah we have a few MAKE_VERSION() sprinkle though the legacy code 
>> 
>> Thanks,
>> NeilBrown
>> 
>>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>>> +					"vers=%lu.%lu", mi->version.major,
>>> +					mi->version.minor);
>>> +			else 
>>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>>> +					"vers=%lu", mi->version.major);
>>> +		} else
>>>  			snprintf(version_opt, sizeof(version_opt) - 1,
>>>  				"vers=%lu.%lu", mi->version.major,
>>>  				mi->version.minor);
>>> -- 
>>> 2.9.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Steve Dickson]

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@redhat.com>
>>>> ---
>>>>  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. 
> 
> 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_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? 

>   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?   
> 
>>
>> 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?
Is that something that is done?

steved.
> 
> Thanks,
> NeilBrown
> 
> 
>>
>> This mean the only way to not used the default version
>> is to specify it.
>>
>>>   vers=4,minorversion=1    if minor == 1
>>>   vers=4.2                 if minor == 2
>>>
>>> This is despite the fact that the comment in the kernel says
>>>                                                   In future,
>>>                  * the mount program should always supply
>>>                  * a NFSv4 minor version number.
>> I agree... 
>>
>>>
>>> I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
>>> versions.
>>>
>>> Alternately we could test the kernel version and behave differently, but
>>> I think that is worse.
>> Yeah we have a few MAKE_VERSION() sprinkle though the legacy code 
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>>>> +					"vers=%lu.%lu", mi->version.major,
>>>> +					mi->version.minor);
>>>> +			else 
>>>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>>>> +					"vers=%lu", mi->version.major);
>>>> +		} else
>>>>  			snprintf(version_opt, sizeof(version_opt) - 1,
>>>>  				"vers=%lu.%lu", mi->version.major,
>>>>  				mi->version.minor);
>>>> -- 
>>>> 2.9.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[NeilBrown]

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

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@redhat.com>
>>>>> ---
>>>>>  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.
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.

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)?

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.


>
>> 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.


>> 
>>>
>>> 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.

Thanks,
NeilBrown

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

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Steve Dickson]

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@redhat.com>
>>>>>> ---
>>>>>>  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

> 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.

> 
> 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... 

> 
> 
>>
>>> 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. 
> 
> Thanks,
> NeilBrown
> 

Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[NeilBrown]

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

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@redhat.com>
>>>>>>> ---
>>>>>>>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2 V2] mount.nfs: Use default minor version when -t nfs4 is specified

[NeilBrown]

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

On Fri, Jun 09 2017, Steve Dickson wrote:

> When the nfs4 filesystem specified, the default major
> and minor versions should be used.
>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  utils/mount/stropts.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index c0266e5..81fb945 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -73,6 +73,13 @@
>  #define NFS_DEF_BG_TIMEOUT_MINUTES	(10000u)
>  #endif
>  
> +#ifndef NFS_DEFAULT_MAJOR
> +#define NFS_DEFAULT_MAJOR	4
> +#endif
> +#ifndef NFS_DEFAULT_MINOR
> +#define NFS_DEFAULT_MINOR	2
> +#endif
> +
>  extern int nfs_mount_data_version;
>  extern char *progname;
>  extern int verbose;
> @@ -124,8 +131,8 @@ static void nfs_default_version(struct nfsmount_info *mi)
>  	}
>  
>  #endif /* MOUNT_CONFIG */
> -	mi->version.major = 4;
> -	mi->version.minor = 2;
> +	mi->version.major = NFS_DEFAULT_MAJOR;
> +	mi->version.minor = NFS_DEFAULT_MINOR;
>  }
>  
>  /*
> @@ -316,8 +323,9 @@ static int nfs_set_version(struct nfsmount_info *mi)
>  		return 0;
>  
>  	if (strncmp(mi->type, "nfs4", 4) == 0) {
> -		mi->version.major = 4;
> -		mi->version.v_mode = V_GENERAL;
> +		/* Set to default values */
> +		mi->version.major = NFS_DEFAULT_MAJOR;
> +		mi->version.minor = NFS_DEFAULT_MINOR;

Why do you remove the setting of v_mode to V_GENERAL?
Won't that leave it as V_DEFAULT, and doesn't that mean
that it will negotiate from v4 down to v3??

NeilBrown


>  	}
>  	/*
>  	 * Before 2.6.32, the kernel NFS client didn't
> -- 
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/2 V2] mount.nfs: Use default minor version when -t nfs4 is specified

[Steve Dickson]

On 06/12/2017 08:59 PM, NeilBrown wrote:
> On Fri, Jun 09 2017, Steve Dickson wrote:
> 
>> When the nfs4 filesystem specified, the default major
>> and minor versions should be used.
>>
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  utils/mount/stropts.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index c0266e5..81fb945 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -73,6 +73,13 @@
>>  #define NFS_DEF_BG_TIMEOUT_MINUTES	(10000u)
>>  #endif
>>  
>> +#ifndef NFS_DEFAULT_MAJOR
>> +#define NFS_DEFAULT_MAJOR	4
>> +#endif
>> +#ifndef NFS_DEFAULT_MINOR
>> +#define NFS_DEFAULT_MINOR	2
>> +#endif
>> +
>>  extern int nfs_mount_data_version;
>>  extern char *progname;
>>  extern int verbose;
>> @@ -124,8 +131,8 @@ static void nfs_default_version(struct nfsmount_info *mi)
>>  	}
>>  
>>  #endif /* MOUNT_CONFIG */
>> -	mi->version.major = 4;
>> -	mi->version.minor = 2;
>> +	mi->version.major = NFS_DEFAULT_MAJOR;
>> +	mi->version.minor = NFS_DEFAULT_MINOR;
>>  }
>>  
>>  /*
>> @@ -316,8 +323,9 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>  		return 0;
>>  
>>  	if (strncmp(mi->type, "nfs4", 4) == 0) {
>> -		mi->version.major = 4;
>> -		mi->version.v_mode = V_GENERAL;
>> +		/* Set to default values */
>> +		mi->version.major = NFS_DEFAULT_MAJOR;
>> +		mi->version.minor = NFS_DEFAULT_MINOR;
> 
> Why do you remove the setting of v_mode to V_GENERAL?
> Won't that leave it as V_DEFAULT, and doesn't that mean
> that it will negotiate from v4 down to v3??
I was thinking v_mode was being set to V_GENERAL by
the previous call to nfs_nfs_version(), but is
not the cause for -t nfs4, so are correct.
The v_mode needs to be set to V_GENERAL in
this case... Thanks!

steved.

> 
> NeilBrown
> 
> 
>>  	}
>>  	/*
>>  	 * Before 2.6.32, the kernel NFS client didn't
>> -- 
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html