text-based mount.nfs:

Introduce a function for probing the server for what it supports, and then rewriting the mount options using the probe results.

/*
+ * Reconstruct the mount option string based on a portmapper probe
+ * of the server. Returns one if the server's portmapper returned
+ * something we can use, otherwise zero.
+ *
+ * Unfortunately, for fallback, we need to parse out the mount options
+ * in user space. Unrecognized options are ignored here.

+ *
+ * Order of processing:
+ * 1. fill in mnt_server and nfs_server, based on existing options

First 5 OK and applied. But I'm a bit uncertain about this one. I think the rest are OK as well, so once we've sorted this one, the rest can be applied.

Apart from the empty subject line :-), it doesn't look right.

In particular....

Quoted 4 lines · On Monday October 8, [email protected] wrote:

Introduce a function for probing the server for what it supports, and then
rewriting the mount options using the probe results.

Quoted 8 lines

/*
+ * Reconstruct the mount option string based on a portmapper probe
+ * of the server. Returns one if the server's portmapper returned
+ * something we can use, otherwise zero.
+ *
+ * Unfortunately, for fallback, we need to parse out the mount options
+ * in user space. Unrecognized options are ignored here.

(I that paragraph of the comment still relevant?)

Quoted 3 lines

+ *
+ * Order of processing:
+ * 1. fill in mnt_server and nfs_server, based on existing options

This statement doesn't seem to be true.
In particular, the 'version number' and 'proto' in the existing options are *not* (that I can see) filled in for nfs_server.

So it would seem that
mount -o vers=3 server:/path /dir

would fall back to v2 if 'server' didn't support v3. I don't think that is right (current mount.nfs doesn't do that).

Did I misunderstand the code?

By the by, when testing what the current code does, I discovered that if

mount server:/path /dir

falls back to v2 because the default (v3) doesn't work, then
umount /dir

fails with

umount.nfs: Server failed to unmount 'server:/path'

which is a worry.

I think there are two problem here:
1/ we should fall back to v2 umount if v3 fails (well, the MOUNT
versions are different, but you know what I mean).
2/ If the call to the server fails, we should still unmount the
filesystem (call del_mtab .... not a good choice for the name of the function that does the actual unmount...)
Thoughts?

Quoted 10 lines

Quoted 8 lines

/*
+ * Reconstruct the mount option string based on a portmapper probe
+ * of the server. Returns one if the server's portmapper returned
+ * something we can use, otherwise zero.
+ *
+ * Unfortunately, for fallback, we need to parse out the mount options
+ * in user space. Unrecognized options are ignored here.

(I that paragraph of the comment still relevant?)

I'm not sure what you think is irrelevant... the bit about unrecognized options? If so, that comment was meant to imply that as we add support for new mount options in the kernel, we needn't be too concerned about adding support for them here as well.

Or maybe you meant the "need to parse out the mount options in user space" -- that implies that, for text-based mounts, mount.nfs still needs to parse the mount options in order to do the port probe and then rewrite them. Our dream of parsing only in the kernel is dashed on the rocks.

I will see about clarifying that when resubmitting the patch.

Quoted 5 lines

By the by, when testing what the current code does, I discovered that if

mount server:/path /dir

Quoted 8 lines

falls back to v2 because the default (v3) doesn't work, then
umount /dir

fails with

umount.nfs: Server failed to unmount 'server:/path'

which is a worry.

OK, it's not supposed to do that either, and I never saw that behavior during testing. (Well, actually we did see it, but it was only with NFSv4, and is supposed to be fixed by one of the patches in the last set I sent you).

Quoted 7 lines

I think there are two problem here:
1/ we should fall back to v2 umount if v3 fails (well, the MOUNT
versions are different, but you know what I mean).
2/ If the call to the server fails, we should still unmount the
filesystem (call del_mtab .... not a good choice for the name of the function that does the actual unmount...)
Thoughts?

First I would like to understand precisely why umount is failing. In this case, mount.nfs should rewrite the mount options, then post them in /etc/mtab so that umount can use the same service endpoint when doing the unmount. The umount should then use the post-fallback options, and succeed. Does "umount -v" provide any helpful information?

Certainly it would be good to rename del_mtab() !

I also agree that it would be an overall improvement if the RPC umount request did not determine whether or not umount also tries to detach the local file system. I don't see any good reason to link the two.

Quoted 22 lines

Quoted 5 lines

By the by, when testing what the current code does, I discovered that if

mount server:/path /dir

Quoted 8 lines

falls back to v2 because the default (v3) doesn't work, then
umount /dir

fails with

umount.nfs: Server failed to unmount 'server:/path'

which is a worry.

OK, it's not supposed to do that either, and I never saw that behavior during testing. (Well, actually we did see it, but it was only with NFSv4, and is supposed to be fixed by one of the patches in the last set I sent you).

Quoted 7 lines

I think there are two problem here:
1/ we should fall back to v2 umount if v3 fails (well, the MOUNT
versions are different, but you know what I mean).
2/ If the call to the server fails, we should still unmount the
filesystem (call del_mtab .... not a good choice for the name of the function that does the actual unmount...)
Thoughts?

Quoted 2 lines

First I would like to understand precisely why umount is failing. In this case, mount.nfs should rewrite the mount options, then post them in /etc/mtab so that umount can use the same service endpoint when doing the unmount. The umount should then use the post-fallback options, and succeed. Does "umount -v" provide any helpful information?

(remembering this is with the binary mount interface, not the new ASCII one) the updated mount options are *not* written to /etc/mtab. It just has

eli:/var/tmp/samba /mnt nfs rw,addr=192.168.1.3 0 0

nfsvers=2 has not been added.
umount -v says:
umount.nfs: trying 192.168.1.3 prog 100005 vers 3 prot UDP port 723 umount.nfs: Server failed to unmount 'eli:/var/tmp/samba'

Quoted 3 lines

Certainly it would be good to rename del_mtab() !

I also agree that it would be an overall improvement if the RPC umount request did not determine whether or not umount also tries to detach the local file system. I don't see any good reason to link the two.

I've broken the link, so the unmount now succeeds. It would be good to get it to send the right request to the server though, either by recording the protocol in mtab, or using probe_mntport or similar.

Quoted 4 lines

I've broken the link, so the unmount now succeeds. It would be good to get it to send the right request to the server though, either by recording the protocol in mtab, or using probe_mntport or similar.

Yeah, that's odd. It is supposed to be probing the server's mountd in nfs_call_umount. The problem may be that it's only trying a single mount RPC (for vers=3), and not doing any kind of version fallback.

Quoted 9 lines · On Wednesday October 10, [email protected] wrote:

Quoted 4 lines

I've broken the link, so the unmount now succeeds. It would be good to get it to send the right request to the server though, either by recording the protocol in mtab, or using probe_mntport or similar.

Yeah, that's odd. It is supposed to be probing the server's mountd in
nfs_call_umount. The problem may be that it's only trying a single mount RPC (for vers=3), and not doing any kind of version fallback.

Thanks for the pointers. I've got it working, though the patch (below) is a big ugly at this stage.

Two key fixes:
1/ Don't give up probing if we get RPC_PROGVERSMISMATCH. 2/ Don't set pm_vers if it isn't set in mtab. Leave it at zero so
the probe routines get a chance to do the right thing.

An awkwardness here was we were overloading the meaning of

        pm_vers == 0

In some contexts it means "Has not been explicitly set, probe". In other contexts it means "Mountd is not used (due to NFSv4)".

I introduced '63356' for the second meaning, but would rather a cleaner approach.

Maybe we don't need the second meaning, as the 2/3 versus 4 distinction is present in mnt_type so we never call do_nfs_umount for a v4 mount. Is that true?

NeilBrown

utils/mount/network.c +1 -1

index 49d3c6b..c37d0f3 100644
@@ -85,7 +85,7 @@ unsigned long nfsvers_to_mnt(const unsigned long  
 vers)
 {
 	if (vers <= 3)
 		return nfs_to_mnt[vers];
-	return 0;
+	return 65536; /* invalid */
 }
 

/*

utils/mount/network.c +2 -1

@@ -408,7 +408,8 @@ static int probe_port(clnt_addr_t *server,  
 const unsigned long *versions,
 					goto out_bad;
 			}
 		}
-		if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED)
+		if (rpc_createerr.cf_stat != RPC_PROGNOTREGISTERED &&
+		    rpc_createerr.cf_stat != RPC_PROGVERSMISMATCH)
 			goto out_bad;
 

utils/mount/network.c +1 -0

 		if (!prot) {

@@ -589,6 +590,7 @@ int nfs_call_umount(clnt_addr_t *mnt_server,  
 dirpath *argp)
 	case 3:
 	case 2:
 	case 1:
+	case 0:
 		if (!probe_mntport(mnt_server))
 			return 0;

utils/mount/network.c +1 -1

 		clnt = mnt_openclnt(mnt_server, &msock);

@@ -602,7 +604,7 @@ int nfs_call_umount(clnt_addr_t *mnt_server,  
 dirpath *argp)
 		if (res == RPC_SUCCESS)
 			return 1;
 		break;
-	default:
+	default: /* invalid - must be NFSv4 which doesn't use MOUNT */
 		res = RPC_SUCCESS;
 		break;

utils/mount/network.c +0 -0

 	}

utils/mount/nfsumount.c +1 -1

index 9e9cd16..051592b 100644
@@ -190,7 +190,7 @@ static int do_nfs_umount(const char *spec, char  
 *opts)
 	}
 
 	pmap->pm_prog = MOUNTPROG;
-	pmap->pm_vers = MOUNTVERS_NFSV3;
+	pmap->pm_vers = 0; /* unknown */
 	if (opts && (p = strstr(opts, "mountprog=")) && isdigit(*(p+10)))
 		pmap->pm_prog = atoi(p+10);

if (opts && (p = strstr(opts, "mountport=")) && isdigit(*(p+10)))

Quoted 5 lines · Neil Brown

Two key fixes:
1/ Don't give up probing if we get RPC_PROGVERSMISMATCH. 2/ Don't set pm_vers if it isn't set in mtab. Leave it at zero so
the probe routines get a chance to do the right thing.

I think that's the right thing to do.

Quoted 7 lines

An awkwardness here was we were overloading the meaning of

        pm_vers == 0

In some contexts it means "Has not been explicitly set, probe". In other contexts it means "Mountd is not used (due to NFSv4)".

I introduced '63356' for the second meaning, but would rather a cleaner approach.

That shouldn't be necessary. Kevin and I just added a bit of logic to prevent the do_nfs_unmount() call for "nfs4" file systems (see nfs- utils commit 97fed306).

That ought to make your patch much simpler, overall.

Quoted 3 lines

Maybe we don't need the second meaning, as the 2/3 versus 4 distinction is present in mnt_type so we never call do_nfs_umount for a v4 mount. Is that true?

Yep!

Actually you could make it clear that do_nfs_unmount() is not for v4 by renaming it do_nfs_unmount23() or something like that.