Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

[Neil Brown]

On Tue, 31 Aug 2010 17:21:31 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> The logic that manages negotiating NFS version and protocol settings
> is getting more complex over time, so let's split this out of
> nfs_try_mount().
> 
> This should make Bruce a little happier, as it eliminates the silent
> switch case fall through in nfs_try_mount().  And it should help Neil
> fix some bugs he's found in this logic.

Hi Chuck,
 thanks for these..
 One question....

.....
> +{
> +	int result;
> +
> +	/*
> +	 * Before 2.6.32, the kernel NFS client didn't support
> +	 * "-t nfs vers=4" mounts, so NFS version 4 cannot be
> +	 * included when autonegotiating while running on
> +	 * those kernels.
> +	 */
> +	if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
> +		goto fall_back;
> +
> +	errno = 0;
> +	result = nfs_try_mount_v4(mi);
> +	switch (errno) {

Should we not have e.g.

   if (result)
         return result;

before the switch(errno)??  Typically errno is 'undefined' in no error is
reported.

I realise the current code doesn't have that test either, but I still think
it is wrong not to.

Thanks
NeilBrown



> +	case EPROTONOSUPPORT:
> +		/* A clear indication that the server or our
> +		 * client does not support NFS version 4. */
> +		goto fall_back;
> +	case ENOENT:
> +		/* Legacy Linux servers don't export an NFS
> +		 * version 4 pseudoroot. */
> +		goto fall_back;
> +	case EPERM:
> +		/* Linux servers prior to 2.6.25 may return
> +		 * EPERM when NFS version 4 is not supported. */
> +		goto fall_back;
> +	default:
> +		return result;
> +	}
> +
> +fall_back:
> +	return nfs_try_mount_v3v2(mi);
> +}
> +
> +/*
>   * This is a single pass through the fg/bg loop.
>   *
>   * Returns TRUE if successful, otherwise FALSE.
> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>  
>  	switch (mi->version) {
>  	case 0:
> -		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
> -			errno = 0;
> -			result = nfs_try_mount_v4(mi);
> -			if (errno != EPROTONOSUPPORT) {
> -				/* 
> -				 * To deal with legacy Linux servers that don't
> -				 * automatically export a pseudo root, retry
> -				 * ENOENT errors using version 3. And for
> -				 * Linux servers prior to 2.6.25, retry EPERM
> -				 */
> -				if (errno != ENOENT && errno != EPERM)
> -					break;
> -			}
> -		}
> +		result = nfs_autonegotiate(mi);
> +		break;
>  	case 2:
>  	case 3:
>  		result = nfs_try_mount_v3v2(mi);

Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

[Chuck Lever]

On Sep 6, 2010, at 9:12 PM, Neil Brown wrote:

> On Tue, 31 Aug 2010 17:21:31 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> The logic that manages negotiating NFS version and protocol settings
>> is getting more complex over time, so let's split this out of
>> nfs_try_mount().
>> 
>> This should make Bruce a little happier, as it eliminates the silent
>> switch case fall through in nfs_try_mount().  And it should help Neil
>> fix some bugs he's found in this logic.
> 
> Hi Chuck,
> thanks for these..
> One question....
> 
> .....
>> +{
>> +	int result;
>> +
>> +	/*
>> +	 * Before 2.6.32, the kernel NFS client didn't support
>> +	 * "-t nfs vers=4" mounts, so NFS version 4 cannot be
>> +	 * included when autonegotiating while running on
>> +	 * those kernels.
>> +	 */
>> +	if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
>> +		goto fall_back;
>> +
>> +	errno = 0;
>> +	result = nfs_try_mount_v4(mi);
>> +	switch (errno) {
> 
> Should we not have e.g.
> 
>   if (result)
>         return result;
> 
> before the switch(errno)??  Typically errno is 'undefined' in no error is
> reported.
> 
> I realise the current code doesn't have that test either, but I still think
> it is wrong not to.

We have

  errno = 0;

just before the nfs_try_mount_v4(mi); call.  This defines the value of errno even if nothing in nfs_try_mount_v4() sets it.

Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?

> Thanks
> NeilBrown
> 
> 
> 
>> +	case EPROTONOSUPPORT:
>> +		/* A clear indication that the server or our
>> +		 * client does not support NFS version 4. */
>> +		goto fall_back;
>> +	case ENOENT:
>> +		/* Legacy Linux servers don't export an NFS
>> +		 * version 4 pseudoroot. */
>> +		goto fall_back;
>> +	case EPERM:
>> +		/* Linux servers prior to 2.6.25 may return
>> +		 * EPERM when NFS version 4 is not supported. */
>> +		goto fall_back;
>> +	default:
>> +		return result;
>> +	}
>> +
>> +fall_back:
>> +	return nfs_try_mount_v3v2(mi);
>> +}
>> +
>> +/*
>>  * This is a single pass through the fg/bg loop.
>>  *
>>  * Returns TRUE if successful, otherwise FALSE.
>> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>> 
>> 	switch (mi->version) {
>> 	case 0:
>> -		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
>> -			errno = 0;
>> -			result = nfs_try_mount_v4(mi);
>> -			if (errno != EPROTONOSUPPORT) {
>> -				/* 
>> -				 * To deal with legacy Linux servers that don't
>> -				 * automatically export a pseudo root, retry
>> -				 * ENOENT errors using version 3. And for
>> -				 * Linux servers prior to 2.6.25, retry EPERM
>> -				 */
>> -				if (errno != ENOENT && errno != EPERM)
>> -					break;
>> -			}
>> -		}
>> +		result = nfs_autonegotiate(mi);
>> +		break;
>> 	case 2:
>> 	case 3:
>> 		result = nfs_try_mount_v3v2(mi);
> 

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

Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

[Neil Brown]

On Thu, 9 Sep 2010 13:27:38 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Sep 6, 2010, at 9:12 PM, Neil Brown wrote:
> 
> > On Tue, 31 Aug 2010 17:21:31 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> >> The logic that manages negotiating NFS version and protocol settings
> >> is getting more complex over time, so let's split this out of
> >> nfs_try_mount().
> >> 
> >> This should make Bruce a little happier, as it eliminates the silent
> >> switch case fall through in nfs_try_mount().  And it should help Neil
> >> fix some bugs he's found in this logic.
> > 
> > Hi Chuck,
> > thanks for these..
> > One question....
> > 
> > .....
> >> +{
> >> +	int result;
> >> +
> >> +	/*
> >> +	 * Before 2.6.32, the kernel NFS client didn't support
> >> +	 * "-t nfs vers=4" mounts, so NFS version 4 cannot be
> >> +	 * included when autonegotiating while running on
> >> +	 * those kernels.
> >> +	 */
> >> +	if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
> >> +		goto fall_back;
> >> +
> >> +	errno = 0;
> >> +	result = nfs_try_mount_v4(mi);
> >> +	switch (errno) {
> > 
> > Should we not have e.g.
> > 
> >   if (result)
> >         return result;
> > 
> > before the switch(errno)??  Typically errno is 'undefined' in no error is
> > reported.
> > 
> > I realise the current code doesn't have that test either, but I still think
> > it is wrong not to.
> 
> We have
> 
>   errno = 0;
> 
> just before the nfs_try_mount_v4(mi); call.  This defines the value of errno even if nothing in nfs_try_mount_v4() sets it.

True, but what if something in nfs_try_mount_v4 does set it, but success is
finally returned?  That is certainly possible if mi has multiple addresses
and the first is unreachable.

I think you must *never* test errno unless the most recent call reported an
error.

> 
> Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?

He's like a guardian angel sitting on you shoulder whispering "too subtle"
all the time :-)  Definitely true this time.

Thanks,
NeilBrown


> 
> > Thanks
> > NeilBrown
> > 
> > 
> > 
> >> +	case EPROTONOSUPPORT:
> >> +		/* A clear indication that the server or our
> >> +		 * client does not support NFS version 4. */
> >> +		goto fall_back;
> >> +	case ENOENT:
> >> +		/* Legacy Linux servers don't export an NFS
> >> +		 * version 4 pseudoroot. */
> >> +		goto fall_back;
> >> +	case EPERM:
> >> +		/* Linux servers prior to 2.6.25 may return
> >> +		 * EPERM when NFS version 4 is not supported. */
> >> +		goto fall_back;
> >> +	default:
> >> +		return result;
> >> +	}
> >> +
> >> +fall_back:
> >> +	return nfs_try_mount_v3v2(mi);
> >> +}
> >> +
> >> +/*
> >>  * This is a single pass through the fg/bg loop.
> >>  *
> >>  * Returns TRUE if successful, otherwise FALSE.
> >> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
> >> 
> >> 	switch (mi->version) {
> >> 	case 0:
> >> -		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
> >> -			errno = 0;
> >> -			result = nfs_try_mount_v4(mi);
> >> -			if (errno != EPROTONOSUPPORT) {
> >> -				/* 
> >> -				 * To deal with legacy Linux servers that don't
> >> -				 * automatically export a pseudo root, retry
> >> -				 * ENOENT errors using version 3. And for
> >> -				 * Linux servers prior to 2.6.25, retry EPERM
> >> -				 */
> >> -				if (errno != ENOENT && errno != EPERM)
> >> -					break;
> >> -			}
> >> -		}
> >> +		result = nfs_autonegotiate(mi);
> >> +		break;
> >> 	case 2:
> >> 	case 3:
> >> 		result = nfs_try_mount_v3v2(mi);
> > 
> 

Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

[Trond Myklebust]

On Fri, 2010-09-10 at 07:23 +1000, Neil Brown wrote:
> On Thu, 9 Sep 2010 13:27:38 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> > Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?
> 
> He's like a guardian angel sitting on you shoulder whispering "too subtle"
> all the time :-)  Definitely true this time.

The mind boggles at the thought of Bruce perching on anyone's
shoulder...

Cheers
  Trond

Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation

[J. Bruce Fields]

On Thu, Sep 09, 2010 at 05:45:34PM -0400, Trond Myklebust wrote:
> On Fri, 2010-09-10 at 07:23 +1000, Neil Brown wrote:
> > On Thu, 9 Sep 2010 13:27:38 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > > 
> > > Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?
> > 
> > He's like a guardian angel sitting on you shoulder whispering "too subtle"
> > all the time :-)  Definitely true this time.
> 
> The mind boggles at the thought of Bruce perching on anyone's
> shoulder...

Would somebody please just offer me a chair?

--b.