[PATCH 0/5] For 2.6.27: NFS mount option parsing fix-ups

[PATCH 0/5] For 2.6.27: NFS mount option parsing fix-ups

[Chuck Lever]

Hi Trond-

As a prelude to new features, here are some fixes and clean-ups for
various minor issues in the NFS client's mount option parser.

Please consider these for 2.6.27.

--
Chuck Lever
chu ckd otl eve rat ora cle dot com

[PATCH 1/5] NFS: Allow any value for the "retry" option

[Chuck Lever]

The kernel NFS mount option parser should ignore the retry= mount option
since it is meaningful only in user space.  Today it expects a number
rather than arbitrary text, so it ignores the option if the value is
numeric, but chokes if there are other characters in the value.

Change it to allow any text (except ",") as its value.

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

 fs/nfs/super.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index ef27035..9f38309 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -101,6 +101,8 @@ enum {
 static match_table_t nfs_mount_option_tokens = {
 	{ Opt_userspace, "bg" },
 	{ Opt_userspace, "fg" },
+	{ Opt_userspace, "retry=%s" },
+
 	{ Opt_soft, "soft" },
 	{ Opt_hard, "hard" },
 	{ Opt_intr, "intr" },
@@ -136,7 +138,6 @@ static match_table_t nfs_mount_option_tokens = {
 	{ Opt_acdirmin, "acdirmin=%u" },
 	{ Opt_acdirmax, "acdirmax=%u" },
 	{ Opt_actimeo, "actimeo=%u" },
-	{ Opt_userspace, "retry=%u" },
 	{ Opt_namelen, "namlen=%u" },
 	{ Opt_mountport, "mountport=%u" },
 	{ Opt_mountvers, "mountvers=%u" },

[PATCH 2/5] NFS: Treat "intr" and "nointr" options as deprecated

[Chuck Lever]

Clean up:  the "intr" and "nointr" mount options were recently retired.
Document this in the NFS mount option parser.

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

 fs/nfs/super.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 9f38309..e9dd5b6 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -65,7 +65,6 @@
 enum {
 	/* Mount options that take no arguments */
 	Opt_soft, Opt_hard,
-	Opt_intr, Opt_nointr,
 	Opt_posix, Opt_noposix,
 	Opt_cto, Opt_nocto,
 	Opt_ac, Opt_noac,
@@ -105,8 +104,8 @@ static match_table_t nfs_mount_option_tokens = {
 
 	{ Opt_soft, "soft" },
 	{ Opt_hard, "hard" },
-	{ Opt_intr, "intr" },
-	{ Opt_nointr, "nointr" },
+	{ Opt_deprecated, "intr" },
+	{ Opt_deprecated, "nointr" },
 	{ Opt_posix, "posix" },
 	{ Opt_noposix, "noposix" },
 	{ Opt_cto, "cto" },
@@ -784,9 +783,6 @@ static int nfs_parse_mount_options(char *raw,
 		case Opt_hard:
 			mnt->flags &= ~NFS_MOUNT_SOFT;
 			break;
-		case Opt_intr:
-		case Opt_nointr:
-			break;
 		case Opt_posix:
 			mnt->flags |= NFS_MOUNT_POSIX;
 			break;
@@ -1102,6 +1098,8 @@ static int nfs_parse_mount_options(char *raw,
 
 		case Opt_userspace:
 		case Opt_deprecated:
+			dfprintk(MOUNT, "NFS:   ignoring mount option "
+					"'%s'\n", p);
 			break;
 
 		default:

[PATCH 3/5] NFS: missing newline in NFS mount debugging message

[Chuck Lever]

Clean up.

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

 fs/nfs/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index e9dd5b6..d59913b 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1187,7 +1187,7 @@ static int nfs_try_mount(struct nfs_parsed_mount_data *args,
 	if (status == 0)
 		return 0;
 
-	dfprintk(MOUNT, "NFS: unable to mount server %s, error %d",
+	dfprintk(MOUNT, "NFS: unable to mount server %s, error %d\n",
 			hostname, status);
 	return status;
 }

[PATCH 4/5] NFS: set transport defaults after mount option parsing is finished

[Chuck Lever]

Address some unfortunate mount option parsing behavior by setting certain
transport-based defaults *after* option parsing is complete.

 o  Some options don't obey the "rightmost wins" rule.  Jeff Layton
    noticed that specifying the "proto=" mount option after the "retrans"
    or "timeo" options will cause the retrans and timeo values to be
    overwritten with default settings.

    Allow these options to be specified in any order without unexpectedly
    reverting retrans and timeo to their default.

 o  I've received several reports that text-based mounting through
    firewalls that block UDP fails, even if "proto=tcp" is specified.

    If a user specifies "proto=tcp" via the legacy mount API, the mount
    command also uses TCP to contact the server's mount daemon.  Ditto
    for "proto=udp".  We want the kernel's mount option to emulate this
    behavior; however, we still want the default mount protocol to be UDP
    if no transport options were specified.

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

 fs/nfs/super.c         |   49 +++++++++++++++++++++++++++++-------------------
 include/linux/nfs_fs.h |    5 +++++
 2 files changed, 35 insertions(+), 19 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d59913b..964b17b 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -816,20 +816,14 @@ static int nfs_parse_mount_options(char *raw,
 		case Opt_udp:
 			mnt->flags &= ~NFS_MOUNT_TCP;
 			mnt->nfs_server.protocol = XPRT_TRANSPORT_UDP;
-			mnt->timeo = 7;
-			mnt->retrans = 5;
 			break;
 		case Opt_tcp:
 			mnt->flags |= NFS_MOUNT_TCP;
 			mnt->nfs_server.protocol = XPRT_TRANSPORT_TCP;
-			mnt->timeo = 600;
-			mnt->retrans = 2;
 			break;
 		case Opt_rdma:
 			mnt->flags |= NFS_MOUNT_TCP; /* for side protocols */
 			mnt->nfs_server.protocol = XPRT_TRANSPORT_RDMA;
-			mnt->timeo = 600;
-			mnt->retrans = 2;
 			break;
 		case Opt_acl:
 			mnt->flags &= ~NFS_MOUNT_NOACL;
@@ -1023,21 +1017,15 @@ static int nfs_parse_mount_options(char *raw,
 			case Opt_xprt_udp:
 				mnt->flags &= ~NFS_MOUNT_TCP;
 				mnt->nfs_server.protocol = XPRT_TRANSPORT_UDP;
-				mnt->timeo = 7;
-				mnt->retrans = 5;
 				break;
 			case Opt_xprt_tcp:
 				mnt->flags |= NFS_MOUNT_TCP;
 				mnt->nfs_server.protocol = XPRT_TRANSPORT_TCP;
-				mnt->timeo = 600;
-				mnt->retrans = 2;
 				break;
 			case Opt_xprt_rdma:
 				/* vector side protocols to TCP */
 				mnt->flags |= NFS_MOUNT_TCP;
 				mnt->nfs_server.protocol = XPRT_TRANSPORT_RDMA;
-				mnt->timeo = 600;
-				mnt->retrans = 2;
 				break;
 			default:
 				goto out_unrec_xprt;
@@ -1110,6 +1098,36 @@ static int nfs_parse_mount_options(char *raw,
 	nfs_set_port((struct sockaddr *)&mnt->nfs_server.address,
 				mnt->nfs_server.port);
 
+	/* Certain default settings are based on the specified transport */
+	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;
+		break;
+	}
+
 	return 1;
 
 out_nomem:
@@ -1223,16 +1241,12 @@ static int nfs_validate_mount_data(void *options,
 	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
 	args->rsize		= NFS_MAX_FILE_IO_SIZE;
 	args->wsize		= NFS_MAX_FILE_IO_SIZE;
-	args->timeo		= 600;
-	args->retrans		= 2;
 	args->acregmin		= 3;
 	args->acregmax		= 60;
 	args->acdirmin		= 30;
 	args->acdirmax		= 60;
 	args->mount_server.port	= 0;	/* autobind unless user sets port */
-	args->mount_server.protocol = XPRT_TRANSPORT_UDP;
 	args->nfs_server.port	= 0;	/* autobind unless user sets port */
-	args->nfs_server.protocol = XPRT_TRANSPORT_TCP;
 
 	switch (data->version) {
 	case 1:
@@ -1806,14 +1820,11 @@ static int nfs4_validate_mount_data(void *options,
 
 	args->rsize		= NFS_MAX_FILE_IO_SIZE;
 	args->wsize		= NFS_MAX_FILE_IO_SIZE;
-	args->timeo		= 600;
-	args->retrans		= 2;
 	args->acregmin		= 3;
 	args->acregmax		= 60;
 	args->acdirmin		= 30;
 	args->acdirmax		= 60;
 	args->nfs_server.port	= NFS_PORT; /* 2049 unless user set port= */
-	args->nfs_server.protocol = XPRT_TRANSPORT_TCP;
 
 	switch (data->version) {
 	case 1:
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 27d6a8d..afc3956 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -12,6 +12,11 @@
 #include <linux/magic.h>
 
 /* Default timeout values */
+#define NFS_DEF_UDP_TIMEO	(7)
+#define NFS_DEF_UDP_RETRANS	(5)
+#define NFS_DEF_TCP_TIMEO	(600)
+#define NFS_DEF_TCP_RETRANS	(2)
+
 #define NFS_MAX_UDP_TIMEOUT	(60*HZ)
 #define NFS_MAX_TCP_TIMEOUT	(600*HZ)
 

Re: [PATCH 4/5] NFS: set transport defaults after mount option parsing is finished

[Trond Myklebust]

On Thu, 2008-06-12 at 12:37 -0400, Chuck Lever wrote:
> Address some unfortunate mount option parsing behavior by setting certain
> transport-based defaults *after* option parsing is complete.
> 
>  o  Some options don't obey the "rightmost wins" rule.  Jeff Layton
>     noticed that specifying the "proto=" mount option after the "retrans"
>     or "timeo" options will cause the retrans and timeo values to be
>     overwritten with default settings.
> 
>     Allow these options to be specified in any order without unexpectedly
>     reverting retrans and timeo to their default.
> 
>  o  I've received several reports that text-based mounting through
>     firewalls that block UDP fails, even if "proto=tcp" is specified.
> 
>     If a user specifies "proto=tcp" via the legacy mount API, the mount
>     command also uses TCP to contact the server's mount daemon.  Ditto
>     for "proto=udp".  We want the kernel's mount option to emulate this
>     behavior; however, we still want the default mount protocol to be UDP
>     if no transport options were specified.

Why is it necessary to set these defaults in nfs_parse_mount_options?
This is quite unrelated to parsing of the mount string.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 4/5] NFS: set transport defaults after mount option parsing is finished

[Chuck Lever]

On Jun 12, 2008, at 12:57 PM, Trond Myklebust wrote:
> On Thu, 2008-06-12 at 12:37 -0400, Chuck Lever wrote:
>> Address some unfortunate mount option parsing behavior by setting  
>> certain
>> transport-based defaults *after* option parsing is complete.
>>
>> o  Some options don't obey the "rightmost wins" rule.  Jeff Layton
>>    noticed that specifying the "proto=" mount option after the  
>> "retrans"
>>    or "timeo" options will cause the retrans and timeo values to be
>>    overwritten with default settings.
>>
>>    Allow these options to be specified in any order without  
>> unexpectedly
>>    reverting retrans and timeo to their default.
>>
>> o  I've received several reports that text-based mounting through
>>    firewalls that block UDP fails, even if "proto=tcp" is specified.
>>
>>    If a user specifies "proto=tcp" via the legacy mount API, the  
>> mount
>>    command also uses TCP to contact the server's mount daemon.  Ditto
>>    for "proto=udp".  We want the kernel's mount option to emulate  
>> this
>>    behavior; however, we still want the default mount protocol to  
>> be UDP
>>    if no transport options were specified.
>
> Why is it necessary to set these defaults in nfs_parse_mount_options?
> This is quite unrelated to parsing of the mount string.

The default transport timer settings are the same for both NFS and  
NFSv4, so I thought it would be best to put a single copy of that  
logic in a common code path.

I suppose we could just factor it into a helper and have both the NFS  
and NFSv4 mount paths call this, but I'm not sure what the value of  
that would be.

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

[PATCH 5/5] NFS: use documenting macro constants for initializing ac{reg, dir}{min, max}

[Chuck Lever]

Clean up.

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

 fs/nfs/nfsroot.c       |    8 ++++----
 fs/nfs/super.c         |   24 ++++++++++++------------
 include/linux/nfs_fs.h |    5 +++++
 3 files changed, 21 insertions(+), 16 deletions(-)


diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
index 531379d..811ac22 100644
--- a/fs/nfs/nfsroot.c
+++ b/fs/nfs/nfsroot.c
@@ -297,10 +297,10 @@ static int __init root_nfs_name(char *name)
 	nfs_data.flags    = NFS_MOUNT_NONLM;	/* No lockd in nfs root yet */
 	nfs_data.rsize    = NFS_DEF_FILE_IO_SIZE;
 	nfs_data.wsize    = NFS_DEF_FILE_IO_SIZE;
-	nfs_data.acregmin = 3;
-	nfs_data.acregmax = 60;
-	nfs_data.acdirmin = 30;
-	nfs_data.acdirmax = 60;
+	nfs_data.acregmin = NFS_DEF_ACREGMIN;
+	nfs_data.acregmax = NFS_DEF_ACREGMAX;
+	nfs_data.acdirmin = NFS_DEF_ACDIRMIN;
+	nfs_data.acdirmax = NFS_DEF_ACDIRMAX;
 	strcpy(buf, NFS_ROOT);
 
 	/* Process options received from the remote server */
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 964b17b..eba5d0d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -514,13 +514,13 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
 	if (nfss->bsize != 0)
 		seq_printf(m, ",bsize=%u", nfss->bsize);
 	seq_printf(m, ",namlen=%u", nfss->namelen);
-	if (nfss->acregmin != 3*HZ || showdefaults)
+	if (nfss->acregmin != NFS_DEF_ACREGMIN*HZ || showdefaults)
 		seq_printf(m, ",acregmin=%u", nfss->acregmin/HZ);
-	if (nfss->acregmax != 60*HZ || showdefaults)
+	if (nfss->acregmax != NFS_DEF_ACREGMAX*HZ || showdefaults)
 		seq_printf(m, ",acregmax=%u", nfss->acregmax/HZ);
-	if (nfss->acdirmin != 30*HZ || showdefaults)
+	if (nfss->acdirmin != NFS_DEF_ACDIRMIN*HZ || showdefaults)
 		seq_printf(m, ",acdirmin=%u", nfss->acdirmin/HZ);
-	if (nfss->acdirmax != 60*HZ || showdefaults)
+	if (nfss->acdirmax != NFS_DEF_ACDIRMAX*HZ || showdefaults)
 		seq_printf(m, ",acdirmax=%u", nfss->acdirmax/HZ);
 	for (nfs_infop = nfs_info; nfs_infop->flag; nfs_infop++) {
 		if (nfss->flags & nfs_infop->flag)
@@ -1241,10 +1241,10 @@ static int nfs_validate_mount_data(void *options,
 	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
 	args->rsize		= NFS_MAX_FILE_IO_SIZE;
 	args->wsize		= NFS_MAX_FILE_IO_SIZE;
-	args->acregmin		= 3;
-	args->acregmax		= 60;
-	args->acdirmin		= 30;
-	args->acdirmax		= 60;
+	args->acregmin		= NFS_DEF_ACREGMIN;
+	args->acregmax		= NFS_DEF_ACREGMAX;
+	args->acdirmin		= NFS_DEF_ACDIRMIN;
+	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 */
 
@@ -1820,10 +1820,10 @@ static int nfs4_validate_mount_data(void *options,
 
 	args->rsize		= NFS_MAX_FILE_IO_SIZE;
 	args->wsize		= NFS_MAX_FILE_IO_SIZE;
-	args->acregmin		= 3;
-	args->acregmax		= 60;
-	args->acdirmin		= 30;
-	args->acdirmax		= 60;
+	args->acregmin		= NFS_DEF_ACREGMIN;
+	args->acregmax		= NFS_DEF_ACREGMAX;
+	args->acdirmin		= NFS_DEF_ACDIRMIN;
+	args->acdirmax		= NFS_DEF_ACDIRMAX;
 	args->nfs_server.port	= NFS_PORT; /* 2049 unless user set port= */
 
 	switch (data->version) {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index afc3956..3c4078e 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -20,6 +20,11 @@
 #define NFS_MAX_UDP_TIMEOUT	(60*HZ)
 #define NFS_MAX_TCP_TIMEOUT	(600*HZ)
 
+#define NFS_DEF_ACREGMIN	(3)
+#define NFS_DEF_ACREGMAX	(60)
+#define NFS_DEF_ACDIRMIN	(30)
+#define NFS_DEF_ACDIRMAX	(60)
+
 /*
  * When flushing a cluster of dirty pages, there can be different
  * strategies: