mount.nfs: Use default minor version when -t nfs4 is specified
diff mbox

Message ID 20170606194812.4248-1-steved@redhat.com
State New
Headers show

Commit Message

Steve Dickson June 6, 2017, 7:48 p.m. UTC
When the nfs4 filesystem specified, the default minor
version should be used not v4.0.

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

Comments

Chuck Lever June 6, 2017, 7:52 p.m. UTC | #1
> On Jun 6, 2017, at 3:48 PM, Steve Dickson <steved@redhat.com> wrote:
> 
> When the nfs4 filesystem specified, the default minor
> version should be used not v4.0.

Isn't the default specified in the mountnfs config file?


> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> utils/mount/stropts.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index c0266e5..57efb26 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
> 
> 	if (strncmp(mi->type, "nfs4", 4) == 0) {
> 		mi->version.major = 4;
> -		mi->version.v_mode = V_GENERAL;
> +		mi->version.minor = 2;
> +		mi->version.v_mode = V_SPECIFIC;
> 	}
> 	/*
> 	 * 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

--
Chuck Lever



--
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
Steve Dickson June 6, 2017, 9:02 p.m. UTC | #2
On 06/06/2017 03:52 PM, Chuck Lever wrote:
> 
>> On Jun 6, 2017, at 3:48 PM, Steve Dickson <steved@redhat.com> wrote:
>>
>> When the nfs4 filesystem specified, the default minor
>> version should be used not v4.0.
> 
> Isn't the default specified in the mountnfs config file?
It can be... But in general its not... 

So the question is when the nfs4 file system (aka -t nfs4)
what minor version does that mean? v4.0 which was the 
default in the past or the new default v4.2?

steved.
> 
> 
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>> utils/mount/stropts.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index c0266e5..57efb26 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>
>> 	if (strncmp(mi->type, "nfs4", 4) == 0) {
>> 		mi->version.major = 4;
>> -		mi->version.v_mode = V_GENERAL;
>> +		mi->version.minor = 2;
>> +		mi->version.v_mode = V_SPECIFIC;
>> 	}
>> 	/*
>> 	 * 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
> 
> --
> Chuck Lever
> 
> 
> 
--
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
Chuck Lever June 6, 2017, 9:10 p.m. UTC | #3
> On Jun 6, 2017, at 5:02 PM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> 
> 
> On 06/06/2017 03:52 PM, Chuck Lever wrote:
>> 
>>> On Jun 6, 2017, at 3:48 PM, Steve Dickson <steved@redhat.com> wrote:
>>> 
>>> When the nfs4 filesystem specified, the default minor
>>> version should be used not v4.0.
>> 
>> Isn't the default specified in the mountnfs config file?
> It can be... But in general its not... 
> 
> So the question is when the nfs4 file system (aka -t nfs4)
> what minor version does that mean? v4.0 which was the 
> default in the past or the new default v4.2?

IMO the admin (and the distributor) should be allowed to
decide which minorversion is the default for vers=4 or
"-t nfs4".

NFSv4.0 is a safe default for now, so I'm just arguing for
taking a little more time to make this change to minor
version 2 using the config file instead of hard coding it.

No strong opinion, I just think it makes better sense to
handle default minor version the way many other default
behaviors are configured.


> steved.
>> 
>> 
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>> utils/mount/stropts.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index c0266e5..57efb26 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>> 
>>> 	if (strncmp(mi->type, "nfs4", 4) == 0) {
>>> 		mi->version.major = 4;
>>> -		mi->version.v_mode = V_GENERAL;
>>> +		mi->version.minor = 2;
>>> +		mi->version.v_mode = V_SPECIFIC;
>>> 	}
>>> 	/*
>>> 	 * 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
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 

--
Chuck Lever



--
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
NeilBrown June 7, 2017, 2:48 a.m. UTC | #4
On Tue, Jun 06 2017, Steve Dickson wrote:

> When the nfs4 filesystem specified, the default minor
> version should be used not v4.0.
>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  utils/mount/stropts.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index c0266e5..57efb26 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
>  
>  	if (strncmp(mi->type, "nfs4", 4) == 0) {
>  		mi->version.major = 4;
> -		mi->version.v_mode = V_GENERAL;
> +		mi->version.minor = 2;
> +		mi->version.v_mode = V_SPECIFIC;

I think this is wrong.

By setting the mode to SPECIFIC, you are saying that if the server
doesn't support v4.2, then fail the mount.  That cannot be right.

Given that (currently) v_mode is not V_SPECIFIC, nfs_set_version()
will go on and call nfs_default_version(), which will use the default
value from the config file - just as Chuck suggests.

If there is no default in the config file .... nfs_default_version()
will do nothing.  So version.minor will probably remain at zero.
So setting
> +		mi->version.minor = 2;

(where 2 is the maximum supported version) is probably correct.
Setting
> +		mi->version.v_mode = V_SPECIFIC;
is unnecessary and wrong.

If there an easy way to find out the maximum minor version that the
kernel supports?  We should really default version.minor to that.
Once we get up to v4.20, it'll seem odd to try to mount 4.20, 4.19,
4.18,... until something succeeds....

Thanks,
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
Steve Dickson June 8, 2017, 3:36 p.m. UTC | #5
On 06/06/2017 10:48 PM, NeilBrown wrote:
> On Tue, Jun 06 2017, Steve Dickson wrote:
> 
>> When the nfs4 filesystem specified, the default minor
>> version should be used not v4.0.
>>
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  utils/mount/stropts.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index c0266e5..57efb26 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>  
>>  	if (strncmp(mi->type, "nfs4", 4) == 0) {
>>  		mi->version.major = 4;
>> -		mi->version.v_mode = V_GENERAL;
>> +		mi->version.minor = 2;
>> +		mi->version.v_mode = V_SPECIFIC;
> 
> I think this is wrong.
> 
> By setting the mode to SPECIFIC, you are saying that if the server
> doesn't support v4.2, then fail the mount.  That cannot be right.
Ok... I though since nfs4 was being specify it should be
V_SPECIFIC but I do see your point about negotiating  down.

It turns out setting  vers4.2=n in /etc/nfs.conf breaks 
all v4 mounts... So I think we have issues in that area
ATM... ;-)

> 
> Given that (currently) v_mode is not V_SPECIFIC, nfs_set_version()
> will go on and call nfs_default_version(), which will use the default
> value from the config file - just as Chuck suggests.
> 
> If there is no default in the config file .... nfs_default_version()
> will do nothing.  So version.minor will probably remain at zero.
> So setting
>> +		mi->version.minor = 2;
> 
> (where 2 is the maximum supported version) is probably correct.
> Setting
>> +		mi->version.v_mode = V_SPECIFIC;
> is unnecessary and wrong.
> 
> If there an easy way to find out the maximum minor version that the
> kernel supports?  We should really default version.minor to that.
> Once we get up to v4.20, it'll seem odd to try to mount 4.20, 4.19,
> 4.18,... until something succeeds....
Maybe it should be something like the server does under /proc/fs/nfsd/version

steved.

> 
> Thanks,
> 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
--
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
Steve Dickson June 8, 2017, 4:04 p.m. UTC | #6
On 06/08/2017 11:36 AM, Steve Dickson wrote:
> 
> 
> On 06/06/2017 10:48 PM, NeilBrown wrote:
>> On Tue, Jun 06 2017, Steve Dickson wrote:
>>
>>> When the nfs4 filesystem specified, the default minor
>>> version should be used not v4.0.
>>>
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>>  utils/mount/stropts.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index c0266e5..57efb26 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -317,7 +317,8 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>>  
>>>  	if (strncmp(mi->type, "nfs4", 4) == 0) {
>>>  		mi->version.major = 4;
>>> -		mi->version.v_mode = V_GENERAL;
>>> +		mi->version.minor = 2;
>>> +		mi->version.v_mode = V_SPECIFIC;
>>
>> I think this is wrong.
>>
>> By setting the mode to SPECIFIC, you are saying that if the server
>> doesn't support v4.2, then fail the mount.  That cannot be right.
> Ok... I though since nfs4 was being specify it should be
> V_SPECIFIC but I do see your point about negotiating  down.
> 
> It turns out setting  vers4.2=n in /etc/nfs.conf breaks 
> all v4 mounts... So I think we have issues in that area
> ATM... ;-)
> 
>>
>> Given that (currently) v_mode is not V_SPECIFIC, nfs_set_version()
>> will go on and call nfs_default_version(), which will use the default
>> value from the config file - just as Chuck suggests.
>>
>> If there is no default in the config file .... nfs_default_version()
>> will do nothing.  So version.minor will probably remain at zero.
>> So setting
>>> +		mi->version.minor = 2;
>>
>> (where 2 is the maximum supported version) is probably correct.
It is because in nfs_default_version() there is this chunk of 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;
        return;
    }
that ends up not setting the minor version at all which
is the reason the minor version is zero. 

steved.

>> Setting
>>> +		mi->version.v_mode = V_SPECIFIC;
>> is unnecessary and wrong.
>>
>> If there an easy way to find out the maximum minor version that the
>> kernel supports?  We should really default version.minor to that.
>> Once we get up to v4.20, it'll seem odd to try to mount 4.20, 4.19,
>> 4.18,... until something succeeds....
> Maybe it should be something like the server does under /proc/fs/nfsd/version
> 
> steved.
> 
>>
>> Thanks,
>> 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
> --
> 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
> 
--
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

Patch
diff mbox

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c0266e5..57efb26 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -317,7 +317,8 @@  static int nfs_set_version(struct nfsmount_info *mi)
 
 	if (strncmp(mi->type, "nfs4", 4) == 0) {
 		mi->version.major = 4;
-		mi->version.v_mode = V_GENERAL;
+		mi->version.minor = 2;
+		mi->version.v_mode = V_SPECIFIC;
 	}
 	/*
 	 * Before 2.6.32, the kernel NFS client didn't