[PATCH 0/4] Handle "proto=rdma" regressions

[PATCH 0/4] Handle "proto=rdma" regressions

[Chuck Lever]

Steve-

This series is entirely untested, and is meant to be only an example
of how we could handle "proto=rdma" and "rdma" in mount.nfs.

The last patch in the series may not work at all.  It depends on the
kernel returning EPROTONOSUPPORT for "vers=4,rdma".

---

Chuck Lever (4):
      mount.nfs: Prepare way for "vers=4,rdma" mounts
      mount.nfs: Support an "rdma" mount option
      mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma
      getport: Recognize "rdma" and "rdma6" netid


 support/include/nfsrpc.h |    6 ++++++
 support/nfs/getport.c    |   25 +++++++++++++++++++++++++
 utils/mount/network.c    |    9 +++++++--
 utils/mount/nfs.man      |   11 ++++++++++-
 utils/mount/stropts.c    |   16 ++++++++--------
 5 files changed, 56 insertions(+), 11 deletions(-)

-- 
Chuck Lever

[PATCH 1/4] getport: Recognize "rdma" and "rdma6" netid

[Chuck Lever]

The mount.nfs command must recognize the values of "rdma" and "rdma6"
with the "proto=" mount option.  Typically the mount.nfs command
relies on libtirpc or getprotobyname(3) to recognize netids and
translate them to protocol numbers.

RFCs 5665 and 5666 define the "rdma" and "rdma6" netids.  IANA defines
a specific port number for NFS over RDMA (20049), but has not provided
a protocol name and number for RDMA transports, and is not expected
to.  The best we can do is translate these by hand, as needed, to get
RDMA mount requests to the kernel without erroring out.

Only the forward translation is needed until such time that "rdma" and
"rdma6" start to appear in rpcbind registries.  For now, the version
and transport negotiation logic is skipped, avoiding rpcbind queries
for RDMA mounts.

Note: As of kernel 2.6.36, the kernel's NFS over RDMA transport
capability does not support IPv6.

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

 support/include/nfsrpc.h |    6 ++++++
 support/nfs/getport.c    |   25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/support/include/nfsrpc.h b/support/include/nfsrpc.h
index 6ebefca..d50fe94 100644
--- a/support/include/nfsrpc.h
+++ b/support/include/nfsrpc.h
@@ -27,6 +27,12 @@
 #include <rpc/clnt.h>
 
 /*
+ * IANA does not define an IP protocol number for RDMA transports.
+ * Choose an arbitrary value we can use locally.
+ */
+#define NFSPROTO_RDMA		(3939)
+
+/*
  * Conventional RPC program numbers
  */
 #ifndef RPCBPROG
diff --git a/support/nfs/getport.c b/support/nfs/getport.c
index c930539..d74400b 100644
--- a/support/nfs/getport.c
+++ b/support/nfs/getport.c
@@ -216,6 +216,21 @@ nfs_get_proto(const char *netid, sa_family_t *family, unsigned long *protocol)
 	struct netconfig *nconf;
 	struct protoent *proto;
 
+	/*
+	 * IANA does not define a protocol number for rdma netids,
+	 * since "rdma" is not an IP protocol.
+	 */
+	if (strcmp(netid, "rdma") == 0) {
+		*family = AF_INET;
+		*protocol = NFSPROTO_RDMA;
+		return 1;
+	}
+	if (strcmp(netid, "rdma6") == 0) {
+		*family = AF_INET6;
+		*protocol = NFSPROTO_RDMA;
+		return 1;
+	}
+
 	nconf = getnetconfigent(netid);
 	if (nconf == NULL)
 		return 0;
@@ -242,6 +257,16 @@ nfs_get_proto(const char *netid, sa_family_t *family, unsigned long *protocol)
 {
 	struct protoent *proto;
 
+	/*
+	 * IANA does not define a protocol number for rdma netids,
+	 * since "rdma" is not an IP protocol.
+	 */
+	if (strcmp(netid, "rdma") == 0) {
+		*family = AF_INET;
+		*protocol = NFSPROTO_RDMA;
+		return 1;
+	}
+
 	proto = getprotobyname(netid);
 	if (proto == NULL)
 		return 0;

[PATCH 2/4] mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma

[Chuck Lever]

Clean up: Now that nfs_get_proto() can recognize "rdma" we can re-use
nfs_nfs_protocol() instead of ad hoc checks for "proto=rdma".

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

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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0241400..9695c73 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -303,8 +303,10 @@ static int nfs_set_version(struct nfsmount_info *mi)
 	if (strncmp(mi->type, "nfs4", 4) == 0)
 		mi->version = 4;
 	else {
-		char *option = po_get(mi->options, "proto");
-		if (option && strcmp(option, "rdma") == 0)
+		unsigned long protocol;
+		if (!nfs_nfs_protocol(mi->options, &protocol))
+			return 0;
+		if (protocol == NFSPROTO_RDMA)
 			mi->version = 3;
 	}
 
@@ -490,14 +492,19 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options)
 	union nfs_sockaddr mnt_address;
 	struct sockaddr *mnt_saddr = &mnt_address.sa;
 	socklen_t mnt_salen = sizeof(mnt_address);
+	unsigned long protocol;
 	struct pmap mnt_pmap;
 	char *option;
 
 	/*
-	 * Skip option negotiation for proto=rdma mounts.
+	 * Version and transport negotiation is not required
+	 * and does not work for RDMA mounts.
 	 */
-	option = po_get(options, "proto");
-	if (option && strcmp(option, "rdma") == 0)
+	if (!nfs_nfs_protocol(options, &protocol)) {
+		errno = EINVAL;
+		return 0;
+	}
+	if (protocol == NFSPROTO_RDMA)
 		goto out;
 
 	/*

[PATCH 3/4] mount.nfs: Support an "rdma" mount option

[Chuck Lever]

The kernel NFS client's mount option parser recognizes a stand-alone
"rdma" mount option, similar to the legacy "udp" and "tcp" options.

The mount.nfs command text-based mount option parser used to pass
"rdma" straight to the kernel, but since we've started handling MNT in
the kernel instead of in user space, "rdma" on the command line has
not worked.

Until now, no-one has noticed, especially since an "rdma" mount option
isn't documented in nfs(5).

Support "rdma" in mount.nfs command, and document it in nfs(5).

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

 utils/mount/network.c |    9 +++++++--
 utils/mount/nfs.man   |   11 ++++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index d6b5205..d612427 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -83,6 +83,7 @@ static const char *nfs_nfs_pgmtbl[] = {
 static const char *nfs_transport_opttbl[] = {
 	"udp",
 	"tcp",
+	"rdma",
 	"proto",
 	NULL,
 };
@@ -1307,7 +1308,10 @@ nfs_nfs_protocol(struct mount_options *options, unsigned long *protocol)
 	case 1: /* tcp */
 		*protocol = IPPROTO_TCP;
 		return 1;
-	case 2: /* proto */
+	case 2: /* rdma */
+		*protocol = NFSPROTO_RDMA;
+		return 1;
+	case 3: /* proto */
 		option = po_get(options, "proto");
 		if (option != NULL) {
 			if (!nfs_get_proto(option, &family, protocol)) {
@@ -1396,10 +1400,11 @@ int nfs_nfs_proto_family(struct mount_options *options,
 	switch (po_rightmost(options, nfs_transport_opttbl)) {
 	case 0:	/* udp */
 	case 1: /* tcp */
+	case 2: /* rdma */
 		/* for compatibility; these are always AF_INET */
 		*family = AF_INET;
 		return 1;
-	case 2: /* proto */
+	case 3: /* proto */
 		option = po_get(options, "proto");
 		if (option != NULL &&
 		    !nfs_get_proto(option, &tmp_family, &protocol)) {
diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
index 3806635..10ac961 100644
--- a/utils/mount/nfs.man
+++ b/utils/mount/nfs.man
@@ -495,7 +495,10 @@ command,
 .I netid
 is a valid netid listed in
 .IR /etc/netconfig .
-Otherwise,
+The value "rdma" may also be specified.
+If the
+.B mount.nfs
+command does not have TI-RPC support, then
 .I netid
 is one of "tcp," "udp," or "rdma," and only IPv4 may be used.
 .IP
@@ -537,6 +540,12 @@ option is an alternative to specifying
 .BR proto=tcp.
 It is included for compatibility with other operating systems.
 .TP 1.5i
+.B rdma
+The
+.B rdma
+option is an alternative to specifying
+.BR proto=rdma.
+.TP 1.5i
 .BI port= n
 The numeric value of the server's NFS service port.
 If the server's NFS service is not available on the specified port,

[PATCH 4/4] mount.nfs: Prepare way for "vers=4,rdma" mounts

[Chuck Lever]

At some point, when the kernel starts to support "vers=4,rdma" mounts,
we will want the mount.nfs command to pass "vers=4,rdma" mounts
instead of rejecting them.

Assuming that the kernel will reject these today with EPROTONOSUPPORT,
that would cause the version fallback logic to go to "vers=3,rdma"
automatically.  So the extra check we have now is not needed anyway.

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

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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 9695c73..a8b22ce 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -302,13 +302,6 @@ static int nfs_set_version(struct nfsmount_info *mi)
 
 	if (strncmp(mi->type, "nfs4", 4) == 0)
 		mi->version = 4;
-	else {
-		unsigned long protocol;
-		if (!nfs_nfs_protocol(mi->options, &protocol))
-			return 0;
-		if (protocol == NFSPROTO_RDMA)
-			mi->version = 3;
-	}
 
 	/*
 	 * If we still don't know, check for version-specific

Re: [PATCH 4/4] mount.nfs: Prepare way for "vers=4,rdma" mounts

[Steve Dickson]

On 09/07/2010 12:26 PM, Chuck Lever wrote:
> At some point, when the kernel starts to support "vers=4,rdma" mounts,
> we will want the mount.nfs command to pass "vers=4,rdma" mounts
> instead of rejecting them.
> 
> Assuming that the kernel will reject these today with EPROTONOSUPPORT,
> that would cause the version fallback logic to go to "vers=3,rdma"
> automatically.  So the extra check we have now is not needed anyway.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 9695c73..a8b22ce 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -302,13 +302,6 @@ static int nfs_set_version(struct nfsmount_info *mi)
>  
>  	if (strncmp(mi->type, "nfs4", 4) == 0)
>  		mi->version = 4;
> -	else {
> -		unsigned long protocol;
> -		if (!nfs_nfs_protocol(mi->options, &protocol))
> -			return 0;
> -		if (protocol == NFSPROTO_RDMA)
> -			mi->version = 3;
> -	}
>  
>  	/*
>  	 * If we still don't know, check for version-specific
> 
This is a show stopper... We can default to v4 because the callbacks.

steved.
 

Re: [PATCH 4/4] mount.nfs: Prepare way for "vers=4,rdma" mounts

[Chuck Lever]

On Sep 7, 2010, at 4:07 PM, Steve Dickson wrote:

> 
> 
> On 09/07/2010 12:26 PM, Chuck Lever wrote:
>> At some point, when the kernel starts to support "vers=4,rdma" mounts,
>> we will want the mount.nfs command to pass "vers=4,rdma" mounts
>> instead of rejecting them.
>> 
>> Assuming that the kernel will reject these today with EPROTONOSUPPORT,
>> that would cause the version fallback logic to go to "vers=3,rdma"
>> automatically.  So the extra check we have now is not needed anyway.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> utils/mount/stropts.c |    7 -------
>> 1 files changed, 0 insertions(+), 7 deletions(-)
>> 
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 9695c73..a8b22ce 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -302,13 +302,6 @@ static int nfs_set_version(struct nfsmount_info *mi)
>> 
>> 	if (strncmp(mi->type, "nfs4", 4) == 0)
>> 		mi->version = 4;
>> -	else {
>> -		unsigned long protocol;
>> -		if (!nfs_nfs_protocol(mi->options, &protocol))
>> -			return 0;
>> -		if (protocol == NFSPROTO_RDMA)
>> -			mi->version = 3;
>> -	}
>> 
>> 	/*
>> 	 * If we still don't know, check for version-specific
>> 
> This is a show stopper... We can default to v4 because the callbacks.

Again, this patch was for comments only, it isn't required.  But I don't understand your comment.

We don't default to NFSv4 for RDMA today, because of the if clause I remove here.  If no version is specified, it defaults to vers=3 for RDMA mounts.

I suggest we make it default to v4 in this case.  What is the callback requirement?

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

Re: [PATCH 4/4] mount.nfs: Prepare way for "vers=4,rdma" mounts

[Steve Dickson]

On 09/07/2010 04:14 PM, Chuck Lever wrote:
> 
> On Sep 7, 2010, at 4:07 PM, Steve Dickson wrote:
> 
>>
>>
>> On 09/07/2010 12:26 PM, Chuck Lever wrote:
>>> At some point, when the kernel starts to support "vers=4,rdma" mounts,
>>> we will want the mount.nfs command to pass "vers=4,rdma" mounts
>>> instead of rejecting them.
>>>
>>> Assuming that the kernel will reject these today with EPROTONOSUPPORT,
>>> that would cause the version fallback logic to go to "vers=3,rdma"
>>> automatically.  So the extra check we have now is not needed anyway.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> utils/mount/stropts.c |    7 -------
>>> 1 files changed, 0 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 9695c73..a8b22ce 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -302,13 +302,6 @@ static int nfs_set_version(struct nfsmount_info *mi)
>>>
>>> 	if (strncmp(mi->type, "nfs4", 4) == 0)
>>> 		mi->version = 4;
>>> -	else {
>>> -		unsigned long protocol;
>>> -		if (!nfs_nfs_protocol(mi->options, &protocol))
>>> -			return 0;
>>> -		if (protocol == NFSPROTO_RDMA)
>>> -			mi->version = 3;
>>> -	}
>>>
>>> 	/*
>>> 	 * If we still don't know, check for version-specific
>>>
>> This is a show stopper... We can default to v4 because the callbacks.
> 
> Again, this patch was for comments only, it isn't required.  
I guess I missed that in your description.

> But I don't understand your comment.
We can not default to v4 because of the callbacks.

> 
> We don't default to NFSv4 for RDMA today, because of the if clause I remove here.  
> If no version is specified, it defaults to vers=3 for RDMA mounts.
no... this is not the case with the above patched applied... 
I do have a way to test... 

> 
> I suggest we make it default to v4 in this case.  What is the callback requirement?
Its not clear they work... 

steved.

Re: [PATCH 0/4] Handle "proto=rdma" regressions

[Steve Dickson]

On 09/07/2010 12:25 PM, Chuck Lever wrote:
> Steve-
> 
> This series is entirely untested, and is meant to be only an example
> of how we could handle "proto=rdma" and "rdma" in mount.nfs.
> 
> The last patch in the series may not work at all.  It depends on the
> kernel returning EPROTONOSUPPORT for "vers=4,rdma".
> 
> ---
> 
> Chuck Lever (4):
>       mount.nfs: Prepare way for "vers=4,rdma" mounts
>       mount.nfs: Support an "rdma" mount option
>       mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma
>       getport: Recognize "rdma" and "rdma6" netid
> 
> 
>  support/include/nfsrpc.h |    6 ++++++
>  support/nfs/getport.c    |   25 +++++++++++++++++++++++++
>  utils/mount/network.c    |    9 +++++++--
>  utils/mount/nfs.man      |   11 ++++++++++-
>  utils/mount/stropts.c    |   16 ++++++++--------
>  5 files changed, 56 insertions(+), 11 deletions(-)
> 
With all due respect... NACK.... Here is why...

1) RDMA is not a network protocol and as I found out this
   weekend it *never* be one, so I see absolutely no reason 
   to treat RDMA like something it will never be. So using 
   nfs_nfs_protocol() is incorrect. 
 
2) Instead of modifying a couple if statements you are
   now making it a two step process to handle RDMA mount,
   which is not simpler.

3) Due to potential problems with callback with v4 the default
   version needs to be v3. But I do feel allowing the version
   to be set on the command line should work....

One thing that has become very apparent to me is, RDMA
is not a network protocol and its not even a well defined 
netid. RDMA a special case (to use your words) so that is
they way we should treated.. 

I will merge your man page changes as well as the comments I 
promised and repost the simpler, one routine patch... 

steved.

Re: [PATCH 0/4] Handle "proto=rdma" regressions

[Chuck Lever]

On Sep 7, 2010, at 1:57 PM, Steve Dickson wrote:

> 
> 
> On 09/07/2010 12:25 PM, Chuck Lever wrote:
>> Steve-
>> 
>> This series is entirely untested, and is meant to be only an example
>> of how we could handle "proto=rdma" and "rdma" in mount.nfs.
>> 
>> The last patch in the series may not work at all.  It depends on the
>> kernel returning EPROTONOSUPPORT for "vers=4,rdma".
>> 
>> ---
>> 
>> Chuck Lever (4):
>>      mount.nfs: Prepare way for "vers=4,rdma" mounts
>>      mount.nfs: Support an "rdma" mount option
>>      mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma
>>      getport: Recognize "rdma" and "rdma6" netid
>> 
>> 
>> support/include/nfsrpc.h |    6 ++++++
>> support/nfs/getport.c    |   25 +++++++++++++++++++++++++
>> utils/mount/network.c    |    9 +++++++--
>> utils/mount/nfs.man      |   11 ++++++++++-
>> utils/mount/stropts.c    |   16 ++++++++--------
>> 5 files changed, 56 insertions(+), 11 deletions(-)
>> 
> With all due respect... NACK.... Here is why...
> 
> 1) RDMA is not a network protocol and as I found out this
>   weekend it *never* be one, so I see absolutely no reason 
>   to treat RDMA like something it will never be. So using 
>   nfs_nfs_protocol() is incorrect. 

I wrote this code, Steve.  That means I can say with authority that claiming this usage of nfs_nfs_protocol() is "incorrect" is entirely your opinion, and a mistaken one at that.

Look very closely at nfs_nfs_protcol().  It, like all the functions in that part of utils/mount/network.c, is nothing more than a parser.  It is designed to parse the "proto=" mount option.  We specify RDMA mounts with "proto=rdma".  Thus it is an existing mechanism that was purposely designed to parse this mount option and this value.

nfs_nfs_protocol() also parses the "tcp" and "udp" mount options.  We want to add support for a stand-alone "rdma" option, which is pretty much the same idea as the existing short-hand stand-alone options "tcp" and "udp" (which are not netids, by the way, they are protocol names).  Why would we not use the existing parsing function for parsing a new transport-related option?

> 2) Instead of modifying a couple if statements you are
>   now making it a two step process to handle RDMA mount,
>   which is not simpler.

I don't follow this at all.  What two step process?

My patches change the code to work exactly as I intended RDMA to work when I wrote this two years ago.  The patches do not add any new steps.  In fact, it does pretty much exactly what your patch does, but it avoids adding the rdma_enabled() function, and reuses code that is specifically designed for this purpose.

Look again very closely at nfs_nfs_protocol().  In my patch it does exactly the same work that rdma_enabled() does by making similar po_foo calls, but it also works for "tcp" "udp" and "proto=netid" in general.  Doesn't that seem to you like the right tool for the job?

> 3) Due to potential problems with callback with v4 the default
>   version needs to be v3. But I do feel allowing the version
>   to be set on the command line should work....

If a specific clientaddr= value is required for NFSv4 over RDMA mounts, that should be documented in nfs(5).  Or, if no clientaddr= should be specified, we should add code in the mount.nfs command to avoid appending clientaddr= for NFSv4 over RDMA mounts.

But this suggests that we haven't tested or documented NFSv4 over RDMA in a public manner.  Is that the case?

> One thing that has become very apparent to me is, RDMA
> is not a network protocol and its not even a well defined 
> netid. RDMA a special case (to use your words) so that is
> they way we should treated..

It is true that RDMA is not a network protocol.  I don't see why that has any bearing on why a separate option parser is needed for "proto=rdma".  The value of proto= is a netid string, and "rdma" is a valid netid string, whether it is completely defined to include a full definition of an underlying transport protocol or not.

If "rdma" has things in common with the other transports we already handle, what benefit is there from adding more logic for it, just because "it's a special case?" As far as I can tell, none.  Adding more logic just makes the code more complicated.  Folding "rdma" into the existing code paths designed for parsing netids and transport protocols therefore makes better long term sense.

If it helps, the existing special casing in stropts.c which you replaced was probably not optimal.  It should have used nfs_nfs_protocol() all along.  But I don't have RDMA here to test it, and no-one has complained about it, so it lays fallow.

For a year now I've been arguing these points around mount.nfs that should be obvious to everyone.  I'm really really tired of it.  I've offered more than once to help by taking upstream and RH mount.nfs bugs, but even though I know you are overworked you still insist on trying to fix these yourself.  You then ask for comments and review, and defensively reject comments and suggestions made by the code's original author, who really is the ultimate authority on this code.

So, go ahead and commit whatever you want.  Since last year, I don't want to argue any more.

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

Re: [PATCH 0/4] Handle "proto=rdma" regressions

[Steve Dickson]

On 09/07/2010 03:21 PM, Chuck Lever wrote:
> 
> On Sep 7, 2010, at 1:57 PM, Steve Dickson wrote:
> 
>>
>>
>> On 09/07/2010 12:25 PM, Chuck Lever wrote:
>>> Steve-
>>>
>>> This series is entirely untested, and is meant to be only an example
>>> of how we could handle "proto=rdma" and "rdma" in mount.nfs.
>>>
>>> The last patch in the series may not work at all.  It depends on the
>>> kernel returning EPROTONOSUPPORT for "vers=4,rdma".
>>>
>>> ---
>>>
>>> Chuck Lever (4):
>>>      mount.nfs: Prepare way for "vers=4,rdma" mounts
>>>      mount.nfs: Support an "rdma" mount option
>>>      mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma
>>>      getport: Recognize "rdma" and "rdma6" netid
>>>
>>>
>>> support/include/nfsrpc.h |    6 ++++++
>>> support/nfs/getport.c    |   25 +++++++++++++++++++++++++
>>> utils/mount/network.c    |    9 +++++++--
>>> utils/mount/nfs.man      |   11 ++++++++++-
>>> utils/mount/stropts.c    |   16 ++++++++--------
>>> 5 files changed, 56 insertions(+), 11 deletions(-)
>>>
>> With all due respect... NACK.... Here is why...
>>
>> 1) RDMA is not a network protocol and as I found out this
>>   weekend it *never* be one, so I see absolutely no reason 
>>   to treat RDMA like something it will never be. So using 
>>   nfs_nfs_protocol() is incorrect. 
> 
> I wrote this code, Steve.  That means I can say with authority that claiming 
> this usage of nfs_nfs_protocol() is "incorrect" is entirely your opinion, 
> and a mistaken one at that.
I have been mistaken before and will be again... I'm able to admit to that.

> 
> Look very closely at nfs_nfs_protcol().  It, like all the functions in that 
> part of utils/mount/network.c, is nothing more than a parser.  It is 
> designed to parse the "proto=" mount option.  We specify RDMA mounts with 
> "proto=rdma".  Thus it is an existing mechanism that was purposely designed 
> to parse this mount option and this value.
> 
> nfs_nfs_protocol() also parses the "tcp" and "udp" mount options.  We want to add support for a stand-alone "rdma" option, which is pretty much the same idea as the existing short-hand stand-alone options "tcp" and "udp" (which are not netids, by the way, they are protocol names).  Why would we not use the existing parsing function for parsing a new transport-related option?
> 
>> 2) Instead of modifying a couple if statements you are
>>   now making it a two step process to handle RDMA mount,
>>   which is not simpler.
> 
> I don't follow this at all.  What two step process?
This 
     if (nfs_nfs_proto())
then 
     if (proto == RDMA)

verses

if (nfs_rdma_option())

Two steps verse one...

> 
> My patches change the code to work exactly as I intended RDMA to work when 
> I wrote this two years ago.  The patches do not add any new steps.  
> In fact, it does pretty much exactly what your patch does, but it 
> avoids adding the rdma_enabled() function, and reuses code that is specifically designed for this purpose.
> 
> Look again very closely at nfs_nfs_protocol().  In my patch it does exactly the same work that rdma_enabled() does by making similar po_foo calls, but it also works for "tcp" "udp" and "proto=netid" in general.  Doesn't that seem to you like the right tool for the job?
> 
>> 3) Due to potential problems with callback with v4 the default
>>   version needs to be v3. But I do feel allowing the version
>>   to be set on the command line should work....
> 
> If a specific clientaddr= value is required for NFSv4 over RDMA mounts, that should be documented in nfs(5).  Or, if no clientaddr= should be specified, we should add code in the mount.nfs command to avoid appending clientaddr= for NFSv4 over RDMA mounts.
> 
> But this suggests that we haven't tested or documented NFSv4 over RDMA in a public manner.  Is that the case?
> 
>> One thing that has become very apparent to me is, RDMA
>> is not a network protocol and its not even a well defined 
>> netid. RDMA a special case (to use your words) so that is
>> they way we should treated..
> 
> It is true that RDMA is not a network protocol.  
> I don't see why that has any bearing on why a separate option parser is 
> needed for "proto=rdma".  The value of proto= is a netid string, and "rdma" 
> is a valid netid string, whether it is completely defined to include a 
> full definition of an underlying transport protocol or not.
> 
> If "rdma" has things in common with the other transports we already handle, 
> what benefit is there from adding more logic for it, just because "it's a 
> special case?" As far as I can tell, none.  Adding more logic just makes 
> the code more complicated.  Folding "rdma" into the existing code paths 
> designed for parsing netids and transport protocols therefore makes better 
> long term sense.
> 
> If it helps, the existing special casing in stropts.c which you replaced 
> was probably not optimal.  It should have used nfs_nfs_protocol() all along.  
> But I don't have RDMA here to test it, and no-one has complained about it, so 
> it lays fallow.
At the end of the day... both patches/ideas do the exact same thing, 
just differently... 

> 
> For a year now I've been arguing these points around mount.nfs that 
> should be obvious to everyone.  I'm really really tired of it.  
I am too... very tired of wasting time such trivial points...
 
Chuck I have the ultimate respect for your passion, intelligence
and global understand of our technology. But its becoming very
tiresome you not allowing anybody to make any changes in the mount
code.. 

> I've offered more than once to help by taking upstream and RH mount.nfs 
> bugs, but even though I know you are overworked you still insist on 
> trying to fix these yourself.  
Well thankfully there are people who think I'm doing a good job
and do think I have a clue as to what I am doing... 

> You then ask for comments and review, and defensively reject comments 
> and suggestions made by the code's original author, who really is the 
> ultimate authority on this code.
So since you wrote most of the code that make you the sole owner 
of the code and only you are allowed to changed it?? How does that
fit in the open source model? Isn't that more of a proprietary model?

> 
> So, go ahead and commit whatever you want.  Since last year, I don't 
> want to argue any more.
> 
Then stop... Arguments like this will stop when you become more open 
to other people's opinion and ideas when it comes to the mounting code.
You do not own the code... the community does... 

To move this forward, I *will* commit your patches as is, assuming
testing goes well... Not for technology reasons, since both patches
are technically equivalent, but to show I do not have to have 
everything exactly the way I want it... Please take note!     

steved.
 

Re: [PATCH 0/4] Handle "proto=rdma" regressions

[Steve Dickson]

On 09/07/2010 12:25 PM, Chuck Lever wrote:
> Steve-
> 
> This series is entirely untested, and is meant to be only an example
> of how we could handle "proto=rdma" and "rdma" in mount.nfs.
> 
> The last patch in the series may not work at all.  It depends on the
> kernel returning EPROTONOSUPPORT for "vers=4,rdma".
> 
> ---
> 
> Chuck Lever (4):
>       mount.nfs: Prepare way for "vers=4,rdma" mounts
>       mount.nfs: Support an "rdma" mount option
>       mount.nfs: Use nfs_nfs_protocol() for checking for proto=rdma
>       getport: Recognize "rdma" and "rdma6" netid
> 
> 
>  support/include/nfsrpc.h |    6 ++++++
>  support/nfs/getport.c    |   25 +++++++++++++++++++++++++
>  utils/mount/network.c    |    9 +++++++--
>  utils/mount/nfs.man      |   11 ++++++++++-
>  utils/mount/stropts.c    |   16 ++++++++--------
>  5 files changed, 56 insertions(+), 11 deletions(-)
> 
The testing when well... v4 is the default version on RDMA
mounts... 

Committed...

steved.