[PATCH 0/2] Two more fixes for mount.nfs

[PATCH 0/2] Two more fixes for mount.nfs

[Chuck Lever]

Here are a couple of bug fixes for mount.nfs.

The second patch in this series makes remount work again, but there is
a lingering problem with it (and maybe some other corner cases, not
related to "remount"). 

It turns out that "remount" mounts (even legacy remount, AFAICT) have
always completely wiped the mount options in /etc/mtab, and replaced
them with "remount,foo".  This breaks umount.nfs, which relies on
the contents of /etc/mtab to know how to perform the unmount request.

I'm working on a solution, but it will still be some time.  I felt
that the remount fix here was important enough to send now.

---

Chuck Lever (2):
      mount.nfs: Don't do anything fancy if this is a remount
      mount.nfs: Refactor mount version and protocol autonegotiation


 utils/mount/stropts.c |   95 ++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 81 insertions(+), 14 deletions(-)

-- 
Chuck Lever

[PATCH 1/2] mount.nfs: Refactor mount version and protocol autonegotiation

[Chuck Lever]

Clean up.

I'm beginning to agree with Bruce and Steve's assessment that the
fallthrough switch case in nfs_try_mount() is more difficult to read
and understand than it needs to be.  The logic that manages
negotiating NFS version and protocol settings is getting more complex
over time anyway.

So let's split the autonegotiation piece out of nfs_try_mount().

We can reduce indenting, and use cleaner switch-based logic.  Also,
adding more comments can only help.

Neil also suggested replacing the pre-call "errno = 0" trick.  The
lower-level functions may try to mount several times (given a list of
addresses to try).  errno could be set by any of those.  The mount
request will succeed at some point, and "success" is returned, but
errno is still set to some non-zero value.

The kernel version check in nfs_try_mount() is more or less loop
invariant: it's impossible for the result of that test to change
between retries.  So we should be able to safely move it to the logic
that sets the initial value of mi->version.

This patch is not supposed to cause a behavioral change.

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

 utils/mount/stropts.c |   67 +++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c82ddfe..c5c4ba1 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -304,6 +304,16 @@ static int nfs_set_version(struct nfsmount_info *mi)
 		mi->version = 4;
 
 	/*
+	 * Before 2.6.32, the kernel NFS client didn't
+	 * support "-t nfs vers=4" mounts, so NFS version
+	 * 4 cannot be included when autonegotiating
+	 * while running on those kernels.
+	 */
+	if (mi->version == 0 &&
+	    linux_version_code() <= MAKE_VERSION(2, 6, 31))
+		mi->version = 3;
+
+	/*
 	 * If we still don't know, check for version-specific
 	 * mount options.
 	 */
@@ -746,6 +756,47 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
 }
 
 /*
+ * Handle NFS version and transport protocol
+ * autonegotiation.
+ *
+ * When no version or protocol is specified on the
+ * command line, mount.nfs negotiates with the server
+ * to determine appropriate settings for the new
+ * mount point.
+ *
+ * Returns TRUE if successful, otherwise FALSE.
+ * "errno" is set to reflect the individual error.
+ */
+static int nfs_autonegotiate(struct nfsmount_info *mi)
+{
+	int result;
+
+	result = nfs_try_mount_v4(mi);
+	if (result)
+		return result;
+		
+	switch (errno) {
+	case EPROTONOSUPPORT:
+		/* A clear indication that the server or our
+		 * client does not support NFS version 4. */
+		goto fall_back;
+	case ENOENT:
+		/* Legacy Linux servers don't export an NFS
+		 * version 4 pseudoroot. */
+		goto fall_back;
+	case EPERM:
+		/* Linux servers prior to 2.6.25 may return
+		 * EPERM when NFS version 4 is not supported. */
+		goto fall_back;
+	default:
+		return result;
+	}
+
+fall_back:
+	return nfs_try_mount_v3v2(mi);
+}
+
+/*
  * This is a single pass through the fg/bg loop.
  *
  * Returns TRUE if successful, otherwise FALSE.
@@ -757,20 +808,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
 
 	switch (mi->version) {
 	case 0:
-		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
-			errno = 0;
-			result = nfs_try_mount_v4(mi);
-			if (errno != EPROTONOSUPPORT) {
-				/* 
-				 * To deal with legacy Linux servers that don't
-				 * automatically export a pseudo root, retry
-				 * ENOENT errors using version 3. And for
-				 * Linux servers prior to 2.6.25, retry EPERM
-				 */
-				if (errno != ENOENT && errno != EPERM)
-					break;
-			}
-		}
+		result = nfs_autonegotiate(mi);
+		break;
 	case 2:
 	case 3:
 		result = nfs_try_mount_v3v2(mi);

Re: [PATCH 1/2] mount.nfs: Refactor mount version and protocol autonegotiation

[J. Bruce Fields]

On Mon, Sep 13, 2010 at 01:17:36PM -0400, Chuck Lever wrote:
> Clean up.
> 
> I'm beginning to agree with Bruce and Steve's assessment that the
> fallthrough switch case in nfs_try_mount() is more difficult to read
> and understand than it needs to be.  The logic that manages
> negotiating NFS version and protocol settings is getting more complex
> over time anyway.
> 
> So let's split the autonegotiation piece out of nfs_try_mount().
> 
> We can reduce indenting, and use cleaner switch-based logic.  Also,
> adding more comments can only help.

Looks OK to me, thanks.--b.

> 
> Neil also suggested replacing the pre-call "errno = 0" trick.  The
> lower-level functions may try to mount several times (given a list of
> addresses to try).  errno could be set by any of those.  The mount
> request will succeed at some point, and "success" is returned, but
> errno is still set to some non-zero value.
> 
> The kernel version check in nfs_try_mount() is more or less loop
> invariant: it's impossible for the result of that test to change
> between retries.  So we should be able to safely move it to the logic
> that sets the initial value of mi->version.
> 
> This patch is not supposed to cause a behavioral change.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/stropts.c |   67 +++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index c82ddfe..c5c4ba1 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -304,6 +304,16 @@ static int nfs_set_version(struct nfsmount_info *mi)
>  		mi->version = 4;
>  
>  	/*
> +	 * Before 2.6.32, the kernel NFS client didn't
> +	 * support "-t nfs vers=4" mounts, so NFS version
> +	 * 4 cannot be included when autonegotiating
> +	 * while running on those kernels.
> +	 */
> +	if (mi->version == 0 &&
> +	    linux_version_code() <= MAKE_VERSION(2, 6, 31))
> +		mi->version = 3;
> +
> +	/*
>  	 * If we still don't know, check for version-specific
>  	 * mount options.
>  	 */
> @@ -746,6 +756,47 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
>  }
>  
>  /*
> + * Handle NFS version and transport protocol
> + * autonegotiation.
> + *
> + * When no version or protocol is specified on the
> + * command line, mount.nfs negotiates with the server
> + * to determine appropriate settings for the new
> + * mount point.
> + *
> + * Returns TRUE if successful, otherwise FALSE.
> + * "errno" is set to reflect the individual error.
> + */
> +static int nfs_autonegotiate(struct nfsmount_info *mi)
> +{
> +	int result;
> +
> +	result = nfs_try_mount_v4(mi);
> +	if (result)
> +		return result;
> +		
> +	switch (errno) {
> +	case EPROTONOSUPPORT:
> +		/* A clear indication that the server or our
> +		 * client does not support NFS version 4. */
> +		goto fall_back;
> +	case ENOENT:
> +		/* Legacy Linux servers don't export an NFS
> +		 * version 4 pseudoroot. */
> +		goto fall_back;
> +	case EPERM:
> +		/* Linux servers prior to 2.6.25 may return
> +		 * EPERM when NFS version 4 is not supported. */
> +		goto fall_back;
> +	default:
> +		return result;
> +	}
> +
> +fall_back:
> +	return nfs_try_mount_v3v2(mi);
> +}
> +
> +/*
>   * This is a single pass through the fg/bg loop.
>   *
>   * Returns TRUE if successful, otherwise FALSE.
> @@ -757,20 +808,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>  
>  	switch (mi->version) {
>  	case 0:
> -		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
> -			errno = 0;
> -			result = nfs_try_mount_v4(mi);
> -			if (errno != EPROTONOSUPPORT) {
> -				/* 
> -				 * To deal with legacy Linux servers that don't
> -				 * automatically export a pseudo root, retry
> -				 * ENOENT errors using version 3. And for
> -				 * Linux servers prior to 2.6.25, retry EPERM
> -				 */
> -				if (errno != ENOENT && errno != EPERM)
> -					break;
> -			}
> -		}
> +		result = nfs_autonegotiate(mi);
> +		break;
>  	case 2:
>  	case 3:
>  		result = nfs_try_mount_v3v2(mi);
> 
> --
> 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

[PATCH 2/2] mount.nfs: Don't do anything fancy if this is a remount

[Chuck Lever]

We don't want to append "vers=4" or perform any negotiation if the
"remount" mount option was specified.  It will just end in tears.

This attempts to address

  https://qa.mandriva.com/show_bug.cgi?id=60311

and

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=187

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

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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c5c4ba1..50a1a2a 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -967,6 +967,26 @@ static int nfsmount_bg(struct nfsmount_info *mi)
 }
 
 /*
+ * Usually all that is needed for an NFS remount is to change
+ * generic mount options like "sync" or "ro".  These generic
+ * options are controlled by mi->flags, not by text-based
+ * options, and no contact with the server is needed.
+ *
+ * Take care with the /etc/mtab entry for this mount; just
+ * calling update_mtab() will change an "-t nfs -o vers=4"
+ * mount to an "-t nfs -o remount" mount, and that will
+ * confuse umount.nfs.
+ *
+ * Returns a valid mount command exit code.
+ */
+static int nfs_remount(struct nfsmount_info *mi)
+{
+	if (nfs_sys_mount(mi, mi->options))
+		return EX_SUCCESS;
+	return EX_FAIL;
+}
+
+/*
  * Process mount options and try a mount system call.
  *
  * Returns a valid mount command exit code.
@@ -982,6 +1002,12 @@ static int nfsmount_start(struct nfsmount_info *mi)
 	if (!nfs_validate_options(mi))
 		return EX_FAIL;
 
+	/*
+	 * Avoid retry and negotiation logic when remounting
+	 */
+	if (mi->flags & MS_REMOUNT)
+		return nfs_remount(mi);
+
 	if (po_rightmost(mi->options, nfs_background_opttbl) == 0)
 		return nfsmount_bg(mi);
 	else
@@ -998,6 +1024,8 @@ static int nfsmount_start(struct nfsmount_info *mi)
  *		(input and output argument)
  * @fake: flag indicating whether to carry out the whole operation
  * @child: one if this is a mount daemon (bg)
+ *
+ * Returns a valid mount command exit code.
  */
 int nfsmount_string(const char *spec, const char *node, const char *type,
 		    int flags, char **extra_opts, int fake, int child)

Re: [PATCH 0/2] Two more fixes for mount.nfs

[Jeff Layton]

On Mon, 13 Sep 2010 13:17:25 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Here are a couple of bug fixes for mount.nfs.
> 
> The second patch in this series makes remount work again, but there is
> a lingering problem with it (and maybe some other corner cases, not
> related to "remount"). 
> 
> It turns out that "remount" mounts (even legacy remount, AFAICT) have
> always completely wiped the mount options in /etc/mtab, and replaced
> them with "remount,foo".  This breaks umount.nfs, which relies on
> the contents of /etc/mtab to know how to perform the unmount request.
> 
> I'm working on a solution, but it will still be some time.  I felt
> that the remount fix here was important enough to send now.
> 
> ---
> 
> Chuck Lever (2):
>       mount.nfs: Don't do anything fancy if this is a remount
>       mount.nfs: Refactor mount version and protocol autonegotiation
> 
> 
>  utils/mount/stropts.c |   95 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 81 insertions(+), 14 deletions(-)
> 

Looks good. Ack on the set...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] Two more fixes for mount.nfs

[Steve Dickson]

On 09/13/2010 01:17 PM, Chuck Lever wrote:
> Here are a couple of bug fixes for mount.nfs.
> 
> The second patch in this series makes remount work again, but there is
> a lingering problem with it (and maybe some other corner cases, not
> related to "remount"). 
> 
> It turns out that "remount" mounts (even legacy remount, AFAICT) have
> always completely wiped the mount options in /etc/mtab, and replaced
> them with "remount,foo".  This breaks umount.nfs, which relies on
> the contents of /etc/mtab to know how to perform the unmount request.
> 
> I'm working on a solution, but it will still be some time.  I felt
> that the remount fix here was important enough to send now.
> 
> ---
> 
> Chuck Lever (2):
>       mount.nfs: Don't do anything fancy if this is a remount
>       mount.nfs: Refactor mount version and protocol autonegotiation
> 
> 
>  utils/mount/stropts.c |   95 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 81 insertions(+), 14 deletions(-)
> 
Both Committed...

steved.

[PATCH 0/2] Improvements to mount's autonegotiation

[Chuck Lever]

Hi Neil-

While reviewing your proposed patch, a couple of ideas occurred to me
that might be helpful.  Compile-tested only.  Let me know what you
think.

---

Chuck Lever (2):
      mount.nfs: Don't try NFSv4 if UDP was requested
      mount.nfs: Refactor mount version and protocol autonegotiation


 utils/mount/stropts.c |   72 +++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 57 insertions(+), 15 deletions(-)

-- 
Chuck Lever

[PATCH 1/2] mount.nfs: Refactor mount version and protocol autonegotiation

[Chuck Lever]

The logic that manages negotiating NFS version and protocol settings
is getting more complex over time, so let's split this out of
nfs_try_mount().

This should make Bruce a little happier, as it eliminates the silent
switch case fall through in nfs_try_mount().  And it should help Neil
fix some bugs he's found in this logic.

We can reduce indenting and use a more clear switch over the errno's
as well.  Adding more comments can only help.

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

 utils/mount/stropts.c |   59 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0241400..94e75be 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -747,6 +747,49 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
 }
 
 /*
+ * Handle NFS version and transport protocol autonegotiation.
+ *
+ * When no version or protocol is specified on the command line,
+ * mount.nfs negotiates with the server to determine appropriate
+ * settings.
+ *
+ * Employ a set of wonky heuristics to work around missing server 
+ * features and work in the face of not-yet-ready servers.
+ */
+static int nfs_autonegotiate(struct nfsmount_info *mi)
+{
+	int result;
+
+	/*
+	 * Before 2.6.31, the kernel NFS client didn't support
+	 * "-t nfs vers=4" mounts, so we can't include NFSv4
+	 * when autonegotiating.
+	 */
+	if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
+		goto fall_back;
+
+	errno = 0;
+	result = nfs_try_mount_v4(mi);
+	switch (errno) {
+	case EPROTONOSUPPORT:
+		goto fall_back;
+	case ENOENT:
+		/* Legacy Linux servers don't export an NFSv4
+		 * pseudoroot. */
+		goto fall_back;
+	case EPERM:
+		/* Linux servers prior to 2.6.25 may return
+		 * EPERM when NFSv4 is not supported. */
+		goto fall_back;
+	default:
+		return result;
+	}
+
+fall_back:
+	return nfs_try_mount_v3v2(mi);
+}
+
+/*
  * This is a single pass through the fg/bg loop.
  *
  * Returns TRUE if successful, otherwise FALSE.
@@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
 
 	switch (mi->version) {
 	case 0:
-		if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
-			errno = 0;
-			result = nfs_try_mount_v4(mi);
-			if (errno != EPROTONOSUPPORT) {
-				/* 
-				 * To deal with legacy Linux servers that don't
-				 * automatically export a pseudo root, retry
-				 * ENOENT errors using version 3. And for
-				 * Linux servers prior to 2.6.25, retry EPERM
-				 */
-				if (errno != ENOENT && errno != EPERM)
-					break;
-			}
-		}
+		result = nfs_autonegotiate(mi);
+		break;
 	case 2:
 	case 3:
 		result = nfs_try_mount_v3v2(mi);