[PATCH 0/2] Two minor bug fixes

[PATCH 0/2] Two minor bug fixes

[Chuck Lever]

Hi Steve-

Please consider the following two patches for nfs-utils.

---

Chuck Lever (2):
      mount.nfs: Assume v2/v3 if mount-related options are present
      mount: ECONNREFUSED is a permanent error


 utils/mount/stropts.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

-- 
Chuck Lever <chuck.lever@oracle.com>

[PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Chuck Lever]

I had assumed early on that mount.nfs should retry a refused connection.

Apparently this is not the case.  Legacy mount.nfs4 fails immediately
if the NFS server refuses the connection.  Legacy mount.nfs and
text-based mount.nfs both fail immediately if the rpcbind service is
refusing connections.

So, banish ECONNREFUSED returns from mount(2) to the domain of
permanent errors.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/stropts.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 069bdc1..0685caa 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -639,7 +639,6 @@ static int nfs_is_permanent_error(int error)
 	switch (error) {
 	case ESTALE:
 	case ETIMEDOUT:
-	case ECONNREFUSED:
 		return 0;	/* temporary */
 	default:
 		return 1;	/* permanent */

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Steve Dickson]

On 10/08/2009 01:37 PM, Chuck Lever wrote:
> I had assumed early on that mount.nfs should retry a refused connection.
> 
> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
> if the NFS server refuses the connection.  Legacy mount.nfs and
> text-based mount.nfs both fail immediately if the rpcbind service is
> refusing connections.
> 
What about if the server is on the way up (i.e the network is up)
but has not started the NFS service? In that window, the server will 
return ECONNREFUSED since nobody is listening, but in a very short time
there will be a listener... The mount should not fail in that case...

steved.
 

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Chuck Lever]

On Oct 9, 2009, at 9:16 AM, Steve Dickson wrote:
> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>> I had assumed early on that mount.nfs should retry a refused  
>> connection.
>>
>> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
>> if the NFS server refuses the connection.  Legacy mount.nfs and
>> text-based mount.nfs both fail immediately if the rpcbind service is
>> refusing connections.
>>
> What about if the server is on the way up (i.e the network is up)
> but has not started the NFS service? In that window, the server will
> return ECONNREFUSED since nobody is listening, but in a very short  
> time
> there will be a listener... The mount should not fail in that case...

I agree, but I think it does fail today, and it has behaved this way  
for a long while.  No one has complained about it.  I'm actually not  
arguing in favor of either behavior; just reporting that the current  
behavior is inconsistent.

With the current code, legacy and text-based v2/v3 fails immediately  
if the server's rpcbind refuses connection... Legacy mount.nfs4 fails  
immediately if the NFS server refuses connection.  Text-based  
mount.nfs4 retries in this case.

So we will either need to fix v2/v3 to continue retrying, or fix NFSv4  
to stop retrying.  The retries would stop after mount.nfs's retry  
timer expires (just like the case where the server isn't responding at  
all).

Automounter might want different behavior in this case, but we should  
ask around before making a final decision, probably.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Steve Dickson]

On 10/09/2009 11:13 AM, Chuck Lever wrote:
> On Oct 9, 2009, at 9:16 AM, Steve Dickson wrote:
>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>> I had assumed early on that mount.nfs should retry a refused connection.
>>>
>>> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
>>> if the NFS server refuses the connection.  Legacy mount.nfs and
>>> text-based mount.nfs both fail immediately if the rpcbind service is
>>> refusing connections.
>>>
>> What about if the server is on the way up (i.e the network is up)
>> but has not started the NFS service? In that window, the server will
>> return ECONNREFUSED since nobody is listening, but in a very short time
>> there will be a listener... The mount should not fail in that case...
> 
> I agree, but I think it does fail today, and it has behaved this way for
> a long while.  No one has complained about it.  I'm actually not arguing
> in favor of either behavior; just reporting that the current behavior is
> inconsistent.
> 
> With the current code, legacy and text-based v2/v3 fails immediately if
> the server's rpcbind refuses connection... Legacy mount.nfs4 fails
> immediately if the NFS server refuses connection.  Text-based mount.nfs4
> retries in this case.
I think the text-based mounts have it right...

> 
> So we will either need to fix v2/v3 to continue retrying, or fix NFSv4
> to stop retrying.  The retries would stop after mount.nfs's retry timer
> expires (just like the case where the server isn't responding at all).
The former, IMHO.. I also notice that the retry timer does not work since
the mount waits in the kernel well passed the timer expiring... 
> 
> Automounter might want different behavior in this case, but we should
> ask around before making a final decision, probably.
Ian... What do you think??

steved.

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Chuck Lever]

On Oct 9, 2009, at 11:20 AM, Steve Dickson wrote:
> On 10/09/2009 11:13 AM, Chuck Lever wrote:
>> On Oct 9, 2009, at 9:16 AM, Steve Dickson wrote:
>>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>>> I had assumed early on that mount.nfs should retry a refused  
>>>> connection.
>>>>
>>>> Apparently this is not the case.  Legacy mount.nfs4 fails  
>>>> immediately
>>>> if the NFS server refuses the connection.  Legacy mount.nfs and
>>>> text-based mount.nfs both fail immediately if the rpcbind service  
>>>> is
>>>> refusing connections.
>>>>
>>> What about if the server is on the way up (i.e the network is up)
>>> but has not started the NFS service? In that window, the server will
>>> return ECONNREFUSED since nobody is listening, but in a very short  
>>> time
>>> there will be a listener... The mount should not fail in that  
>>> case...
>>
>> I agree, but I think it does fail today, and it has behaved this  
>> way for
>> a long while.  No one has complained about it.  I'm actually not  
>> arguing
>> in favor of either behavior; just reporting that the current  
>> behavior is
>> inconsistent.
>>
>> With the current code, legacy and text-based v2/v3 fails  
>> immediately if
>> the server's rpcbind refuses connection... Legacy mount.nfs4 fails
>> immediately if the NFS server refuses connection.  Text-based  
>> mount.nfs4
>> retries in this case.
> I think the text-based mounts have it right...

It's a change from legacy behavior, however, so we should test  
carefully.  The trade-off is that the mount.nfs command is less  
responsive because it's retrying a connection refusal, but it's more  
likely that the mount request will succeed.

Again, I'm not advocating for one or the other, just pointing out the  
compromises.

>> So we will either need to fix v2/v3 to continue retrying, or fix  
>> NFSv4
>> to stop retrying.  The retries would stop after mount.nfs's retry  
>> timer
>> expires (just like the case where the server isn't responding at  
>> all).
> The former, IMHO.. I also notice that the retry timer does not work  
> since
> the mount waits in the kernel well passed the timer expiring...

It does work, after a fashion, but yes, it's less responsive than it  
was before.  For background mounts it hardly matters because bg mounts  
retry for a good long while.  The case where it gets a little ugly is  
fg, when mount.nfs's retry timer is nearly always shorter than the  
kernel's connect retry timeout.

I've got some kernel level fixes for this... see the SOFTCONN patches  
from earlier this week.  Shortening the initial connect retry timeout  
in the kernel will also help the case where the server isn't  
responding at all.

>> Automounter might want different behavior in this case, but we should
>> ask around before making a final decision, probably.
> Ian... What do you think??

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Ian Kent]

Chuck Lever wrote:
> On Oct 9, 2009, at 11:20 AM, Steve Dickson wrote:
>> On 10/09/2009 11:13 AM, Chuck Lever wrote:
>>> On Oct 9, 2009, at 9:16 AM, Steve Dickson wrote:
>>>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>>>> I had assumed early on that mount.nfs should retry a refused
>>>>> connection.
>>>>>
>>>>> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
>>>>> if the NFS server refuses the connection.  Legacy mount.nfs and
>>>>> text-based mount.nfs both fail immediately if the rpcbind service is
>>>>> refusing connections.
>>>>>
>>>> What about if the server is on the way up (i.e the network is up)
>>>> but has not started the NFS service? In that window, the server will
>>>> return ECONNREFUSED since nobody is listening, but in a very short time
>>>> there will be a listener... The mount should not fail in that case...
>>>
>>> I agree, but I think it does fail today, and it has behaved this way for
>>> a long while.  No one has complained about it.  I'm actually not arguing
>>> in favor of either behavior; just reporting that the current behavior is
>>> inconsistent.
>>>
>>> With the current code, legacy and text-based v2/v3 fails immediately if
>>> the server's rpcbind refuses connection... Legacy mount.nfs4 fails
>>> immediately if the NFS server refuses connection.  Text-based mount.nfs4
>>> retries in this case.
>> I think the text-based mounts have it right...
> 
> It's a change from legacy behavior, however, so we should test
> carefully.  The trade-off is that the mount.nfs command is less
> responsive because it's retrying a connection refusal, but it's more
> likely that the mount request will succeed.
> 
> Again, I'm not advocating for one or the other, just pointing out the
> compromises.
> 
>>> So we will either need to fix v2/v3 to continue retrying, or fix NFSv4
>>> to stop retrying.  The retries would stop after mount.nfs's retry timer
>>> expires (just like the case where the server isn't responding at all).
>> The former, IMHO.. I also notice that the retry timer does not work since
>> the mount waits in the kernel well passed the timer expiring...
> 
> It does work, after a fashion, but yes, it's less responsive than it was
> before.  For background mounts it hardly matters because bg mounts retry
> for a good long while.  The case where it gets a little ugly is fg, when
> mount.nfs's retry timer is nearly always shorter than the kernel's
> connect retry timeout.
> 
> I've got some kernel level fixes for this... see the SOFTCONN patches
> from earlier this week.  Shortening the initial connect retry timeout in
> the kernel will also help the case where the server isn't responding at
> all.
> 
>>> Automounter might want different behavior in this case, but we should
>>> ask around before making a final decision, probably.
>> Ian... What do you think??

We've been here recently I think, a very similar discussion anyway.

I think the interactive mounts should wait for the reasons that Chuck
points out and Steve agrees with.

The later point sounds like RPC not letting go after a specified
timeout. I think that the any timeout that is given, user space or
kernel space, should be obeyed and whatever needs to be done to achieve
it should be done. That only leaves the decision of retries, which comes
back to interactive mounts should wait (I think even system startup can
be considered interactive in this case).

As for autofs, that's a different story.

Our recent, similar discussion, lead to the addition of an autofs option
to set a timeout to wait for a mount (considered to be interactive) to
complete which overrides the behaviour of mount. But this has two
problems. First, sending a TERM signal to the mount(8) process kills it
but leaves the mount.nfs(8) process to run to completion. While this
isn't to good it achieves what's required from the interactive user
perspective. And second, it looks like the task killable patches will
stop the mount.nfs(8) task from terminating on a TERM signal even if we
were to locate the process and send the signal to it.

Mmmm ... bit off topic I guess.
Ian

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Steve Dickson]

On 10/08/2009 01:37 PM, Chuck Lever wrote:
> I had assumed early on that mount.nfs should retry a refused connection.
> 
> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
> if the NFS server refuses the connection.  Legacy mount.nfs and
> text-based mount.nfs both fail immediately if the rpcbind service is
> refusing connections.
> 
> So, banish ECONNREFUSED returns from mount(2) to the domain of
> permanent errors.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 069bdc1..0685caa 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -639,7 +639,6 @@ static int nfs_is_permanent_error(int error)
>  	switch (error) {
>  	case ESTALE:
>  	case ETIMEDOUT:
> -	case ECONNREFUSED:
>  		return 0;	/* temporary */
>  	default:
>  		return 1;	/* permanent */
> 
I'm going to hold of on this one... I just think we should 
continue to try on ECONNREFUSED errors until the retry timer pops...

steved.

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Chuck Lever]

On Nov 16, 2009, at 1:13 PM, Steve Dickson wrote:

>
>
> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>> I had assumed early on that mount.nfs should retry a refused  
>> connection.
>>
>> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
>> if the NFS server refuses the connection.  Legacy mount.nfs and
>> text-based mount.nfs both fail immediately if the rpcbind service is
>> refusing connections.
>>
>> So, banish ECONNREFUSED returns from mount(2) to the domain of
>> permanent errors.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> utils/mount/stropts.c |    1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 069bdc1..0685caa 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -639,7 +639,6 @@ static int nfs_is_permanent_error(int error)
>> 	switch (error) {
>> 	case ESTALE:
>> 	case ETIMEDOUT:
>> -	case ECONNREFUSED:
>> 		return 0;	/* temporary */
>> 	default:
>> 		return 1;	/* permanent */
>>
> I'm going to hold of on this one... I just think we should
> continue to try on ECONNREFUSED errors until the retry timer pops...

I thought we had agreed that ECONNREFUSED should be retried (ie this  
particular patch was vetoed a while ago).  Currently it is retried for  
NFSv4, but not for NFSv2/3, and I have a patch that makes v2/v3 retry.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/2] mount: ECONNREFUSED is a permanent error

[Steve Dickson]

On 11/16/2009 01:16 PM, Chuck Lever wrote:
> 
> On Nov 16, 2009, at 1:13 PM, Steve Dickson wrote:
> 
>>
>>
>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>> I had assumed early on that mount.nfs should retry a refused connection.
>>>
>>> Apparently this is not the case.  Legacy mount.nfs4 fails immediately
>>> if the NFS server refuses the connection.  Legacy mount.nfs and
>>> text-based mount.nfs both fail immediately if the rpcbind service is
>>> refusing connections.
>>>
>>> So, banish ECONNREFUSED returns from mount(2) to the domain of
>>> permanent errors.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> utils/mount/stropts.c |    1 -
>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 069bdc1..0685caa 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -639,7 +639,6 @@ static int nfs_is_permanent_error(int error)
>>>     switch (error) {
>>>     case ESTALE:
>>>     case ETIMEDOUT:
>>> -    case ECONNREFUSED:
>>>         return 0;    /* temporary */
>>>     default:
>>>         return 1;    /* permanent */
>>>
>> I'm going to hold of on this one... I just think we should
>> continue to try on ECONNREFUSED errors until the retry timer pops...
> 
> I thought we had agreed that ECONNREFUSED should be retried (ie this
> particular patch was vetoed a while ago).  
Right... Just clearing my que... ;-)

> Currently it is retried for NFSv4, but not for NFSv2/3, and I have 
> a patch that makes v2/v3 retry.
Post it when its ready...

steved.

[PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Chuck Lever]

Don't try NFSv4 if any MNT protocol related options were presented by
the user.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/stropts.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0685caa..3401f63 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
 	}
 
 	if (mi->version == 0) {
+		if (po_contains(options, "mounthost") ||
+		    po_contains(options, "mountaddr") ||
+		    po_contains(options, "mountvers") ||
+		    po_contains(options, "mountproto")) {
+			errno = EPROTONOSUPPORT;
+			goto out_fail;
+		}
 		if (po_append(options, "vers=4") == PO_FAILED) {
 			errno = EINVAL;
 			goto out_fail;

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Trond Myklebust]

On Thu, 2009-10-08 at 13:37 -0400, Chuck Lever wrote:
> Don't try NFSv4 if any MNT protocol related options were presented by
> the user.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0685caa..3401f63 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
>  	}
>  
>  	if (mi->version == 0) {
> +		if (po_contains(options, "mounthost") ||
> +		    po_contains(options, "mountaddr") ||
> +		    po_contains(options, "mountvers") ||
> +		    po_contains(options, "mountproto")) {
> +			errno = EPROTONOSUPPORT;

Shouldn't this be EINVAL ?

> +			goto out_fail;
> +		}
>  		if (po_append(options, "vers=4") == PO_FAILED) {
>  			errno = EINVAL;
>  			goto out_fail;
> 
> --
> 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] mount.nfs: Assume v2/v3 if mount-related options are present

[Chuck Lever]

On Oct 8, 2009, at 1:45 PM, Trond Myklebust wrote:

> On Thu, 2009-10-08 at 13:37 -0400, Chuck Lever wrote:
>> Don't try NFSv4 if any MNT protocol related options were presented by
>> the user.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> utils/mount/stropts.c |    7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 0685caa..3401f63 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct  
>> nfsmount_info *mi)
>> 	}
>>
>> 	if (mi->version == 0) {
>> +		if (po_contains(options, "mounthost") ||
>> +		    po_contains(options, "mountaddr") ||
>> +		    po_contains(options, "mountvers") ||
>> +		    po_contains(options, "mountproto")) {
>> +			errno = EPROTONOSUPPORT;
>
> Shouldn't this be EINVAL ?

Since this is behind the mi->version == 0 check, we know that the user  
didn't specify an NFS version.  In any other v4 case, the kernel's  
mount option parser will kick these out with EINVAL.  But here, we  
just want to avoid trying to negotiate NFSv4.  So the EPROTONOSUPPORT  
return code will cause the logic in nfs_try_mount() to try v3/v2 next.

Basically the bug is this:  before, if I didn't specify an NFS  
version, but added some mountfoo= option, the mount.nfs command will  
try negotiating NFSv2/v3.  Now, it will try NFSv4 first, but these are  
illegal options for NFSv4, and the mount command will fail.  So,  
regression.  This new logic simply skips trying NFSv4 in this case.

>
>> +			goto out_fail;
>> +		}
>> 		if (po_append(options, "vers=4") == PO_FAILED) {
>> 			errno = EINVAL;
>> 			goto out_fail;
>>
>> --
>> 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[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Steve Dickson]

On 10/08/2009 01:37 PM, Chuck Lever wrote:
> Don't try NFSv4 if any MNT protocol related options were presented by
> the user.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0685caa..3401f63 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
>  	}
>  
>  	if (mi->version == 0) {
> +		if (po_contains(options, "mounthost") ||
> +		    po_contains(options, "mountaddr") ||
> +		    po_contains(options, "mountvers") ||
> +		    po_contains(options, "mountproto")) {
> +			errno = EPROTONOSUPPORT;
> +			goto out_fail;
> +		}
I think it make senses to assume the version negation should 
start version 3 when  mountXXXX options exist instead of 
failing a mount they probably didn't want.. 

steved.
 

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Chuck Lever]

On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:

>
>
> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>> Don't try NFSv4 if any MNT protocol related options were presented by
>> the user.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> utils/mount/stropts.c |    7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 0685caa..3401f63 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct  
>> nfsmount_info *mi)
>> 	}
>>
>> 	if (mi->version == 0) {
>> +		if (po_contains(options, "mounthost") ||
>> +		    po_contains(options, "mountaddr") ||
>> +		    po_contains(options, "mountvers") ||
>> +		    po_contains(options, "mountproto")) {
>> +			errno = EPROTONOSUPPORT;
>> +			goto out_fail;
>> +		}
> I think it make senses to assume the version negation should
> start version 3 when  mountXXXX options exist instead of
> failing a mount they probably didn't want..

Yes, that's exactly what this patch does.  NFSv4 negotiation is  
skipped if any mountfoo options are presented by the user.

It's arguable where to put this check.  This seemed like the most  
straightforward way to deal with it.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Steve Dickson]

On 10/09/2009 11:03 AM, Chuck Lever wrote:
> 
> On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:
> 
>>
>>
>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>> Don't try NFSv4 if any MNT protocol related options were presented by
>>> the user.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> utils/mount/stropts.c |    7 +++++++
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 0685caa..3401f63 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info
>>> *mi)
>>>     }
>>>
>>>     if (mi->version == 0) {
>>> +        if (po_contains(options, "mounthost") ||
>>> +            po_contains(options, "mountaddr") ||
>>> +            po_contains(options, "mountvers") ||
>>> +            po_contains(options, "mountproto")) {
>>> +            errno = EPROTONOSUPPORT;
>>> +            goto out_fail;
>>> +        }
>> I think it make senses to assume the version negation should
>> start version 3 when  mountXXXX options exist instead of
>> failing a mount they probably didn't want..
> 
> Yes, that's exactly what this patch does.  NFSv4 negotiation is skipped
> if any mountfoo options are presented by the user.
> 
> It's arguable where to put this check.  This seemed like the most
> straightforward way to deal with it.
I guess a comment would have made it a bit clear... and I was thinking
the check should be made before the nfs_try_mount_v4() verses having
nfs_try_mount_v4() fail in a recoverable way...

steved.

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Chuck Lever]

On Oct 9, 2009, at 11:12 AM, Steve Dickson wrote:

>
>
> On 10/09/2009 11:03 AM, Chuck Lever wrote:
>>
>> On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:
>>
>>>
>>>
>>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>>> Don't try NFSv4 if any MNT protocol related options were  
>>>> presented by
>>>> the user.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> utils/mount/stropts.c |    7 +++++++
>>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>> index 0685caa..3401f63 100644
>>>> --- a/utils/mount/stropts.c
>>>> +++ b/utils/mount/stropts.c
>>>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct  
>>>> nfsmount_info
>>>> *mi)
>>>>    }
>>>>
>>>>    if (mi->version == 0) {
>>>> +        if (po_contains(options, "mounthost") ||
>>>> +            po_contains(options, "mountaddr") ||
>>>> +            po_contains(options, "mountvers") ||
>>>> +            po_contains(options, "mountproto")) {
>>>> +            errno = EPROTONOSUPPORT;
>>>> +            goto out_fail;
>>>> +        }
>>> I think it make senses to assume the version negation should
>>> start version 3 when  mountXXXX options exist instead of
>>> failing a mount they probably didn't want..
>>
>> Yes, that's exactly what this patch does.  NFSv4 negotiation is  
>> skipped
>> if any mountfoo options are presented by the user.
>>
>> It's arguable where to put this check.  This seemed like the most
>> straightforward way to deal with it.
> I guess a comment would have made it a bit clear...

Yes, it does need a comment.  I thought that placing this inside the  
mi->version == 0 check would have been clear enough, but I guess  
not.  :-)

> and I was thinking
> the check should be made before the nfs_try_mount_v4() verses having
> nfs_try_mount_v4() fail in a recoverable way...

I'll cobble up a version that does this check in nfs_try_mount()  
instead, just to see what it looks like.

Alternately we can do this before starting the retry loop, but setting  
mi->version = 3 would be less clear, I think, then skipping the v4  
negotiation in the loop.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] mount.nfs: Assume v2/v3 if mount-related options are present

[Steve Dickson]

On 10/08/2009 01:37 PM, Chuck Lever wrote:
> Don't try NFSv4 if any MNT protocol related options were presented by
> the user.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0685caa..3401f63 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
>  	}
>  
>  	if (mi->version == 0) {
> +		if (po_contains(options, "mounthost") ||
> +		    po_contains(options, "mountaddr") ||
> +		    po_contains(options, "mountvers") ||
> +		    po_contains(options, "mountproto")) {
> +			errno = EPROTONOSUPPORT;
> +			goto out_fail;
> +		}
>  		if (po_append(options, "vers=4") == PO_FAILED) {
>  			errno = EINVAL;
>  			goto out_fail;
> 
Committed... with a comment...

steved.