* Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation [not found] ` <20100831212130.21061.10519.stgit@matisse.1015granger.net> @ 2010-09-07 1:12 ` Neil Brown 2010-09-09 17:27 ` Chuck Lever 0 siblings, 1 reply; 5+ messages in thread From: Neil Brown @ 2010-09-07 1:12 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation 2010-09-07 1:12 ` [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation Neil Brown @ 2010-09-09 17:27 ` Chuck Lever 2010-09-09 21:23 ` Neil Brown 0 siblings, 1 reply; 5+ messages in thread From: Chuck Lever @ 2010-09-09 17:27 UTC (permalink / raw) To: Neil Brown; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation 2010-09-09 17:27 ` Chuck Lever @ 2010-09-09 21:23 ` Neil Brown 2010-09-09 21:45 ` Trond Myklebust 0 siblings, 1 reply; 5+ messages in thread From: Neil Brown @ 2010-09-09 21:23 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs 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); > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation 2010-09-09 21:23 ` Neil Brown @ 2010-09-09 21:45 ` Trond Myklebust [not found] ` <1284068734.14656.39.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2010-09-09 21:45 UTC (permalink / raw) To: Neil Brown; +Cc: Chuck Lever, linux-nfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <1284068734.14656.39.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation [not found] ` <1284068734.14656.39.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> @ 2010-09-11 4:06 ` J. Bruce Fields 0 siblings, 0 replies; 5+ messages in thread From: J. Bruce Fields @ 2010-09-11 4:06 UTC (permalink / raw) To: Trond Myklebust; +Cc: Neil Brown, Chuck Lever, linux-nfs 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-09-11 4:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20100831212006.21061.17002.stgit@matisse.1015granger.net> [not found] ` <20100831212130.21061.10519.stgit@matisse.1015granger.net> 2010-09-07 1:12 ` [PATCH 1/3] mount.nfs: Refactor mount version and protocol autonegotiation Neil Brown 2010-09-09 17:27 ` Chuck Lever 2010-09-09 21:23 ` Neil Brown 2010-09-09 21:45 ` Trond Myklebust [not found] ` <1284068734.14656.39.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 2010-09-11 4:06 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.