Re: inux-next: Tree for July 1

[Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>]

On Wed, 2008-07-02 at 13:43 -0400, Chuck Lever wrote:
> On Wed, Jul 2, 2008 at 1:15 PM, Trond Myklebust
> <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wed, 2008-07-02 at 10:34 -0400, Chuck Lever wrote:
> >> On Tue, Jul 1, 2008 at 8:49 PM, Trond Myklebust
> >> <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Tue, 2008-07-01 at 22:36 +0200, Rafael J. Wysocki wrote:
> >> >> I can't mount NFS shares with this kernel.  I get something of this sort in
> >> >> dmesg and it seems to be 100% reproducible:
> >> >>
> >> >> [  314.058858] RPC: Registered udp transport module.
> >> >> [  314.058863] RPC: Registered tcp transport module.
> >> >> [  314.490970] RPC: transport (0) not supported
> >> >> [  319.246987] __ratelimit: 23 messages suppressed
> >> >
> >> > Does this patch fix the problem for you?
> >> > -----------------------------------------------------------------------------------
> >> > From: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> >> > NFS: Fix the mount protocol defaults for binary mounts
> >> >
> >> > Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >
> >> >  fs/nfs/super.c |    1 +
> >> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >> > index e09b1c2..85fbb98 100644
> >> > --- a/fs/nfs/super.c
> >> > +++ b/fs/nfs/super.c
> >> > @@ -1575,6 +1575,7 @@ static int nfs_validate_mount_data(void *options,
> >> >
> >> >                if (!(data->flags & NFS_MOUNT_TCP))
> >> >                        args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
> >> > +               nfs_set_transport_defaults(args);
> >>
> >> nfs_set_transport_defaults() is overkill for the legacy mount path.
> >> The bug is that the logic here assumes that nfs_server.protocol
> >> already has the default value of XPRT_TRANSPORT_TCP, but commit
> >> 8b59ea3c removed that default.  The correct fix is to add
> >>
> >>             args->nfs_server.protocol = XPRT_TRANSPORT_TCP;
> >>
> >> just before the 'if' statement.  We should fold that into 8b59ea3c to
> >> preserve bisectability.
> >
> > NACK. You still need to set the appropriate retrans and timeo defaults.
> 
> NACK squared [Bruce told me to write this].
> 
> The only bug involves the transport protocol setting for NFSv2/v3 legacy mounts.
> 
> The legacy mount command already sets appropriate default values for
> the timeout fields, and the code in that arm of
> nfs_validate_mount_options() _unconditionally_ copies the mount
> command's timeout settings into the nfs_parse_mount_options structure.
> 
> This is how it worked before commit 8b59ea3c, and 8b59ea3c doesn't
> change this behavior.

That's utter nonsense. We _never_ relied on the mount command to set
defaults for us. I remember all too well debugging old versions of
am-utils/amd that set the TCP flag and then happily set timeout values
of 7/10 second. As far as I know, all am-utils versions since then have
used timeo=0, retrans=0.

However, that does illustrate something else:
nfs_set_transport_defaults() has never been necessary for setting timeo
and retrans, since the zero case is already covered in
nfs_init_timeout_values() (which is also where the sanity checks are
applied).

---------------------------------------------------------------------------------
From: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
Date: Wed, 2 Jul 2008 14:43:47 -0400
NFS: Fix the mount protocol defaults for binary mounts

Move the UDP/TCP default timeo/retrans settings for text mounts to
nfs_init_timeout_values(), which was were they were always being
initialised for binary mounts.

Ensure we do initialise the transport protocol for the legacy binary mount
case in nfs_validate_mount_data.

Ensure that we sanity check the transport protocol in the legacy binary
mount case in nfs4_validate_mount_data

Fix up the incorrect values of NFS_DEF_UDP_TIMEO, NFS_DEF_UDP_RETRANS to
match the nfs manpage documentation.

Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
---

 fs/nfs/client.c        |   13 +++++++-----
 fs/nfs/super.c         |   53 ++++++++++++++++++++++++------------------------
 include/linux/nfs_fs.h |    4 ++--
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index f2a092c..5ee23e7 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -431,14 +431,14 @@ static void nfs_init_timeout_values(struct rpc_timeout *to, int proto,
 {
 	to->to_initval = timeo * HZ / 10;
 	to->to_retries = retrans;
-	if (!to->to_retries)
-		to->to_retries = 2;
 
 	switch (proto) {
 	case XPRT_TRANSPORT_TCP:
 	case XPRT_TRANSPORT_RDMA:
+		if (to->to_retries == 0)
+			to->to_retries = NFS_DEF_TCP_RETRANS;
 		if (to->to_initval == 0)
-			to->to_initval = 60 * HZ;
+			to->to_initval = NFS_DEF_TCP_TIMEO * HZ / 10;
 		if (to->to_initval > NFS_MAX_TCP_TIMEOUT)
 			to->to_initval = NFS_MAX_TCP_TIMEOUT;
 		to->to_increment = to->to_initval;
@@ -450,14 +450,17 @@ static void nfs_init_timeout_values(struct rpc_timeout *to, int proto,
 		to->to_exponential = 0;
 		break;
 	case XPRT_TRANSPORT_UDP:
-	default:
+		if (to->to_retries == 0)
+			to->to_retries = NFS_DEF_UDP_RETRANS;
 		if (!to->to_initval)
-			to->to_initval = 11 * HZ / 10;
+			to->to_initval = NFS_DEF_UDP_TIMEO * HZ / 10;
 		if (to->to_initval > NFS_MAX_UDP_TIMEOUT)
 			to->to_initval = NFS_MAX_UDP_TIMEOUT;
 		to->to_maxval = NFS_MAX_UDP_TIMEOUT;
 		to->to_exponential = 1;
 		break;
+	default:
+		BUG();
 	}
 }
 
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index e09b1c2..47cf83e 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -819,40 +819,39 @@ static void nfs_parse_ip_address(char *string, size_t str_len,
 }
 
 /*
- * Time-out and mount transport default settings are based on the
- * specified NFS transport.  For legacy mounts, these are set by
- * the mount command before mount(2) is invoked.  For text-based
- * mounts, the kernel must take care to set these.
+ * Sanity check the NFS transport protocol.
+ *
  */
-static void nfs_set_transport_defaults(struct nfs_parsed_mount_data *mnt)
+static void nfs_validate_transport_protocol(struct nfs_parsed_mount_data *mnt)
 {
 	switch (mnt->nfs_server.protocol) {
 	case XPRT_TRANSPORT_UDP:
-		if (mnt->mount_server.protocol == 0)
-			mnt->mount_server.protocol = XPRT_TRANSPORT_UDP;
-		if (mnt->timeo == 0)
-			mnt->timeo = NFS_DEF_UDP_TIMEO;
-		if (mnt->retrans == 0)
-			mnt->retrans = NFS_DEF_UDP_RETRANS;
-		break;
 	case XPRT_TRANSPORT_TCP:
 	case XPRT_TRANSPORT_RDMA:
-		if (mnt->mount_server.protocol == 0)
-			mnt->mount_server.protocol = XPRT_TRANSPORT_TCP;
-		if (mnt->timeo == 0)
-			mnt->timeo = NFS_DEF_TCP_TIMEO;
-		if (mnt->retrans == 0)
-			mnt->retrans = NFS_DEF_TCP_RETRANS;
 		break;
 	default:
 		mnt->nfs_server.protocol = XPRT_TRANSPORT_TCP;
-		if (mnt->mount_server.protocol == 0)
-			mnt->mount_server.protocol = XPRT_TRANSPORT_UDP;
-		if (mnt->timeo == 0)
-			mnt->timeo = NFS_DEF_TCP_TIMEO;
-		if (mnt->retrans == 0)
-			mnt->retrans = NFS_DEF_TCP_RETRANS;
+	}
+}
+
+/*
+ * For text based NFSv2/v3 mounts, the mount protocol transport default
+ * settings should depend upon the specified NFS transport.
+ */
+static void nfs_set_mount_transport_protocol(struct nfs_parsed_mount_data *mnt)
+{
+	nfs_validate_transport_protocol(mnt);
+
+	if (mnt->mount_server.protocol == XPRT_TRANSPORT_UDP ||
+	    mnt->mount_server.protocol == XPRT_TRANSPORT_TCP)
+			return;
+	switch (mnt->nfs_server.protocol) {
+	case XPRT_TRANSPORT_UDP:
+		mnt->mount_server.protocol = XPRT_TRANSPORT_UDP;
 		break;
+	case XPRT_TRANSPORT_TCP:
+	case XPRT_TRANSPORT_RDMA:
+		mnt->mount_server.protocol = XPRT_TRANSPORT_TCP;
 	}
 }
 
@@ -1521,6 +1520,7 @@ static int nfs_validate_mount_data(void *options,
 	args->acdirmax		= NFS_DEF_ACDIRMAX;
 	args->mount_server.port	= 0;	/* autobind unless user sets port */
 	args->nfs_server.port	= 0;	/* autobind unless user sets port */
+	args->nfs_server.protocol = XPRT_TRANSPORT_TCP;
 	args->auth_flavors[0]	= RPC_AUTH_UNIX;
 
 	switch (data->version) {
@@ -1625,7 +1625,7 @@ static int nfs_validate_mount_data(void *options,
 		nfs_set_port((struct sockaddr *)&args->nfs_server.address,
 				args->nfs_server.port);
 
-		nfs_set_transport_defaults(args);
+		nfs_set_mount_transport_protocol(args);
 
 		status = nfs_parse_devname(dev_name,
 					   &args->nfs_server.hostname,
@@ -2235,6 +2235,7 @@ static int nfs4_validate_mount_data(void *options,
 		args->acdirmin	= data->acdirmin;
 		args->acdirmax	= data->acdirmax;
 		args->nfs_server.protocol = data->proto;
+		nfs_validate_transport_protocol(args);
 
 		break;
 	default: {
@@ -2250,7 +2251,7 @@ static int nfs4_validate_mount_data(void *options,
 		nfs_set_port((struct sockaddr *)&args->nfs_server.address,
 				args->nfs_server.port);
 
-		nfs_set_transport_defaults(args);
+		nfs_validate_transport_protocol(args);
 
 		if (args->auth_flavor_len > 1)
 			goto out_inval_auth;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 3c4078e..29d2619 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -12,8 +12,8 @@
 #include <linux/magic.h>
 
 /* Default timeout values */
-#define NFS_DEF_UDP_TIMEO	(7)
-#define NFS_DEF_UDP_RETRANS	(5)
+#define NFS_DEF_UDP_TIMEO	(11)
+#define NFS_DEF_UDP_RETRANS	(3)
 #define NFS_DEF_TCP_TIMEO	(600)
 #define NFS_DEF_TCP_RETRANS	(2)
 

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html