[PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

This is the promised patch that adds mountproto=tcp to the string
mount options if needed.
We still get a 90second timeout, but at least it works rather than
saying "mount.nfs: internal error".

It seems to me that it would be best to avoid the first call to mount
altogether.  Simply always do a probe_both and then do a mount based
on the results of that.
Is there a good reason not to?

BTW, I tested this by running 
    iptables -A INPUT -p udp --dport 111 -j REJECT

on the NFS server.
Accessing an NFS server over an SSH tunnel would also be a good test
that people are using in real life (IRL as my son says...).

NeilBrown


>From cec4bf512cce4686ed5dd25a9bb489f9e521721d Mon Sep 17 00:00:00 2001
From: Neil Brown <neilb@suse.de>
Date: Tue, 15 Jul 2008 22:38:17 +1000

If an NFS server is only listening on TCP for portmap (as apparently
MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
logic in case the initial choice of version/etc doesn't work, but it
doesn't cope with mountd needing tcp.
So:
  Fix probe_port so that a TIMEDOUT error doesn't simply abort
    but probes with other protocols (e.g. tcp).
  Fix rewrite_mount_options to extract the mountproto option before
    doing a probe, then set mountproto  (and mount prot) based
    on the result.
  Enable retry if the mount call returns EIO, as that is what happens
    if the UDP requests to portmap get no response.

Signed-off-by: Neil Brown <neilb@suse.de>
---
 utils/mount/network.c |    6 ++++--
 utils/mount/stropts.c |   31 +++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index ab7f6d0..cde6b2c 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -408,11 +408,10 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
                                 }
 				if (clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
 					goto out_ok;
-				if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
-					goto out_bad;
 			}
 		}
 		if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
+		    rpc_createerr.cf_stat != RPC_TIMEDOUT &&
 		    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
 			goto out_bad;
 
@@ -421,6 +420,9 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
 				continue;
 			p_prot = protos;
 		}
+		if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
+			goto out_bad;
+
 		if (vers || !*++p_vers)
 			break;
 	}
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 967fd69..f06e313 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -386,6 +386,17 @@ static struct mount_options *rewrite_mount_options(char *str)
 	option = po_get(options, "mountvers");
 	if (option)
 		mnt_server.pmap.pm_vers = atoi(option);
+	option = po_get(options, "mountproto");
+	if (option) {
+		if (strcmp(option, "tcp") == 0) {
+			mnt_server.pmap.pm_prot = IPPROTO_TCP;
+			po_remove_all(options, "mountproto");
+		}
+		if (strcmp(option, "udp") == 0) {
+			mnt_server.pmap.pm_prot = IPPROTO_UDP;
+			po_remove_all(options, "mountproto");
+		}
+	}
 
 	option = po_get(options, "port");
 	if (option) {
@@ -454,6 +465,20 @@ static struct mount_options *rewrite_mount_options(char *str)
 
 	}
 
+	if (mnt_server.pmap.pm_prot == IPPROTO_TCP)
+		snprintf(new_option, sizeof(new_option) - 1,
+			 "mountproto=tcp");
+	else
+		snprintf(new_option, sizeof(new_option) - 1,
+			 "mountproto=udp");
+	if (po_append(options, new_option) == PO_FAILED)
+		goto err;
+
+	snprintf(new_option, sizeof(new_option) - 1,
+		 "mountport=%lu", mnt_server.pmap.pm_port);
+	if (po_append(options, new_option) == PO_FAILED)
+		goto err;
+
 	errno = 0;
 	return options;
 
@@ -560,9 +585,11 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
 
 	/*
 	 * The kernel returns EOPNOTSUPP if the RPC bind failed,
-	 * and EPROTONOSUPPORT if the version isn't supported.
+	 * and EPROTONOSUPPORT if the version isn't supported,
+	 * and EIO if the protocol doesn't work.
 	 */
-	if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT)
+	if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT &&
+	    errno != EIO)
 		return 0;
 
 	return nfs_retry_nfs23mount(mi);
-- 
1.5.6.2

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Tue, Jul 15, 2008 at 8:56 AM, Neil Brown <neilb@suse.de> wrote:
> This is the promised patch that adds mountproto=tcp to the string
> mount options if needed.
> We still get a 90second timeout, but at least it works rather than
> saying "mount.nfs: internal error".
>
> It seems to me that it would be best to avoid the first call to mount
> altogether.  Simply always do a probe_both and then do a mount based
> on the results of that.
> Is there a good reason not to?

If I understand the question correctly, I think it doesn't because in
the most common cases, this isn't necessary.  The mount options are
usually adequate, and most servers support all the necessary NFS
versions and transport protocols.  This saves ephemeral ports and uses
less network traffic.

> BTW, I tested this by running
>    iptables -A INPUT -p udp --dport 111 -j REJECT
>
> on the NFS server.
> Accessing an NFS server over an SSH tunnel would also be a good test
> that people are using in real life (IRL as my son says...).
>
> NeilBrown
>
>
> From cec4bf512cce4686ed5dd25a9bb489f9e521721d Mon Sep 17 00:00:00 2001
> From: Neil Brown <neilb@suse.de>
> Date: Tue, 15 Jul 2008 22:38:17 +1000
>
> If an NFS server is only listening on TCP for portmap (as apparently
> MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
> logic in case the initial choice of version/etc doesn't work, but it
> doesn't cope with mountd needing tcp.
> So:
>  Fix probe_port so that a TIMEDOUT error doesn't simply abort
>    but probes with other protocols (e.g. tcp).

That seems reasonable and will update the behavior for both legacy and
text-based mounts.

But should you teach connect_to() to specifically handle ECONNREFUSED
as well?  I don't see why there would be a long timeout in that case.

>  Fix rewrite_mount_options to extract the mountproto option before
>    doing a probe, then set mountproto  (and mount prot) based
>    on the result.

Yes, it should have been doing that anyway.  There's a patch queued up
in my IPv6 series that addresses this along with other issues, but
it's reasonable to fix it now.

>  Enable retry if the mount call returns EIO, as that is what happens
>    if the UDP requests to portmap get no response.

I would rather see this one addressed in the kernel.  I think the EIO
return is a kernel bug.  If the server doesn't support a particular
transport protocol for portmap, mountd, or NFS, the mount(2) system
call should always return EPROTONOSUPPORT.

>
> Signed-off-by: Neil Brown <neilb@suse.de>
> ---
>  utils/mount/network.c |    6 ++++--
>  utils/mount/stropts.c |   31 +++++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index ab7f6d0..cde6b2c 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -408,11 +408,10 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>                                 }
>                                if (clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
>                                        goto out_ok;
> -                               if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
> -                                       goto out_bad;
>                        }
>                }
>                if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
> +                   rpc_createerr.cf_stat != RPC_TIMEDOUT &&
>                    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
>                        goto out_bad;
>
> @@ -421,6 +420,9 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>                                continue;
>                        p_prot = protos;
>                }
> +               if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
> +                       goto out_bad;
> +
>                if (vers || !*++p_vers)
>                        break;
>        }
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 967fd69..f06e313 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -386,6 +386,17 @@ static struct mount_options *rewrite_mount_options(char *str)
>        option = po_get(options, "mountvers");
>        if (option)
>                mnt_server.pmap.pm_vers = atoi(option);
> +       option = po_get(options, "mountproto");
> +       if (option) {
> +               if (strcmp(option, "tcp") == 0) {
> +                       mnt_server.pmap.pm_prot = IPPROTO_TCP;
> +                       po_remove_all(options, "mountproto");
> +               }
> +               if (strcmp(option, "udp") == 0) {
> +                       mnt_server.pmap.pm_prot = IPPROTO_UDP;
> +                       po_remove_all(options, "mountproto");
> +               }
> +       }
>
>        option = po_get(options, "port");
>        if (option) {
> @@ -454,6 +465,20 @@ static struct mount_options *rewrite_mount_options(char *str)
>
>        }
>
> +       if (mnt_server.pmap.pm_prot == IPPROTO_TCP)
> +               snprintf(new_option, sizeof(new_option) - 1,
> +                        "mountproto=tcp");
> +       else
> +               snprintf(new_option, sizeof(new_option) - 1,
> +                        "mountproto=udp");
> +       if (po_append(options, new_option) == PO_FAILED)
> +               goto err;
> +
> +       snprintf(new_option, sizeof(new_option) - 1,
> +                "mountport=%lu", mnt_server.pmap.pm_port);
> +       if (po_append(options, new_option) == PO_FAILED)
> +               goto err;
> +
>        errno = 0;
>        return options;
>
> @@ -560,9 +585,11 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
>
>        /*
>         * The kernel returns EOPNOTSUPP if the RPC bind failed,
> -        * and EPROTONOSUPPORT if the version isn't supported.
> +        * and EPROTONOSUPPORT if the version isn't supported,
> +        * and EIO if the protocol doesn't work.
>         */
> -       if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT)
> +       if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT &&
> +           errno != EIO)
>                return 0;
>
>        return nfs_retry_nfs23mount(mi);
> --
> 1.5.6.2
>
> --
> 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
>



-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Steve Dickson]

Chuck Lever wrote:
> On Tue, Jul 15, 2008 at 8:56 AM, Neil Brown <neilb@suse.de> wrote:
>> on the NFS server.
>> Accessing an NFS server over an SSH tunnel would also be a good test
>> that people are using in real life (IRL as my son says...).
>>
>> NeilBrown
>>
>>
>> From cec4bf512cce4686ed5dd25a9bb489f9e521721d Mon Sep 17 00:00:00 2001
>> From: Neil Brown <neilb@suse.de>
>> Date: Tue, 15 Jul 2008 22:38:17 +1000
>>
>> If an NFS server is only listening on TCP for portmap (as apparently
>> MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
>> logic in case the initial choice of version/etc doesn't work, but it
>> doesn't cope with mountd needing tcp.
>> So:
>>  Fix probe_port so that a TIMEDOUT error doesn't simply abort
>>    but probes with other protocols (e.g. tcp).
> 
> That seems reasonable and will update the behavior for both legacy and
> text-based mounts.
I agree... 
> 
> But should you teach connect_to() to specifically handle ECONNREFUSED
> as well?  I don't see why there would be a long timeout in that case.
> 
>>  Fix rewrite_mount_options to extract the mountproto option before
>>    doing a probe, then set mountproto  (and mount prot) based
>>    on the result.
> 
> Yes, it should have been doing that anyway.  There's a patch queued up
> in my IPv6 series that addresses this along with other issues, but
> it's reasonable to fix it now.
I agree... 
> 
>>  Enable retry if the mount call returns EIO, as that is what happens
>>    if the UDP requests to portmap get no response.
> 
> I would rather see this one addressed in the kernel.  I think the EIO
> return is a kernel bug.  If the server doesn't support a particular
> transport protocol for portmap, mountd, or NFS, the mount(2) system
> call should always return EPROTONOSUPPORT.
Since EIO has so many meanings... I think it should stay as a fatal 
error as well...

steved.

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Wednesday July 16, SteveD@redhat.com wrote:
> >>  Enable retry if the mount call returns EIO, as that is what happens
> >>    if the UDP requests to portmap get no response.
> > 
> > I would rather see this one addressed in the kernel.  I think the EIO
> > return is a kernel bug.  If the server doesn't support a particular
> > transport protocol for portmap, mountd, or NFS, the mount(2) system
> > call should always return EPROTONOSUPPORT.
> Since EIO has so many meanings... I think it should stay as a fatal 
> error as well...
> 

That would be best.  But the reality is that there are kernels in the
wild where -EIO indicates and error that isn't really fatal.

We already do kernel-detection in mount.  How about extending it like
this?

Thanks,
NeilBrown

From: Neil Brown <neilb@suse.de>
Date: Thu, 17 Jul 2008 12:23:03 +1000
Subject: [PATCH] mount: Recognised EIO from early kernels as possibly indicating a protocol error.

In kernels up to and including 2.6.26, mount reports "connection
refused" type messages as EIO rather than EPROTONOSUPPORT.
So detect those kernels and don't treat EIO as so fatal.

Signed-off-by: Neil Brown <neilb@suse.de>
---
 utils/mount/mount.c   |    8 +++++++-
 utils/mount/stropts.c |    7 +++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 06e2804..2163e4e 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -174,9 +174,15 @@ static void discover_nfs_mount_data_version(void)
 	}
 	if (nfs_mount_data_version > NFS_MOUNT_VERSION)
 		nfs_mount_data_version = NFS_MOUNT_VERSION;
-	else
+	else {
 		if (kernel_version > MAKE_VERSION(2, 6, 22))
 			string++;
+		/* up to and including 2.6.26, mount might return EIO
+		 * when it means EPROTONOSUPPORT
+		 */
+		if (kernel_version > MAKE_VERSION(2, 6, 26))
+			string++;
+	}
 }
 
 static void print_one(char *spec, char *node, char *type, char *opts)
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 09fca86..df4d9c1 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -552,9 +552,12 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
 
 	/*
 	 * The kernel returns EOPNOTSUPP if the RPC bind failed,
-	 * and EPROTONOSUPPORT if the version isn't supported.
+	 * and EPROTONOSUPPORT if the version isn't supported,
+	 * or the protocol isn't accepted.
+	 * Up to 2.6.26, the later causes EIO.
 	 */
-	if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT)
+	if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT &&
+	    (string >= 2 || errno != EIO))
 		return 0;
 
 	return nfs_retry_nfs23mount(mi);
-- 
1.5.6.2

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Steve Dickson]

Neil Brown wrote:
> On Wednesday July 16, SteveD@redhat.com wrote:
>>>>  Enable retry if the mount call returns EIO, as that is what happens
>>>>    if the UDP requests to portmap get no response.
>>> I would rather see this one addressed in the kernel.  I think the EIO
>>> return is a kernel bug.  If the server doesn't support a particular
>>> transport protocol for portmap, mountd, or NFS, the mount(2) system
>>> call should always return EPROTONOSUPPORT.
>> Since EIO has so many meanings... I think it should stay as a fatal 
>> error as well...
>>
> 
> That would be best.  But the reality is that there are kernels in the
> wild where -EIO indicates and error that isn't really fatal.
Yeah I know... and thats the problem.. EIO is basically a catch all errno.. 

> 
> We already do kernel-detection in mount.  How about extending it like
> this?
> 
> Thanks,
> NeilBrown
> 
> From: Neil Brown <neilb@suse.de>
> Date: Thu, 17 Jul 2008 12:23:03 +1000
> Subject: [PATCH] mount: Recognised EIO from early kernels as possibly indicating a protocol error.
> 
> In kernels up to and including 2.6.26, mount reports "connection
> refused" type messages as EIO rather than EPROTONOSUPPORT.
> So detect those kernels and don't treat EIO as so fatal.
Should we just fix the kernel to do the right thing?

steved.

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Thursday July 17, SteveD@redhat.com wrote:
> > In kernels up to and including 2.6.26, mount reports "connection
> > refused" type messages as EIO rather than EPROTONOSUPPORT.
> > So detect those kernels and don't treat EIO as so fatal.
> Should we just fix the kernel to do the right thing?

Undoubtedly we should fix the kernel to do the right thing.
But some people find it much easier to upgrade user-space tools like
nfs-utils than to upgrade kernels.
We already have code in mount.nfs to cope with earlier kernels which
had less complete interfaces with userspace.
We can see this lack of distinction in error code simple as a less
complete interface in earlier kernels and treat it the same way.

I don't feel so strongly about this that I will fight to the end, but
I really think that if we can make life easier for our users with
compromising our integrity, we should.  And handling EIO differently
depending on kernel version seems to fit that goal.

NeilBrown

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Tuesday July 15, chuck.lever@oracle.com wrote:
> On Tue, Jul 15, 2008 at 8:56 AM, Neil Brown <neilb@suse.de> wrote:
> > This is the promised patch that adds mountproto=tcp to the string
> > mount options if needed.
> > We still get a 90second timeout, but at least it works rather than
> > saying "mount.nfs: internal error".
> >
> > It seems to me that it would be best to avoid the first call to mount
> > altogether.  Simply always do a probe_both and then do a mount based
> > on the results of that.
> > Is there a good reason not to?
> 
> If I understand the question correctly, I think it doesn't because in
> the most common cases, this isn't necessary.  The mount options are
> usually adequate, and most servers support all the necessary NFS
> versions and transport protocols.  This saves ephemeral ports and uses
> less network traffic.

Yes, I think you understand the question correctly.

Your point about saving ephemeral ports is a strong one.

The "most servers" point is less strong.  If there are any valid uses
were the current code causes unnecessary delays we should try to
address them, even if they are relatively few.

Suppose we were to take this approach:

   mount.nfs does DNS lookup and portmap look to find IP address and
      port number.  However it *doesn't* send a 'clnt_ping' as
      probe_port currently does.
   The information it collects is explicitly given to the kernel with
       mountproto= mountport= etc.
   The kernel talks directly to mountd (given proto/addr/port) to get
      the filehandle and so forth.  It doesn't talk to portmap at all
      if it is given the required port numbers.

This way there is no duplication of effort, but the "try this/try that"
heuristics are all in user-space where they (arguably) belong and
where it is easier to have control over timeouts.

The only case where the above would not easily do the right thing is
when portmap reports a port that the kernel cannot successfully talk
to.  That is really a configuration error (rather than just an
'interesting' configuration).  In that case, mount.nfs could
retry probe_both but this time do the clnt_ping to make sure the
service really is there.

Thoughts?

> >
> > If an NFS server is only listening on TCP for portmap (as apparently
> > MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
> > logic in case the initial choice of version/etc doesn't work, but it
> > doesn't cope with mountd needing tcp.
> > So:
> >  Fix probe_port so that a TIMEDOUT error doesn't simply abort
> >    but probes with other protocols (e.g. tcp).
> 
> That seems reasonable and will update the behavior for both legacy and
> text-based mounts.
> 
> But should you teach connect_to() to specifically handle ECONNREFUSED
> as well?  I don't see why there would be a long timeout in that case.

I don't think connect_to is the problem.
The (current) problem is when portmap cannot be reached by UDP.
This is attempted in clnt_call called from getport.
This is done over an unconnected socket so ICMP errors don't come back
(I think) so the timeout is all that clnt_call gets to know there is
an error... I wonder what would happen if we just changed that to be a
connected socket...

Thanks,
NeilBrown

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Friday July 18, neilb@suse.de wrote:
> This is done over an unconnected socket so ICMP errors don't come back
> (I think) so the timeout is all that clnt_call gets to know there is
> an error... I wonder what would happen if we just changed that to be a
> connected socket...

Yep, that was a good idea.
The patch below causes a connected socket to be used when talking to
portmap over UDP.  That means that we get ICMP errors back.
And if we handle the errors correctly, we avoid the timeout.

With this added to all my other current patches, attempting to mount
when udp/portmap is blocked succeeds in 90 seconds rather than 112
seconds, and the 90 seconds is all in the mount system call.

If the kernel used connected UDP ports to talk to portmap, it might
benefit too (didn't I see some emails about connected UDP ports in
net/sunrpc recently - I'm afraid I didn't pay much attention).

NeilBrown


From: NeilBrown <neilb@suse.de>

Use a connected port when talking to portmap via UDP.

This allows us to get ICMP errors reported back so we can avoid
timeouts.  Also catch the error (RPC_CANTRECV) properly in getport.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 75354a7..ff50512 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -447,7 +447,7 @@ static unsigned short getport(struct sockaddr_in *saddr,
 	bind_saddr = *saddr;
 	bind_saddr.sin_port = htons(PMAPPORT);
 
-	socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, FALSE);
+	socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, TRUE);
 	if (socket == RPC_ANYSOCK) {
 		if (proto == IPPROTO_TCP &&
 		    rpc_createerr.cf_error.re_errno == ETIMEDOUT)
@@ -536,6 +536,7 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
 		}
 		if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
 		    rpc_createerr.cf_stat != RPC_TIMEDOUT &&
+		    rpc_createerr.cf_stat != RPC_CANTRECV &&
 		    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
 			goto out_bad;
 
@@ -544,7 +545,8 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
 				continue;
 			p_prot = protos;
 		}
-		if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
+		if (rpc_createerr.cf_stat == RPC_TIMEDOUT ||
+		    rpc_createerr.cf_stat == RPC_CANTRECV)
 			goto out_bad;
 
 		if (vers || !*++p_vers)

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Thu, Jul 17, 2008 at 8:12 PM, Neil Brown <neilb@suse.de> wrote:
> On Friday July 18, neilb@suse.de wrote:
>> This is done over an unconnected socket so ICMP errors don't come back
>> (I think) so the timeout is all that clnt_call gets to know there is
>> an error... I wonder what would happen if we just changed that to be a
>> connected socket...
>
> Yep, that was a good idea.
> The patch below causes a connected socket to be used when talking to
> portmap over UDP.  That means that we get ICMP errors back.
> And if we handle the errors correctly, we avoid the timeout.

Excellent.

> With this added to all my other current patches, attempting to mount
> when udp/portmap is blocked succeeds in 90 seconds rather than 112
> seconds, and the 90 seconds is all in the mount system call.
>
> If the kernel used connected UDP ports to talk to portmap, it might
> benefit too (didn't I see some emails about connected UDP ports in
> net/sunrpc recently - I'm afraid I didn't pay much attention).

Yes, I think we are going that direction with the kernel as well.  The
trade-off is at one point we were considering using a single UDP port
for all UDP-based RPC traffic, and using connected UDP would make that
more challenging.

I'm also a little worried about cases where a firewall might purposely
drop ICMP, but allow UDP (this is how my home firewall is configured).
 But I think the worst that can happen there is that we are back in
the same boat of waiting a long while if a UDP listener isn't
available.

> From: NeilBrown <neilb@suse.de>
>
> Use a connected port when talking to portmap via UDP.
>
> This allows us to get ICMP errors reported back so we can avoid
> timeouts.  Also catch the error (RPC_CANTRECV) properly in getport.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index 75354a7..ff50512 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -447,7 +447,7 @@ static unsigned short getport(struct sockaddr_in *saddr,
>        bind_saddr = *saddr;
>        bind_saddr.sin_port = htons(PMAPPORT);
>
> -       socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, FALSE);
> +       socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, TRUE);
>        if (socket == RPC_ANYSOCK) {
>                if (proto == IPPROTO_TCP &&
>                    rpc_createerr.cf_error.re_errno == ETIMEDOUT)
> @@ -536,6 +536,7 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>                }
>                if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
>                    rpc_createerr.cf_stat != RPC_TIMEDOUT &&
> +                   rpc_createerr.cf_stat != RPC_CANTRECV &&
>                    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
>                        goto out_bad;
>
> @@ -544,7 +545,8 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>                                continue;
>                        p_prot = protos;
>                }
> -               if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
> +               if (rpc_createerr.cf_stat == RPC_TIMEDOUT ||
> +                   rpc_createerr.cf_stat == RPC_CANTRECV)
>                        goto out_bad;
>
>                if (vers || !*++p_vers)
>

-- 
Chuck Lever

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

Hi Neil-

On Thu, Jul 17, 2008 at 7:15 PM, Neil Brown <neilb@suse.de> wrote:
> On Tuesday July 15, chuck.lever@oracle.com wrote:
>> On Tue, Jul 15, 2008 at 8:56 AM, Neil Brown <neilb@suse.de> wrote:
>> > This is the promised patch that adds mountproto=tcp to the string
>> > mount options if needed.
>> > We still get a 90second timeout, but at least it works rather than
>> > saying "mount.nfs: internal error".
>> >
>> > It seems to me that it would be best to avoid the first call to mount
>> > altogether.  Simply always do a probe_both and then do a mount based
>> > on the results of that.
>> > Is there a good reason not to?
>>
>> If I understand the question correctly, I think it doesn't because in
>> the most common cases, this isn't necessary.  The mount options are
>> usually adequate, and most servers support all the necessary NFS
>> versions and transport protocols.  This saves ephemeral ports and uses
>> less network traffic.
>
> Yes, I think you understand the question correctly.
>
> Your point about saving ephemeral ports is a strong one.
>
> The "most servers" point is less strong.  If there are any valid uses
> were the current code causes unnecessary delays we should try to
> address them, even if they are relatively few.

I think it's reasonable to look at the less common cases carefully to
see if we can improve things without making the common cases worse.

> Suppose we were to take this approach:
>
>   mount.nfs does DNS lookup and portmap look to find IP address and
>      port number.  However it *doesn't* send a 'clnt_ping' as
>      probe_port currently does.
>   The information it collects is explicitly given to the kernel with
>       mountproto= mountport= etc.
>   The kernel talks directly to mountd (given proto/addr/port) to get
>      the filehandle and so forth.  It doesn't talk to portmap at all
>      if it is given the required port numbers.
>
> This way there is no duplication of effort, but the "try this/try that"
> heuristics are all in user-space where they (arguably) belong and
> where it is easier to have control over timeouts.
>
> The only case where the above would not easily do the right thing is
> when portmap reports a port that the kernel cannot successfully talk
> to.  That is really a configuration error (rather than just an
> 'interesting' configuration).  In that case, mount.nfs could
> retry probe_both but this time do the clnt_ping to make sure the
> service really is there.
>
> Thoughts?

Using a connected UDP socket for both the kernel's rpcbind and it's
mountd client could help in many cases, including, probably, the one
you mention below, without the need for changing the current
architecture.

One thing about explicitly specifying mountport and mountproto during
a mount is that the umount.nfs command may have to include some logic
to throw those out and reprobe if those settings don't work at unmount
time.  These options were added to allow traversing a firewall using a
fixed port and protocol; overloading them for the case you describe
above may perhaps have some unpleasant consequences for the fixed
port/protocol case.

>> > If an NFS server is only listening on TCP for portmap (as apparently
>> > MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
>> > logic in case the initial choice of version/etc doesn't work, but it
>> > doesn't cope with mountd needing tcp.

I think that is mostly because the text-based mount option rewriting
logic isn't robust yet.  I have several patches in the IPv6 series
that should address some of this.

But many of Linux's NFS auxiliary services are UDP-only.  statd and
sm-notify, for instance, are UDP-only as far as I can tell.  And
recently the kernel's NLM service was changed to listen only on TCP if
clients are connecting to servers only via TCP -- and that breaks some
local Linux services that assume UDP will always be there (like
SM_MON).

sm-notify, specifically, will be difficult to convert to use TCP as it
capitalizes on the use of a single unconnected UDP socket to send
portmap requests and reboot notifications to multiple hosts using only
a single port.

I think we have more problems with a TCP-only NFSv2/v3 server than
just the behavior of text-based mounts.

-- 
Chuck Lever

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Saturday July 19, chuck.lever@oracle.com wrote:
> > Suppose we were to take this approach:
> >
> >   mount.nfs does DNS lookup and portmap look to find IP address and
> >      port number.  However it *doesn't* send a 'clnt_ping' as
> >      probe_port currently does.
> >   The information it collects is explicitly given to the kernel with
> >       mountproto= mountport= etc.
> >   The kernel talks directly to mountd (given proto/addr/port) to get
> >      the filehandle and so forth.  It doesn't talk to portmap at all
> >      if it is given the required port numbers.
> >
...
> >
> > Thoughts?
> 
> Using a connected UDP socket for both the kernel's rpcbind and it's
> mountd client could help in many cases, including, probably, the one
> you mention below, without the need for changing the current
> architecture.
> 
> One thing about explicitly specifying mountport and mountproto during
> a mount is that the umount.nfs command may have to include some logic
> to throw those out and reprobe if those settings don't work at unmount
> time.  These options were added to allow traversing a firewall using a
> fixed port and protocol; overloading them for the case you describe
> above may perhaps have some unpleasant consequences for the fixed
> port/protocol case.
> 

Yes....  umount wouldn't know the difference between:
  - don't use portmap, it doesn't work.  Just use this port number.
and
  - I used portmap and got this port number, so maybe it will be
    useful to you to.

In the first case umount should only use the mountport given.  In the
second case it arguably should not and should always talk to portmap
to get he port number (as it could be much later and mountd may well
be on a different port).


> >> > If an NFS server is only listening on TCP for portmap (as apparently
> >> > MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
> >> > logic in case the initial choice of version/etc doesn't work, but it
> >> > doesn't cope with mountd needing tcp.
> 
> I think that is mostly because the text-based mount option rewriting
> logic isn't robust yet.  I have several patches in the IPv6 series
> that should address some of this.

Any chance of pulling these out and sending them upstream now?  or is
the IPv6 series very close to release?

> 
> But many of Linux's NFS auxiliary services are UDP-only.  statd and
> sm-notify, for instance, are UDP-only as far as I can tell.  And
> recently the kernel's NLM service was changed to listen only on TCP if
> clients are connecting to servers only via TCP -- and that breaks some
> local Linux services that assume UDP will always be there (like
> SM_MON).

I'm not so much focussed on "make it work without UDP" as "avoid a
regression".  The current code doesn't work in situations where the
old code does work.  That is bad.

NLM without UDP has probably never worked for exactly the reasons you
mention, so it is not clear that anything needs to be "fixed" there.

I'd just like to get the regression fixed.

Thanks,
NeilBrown

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Sun, Jul 20, 2008 at 2:48 AM, Neil Brown <neilb@suse.de> wrote:
> On Saturday July 19, chuck.lever@oracle.com wrote:
>> > Suppose we were to take this approach:
>> >
>> >   mount.nfs does DNS lookup and portmap look to find IP address and
>> >      port number.  However it *doesn't* send a 'clnt_ping' as
>> >      probe_port currently does.
>> >   The information it collects is explicitly given to the kernel with
>> >       mountproto= mountport= etc.
>> >   The kernel talks directly to mountd (given proto/addr/port) to get
>> >      the filehandle and so forth.  It doesn't talk to portmap at all
>> >      if it is given the required port numbers.
>> >
> ...
>> >
>> > Thoughts?
>>
>> Using a connected UDP socket for both the kernel's rpcbind and it's
>> mountd client could help in many cases, including, probably, the one
>> you mention below, without the need for changing the current
>> architecture.
>>
>> One thing about explicitly specifying mountport and mountproto during
>> a mount is that the umount.nfs command may have to include some logic
>> to throw those out and reprobe if those settings don't work at unmount
>> time.  These options were added to allow traversing a firewall using a
>> fixed port and protocol; overloading them for the case you describe
>> above may perhaps have some unpleasant consequences for the fixed
>> port/protocol case.
>>
>
> Yes....  umount wouldn't know the difference between:
>  - don't use portmap, it doesn't work.  Just use this port number.
> and
>  - I used portmap and got this port number, so maybe it will be
>    useful to you to.
>
> In the first case umount should only use the mountport given.  In the
> second case it arguably should not and should always talk to portmap
> to get he port number (as it could be much later and mountd may well
> be on a different port).
>
>
>> >> > If an NFS server is only listening on TCP for portmap (as apparently
>> >> > MS-Windows-Server2003R2SP2 does), mount doesn't cope.  There is retry
>> >> > logic in case the initial choice of version/etc doesn't work, but it
>> >> > doesn't cope with mountd needing tcp.
>>
>> I think that is mostly because the text-based mount option rewriting
>> logic isn't robust yet.  I have several patches in the IPv6 series
>> that should address some of this.
>
> Any chance of pulling these out and sending them upstream now?  or is
> the IPv6 series very close to release?

IPv6 is actively being worked on.  I expect all these will be
available in the next 6 months.  However this specific set of patches
is dependent on some extensive changes (like, a complete
re-implementation of mount's portmap client).  Pulling them out now
would be a lot of work and I'm not sure it's worth the distraction to
the IPv6 effort, which essentially needed to be complete last month.

>> But many of Linux's NFS auxiliary services are UDP-only.  statd and
>> sm-notify, for instance, are UDP-only as far as I can tell.  And
>> recently the kernel's NLM service was changed to listen only on TCP if
>> clients are connecting to servers only via TCP -- and that breaks some
>> local Linux services that assume UDP will always be there (like
>> SM_MON).
>
> I'm not so much focussed on "make it work without UDP" as "avoid a
> regression".  The current code doesn't work in situations where the
> old code does work.  That is bad.

So, it is well known that there are some areas where text-based mounts
don't work as well as legacy mounts.  The problem is we don't have a
anything like a complete requirements specification for NFS mounting
(let alone a sophisticated or even simple regression/unit test suite
for mount), so it's really quite impossible to expect that a
completely new architecture will work out of the box.  We do know that
the existing legacy ABI is insufficient for many reasons.  I don't
think any one of us on this list has a complete enough history with
mount to know precisely which use cases are the ones we need to carry
forward to the text-based mount interface.

I've expected some problems in this area, and the only thing I can
hope for is that people will diligently report problems.  If you have
specific problems with the text-based implementation, I'd like to hear
about them so we can clear them up as quickly as possible.  I never
claimed that work is complete yet.

> I'd just like to get the regression fixed.

Me too.

Is it not sufficient to use connected UDP sockets in both user space
and the kernel, and fix the kernel to return EPROTONOTSUPP where
appropriate?

A healing balm for those who would like to use late model nfs-utils
releases but don't want the hassle of the remaining bugs in the
text-based mounts may be in order.  It would be easy to add a
configure switch to use only the legacy ABI interface for mount.nfs.

-- 
Chuck Lever

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Sunday July 20, chucklever@gmail.com wrote:
> On Sun, Jul 20, 2008 at 2:48 AM, Neil Brown <neilb@suse.de> wrote:
> >
> > Any chance of pulling these out and sending them upstream now?  or is
> > the IPv6 series very close to release?
> 
> IPv6 is actively being worked on.  I expect all these will be
> available in the next 6 months.  However this specific set of patches
> is dependent on some extensive changes (like, a complete
> re-implementation of mount's portmap client).  Pulling them out now
> would be a lot of work and I'm not sure it's worth the distraction to
> the IPv6 effort, which essentially needed to be complete last month.

Fair enough....
Last month hasn't finished yet, has it?  That would be awkward.  There
is still lots to be done in June :-)

> 
> I've expected some problems in this area, and the only thing I can
> hope for is that people will diligently report problems.  If you have
> specific problems with the text-based implementation, I'd like to hear
> about them so we can clear them up as quickly as possible.  I never
> claimed that work is complete yet.

Just that "portmap doesn't respond to UDP" causes problems.

> 
> > I'd just like to get the regression fixed.
> 
> Me too.
> 
> Is it not sufficient to use connected UDP sockets in both user space
> and the kernel, and fix the kernel to return EPROTONOTSUPP where
> appropriate?

Well I just noticed that it is now "fixed" in the kernel by defaulting
"mount_server" to use TCP when "nfs_server" is using TCP.
 commit 259875efed06d6936f54c9a264e868937f1bc217

However I would like nfs-utils to work well with all released kernels
(or at least those release this millennium) where possible.  So while
a kernel fix is good, it isn't a panacea.

> 
> A healing balm for those who would like to use late model nfs-utils
> releases but don't want the hassle of the remaining bugs in the
> text-based mounts may be in order.  It would be easy to add a
> configure switch to use only the legacy ABI interface for mount.nfs.

While tempting, I don't think that is a good idea.  As you say, we
want people to report bugs.
Maybe if we had a switch to use the legacy ABI (the inverse of your
short-lived "-s") that might have helped.  But it is too late to
bother with that now I think.  'string options' mostly works and is
clearly the right way forward.  Let's go there.

How about this?
It always does "probe_bothports" before a mount so we get the retry
heuristics that are useful, but the probe_port does *not* ping the
actual service, as that can consume a precious reserved port, and
shouldn't be needed.

If mount still fails, try with a ping to get the old, thorough,
behaviour.

This goes on top of the patch to use connected UDP ports to talk to
portmap.

NeilBrown

>From 852424a9a02dbe1a7c3b75a014bc71cad2ab6d5e Mon Sep 17 00:00:00 2001
From: Neil Brown <neilb@suse.de>
Date: Mon, 21 Jul 2008 12:45:50 +1000
Subject: [PATCH] Check nfs options (vers/protocol) before trying mount.

As the kernels nfs-mount client does not have heuristics to pick the
best protocol/version, also check with portmap to find what is
available before requesting a mount.

However don't try to 'ping' the services.  For NFS, this ping would
need to come from a reserved port, and these are a scarce resource.

If the mount found, retry the probe doing any ping that might be
needed in the hope of finding the problem.

Note: this patch also removes the (recently added) setting of
mountport= in the mount arguments. This is because:
  1/ the kernel can find it easily itself
  2/ it could confuse unmount which may be run much later
      when mountd is running on a different port.


Signed-off-by: Neil Brown <neilb@suse.de>
---
 utils/mount/network.c  |   35 +++++++++++++++++++++--------------
 utils/mount/network.h  |    2 +-
 utils/mount/nfsmount.c |    2 +-
 utils/mount/stropts.c  |   18 +++++++-----------
 4 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index ff50512..e53bd53 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -500,9 +500,11 @@ static unsigned short getport(struct sockaddr_in *saddr,
  * Use the portmapper to discover whether or not the service we want is
  * available. The lists 'versions' and 'protos' define ordered sequences
  * of service versions and udp/tcp protocols to probe for.
+ * If 'ping' is set, set an RPC NULL request to make sure the service
+ * is there.  Else just assume that it is.
  */
 static int probe_port(clnt_addr_t *server, const unsigned long *versions,
-			const unsigned int *protos)
+			const unsigned int *protos, int ping)
 {
 	struct sockaddr_in *saddr = &server->saddr;
 	struct pmap *pmap = &server->pmap;
@@ -530,7 +532,8 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
 							_("UDP") : _("TCP"),
 						p_port);
                                 }
-				if (clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
+				if (!ping ||
+				    clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
 					goto out_ok;
 			}
 		}
@@ -567,7 +570,7 @@ out_ok:
 	return 1;
 }
 
-static int probe_nfsport(clnt_addr_t *nfs_server)
+static int probe_nfsport(clnt_addr_t *nfs_server, int ping)
 {
 	struct pmap *pmap = &nfs_server->pmap;
 
@@ -575,12 +578,14 @@ static int probe_nfsport(clnt_addr_t *nfs_server)
 		return 1;
 
 	if (nfs_mount_data_version >= 4)
-		return probe_port(nfs_server, probe_nfs3_first, probe_tcp_first);
+		return probe_port(nfs_server, probe_nfs3_first, probe_tcp_first,
+				  ping);
 	else
-		return probe_port(nfs_server, probe_nfs2_only, probe_udp_only);
+		return probe_port(nfs_server, probe_nfs2_only, probe_udp_only,
+				  ping);
 }
 
-static int probe_mntport(clnt_addr_t *mnt_server)
+static int probe_mntport(clnt_addr_t *mnt_server, int ping)
 {
 	struct pmap *pmap = &mnt_server->pmap;
 
@@ -588,9 +593,11 @@ static int probe_mntport(clnt_addr_t *mnt_server)
 		return 1;
 
 	if (nfs_mount_data_version >= 4)
-		return probe_port(mnt_server, probe_mnt3_first, probe_udp_first);
+		return probe_port(mnt_server, probe_mnt3_first, probe_udp_first,
+				  ping);
 	else
-		return probe_port(mnt_server, probe_mnt1_first, probe_udp_only);
+		return probe_port(mnt_server, probe_mnt1_first, probe_udp_only,
+				  ping);
 }
 
 /**
@@ -603,7 +610,7 @@ static int probe_mntport(clnt_addr_t *mnt_server)
  *
  * A side effect of calling this function is that rpccreateerr is set.
  */
-int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
+int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server, int ping)
 {
 	struct pmap *nfs_pmap = &nfs_server->pmap;
 	struct pmap *mnt_pmap = &mnt_server->pmap;
@@ -625,9 +632,9 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 
 	for (; *probe_vers; probe_vers++) {
 		nfs_pmap->pm_vers = mntvers_to_nfs(*probe_vers);
-		if ((res = probe_nfsport(nfs_server) != 0)) {
+		if ((res = probe_nfsport(nfs_server, ping) != 0)) {
 			mnt_pmap->pm_vers = *probe_vers;
-			if ((res = probe_mntport(mnt_server)) != 0)
+			if ((res = probe_mntport(mnt_server, ping)) != 0)
 				return 1;
 			memcpy(mnt_pmap, &save_mnt, sizeof(*mnt_pmap));
 		}
@@ -645,9 +652,9 @@ out_bad:
 	return 0;
 
 version_fixed:
-	if (!probe_nfsport(nfs_server))
+	if (!probe_nfsport(nfs_server, ping))
 		goto out_bad;
-	return probe_mntport(mnt_server);
+	return probe_mntport(mnt_server, ping);
 }
 
 static int probe_statd(void)
@@ -714,7 +721,7 @@ int nfs_call_umount(clnt_addr_t *mnt_server, dirpath *argp)
 	enum clnt_stat res = 0;
 	int msock;
 
-	if (!probe_mntport(mnt_server))
+	if (!probe_mntport(mnt_server, 0))
 		return 0;
 	clnt = mnt_openclnt(mnt_server, &msock);
 	if (!clnt)
diff --git a/utils/mount/network.h b/utils/mount/network.h
index a4dba1b..6c2013f 100644
--- a/utils/mount/network.h
+++ b/utils/mount/network.h
@@ -39,7 +39,7 @@ typedef struct {
 static const struct timeval TIMEOUT = { 20, 0 };
 static const struct timeval RETRY_TIMEOUT = { 3, 0 };
 
-int probe_bothports(clnt_addr_t *, clnt_addr_t *);
+int probe_bothports(clnt_addr_t *, clnt_addr_t *, int);
 int nfs_gethostbyname(const char *, struct sockaddr_in *);
 int nfs_name_to_address(const char *, const sa_family_t,
 		struct sockaddr *, socklen_t *);
diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
index 6355681..905d61d 100644
--- a/utils/mount/nfsmount.c
+++ b/utils/mount/nfsmount.c
@@ -129,7 +129,7 @@ nfs_call_mount(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server,
 	enum clnt_stat stat;
 	int msock;
 
-	if (!probe_bothports(mnt_server, nfs_server))
+	if (!probe_bothports(mnt_server, nfs_server, 1))
 		goto out_bad;
 
 	clnt = mnt_openclnt(mnt_server, &msock);
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 09fca86..767d481 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -314,7 +314,7 @@ static int nfs_is_permanent_error(int error)
  * Returns a new group of mount options if successful; otherwise
  * NULL is returned if some failure occurred.
  */
-static struct mount_options *nfs_rewrite_mount_options(char *str)
+static struct mount_options *nfs_rewrite_mount_options(char *str, int ping)
 {
 	struct mount_options *options;
 	char *option, new_option[64];
@@ -405,7 +405,7 @@ static struct mount_options *nfs_rewrite_mount_options(char *str)
 	po_remove_all(options, "tcp");
 	po_remove_all(options, "udp");
 
-	if (!probe_bothports(&mnt_server, &nfs_server)) {
+	if (!probe_bothports(&mnt_server, &nfs_server, ping)) {
 		errno = ESPIPE;
 		goto err;
 	}
@@ -441,11 +441,6 @@ static struct mount_options *nfs_rewrite_mount_options(char *str)
 	if (po_append(options, new_option) == PO_FAILED)
 		goto err;
 
-	snprintf(new_option, sizeof(new_option) - 1,
-		 "mountport=%lu", mnt_server.pmap.pm_port);
-	if (po_append(options, new_option) == PO_FAILED)
-		goto err;
-
 	errno = 0;
 	return options;
 
@@ -486,13 +481,13 @@ static int nfs_sys_mount(const struct nfsmount_info *mi, const char *type,
  * 'extra_opts' are updated to reflect the mount options that worked.
  * If the retry fails, 'options' and 'extra_opts' are left unchanged.
  */
-static int nfs_retry_nfs23mount(struct nfsmount_info *mi)
+static int nfs_try_nfs23mount_probe(struct nfsmount_info *mi, int ping)
 {
 	struct mount_options *retry_options;
 	char *retry_str = NULL;
 	char **extra_opts = mi->extra_opts;
 
-	retry_options = nfs_rewrite_mount_options(*extra_opts);
+	retry_options = nfs_rewrite_mount_options(*extra_opts, ping);
 	if (!retry_options)
 		return 0;
 
@@ -547,7 +542,7 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
 	if (mi->fake)
 		return 1;
 
-	if (nfs_sys_mount(mi, "nfs", *extra_opts))
+	if (nfs_try_nfs23mount_probe(mi, 0))
 		return 1;
 
 	/*
@@ -557,7 +552,8 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
 	if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT)
 		return 0;
 
-	return nfs_retry_nfs23mount(mi);
+	/* Probe harder */
+	return nfs_try_nfs23mount_probe(mi, 1);
 }
 
 /*
-- 
1.5.6.3

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Monday July 21, neilb@suse.de wrote:
> 
> How about this?

BTW, this and two other patches are in my git.

The first is the sm-notify fix, as modified, and with another fix
since Steve's last post.  There was a "return" which should have been
inside and 'if' but was outside it.

The second causes portmap lookup to use connected UDP ports.

The third is the one I just posted.

BTW Steve, it is a small thing, but when you apply patches to git you
often seem to lose the subject line.  This causes the first line of
the description to be using as the one line summary, which sometimes
looks rather strange.

Thanks,
NeilBrown

The following changes since commit ba8dd9533e647b70d6e46beed3dcd8d8036b02af:
  Neil Brown (1):
        If portmap is not listening on UDP (as apparently happens with

are available in the git repository at:

  git://neil.brown.name/nfs-utils/ for-steved

Neil Brown (3):
      sm-notify: perform DNS lookup in the background.
      Use connected socket when probing portmap with UDP.
      Check nfs options (vers/protocol) before trying mount.

 utils/mount/network.c   |   41 +++++++++++++++++++++++++----------------
 utils/mount/network.h   |    2 +-
 utils/mount/nfsmount.c  |    2 +-
 utils/mount/stropts.c   |   18 +++++++-----------
 utils/statd/sm-notify.c |   39 +++++++++++++++++++++++++--------------
 5 files changed, 59 insertions(+), 43 deletions(-)

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Sun, Jul 20, 2008 at 10:49 PM, Neil Brown <neilb@suse.de> wrote:
> On Sunday July 20, chucklever@gmail.com wrote:
>> On Sun, Jul 20, 2008 at 2:48 AM, Neil Brown <neilb@suse.de> wrote:
>> >
>> > Any chance of pulling these out and sending them upstream now?  or is
>> > the IPv6 series very close to release?
>>
>> IPv6 is actively being worked on.  I expect all these will be
>> available in the next 6 months.  However this specific set of patches
>> is dependent on some extensive changes (like, a complete
>> re-implementation of mount's portmap client).  Pulling them out now
>> would be a lot of work and I'm not sure it's worth the distraction to
>> the IPv6 effort, which essentially needed to be complete last month.
>
> Fair enough....
> Last month hasn't finished yet, has it?  That would be awkward.  There
> is still lots to be done in June :-)
>
>>
>> I've expected some problems in this area, and the only thing I can
>> hope for is that people will diligently report problems.  If you have
>> specific problems with the text-based implementation, I'd like to hear
>> about them so we can clear them up as quickly as possible.  I never
>> claimed that work is complete yet.
>
> Just that "portmap doesn't respond to UDP" causes problems.
>
>>
>> > I'd just like to get the regression fixed.
>>
>> Me too.
>>
>> Is it not sufficient to use connected UDP sockets in both user space
>> and the kernel, and fix the kernel to return EPROTONOTSUPP where
>> appropriate?
>
> Well I just noticed that it is now "fixed" in the kernel by defaulting
> "mount_server" to use TCP when "nfs_server" is using TCP.
>  commit 259875efed06d6936f54c9a264e868937f1bc217

Ah, that one.  Yes, I should have remembered that.  That indeed was a
regression from the behavior of legacy mounts.

As far as I can tell, if I try to mount with "proto=udp" and the
server doesn't support it, the mount command isn't supposed to switch
to TCP automatically, it's supposed to fail the mount.

If I try to mount but don't specify any transport protocol, it is
supposed to default to UDP for mountd and TCP for NFS.  In that case
the kernel should punt when the default protocols don't work and let
user space work out the right transports and retry.

> However I would like nfs-utils to work well with all released kernels
> (or at least those release this millennium) where possible.  So while
> a kernel fix is good, it isn't a panacea.

The workaround for older nfs-utils has been to specify
"mountproto=udp/tcp" explicitly.

It's a laudable goal to have completely interchangeable parts, but it
seems like we pay a bit of a price in this case.  And besides,
distributors are supposed to be adding value by ensuring that the
parts work together (and known bugs have been addressed) before they
ship... :-)  They can certainly add patches like this one without
having them go upstream.

I'm mainly concerned that the enterprise kernels work well.  The less
stable kernels like Fedora and Debian that use whatever is the latest
version are expected to have some issues.

>> A healing balm for those who would like to use late model nfs-utils
>> releases but don't want the hassle of the remaining bugs in the
>> text-based mounts may be in order.  It would be easy to add a
>> configure switch to use only the legacy ABI interface for mount.nfs.
>
> While tempting, I don't think that is a good idea.  As you say, we
> want people to report bugs.
> Maybe if we had a switch to use the legacy ABI (the inverse of your
> short-lived "-s") that might have helped.  But it is too late to
> bother with that now I think.  'string options' mostly works and is
> clearly the right way forward.  Let's go there.

> How about this?
> It always does "probe_bothports" before a mount so we get the retry
> heuristics that are useful, but the probe_port does *not* ping the
> actual service, as that can consume a precious reserved port, and
> shouldn't be needed.

I think the extra port probe is only needed if "proto=" is not
specified, and only for text-based mounts on pre .27 kernels, and only
if the server's portmapper doesn't support UDP at all (which is a
somewhat rare case I think).  Lots of "ifs".

So, yes, this will address it, but I'm not terribly happy about the
additional complexity in mount.nfs that will eventually not be needed
when the kernel is working completely correctly.  I feel like this
breaks the whole architecture of letting the kernel try the defaults,
and then user space does the recovery if that doesn't work.

I think this works around the problem of having a server who's
portmapper listens only on TCP.  The underlying portmap client in the
mount command (and in the kernel) should be able to deal with that
without tying the rpcbind transport to the transport that is being
queried, and that is what is truly broken here.

So I think I would rather see this problem addressed in the
portmap/rpcbind client implementations and not in the upper level
mountd clients.  If a portmap query doesn't work over UDP, it seems
like it should try it over TCP instead, and then remember that for
later queries.

I think your objection would be that this doesn't address the
regression for particular combinations of nfs-utils and kernel
versions, but I don't have a better suggestion at the moment.

> If mount still fails, try with a ping to get the old, thorough,
> behaviour.
>
> This goes on top of the patch to use connected UDP ports to talk to
> portmap.
>
> NeilBrown
>
> From 852424a9a02dbe1a7c3b75a014bc71cad2ab6d5e Mon Sep 17 00:00:00 2001
> From: Neil Brown <neilb@suse.de>
> Date: Mon, 21 Jul 2008 12:45:50 +1000
> Subject: [PATCH] Check nfs options (vers/protocol) before trying mount.
>
> As the kernels nfs-mount client does not have heuristics to pick the
> best protocol/version, also check with portmap to find what is
> available before requesting a mount.

Again, I may not be clear on the original requirements here.  My
understanding is that the legacy mount command uses default settings
if "proto/mountproto" are not specified.  It does not attempt to use
"the best" settings unless the default settings don't work.  This is
what the text-based mount logic does -- it uses default settings
(NFSv3 over TCP, mountd v3 over UDP), then punts if it needs service
discovery.

I'm not convinced the current text-based co-operative model is
inadequate and should be modified so that the mount.nfs command probes
then rewrites the mount options *before* calling the kernel the first
time.  This is what we are trying to avoid by doing the majority of
the mount option parsing in the kernel and not in user space.  It
feels as if we are throwing out the baby with the bath water.

>
> However don't try to 'ping' the services.  For NFS, this ping would
> need to come from a reserved port, and these are a scarce resource.
>
> If the mount found, retry the probe doing any ping that might be
> needed in the hope of finding the problem.
>
> Note: this patch also removes the (recently added) setting of
> mountport= in the mount arguments. This is because:
>  1/ the kernel can find it easily itself
>  2/ it could confuse unmount which may be run much later
>      when mountd is running on a different port.
>
>
> Signed-off-by: Neil Brown <neilb@suse.de>
> ---
>  utils/mount/network.c  |   35 +++++++++++++++++++++--------------
>  utils/mount/network.h  |    2 +-
>  utils/mount/nfsmount.c |    2 +-
>  utils/mount/stropts.c  |   18 +++++++-----------
>  4 files changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index ff50512..e53bd53 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -500,9 +500,11 @@ static unsigned short getport(struct sockaddr_in *saddr,
>  * Use the portmapper to discover whether or not the service we want is
>  * available. The lists 'versions' and 'protos' define ordered sequences
>  * of service versions and udp/tcp protocols to probe for.
> + * If 'ping' is set, set an RPC NULL request to make sure the service
> + * is there.  Else just assume that it is.
>  */
>  static int probe_port(clnt_addr_t *server, const unsigned long *versions,
> -                       const unsigned int *protos)
> +                       const unsigned int *protos, int ping)
>  {
>        struct sockaddr_in *saddr = &server->saddr;
>        struct pmap *pmap = &server->pmap;
> @@ -530,7 +532,8 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>                                                        _("UDP") : _("TCP"),
>                                                p_port);
>                                 }
> -                               if (clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
> +                               if (!ping ||
> +                                   clnt_ping(saddr, prog, *p_vers, *p_prot, NULL))
>                                        goto out_ok;
>                        }
>                }
> @@ -567,7 +570,7 @@ out_ok:
>        return 1;
>  }
>
> -static int probe_nfsport(clnt_addr_t *nfs_server)
> +static int probe_nfsport(clnt_addr_t *nfs_server, int ping)
>  {
>        struct pmap *pmap = &nfs_server->pmap;
>
> @@ -575,12 +578,14 @@ static int probe_nfsport(clnt_addr_t *nfs_server)
>                return 1;
>
>        if (nfs_mount_data_version >= 4)
> -               return probe_port(nfs_server, probe_nfs3_first, probe_tcp_first);
> +               return probe_port(nfs_server, probe_nfs3_first, probe_tcp_first,
> +                                 ping);
>        else
> -               return probe_port(nfs_server, probe_nfs2_only, probe_udp_only);
> +               return probe_port(nfs_server, probe_nfs2_only, probe_udp_only,
> +                                 ping);
>  }
>
> -static int probe_mntport(clnt_addr_t *mnt_server)
> +static int probe_mntport(clnt_addr_t *mnt_server, int ping)
>  {
>        struct pmap *pmap = &mnt_server->pmap;
>
> @@ -588,9 +593,11 @@ static int probe_mntport(clnt_addr_t *mnt_server)
>                return 1;
>
>        if (nfs_mount_data_version >= 4)
> -               return probe_port(mnt_server, probe_mnt3_first, probe_udp_first);
> +               return probe_port(mnt_server, probe_mnt3_first, probe_udp_first,
> +                                 ping);
>        else
> -               return probe_port(mnt_server, probe_mnt1_first, probe_udp_only);
> +               return probe_port(mnt_server, probe_mnt1_first, probe_udp_only,
> +                                 ping);
>  }
>
>  /**
> @@ -603,7 +610,7 @@ static int probe_mntport(clnt_addr_t *mnt_server)
>  *
>  * A side effect of calling this function is that rpccreateerr is set.
>  */
> -int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
> +int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server, int ping)
>  {
>        struct pmap *nfs_pmap = &nfs_server->pmap;
>        struct pmap *mnt_pmap = &mnt_server->pmap;
> @@ -625,9 +632,9 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
>
>        for (; *probe_vers; probe_vers++) {
>                nfs_pmap->pm_vers = mntvers_to_nfs(*probe_vers);
> -               if ((res = probe_nfsport(nfs_server) != 0)) {
> +               if ((res = probe_nfsport(nfs_server, ping) != 0)) {
>                        mnt_pmap->pm_vers = *probe_vers;
> -                       if ((res = probe_mntport(mnt_server)) != 0)
> +                       if ((res = probe_mntport(mnt_server, ping)) != 0)
>                                return 1;
>                        memcpy(mnt_pmap, &save_mnt, sizeof(*mnt_pmap));
>                }
> @@ -645,9 +652,9 @@ out_bad:
>        return 0;
>
>  version_fixed:
> -       if (!probe_nfsport(nfs_server))
> +       if (!probe_nfsport(nfs_server, ping))
>                goto out_bad;
> -       return probe_mntport(mnt_server);
> +       return probe_mntport(mnt_server, ping);
>  }
>
>  static int probe_statd(void)
> @@ -714,7 +721,7 @@ int nfs_call_umount(clnt_addr_t *mnt_server, dirpath *argp)
>        enum clnt_stat res = 0;
>        int msock;
>
> -       if (!probe_mntport(mnt_server))
> +       if (!probe_mntport(mnt_server, 0))
>                return 0;
>        clnt = mnt_openclnt(mnt_server, &msock);
>        if (!clnt)
> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index a4dba1b..6c2013f 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -39,7 +39,7 @@ typedef struct {
>  static const struct timeval TIMEOUT = { 20, 0 };
>  static const struct timeval RETRY_TIMEOUT = { 3, 0 };
>
> -int probe_bothports(clnt_addr_t *, clnt_addr_t *);
> +int probe_bothports(clnt_addr_t *, clnt_addr_t *, int);
>  int nfs_gethostbyname(const char *, struct sockaddr_in *);
>  int nfs_name_to_address(const char *, const sa_family_t,
>                struct sockaddr *, socklen_t *);
> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> index 6355681..905d61d 100644
> --- a/utils/mount/nfsmount.c
> +++ b/utils/mount/nfsmount.c
> @@ -129,7 +129,7 @@ nfs_call_mount(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server,
>        enum clnt_stat stat;
>        int msock;
>
> -       if (!probe_bothports(mnt_server, nfs_server))
> +       if (!probe_bothports(mnt_server, nfs_server, 1))
>                goto out_bad;
>
>        clnt = mnt_openclnt(mnt_server, &msock);
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 09fca86..767d481 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -314,7 +314,7 @@ static int nfs_is_permanent_error(int error)
>  * Returns a new group of mount options if successful; otherwise
>  * NULL is returned if some failure occurred.
>  */
> -static struct mount_options *nfs_rewrite_mount_options(char *str)
> +static struct mount_options *nfs_rewrite_mount_options(char *str, int ping)
>  {
>        struct mount_options *options;
>        char *option, new_option[64];
> @@ -405,7 +405,7 @@ static struct mount_options *nfs_rewrite_mount_options(char *str)
>        po_remove_all(options, "tcp");
>        po_remove_all(options, "udp");
>
> -       if (!probe_bothports(&mnt_server, &nfs_server)) {
> +       if (!probe_bothports(&mnt_server, &nfs_server, ping)) {
>                errno = ESPIPE;
>                goto err;
>        }
> @@ -441,11 +441,6 @@ static struct mount_options *nfs_rewrite_mount_options(char *str)
>        if (po_append(options, new_option) == PO_FAILED)
>                goto err;
>
> -       snprintf(new_option, sizeof(new_option) - 1,
> -                "mountport=%lu", mnt_server.pmap.pm_port);
> -       if (po_append(options, new_option) == PO_FAILED)
> -               goto err;
> -
>        errno = 0;
>        return options;
>
> @@ -486,13 +481,13 @@ static int nfs_sys_mount(const struct nfsmount_info *mi, const char *type,
>  * 'extra_opts' are updated to reflect the mount options that worked.
>  * If the retry fails, 'options' and 'extra_opts' are left unchanged.
>  */
> -static int nfs_retry_nfs23mount(struct nfsmount_info *mi)
> +static int nfs_try_nfs23mount_probe(struct nfsmount_info *mi, int ping)
>  {
>        struct mount_options *retry_options;
>        char *retry_str = NULL;
>        char **extra_opts = mi->extra_opts;
>
> -       retry_options = nfs_rewrite_mount_options(*extra_opts);
> +       retry_options = nfs_rewrite_mount_options(*extra_opts, ping);
>        if (!retry_options)
>                return 0;
>
> @@ -547,7 +542,7 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
>        if (mi->fake)
>                return 1;
>
> -       if (nfs_sys_mount(mi, "nfs", *extra_opts))
> +       if (nfs_try_nfs23mount_probe(mi, 0))
>                return 1;
>
>        /*
> @@ -557,7 +552,8 @@ static int nfs_try_nfs23mount(struct nfsmount_info *mi)
>        if (errno != EOPNOTSUPP && errno != EPROTONOSUPPORT)
>                return 0;
>
> -       return nfs_retry_nfs23mount(mi);
> +       /* Probe harder */
> +       return nfs_try_nfs23mount_probe(mi, 1);
>  }
>
>  /*
> --
> 1.5.6.3
>
>



-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Neil Brown]

On Monday July 21, chucklever@gmail.com wrote:
> 
> The workaround for older nfs-utils has been to specify
> "mountproto=udp/tcp" explicitly.

Yes, that's a usable workaround.

> I think this works around the problem of having a server who's
> portmapper listens only on TCP.  The underlying portmap client in the
> mount command (and in the kernel) should be able to deal with that
> without tying the rpcbind transport to the transport that is being
> queried, and that is what is truly broken here.
> 
> So I think I would rather see this problem addressed in the
> portmap/rpcbind client implementations and not in the upper level
> mountd clients.  If a portmap query doesn't work over UDP, it seems
> like it should try it over TCP instead, and then remember that for
> later queries.
> 
> I think your objection would be that this doesn't address the
> regression for particular combinations of nfs-utils and kernel
> versions, but I don't have a better suggestion at the moment.

My other objection is the you are pushing too much functionality into
the kernel.
The portmap lookup can be done in userspace, and so "should" be.
I'm not even convinced that the mount lookup belongs in the kernel,
though passing a (hex encoded) file handle down might be a bit clumsy.

But I've decided that there are bigger fish in the sea and I'm going
to let this one swim away.  As you say, the enterprise releases are
more important, and it looks like enough fixes will make their way in
before SLES11 that this won't be an issue any more.

I still hope the other two nfs-utils patches will get in.

> 
> Again, I may not be clear on the original requirements here.  My
> understanding is that the legacy mount command uses default settings
> if "proto/mountproto" are not specified.  It does not attempt to use
> "the best" settings unless the default settings don't work.  This is
> what the text-based mount logic does -- it uses default settings
> (NFSv3 over TCP, mountd v3 over UDP), then punts if it needs service
> discovery.

"requirements"? that would be nice?
The legacy code tries every possibly combination of values that
weren't explicitly given until it finds a combination that "works".
It goes from most desirable to least desirable.

The text-based mount logic doesn't do what you said any more.
It default to mountd over TCP thanks to
nfs_set_mount_transport_protocol().

Will this be a regression for someone else I wonder ....

NeilBrown

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Mon, Jul 21, 2008 at 2:06 AM, Neil Brown <neilb@suse.de> wrote:
> On Monday July 21, chucklever@gmail.com wrote:
>>
>> The workaround for older nfs-utils has been to specify
>> "mountproto=udp/tcp" explicitly.
>
> Yes, that's a usable workaround.
>
>> I think this works around the problem of having a server who's
>> portmapper listens only on TCP.  The underlying portmap client in the
>> mount command (and in the kernel) should be able to deal with that
>> without tying the rpcbind transport to the transport that is being
>> queried, and that is what is truly broken here.
>>
>> So I think I would rather see this problem addressed in the
>> portmap/rpcbind client implementations and not in the upper level
>> mountd clients.  If a portmap query doesn't work over UDP, it seems
>> like it should try it over TCP instead, and then remember that for
>> later queries.
>>
>> I think your objection would be that this doesn't address the
>> regression for particular combinations of nfs-utils and kernel
>> versions, but I don't have a better suggestion at the moment.
>
> My other objection is the you are pushing too much functionality into
> the kernel.

Perhaps...

> The portmap lookup can be done in userspace, and so "should" be.

Well, the rpcbind step comes quite automatically for the mountd client
in the kernel.  It's part of the RPC state machine for all client
connections.  There is no extra logic in the kernel to do it for the
mountd client.  It's really quite natural for it to do.

> I'm not even convinced that the mount lookup belongs in the kernel,
> though passing a (hex encoded) file handle down might be a bit clumsy.

The mountd client was already in the kernel (used for NFSROOT).  We
simply adopted it for normal mounts too.  User space has absolutely no
need to know the root file handle, but the kernel does.  So I think it
makes more sense for the kernel to handle the mountd lookup.

> But I've decided that there are bigger fish in the sea and I'm going
> to let this one swim away.  As you say, the enterprise releases are
> more important, and it looks like enough fixes will make their way in
> before SLES11 that this won't be an issue any more.
>
> I still hope the other two nfs-utils patches will get in.
>
>>
>> Again, I may not be clear on the original requirements here.  My
>> understanding is that the legacy mount command uses default settings
>> if "proto/mountproto" are not specified.  It does not attempt to use
>> "the best" settings unless the default settings don't work.  This is
>> what the text-based mount logic does -- it uses default settings
>> (NFSv3 over TCP, mountd v3 over UDP), then punts if it needs service
>> discovery.
>
> "requirements"? that would be nice?
> The legacy code tries every possibly combination of values that
> weren't explicitly given until it finds a combination that "works".
> It goes from most desirable to least desirable.

Yes, and the text-based mount command should do that as well if the
initial defaults don't work.

> The text-based mount logic doesn't do what you said any more.
> It default to mountd over TCP thanks to
> nfs_set_mount_transport_protocol().

I will have to look at it again.  My version of this fix, at least,
made the kernel use TCP for mountd and NFS if "proto=tcp" is
specified, UDP for both if "proto=udp" is specified, and TCP for NFS
and UDP for mountd if none were specified.  This is exactly how the
legacy mount command starts off.

If Trond's version of the fix doesn't do that, then that is a behavior
regression.

> Will this be a regression for someone else I wonder ....

-- 
Chuck Lever

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Jul 21, 2008, at 2:06 AM, Neil Brown wrote:

> On Monday July 21, chucklever@gmail.com wrote:
>>
>> The workaround for older nfs-utils has been to specify
>> "mountproto=udp/tcp" explicitly.
>
> Yes, that's a usable workaround.
>
>> I think this works around the problem of having a server who's
>> portmapper listens only on TCP.  The underlying portmap client in the
>> mount command (and in the kernel) should be able to deal with that
>> without tying the rpcbind transport to the transport that is being
>> queried, and that is what is truly broken here.
>>
>> So I think I would rather see this problem addressed in the
>> portmap/rpcbind client implementations and not in the upper level
>> mountd clients.  If a portmap query doesn't work over UDP, it seems
>> like it should try it over TCP instead, and then remember that for
>> later queries.
>>
>> I think your objection would be that this doesn't address the
>> regression for particular combinations of nfs-utils and kernel
>> versions, but I don't have a better suggestion at the moment.
>
> My other objection is the you are pushing too much functionality into
> the kernel.
> The portmap lookup can be done in userspace, and so "should" be.
> I'm not even convinced that the mount lookup belongs in the kernel,
> though passing a (hex encoded) file handle down might be a bit clumsy.
>
> But I've decided that there are bigger fish in the sea and I'm going
> to let this one swim away.  As you say, the enterprise releases are
> more important, and it looks like enough fixes will make their way in
> before SLES11 that this won't be an issue any more.
>
> I still hope the other two nfs-utils patches will get in.

FWIW, I'm comfortable with your suggested changes to sm-notify and  
with using connected UDP sockets where we can in mount.nfs.

In combination with using connected UDP sockets for kernel RPC  
clients, and with the recent fixes to the kernel's mount option parser  
to use appropriate transport protocol defaults, I think we will be in  
much better shape.

>> Again, I may not be clear on the original requirements here.  My
>> understanding is that the legacy mount command uses default settings
>> if "proto/mountproto" are not specified.  It does not attempt to use
>> "the best" settings unless the default settings don't work.  This is
>> what the text-based mount logic does -- it uses default settings
>> (NFSv3 over TCP, mountd v3 over UDP), then punts if it needs service
>> discovery.
>
> "requirements"? that would be nice?
> The legacy code tries every possibly combination of values that
> weren't explicitly given until it finds a combination that "works".
> It goes from most desirable to least desirable.
>
> The text-based mount logic doesn't do what you said any more.
> It default to mountd over TCP thanks to
> nfs_set_mount_transport_protocol().
>
> Will this be a regression for someone else I wonder ....
>
> NeilBrown
> --
> 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] mount: enable retry for nfs23 to set the correct protocol for mount.

[Trond Myklebust]

On Mon, 2008-07-21 at 11:32 -0400, Chuck Lever wrote:

> I will have to look at it again.  My version of this fix, at least,
> made the kernel use TCP for mountd and NFS if "proto=tcp" is
> specified, UDP for both if "proto=udp" is specified, and TCP for NFS
> and UDP for mountd if none were specified.  This is exactly how the
> legacy mount command starts off.
> 
> If Trond's version of the fix doesn't do that, then that is a behavior
> regression.

A regression w.r.t. what, exactly?

In the binary mount case, we certainly _always_ used TCP to talk to
mountd, when specifying NFS-over-TCP.

I don't remember seeing a public discussion as to why we should change
the default to using UDP when talking to mountd for the NFS-over-TCP
case, or even whether or not it is a safe to assume that we can. Will
the text mount retry using mountproto=tcp if mountproto=udp fails? If it
doesn't, then that would be a regression w.r.t. previously working
binary setups...

Trond

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Mon, Jul 21, 2008 at 1:40 PM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> On Mon, 2008-07-21 at 11:32 -0400, Chuck Lever wrote:
>
>> I will have to look at it again.  My version of this fix, at least,
>> made the kernel use TCP for mountd and NFS if "proto=tcp" is
>> specified, UDP for both if "proto=udp" is specified, and TCP for NFS
>> and UDP for mountd if none were specified.  This is exactly how the
>> legacy mount command starts off.
>>
>> If Trond's version of the fix doesn't do that, then that is a behavior
>> regression.
>
> A regression w.r.t. what, exactly?
>
> In the binary mount case, we certainly _always_ used TCP to talk to
> mountd, when specifying NFS-over-TCP.

That's correct, but it's not what I was referring to above.

> I don't remember seeing a public discussion as to why we should change
> the default to using UDP when talking to mountd for the NFS-over-TCP
> case, or even whether or not it is a safe to assume that we can. Will
> the text mount retry using mountproto=tcp if mountproto=udp fails? If it
> doesn't, then that would be a regression w.r.t. previously working
> binary setups...

My understanding of the legacy behavior is:

1.  If "proto=tcp" or "tcp" is specified but "mountproto=" is not
specified, then TCP is used for both the mountd request and the NFS
transport

2.  If "proto=udp" or "udp" is specified but "mountproto=" is not
specified, then UDP is used for both the mountd request and the NFS
transport

Explicitly specifying "proto=" usually means the sysadmin has some
kind of transport-specific filtering or firewalling in place; or, in
the TCP case, is attempting to mount a server that is more than
several router hops away and wants a better overall performance for
both NFS and mount requests; or in the UDP case, wants all traffic to
go over UDP (for example, on certain high performance networks where
very small packets like TCP ACKs are very inefficient).  So this is a
shorthand way to get most NFS-related traffic to go over a specific
transport.

In both of these cases, if the server's NFS or mountd service does not
support the requested protocol, the mount request fails outright.

Note that even if "proto=tcp" is specified, certain auxiliary
protocols (like NSM) still use the UDP transport.

[ Before .27, for text-based mounts, "proto=" only controlled the NFS
transport protocol.  The mountd transport protocol always used the
default transport if "mountproto=" was not specified ].

3.  If neither "mountproto=", nor "proto=", nor either "udp" or "tcp"
is specified, then the defaults are used:  UDP is used for mountd and
TCP is used for NFS.

The reason for this is that most common NFS servers support UDP for
mountd, it is less network traffic, and it doesn't leave two extra
ports in TIME_WAIT (one for a TCP rpcbind query to discover the mountd
service's port, and one for a TCP mountd request) after the mountd
request is complete.

[ I think this is reasonable behavior to keep, and I thought that Neil
was suggesting that it no longer works this way in .27-rc.  I'm still
catching up with e-mail so I haven't looked into this yet. ]

If the server doesn't support these transports, the mount.nfs command
will attempt to discover what the server supports and retry the mount
request with the discovered transport options.  If the server doesn't
support any transport supported by the client, or is misconfigured,
the mount request fails.

4.  If "mountproto=" is specified but none of "proto=", "udp" or "tcp"
is specified, then the specified transport is used for the mountd
request, but the default transport (TCP) is used for NFS.

5.  If both "mountproto=" and "proto=" (or "udp" or "tcp") are
specified, then the transport specified by "mountproto=" is used for
the mountd request, and the transport specified by "proto=" (or "udp"
or "tcp") is used for NFS, no matter which order these appear in.

[ This last one was not working properly with text-based mounts prior
to .27 -- if "proto=" appeared after "mountproto=", then in some
cases, "mountproto=" would be overridden. ]

I'm not sure if mount.nfs will try service discovery in these last two
cases, but if it is behaving consistently it should fail such
requests.

6.  If any of "proto=", "udp", "tcp", or "mountproto=" is specified
more than once on the same mount command line, then the value of the
rightmost instance of these options takes effect.

[ And this one was subject to the bug mentioned just above, but should
be fixed in .27. ]

This list would probably be a good addendum to nfs(5).

-- 
Chuck Lever

Re: [PATCH] mount: enable retry for nfs23 to set the correct protocol for mount.

[Chuck Lever]

On Jul 20, 2008, at 10:55 PM, Neil Brown wrote:

> On Monday July 21, neilb@suse.de wrote:
>>
>> How about this?
>
> BTW, this and two other patches are in my git.
>
> The first is the sm-notify fix, as modified, and with another fix
> since Steve's last post.  There was a "return" which should have been
> inside and 'if' but was outside it.
>
> The second causes portmap lookup to use connected UDP ports.
>
> The third is the one I just posted.
>
> BTW Steve, it is a small thing, but when you apply patches to git you
> often seem to lose the subject line.  This causes the first line of
> the description to be using as the one line summary, which sometimes
> looks rather strange.

Yep, I noticed this too.  My patch descriptions usually make better  
sense with the short description left intact.

> The following changes since commit  
> ba8dd9533e647b70d6e46beed3dcd8d8036b02af:
>  Neil Brown (1):
>        If portmap is not listening on UDP (as apparently happens with
>
> are available in the git repository at:
>
>  git://neil.brown.name/nfs-utils/ for-steved
>
> Neil Brown (3):
>      sm-notify: perform DNS lookup in the background.
>      Use connected socket when probing portmap with UDP.
>      Check nfs options (vers/protocol) before trying mount.
>
> utils/mount/network.c   |   41 ++++++++++++++++++++++++ 
> +----------------
> utils/mount/network.h   |    2 +-
> utils/mount/nfsmount.c  |    2 +-
> utils/mount/stropts.c   |   18 +++++++-----------
> utils/statd/sm-notify.c |   39 +++++++++++++++++++++++++--------------
> 5 files changed, 59 insertions(+), 43 deletions(-)
> --
> 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] mount: enable retry for nfs23 to set the correct protocol for mount.

[Steve Dickson]

Neil Brown wrote:
> On Friday July 18, neilb@suse.de wrote:
>> This is done over an unconnected socket so ICMP errors don't come back
>> (I think) so the timeout is all that clnt_call gets to know there is
>> an error... I wonder what would happen if we just changed that to be a
>> connected socket...
> 
> Yep, that was a good idea.
> The patch below causes a connected socket to be used when talking to
> portmap over UDP.  That means that we get ICMP errors back.
> And if we handle the errors correctly, we avoid the timeout.
> 
> With this added to all my other current patches, attempting to mount
> when udp/portmap is blocked succeeds in 90 seconds rather than 112
> seconds, and the 90 seconds is all in the mount system call.
> 
> If the kernel used connected UDP ports to talk to portmap, it might
> benefit too (didn't I see some emails about connected UDP ports in
> net/sunrpc recently - I'm afraid I didn't pay much attention).
> 
> NeilBrown
> 
> 
> From: NeilBrown <neilb@suse.de>
> 
> Use a connected port when talking to portmap via UDP.
> 
> This allows us to get ICMP errors reported back so we can avoid
> timeouts.  Also catch the error (RPC_CANTRECV) properly in getport.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index 75354a7..ff50512 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -447,7 +447,7 @@ static unsigned short getport(struct sockaddr_in *saddr,
>  	bind_saddr = *saddr;
>  	bind_saddr.sin_port = htons(PMAPPORT);
>  
> -	socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, FALSE);
> +	socket = get_socket(&bind_saddr, proto, PMAP_TIMEOUT, FALSE, TRUE);
>  	if (socket == RPC_ANYSOCK) {
>  		if (proto == IPPROTO_TCP &&
>  		    rpc_createerr.cf_error.re_errno == ETIMEDOUT)
> @@ -536,6 +536,7 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>  		}
>  		if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
>  		    rpc_createerr.cf_stat != RPC_TIMEDOUT &&
> +		    rpc_createerr.cf_stat != RPC_CANTRECV &&
>  		    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
>  			goto out_bad;
>  
> @@ -544,7 +545,8 @@ static int probe_port(clnt_addr_t *server, const unsigned long *versions,
>  				continue;
>  			p_prot = protos;
>  		}
> -		if (rpc_createerr.cf_stat == RPC_TIMEDOUT)
> +		if (rpc_createerr.cf_stat == RPC_TIMEDOUT ||
> +		    rpc_createerr.cf_stat == RPC_CANTRECV)
>  			goto out_bad;
>  
>  		if (vers || !*++p_vers)

Committed....

steved.