Assorted mount-related nfs-utils patches.

It is completely ineffective.

Signed-off-by NeilBrown <[email protected]>

---
 utils/mountd/auth.c     |    5 +----
 utils/mountd/mountd.c   |   11 +++--------
 utils/mountd/mountd.h   |    2 +-
 utils/mountd/mountd.man |    8 --------
 4 files changed, 5 insertions(+), 21 deletions(-)

utils/mountd/auth.c +1 -4

index 894a7a53957f..0881d9a6edba 100644
@@ -36,7 +36,6 @@ enum auth_error
 };
 
 static void		auth_fixpath(char *path);
-static char	*export_file = NULL;
 static nfs_export my_exp;
 static nfs_client my_client;
 

@@ -44,10 +43,8 @@ extern int new_cache;
 extern int use_ipaddr;
 
 void
-auth_init(char *exports)
+auth_init(void)
 {
-
-	export_file = exports;
 	auth_reload();
 	xtab_mount_write();
 }

utils/mountd/mountd.c +3 -8

index 063da269f895..7a51b093f66a 100644
@@ -57,7 +57,6 @@ static struct option longopts[] =
 	{ "descriptors", 1, 0, 'o' },
 	{ "debug", 1, 0, 'd' },
 	{ "help", 0, 0, 'h' },
-	{ "exports-file", 1, 0, 'f' },
 	{ "nfs-version", 1, 0, 'V' },
 	{ "no-nfs-version", 1, 0, 'N' },
 	{ "version", 0, 0, 'v' },

@@ -689,7 +688,6 @@ get_exportlist(void)
 int
 main(int argc, char **argv)
 {
-	char	*export_file = _PATH_EXPORTS;
 	char    *state_dir = NFS_STATEDIR;
 	char	*progname;
 	unsigned int listeners = 0;

@@ -709,7 +707,7 @@ main(int argc, char **argv)
 
 	/* Parse the command line options and arguments. */
 	opterr = 0;
-	while ((c = getopt_long(argc, argv, "o:nFd:f:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
+	while ((c = getopt_long(argc, argv, "o:nFd:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
 		switch (c) {
 		case 'g':
 			manage_gids = 1;

@@ -728,9 +726,6 @@ main(int argc, char **argv)
 		case 'd':
 			xlog_sconfig(optarg, 1);
 			break;
-		case 'f':
-			export_file = optarg;
-			break;
 		case 'H': /* PRC: specify a high-availability callout program */
 			ha_callout_prog = optarg;
 			break;

@@ -862,7 +857,7 @@ main(int argc, char **argv)
 	sa.sa_handler = sig_hup;
 	sigaction(SIGHUP, &sa, NULL);
 
-	auth_init(export_file);
+	auth_init();
 
 	if (!foreground) {
 		/* We first fork off a child. */

@@ -908,7 +903,7 @@ usage(const char *prog, int n)
 {
 	fprintf(stderr,
 "Usage: %s [-F|--foreground] [-h|--help] [-v|--version] [-d kind|--debug kind]\n"
-"	[-o num|--descriptors num] [-f exports-file|--exports-file=file]\n"
+"	[-o num|--descriptors num]\n"
 "	[-p|--port port] [-V version|--nfs-version version]\n"
 "	[-N version|--no-nfs-version version] [-n|--no-tcp]\n"
 "	[-H prog |--ha-callout prog] [-r |--reverse-lookup]\n"

utils/mountd/mountd.h +1 -1

index 6d358a75d9f3..f058f01d3584 100644
@@ -39,7 +39,7 @@ bool_t		mount_pathconf_2_svc(struct svc_req *, dirpath *, ppathcnf *);
 bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
-void		auth_init(char *export_file);
+void		auth_init(void);
 unsigned int	auth_reload(void);
 nfs_export *	auth_authenticate(const char *what,
 					const struct sockaddr *caller,

utils/mountd/mountd.man +0 -9

index 66e3bba7e865..e0d1a0acba3a 100644
@@ -86,14 +86,6 @@ Turn on debugging. Valid kinds are: all, auth, call, general and parse.
 .B \-F " or " \-\-foreground
 Run in foreground (do not daemonize)
 .TP
-.B \-f export-file " or " \-\-exports-file export-file
-This option specifies the exports file, listing the clients that this
-server is prepared to serve and parameters to apply to each
-such mount (see
-.BR exports (5)).
-By default, export information is read from
-.IR /etc/exports .
-.TP
 .B \-h " or " \-\-help
 Display usage message.
 .TP
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I now think this was a mistaken idea.

If a filesystem is exported with the "mountpoint" or "mp" option, it should only be exported if the directory is a mount point. The intention is that if there is a problem with one filesystem on a server, the others can still be exported, but clients won't incorrectly see the empty directory on the parent when accessing the missing filesystem, they will see clearly that the filesystem is missing.

It is easy to handle this correctly for NFSv3 MOUNT requests, but what is the correct behavior if a client already has the filesystem mounted and so has a filehandle? Maybe the server rebooted and came back with one device missing. What should the client see?

The "dev_missing" code tries to detect this case and causes the server to respond with silence rather than ESTALE. The idea was that the client would retry and when (or if) the filesystem came back, service would be transparently restored.

The problem with this is that arbitrarily long delays are not what people would expect, and can be quite annoying. ESTALE, while unpleasant, it at least easily understood. A device disappearing is a fairly significant event and hiding it doesn't really serve anyone.

So: remove the code and allow ESTALE.

Signed-off-by NeilBrown <[email protected]>

---
 utils/mountd/cache.c |   12 ------------
 1 file changed, 12 deletions(-)

utils/mountd/cache.c +0 -13

index ec86a22613cf..346a8b3af8b5 100644
@@ -687,7 +687,6 @@ static void nfsd_fh(int f)
 	char *found_path = NULL;
 	nfs_export *exp;
 	int i;
-	int dev_missing = 0;
 	char buf[RPC_CHAN_BUF_SIZE], *bp;
 	int blen;
 

@@ -754,11 +753,6 @@ static void nfsd_fh(int f)
 			if (!is_ipaddr_client(dom)
 					&& !namelist_client_matches(exp, dom))
 				continue;
-			if (exp->m_export.e_mountpoint &&
-			    !is_mountpoint(exp->m_export.e_mountpoint[0]?
-					   exp->m_export.e_mountpoint:
-					   exp->m_export.e_path))
-				dev_missing ++;
 
 			if (!match_fsid(&parsed, exp, path))
 				continue;

@@ -801,12 +795,6 @@ static void nfsd_fh(int f)
 		/* FIXME we need to make sure we re-visit this later */
 		goto out;
 	}
-	if (!found && dev_missing) {
-		/* The missing dev could be what we want, so just be
-		 * quite rather than returning stale yet
-		 */
-		goto out;
-	}
 
 	if (found)
 		if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 20 lines · NeilBrown

I now think this was a mistaken idea.

If a filesystem is exported with the "mountpoint" or "mp" option, it should only be exported if the directory is a mount point. The intention is that if there is a problem with one filesystem on a server, the others can still be exported, but clients won't incorrectly see the empty directory on the parent when accessing the missing filesystem, they will see clearly that the filesystem is missing.

It is easy to handle this correctly for NFSv3 MOUNT requests, but what is the correct behavior if a client already has the filesystem mounted and so has a filehandle? Maybe the server rebooted and came back with one device missing. What should the client see?

The "dev_missing" code tries to detect this case and causes the server to respond with silence rather than ESTALE. The idea was that the client would retry and when (or if) the filesystem came back, service would be transparently restored.

Quoted 4 lines

The problem with this is that arbitrarily long delays are not what people would expect, and can be quite annoying. ESTALE, while unpleasant, it at least easily understood. A device disappearing is a fairly significant event and hiding it doesn't really serve anyone.

It could also be a filesystem disappearing because it failed to mount in time on a reboot.

Quoted 2 lines

So: remove the code and allow ESTALE.

I'm not completely sure I understand the justification.

I don't like the current behavior either--I'd be happy if we could deprecate "mountpoint" entirely--but changing it now would seem to risk regressions if anyone depends on it.

Quoted 26 lines · J. Bruce Fields

Quoted 24 lines · NeilBrown

I now think this was a mistaken idea.

If a filesystem is exported with the "mountpoint" or "mp" option, it should only be exported if the directory is a mount point. The intention is that if there is a problem with one filesystem on a server, the others can still be exported, but clients won't incorrectly see the empty directory on the parent when accessing the missing filesystem, they will see clearly that the filesystem is missing.

It is easy to handle this correctly for NFSv3 MOUNT requests, but what is the correct behavior if a client already has the filesystem mounted and so has a filehandle? Maybe the server rebooted and came back with one device missing. What should the client see?

The "dev_missing" code tries to detect this case and causes the server to respond with silence rather than ESTALE. The idea was that the client would retry and when (or if) the filesystem came back, service would be transparently restored.

The problem with this is that arbitrarily long delays are not what people would expect, and can be quite annoying. ESTALE, while unpleasant, it at least easily understood. A device disappearing is a fairly significant event and hiding it doesn't really serve anyone.

Quoted 2 lines

It could also be a filesystem disappearing because it failed to mount in time on a reboot.

I don't think "in time" is really an issue. Boot sequencing should not start nfsd until everything in /etc/fstab is mounted, has failed and the failure has been deemed acceptable.
That is why nfs-server.services has "After= local-fs.target"

Quoted 31 lines · NeilBrown

Quoted 28 lines · J. Bruce Fields

Quoted 20 lines · NeilBrown

I now think this was a mistaken idea.

If a filesystem is exported with the "mountpoint" or "mp" option, it should only be exported if the directory is a mount point. The intention is that if there is a problem with one filesystem on a server, the others can still be exported, but clients won't incorrectly see the empty directory on the parent when accessing the missing filesystem, they will see clearly that the filesystem is missing.

It is easy to handle this correctly for NFSv3 MOUNT requests, but what is the correct behavior if a client already has the filesystem mounted and so has a filehandle? Maybe the server rebooted and came back with one device missing. What should the client see?

The "dev_missing" code tries to detect this case and causes the server to respond with silence rather than ESTALE. The idea was that the client would retry and when (or if) the filesystem came back, service would be transparently restored.

Quoted 4 lines

The problem with this is that arbitrarily long delays are not what people would expect, and can be quite annoying. ESTALE, while unpleasant, it at least easily understood. A device disappearing is a fairly significant event and hiding it doesn't really serve anyone.

It could also be a filesystem disappearing because it failed to mount in time on a reboot.

Quoted 4 lines

I don't think "in time" is really an issue. Boot sequencing should not start nfsd until everything in /etc/fstab is mounted, has failed and the failure has been deemed acceptable.
That is why nfs-server.services has "After= local-fs.target"

Yeah, I agree, that's the right way to do it. [snip]

Quoted 37 lines · J. Bruce Fields

Quoted 35 lines · NeilBrown

Quoted 21 lines · J. Bruce Fields

Quoted 20 lines · NeilBrown

I now think this was a mistaken idea.

If a filesystem is exported with the "mountpoint" or "mp" option, it should only be exported if the directory is a mount point. The intention is that if there is a problem with one filesystem on a server, the others can still be exported, but clients won't incorrectly see the empty directory on the parent when accessing the missing filesystem, they will see clearly that the filesystem is missing.

It is easy to handle this correctly for NFSv3 MOUNT requests, but what is the correct behavior if a client already has the filesystem mounted and so has a filehandle? Maybe the server rebooted and came back with one device missing. What should the client see?

The "dev_missing" code tries to detect this case and causes the server to respond with silence rather than ESTALE. The idea was that the client would retry and when (or if) the filesystem came back, service would be transparently restored.

Quoted 7 lines

Quoted 4 lines

The problem with this is that arbitrarily long delays are not what people would expect, and can be quite annoying. ESTALE, while unpleasant, it at least easily understood. A device disappearing is a fairly significant event and hiding it doesn't really serve anyone.

It could also be a filesystem disappearing because it failed to mount in time on a reboot.

I don't think "in time" is really an issue. Boot sequencing should not start nfsd until everything in /etc/fstab is mounted, has failed and the failure has been deemed acceptable.
That is why nfs-server.services has "After= local-fs.target"

Quoted 1 line

Yeah, I agree, that's the right way to do it. [snip]

There is actually more here ... don't you love getting drip-feed symptoms and requirements :-)

It turns out the the customer is NFS-exporting a filesystem mounted using iSCSI. Such filesystems are treated by systemd as "network" filesystem, which seems at least a little bit reasonable. So it is "remote-fs" that applies, or more accurately "remote-fs-pre.target"
And nfs-server.services contains:

Before=remote-fs-pre.target

So nfsd is likely to start up before the iSCSI filesystems are mounted.

The customer tried to stop this bt using a systemd drop-in to add RequiresMountsFor= for the remote filesystem, but that causes a loop with the Before=remote-fs-pre.target.

I don't think we need this line for sequencing start-up, but it does seem to be useful for sequencing shutdown - so that nfs-server is stopped after remote-fs-pre, which is stopped after things are unmounted.
"useful", but not "right". This doesn't stop remote servers from shutting down in the wrong order.
We should probably remove this line and teach systemd to use "umount -f" which doesn't block when the server is down. If systemd just used a script, that would be easy....

I'm not 100% certain that "umount -f" is correct. We just want to stop umount from stating the mountpoint, we don't need to send MNT_FORCE. I sometimes think it would be really nice if NFS didn't block a 'getattr' request of the mountpoint. That would remove some pain from unmount and other places where the server was down, but probably would cause other problem.

Does anyone have any opinions on the best way to make sure systemd doesn't hang when it tries to umount a filesystem from an unresponsive server? Is "-f" best, or something else.

There is another issue related to this that I've been meaning to mention. It related to the start-up ordering rather than shut down.

When you try to mount an NFS filesystem and the server isn't responding, mount.nfs retries for a little while and then - if "bg" is given - it forks and retries a bit longer.
While it keeps gets failures that appear temporary, like ECONNREFUSED or ETIMEDOUT (see nfs_is_permanent_error()) it keeps retrying.

There is typically a window between when rpcbind starts responding to queries, and when nfsd has registered with it. If mount.nfs sends an rpcbind query in this window. It gets RPC_PROGNOTREGISTERED which nfs_rewrite_pmap_mount_options maps to EOPNOTSUPP, and nfs_is_permanent_error() thinks that is a permanent error.

Strangely, when the 'bg' option is used, there is special-case code
Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.") to stop EOPNOTSUPP from being a permanent error.

Do people think it would be reasonable to make it a transient error for foreground mounts too?

Thanks,
NeilBrown

Quoted 9 lines

It turns out the the customer is NFS-exporting a filesystem mounted using iSCSI. Such filesystems are treated by systemd as "network" filesystem, which seems at least a little bit reasonable. So it is "remote-fs" that applies, or more accurately "remote-fs-pre.target"
And nfs-server.services contains:

Before=remote-fs-pre.target

This is to handle the loopback case?

In which case what it really wants to say is "before nfs mounts" (or even "before nfs mounts of localhost"; and vice versa on shutdown). I can't tell if there's an easy way to get say that.

Quoted 17 lines

So nfsd is likely to start up before the iSCSI filesystems are mounted.

The customer tried to stop this bt using a systemd drop-in to add RequiresMountsFor= for the remote filesystem, but that causes a loop with the Before=remote-fs-pre.target.

I don't think we need this line for sequencing start-up, but it does seem to be useful for sequencing shutdown - so that nfs-server is stopped after remote-fs-pre, which is stopped after things are unmounted.
"useful", but not "right". This doesn't stop remote servers from shutting down in the wrong order.
We should probably remove this line and teach systemd to use "umount -f" which doesn't block when the server is down. If systemd just used a script, that would be easy....

Quoted 6 lines

I'm not 100% certain that "umount -f" is correct. We just want to stop umount from stating the mountpoint, we don't need to send MNT_FORCE. I sometimes think it would be really nice if NFS didn't block a 'getattr' request of the mountpoint. That would remove some pain from unmount and other places where the server was down, but probably would cause other problem.

I thought I remembered Jeff Layton doing some work to remove the need to revalidate the mountpoint on unmount in recent kernels.

Is that the only risk, though? Maybe so--presumably you've killed any users, so any write data associated with opens should be flushed. And if you do a sync after that you take care of write delegations too.

Quoted 16 lines

Does anyone have any opinions on the best way to make sure systemd doesn't hang when it tries to umount a filesystem from an unresponsive server? Is "-f" best, or something else.

There is another issue related to this that I've been meaning to mention. It related to the start-up ordering rather than shut down.

When you try to mount an NFS filesystem and the server isn't responding, mount.nfs retries for a little while and then - if "bg" is given - it forks and retries a bit longer.
While it keeps gets failures that appear temporary, like ECONNREFUSED or ETIMEDOUT (see nfs_is_permanent_error()) it keeps retrying.

Quoted 5 lines

There is typically a window between when rpcbind starts responding to queries, and when nfsd has registered with it. If mount.nfs sends an rpcbind query in this window. It gets RPC_PROGNOTREGISTERED which nfs_rewrite_pmap_mount_options maps to EOPNOTSUPP, and nfs_is_permanent_error() thinks that is a permanent error.

Looking at rpcbind(8).... Shouldn't "-w" prevent this by loading some registrations before it starts responding to requests?

--b.

Quoted 10 lines

Strangely, when the 'bg' option is used, there is special-case code
Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.") to stop EOPNOTSUPP from being a permanent error.

Do people think it would be reasonable to make it a transient error for foreground mounts too?

Thanks,
NeilBrown

Quoted 11 lines · J. Bruce Fields

Quoted 11 lines

There is another issue related to this that I've been meaning to mention. It related to the start-up ordering rather than shut down.

When you try to mount an NFS filesystem and the server isn't responding, mount.nfs retries for a little while and then - if "bg" is given - it forks and retries a bit longer.
While it keeps gets failures that appear temporary, like ECONNREFUSED or ETIMEDOUT (see nfs_is_permanent_error()) it keeps retrying.

Quoted 8 lines

Quoted 5 lines

There is typically a window between when rpcbind starts responding to queries, and when nfsd has registered with it. If mount.nfs sends an rpcbind query in this window. It gets RPC_PROGNOTREGISTERED which nfs_rewrite_pmap_mount_options maps to EOPNOTSUPP, and nfs_is_permanent_error() thinks that is a permanent error.

Looking at rpcbind(8).... Shouldn't "-w" prevent this by loading some registrations before it starts responding to requests?

"-w" (which isn't listed in the SYNOPSIS!) only applies to a warm-start where the daemons which previously registered are still running. The problem case is that the daemons haven't registered yet (so we don't necessarily know what port number they will get).

To address the issue in rpcbind, we would need a flag to say "don't respond to lookup requests, just accept registrations", then when all registrations are complete, send some message to rpcbind to say "OK, respond to lookups now". That could even be done by killing and restarting with "-w", though that it a bit ugly.

I'm leaning towards having mount retry after RPC_PROGNOTREGISTERED for fg like it does with bg.

Quoted 28 lines · NeilBrown

Quoted 11 lines · J. Bruce Fields

Quoted 11 lines

There is another issue related to this that I've been meaning to mention. It related to the start-up ordering rather than shut down.

When you try to mount an NFS filesystem and the server isn't responding, mount.nfs retries for a little while and then - if "bg" is given - it forks and retries a bit longer.
While it keeps gets failures that appear temporary, like ECONNREFUSED or ETIMEDOUT (see nfs_is_permanent_error()) it keeps retrying.

Quoted 8 lines

Quoted 5 lines

There is typically a window between when rpcbind starts responding to queries, and when nfsd has registered with it. If mount.nfs sends an rpcbind query in this window. It gets RPC_PROGNOTREGISTERED which nfs_rewrite_pmap_mount_options maps to EOPNOTSUPP, and nfs_is_permanent_error() thinks that is a permanent error.

Looking at rpcbind(8).... Shouldn't "-w" prevent this by loading some registrations before it starts responding to requests?

"-w" (which isn't listed in the SYNOPSIS!) only applies to a warm-start where the daemons which previously registered are still running. The problem case is that the daemons haven't registered yet (so we don't necessarily know what port number they will get).

Quoted 5 lines

To address the issue in rpcbind, we would need a flag to say "don't respond to lookup requests, just accept registrations", then when all registrations are complete, send some message to rpcbind to say "OK, respond to lookups now". That could even be done by killing and restarting with "-w", though that it a bit ugly.

An alternative would be to create a temporary firewall rule that blocked port 111 to remote connections. Once local RPC services had registered, the rule is removed.

Just a thought.

Quoted 4 lines

I'm leaning towards having mount retry after RPC_PROGNOTREGISTERED for fg like it does with bg.

It probably should do that. If rpcbind is up, then the other services are probably on their way.

--
Chuck Lever

Quoted 3 lines · J. Bruce Fields

In which case what it really wants to say is "before nfs mounts" (or even "before nfs mounts of localhost"; and vice versa on shutdown). I can't tell if there's an easy way to get say that.

I'd be happy with a difficult/complex way, if it was reliable. Could we write a systemd generator which parses /etc/fstab, determines all mount points which a loop-back NFS mounts (or even just any NFS mounts) and creates a drop-in for nfs-server which adds
Before=mount-point.mount
for each /mount/point.

Could that be reliable? I might try.

Quoted 3 lines

Is that the only risk, though? Maybe so--presumably you've killed any users, so any write data associated with opens should be flushed. And if you do a sync after that you take care of write delegations too.

In the easily reproducible case, all user processes are gone. It would be worth checking what happens if processes are accessing a filesystem from an unreachable server at shutdown.
"kill -9" should get rid of them all now, so it might be OK. "sync" would hang though. I'd be happy for that to cause a delay of a minute or so, but hopefully systemd would (or could be told to) kill -9 a sync if it took too long.

Quoted 2 lines

Looking at rpcbind(8).... Shouldn't "-w" prevent this by loading some registrations before it starts responding to requests?

"-w" (which isn't listed in the SYNOPSIS!) only applies to a warm-start where the daemons which previously registered are still running. The problem case is that the daemons haven't registered yet (so we don't necessarily know what port number they will get).

To address the issue in rpcbind, we would need a flag to say "don't respond to lookup requests, just accept registrations", then when all registrations are complete, send some message to rpcbind to say "OK, respond to lookups now". That could even be done by killing and restarting with "-w", though that it a bit ugly.

I'm leaning towards having mount retry after RPC_PROGNOTREGISTERED for fg like it does with bg.

Not really arguing--I'll trust your judgement--just some random ideas:

Quoted 12 lines · NeilBrown

Quoted 3 lines · J. Bruce Fields

In which case what it really wants to say is "before nfs mounts" (or even "before nfs mounts of localhost"; and vice versa on shutdown). I can't tell if there's an easy way to get say that.

I'd be happy with a difficult/complex way, if it was reliable. Could we write a systemd generator which parses /etc/fstab, determines all mount points which a loop-back NFS mounts (or even just any NFS mounts) and creates a drop-in for nfs-server which adds
Before=mount-point.mount
for each /mount/point.

Quoted 1 line

Could that be reliable? I might try.

Digging around... we've also got this callout from mount to start-statd, can we use something like that to make loopback nfs mounts wait on nfs server startup?

Quoted 5 lines

Quoted 4 lines

Is that the only risk, though? Maybe so--presumably you've killed any users, so any write data associated with opens should be flushed. And if you do a sync after that you take care of write delegations too.

Quoted 7 lines

In the easily reproducible case, all user processes are gone. It would be worth checking what happens if processes are accessing a filesystem from an unreachable server at shutdown.
"kill -9" should get rid of them all now, so it might be OK. "sync" would hang though. I'd be happy for that to cause a delay of a minute or so, but hopefully systemd would (or could be told to) kill -9 a sync if it took too long.

We shouldn't have to resort to that in the loopback nfs case, where we control ordering. So in that case, I'm just pointing out that:

kill -9 all users of the filesystem
shutdown nfs server
umount nfs filesystems

isn't the right ordering, because in the presence of write delegations there could still be writeback data.

(OK, actually, knfsd doesn't currently implement write delegations--but we shouldn't depend on that assumption.)

Adding a sync between the first two steps might help, though the write delegations themselves could still linger, and I don't know how the client will behave when it finds it can't return them.

So it'd be nice if we could just order the umount before the server shutdown.

The case of a remote server shut down too early is different of course.

Quoted 7 lines

Quoted 2 lines

Looking at rpcbind(8).... Shouldn't "-w" prevent this by loading some registrations before it starts responding to requests?

"-w" (which isn't listed in the SYNOPSIS!) only applies to a warm-start where the daemons which previously registered are still running. The problem case is that the daemons haven't registered yet (so we don't necessarily know what port number they will get).

We probably know the port in the specific case of nfsd, and could fake up rpcbind's state file if necessary. Eh, your idea's not as bad:

Quoted 6 lines

To address the issue in rpcbind, we would need a flag to say "don't respond to lookup requests, just accept registrations", then when all registrations are complete, send some message to rpcbind to say "OK, respond to lookups now". That could even be done by killing and restarting with "-w", though that it a bit ugly.

Quoted 2 lines

I'm leaning towards having mount retry after RPC_PROGNOTREGISTERED for fg like it does with bg.

Anyway, sounds OK to me.

Quoted 17 lines · J. Bruce Fields

Not really arguing--I'll trust your judgement--just some random ideas:

Quoted 13 lines · NeilBrown

Quoted 3 lines · J. Bruce Fields

In which case what it really wants to say is "before nfs mounts" (or even "before nfs mounts of localhost"; and vice versa on shutdown). I can't tell if there's an easy way to get say that.

I'd be happy with a difficult/complex way, if it was reliable. Could we write a systemd generator which parses /etc/fstab, determines all mount points which a loop-back NFS mounts (or even just any NFS mounts) and creates a drop-in for nfs-server which adds
Before=mount-point.mount
for each /mount/point.

Could that be reliable? I might try.

Quoted 3 lines

Digging around... we've also got this callout from mount to start-statd, can we use something like that to make loopback nfs mounts wait on nfs server startup?

An nfs mount already waits for the server to start up. The ordering dependency between NFS mounts and the nfs-server only really matters at shutdown, and we cannot enhance mount.nfs to wait for a negative amount of time (also known as "time travel")

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Signed-off-by NeilBrown <[email protected]>

---
 support/include/v4root.h |    2 +-
 utils/mountd/auth.c      |   29 +++++++++++++++++++++++++++--
 utils/mountd/v4root.c    |   11 ++++++++++-
 3 files changed, 38 insertions(+), 4 deletions(-)

support/include/v4root.h +1 -1

index 706c15c70d95..406fd4e43e5a 100644
@@ -10,6 +10,6 @@
 #define V4ROOT_H
 
 extern int v4root_needed;
-extern void v4root_set(void);
+extern void v4root_set(int *mountpoints_checked);
 
 #endif /* V4ROOT_H */

utils/mountd/auth.c +27 -2

index 0881d9a6edba..5bd7e6622307 100644
@@ -77,6 +77,29 @@ check_useipaddr(void)
 		cache_flush(1);
 }
 
+static int mountpoints_changed(void)
+{
+	static int last_size = 0;
+	int size;
+	int fd;
+	char buf[4096];
+	int n;
+
+	fd = open("/proc/mounts", O_RDONLY);
+	if (fd < 0)
+		/* ignore mountpoint changes if we cannot read /proc/mounts */
+		return 0;
+	size = 0;
+	while ((n = read(fd, buf, sizeof(buf))) > 0)
+		size += n;
+	if (n < 0)
+		return 0;
+	if (size == last_size)
+		return 0;
+	last_size = size;
+	return 1;
+}
+
 unsigned int
 auth_reload()
 {

@@ -84,6 +107,7 @@ auth_reload()
 	static ino_t		last_inode;
 	static int		last_fd = -1;
 	static unsigned int	counter;
+	static int		mountpoints_checked = 0;
 	int			fd;
 
 	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {

@@ -91,7 +115,8 @@ auth_reload()
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
 		close(fd);
-	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+	} else if (last_fd != -1 && stb.st_ino == last_inode &&
+		   (!mountpoints_checked || !mountpoints_changed())) {
 		/* We opened the etab file before, and its inode
 		 * number hasn't changed since then.
 		 */

@@ -114,7 +139,7 @@ auth_reload()
 	memset(&my_client, 0, sizeof(my_client));
 	xtab_export_read();
 	check_useipaddr();
-	v4root_set();
+	v4root_set(&mountpoints_checked);
 
 	++counter;
 

utils/mountd/v4root.c +10 -2

index d52172592823..1a5778f9c7de 100644
@@ -183,7 +183,7 @@ static int v4root_add_parents(nfs_export *exp)
  * looking for components of the v4 mount.
  */
 void
-v4root_set()
+v4root_set(int *mountpoints_checked)
 {
 	nfs_export	*exp;
 	int	i;

@@ -193,6 +193,7 @@ v4root_set()
 	if (!v4root_support())
 		return;
 
+	*mountpoints_checked = 0;
 	for (i = 0; i < MCL_MAXTYPES; i++) {
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (exp->m_export.e_flags & NFSEXP_V4ROOT)

@@ -202,6 +203,14 @@ v4root_set()
 				 */
 				continue;
 
+			if (exp->m_export.e_mountpoint) {
+				*mountpoints_checked = 1;
+				if (!is_mountpoint(exp->m_export.e_mountpoint[0]?
+						   exp->m_export.e_mountpoint:
+						   exp->m_export.e_path))
+					continue;
+			}
+
 			if (strcmp(exp->m_export.e_path, "/") == 0 &&
 			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
 				/* Force '/' to be exported as fsid == 0*/
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · NeilBrown

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Surely there's some more reliable measurement--could we track some data about the mountpoint itself, maybe?

But I'd still like some more justification for this change in logic. Does anyone currently use the "mp" option?

If not, could we just
deprecate it? If so, can we really get away with changing it this way?

--b.

Quoted 119 lines

Signed-off-by NeilBrown <[email protected]>

---

support/include/v4root.h |    2 +-
utils/mountd/auth.c      |   29 +++++++++++++++++++++++++++--
utils/mountd/v4root.c    |   11 ++++++++++-
3 files changed, 38 insertions(+), 4 deletions(-)

support/include/v4root.h +1 -1

index 706c15c70d95..406fd4e43e5a 100644
@@ -10,6 +10,6 @@
 #define V4ROOT_H
 
 extern int v4root_needed;
-extern void v4root_set(void);
+extern void v4root_set(int *mountpoints_checked);
 
 #endif /* V4ROOT_H */

utils/mountd/auth.c +23 -0

index 0881d9a6edba..5bd7e6622307 100644
@@ -77,6 +77,29 @@ check_useipaddr(void)
 		cache_flush(1);
 }
 
+static int mountpoints_changed(void)
+{
+	static int last_size = 0;
+	int size;
+	int fd;
+	char buf[4096];
+	int n;
+
+	fd = open("/proc/mounts", O_RDONLY);
+	if (fd < 0)
+		/* ignore mountpoint changes if we cannot read /proc/mounts */
+		return 0;
+	size = 0;
+	while ((n = read(fd, buf, sizeof(buf))) > 0)
+		size += n;
+	if (n < 0)
+		return 0;
+	if (size == last_size)
+		return 0;
+	last_size = size;
+	return 1;
+}
+

unsigned int
auth_reload()
{

utils/mountd/auth.c +4 -2

@@ -84,6 +107,7 @@ auth_reload()
 	static ino_t		last_inode;
 	static int		last_fd = -1;
 	static unsigned int	counter;
+	static int		mountpoints_checked = 0;
 	int			fd;
 
 	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {

@@ -91,7 +115,8 @@ auth_reload()
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
 		close(fd);
-	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+	} else if (last_fd != -1 && stb.st_ino == last_inode &&
+		   (!mountpoints_checked || !mountpoints_changed())) {
 		/* We opened the etab file before, and its inode
 		 * number hasn't changed since then.
 		 */

@@ -114,7 +139,7 @@ auth_reload()
 	memset(&my_client, 0, sizeof(my_client));
 	xtab_export_read();
 	check_useipaddr();
-	v4root_set();
+	v4root_set(&mountpoints_checked);
 
 	++counter;
 

utils/mountd/v4root.c +0 -0

index d52172592823..1a5778f9c7de 100644
@@ -183,7 +183,7 @@ static int v4root_add_parents(nfs_export *exp)
 * looking for components of the v4 mount.
 */

void

utils/mountd/v4root.c +1 -1

-v4root_set()
+v4root_set(int *mountpoints_checked)

{
	nfs_export	*exp;
	int	i;

utils/mountd/v4root.c +9 -0

@@ -193,6 +193,7 @@ v4root_set()
 	if (!v4root_support())
 		return;
 
+	*mountpoints_checked = 0;
 	for (i = 0; i < MCL_MAXTYPES; i++) {
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (exp->m_export.e_flags & NFSEXP_V4ROOT)

@@ -202,6 +203,14 @@ v4root_set()
 				 */
 				continue;
 
+			if (exp->m_export.e_mountpoint) {
+				*mountpoints_checked = 1;
+				if (!is_mountpoint(exp->m_export.e_mountpoint[0]?
+						   exp->m_export.e_mountpoint:
+						   exp->m_export.e_path))
+					continue;
+			}
+
 			if (strcmp(exp->m_export.e_path, "/") == 0 &&
 			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
 				/* Force '/' to be exported as fsid == 0*/

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · NeilBrown

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

Quoted 4 lines

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Surely there's some more reliable measurement--could we track some data about the mountpoint itself, maybe?

But I'd still like some more justification for this change in logic. Does anyone currently use the "mp" option? If not, could we just deprecate it? If so, can we really get away with changing it this way?

--b.

Quoted 123 lines

Signed-off-by NeilBrown <[email protected]>

---
 support/include/v4root.h |    2 +-
 utils/mountd/auth.c      |   29 +++++++++++++++++++++++++++--
 utils/mountd/v4root.c    |   11 ++++++++++-
 3 files changed, 38 insertions(+), 4 deletions(-)

support/include/v4root.h +1 -1

index 706c15c70d95..406fd4e43e5a 100644
@@ -10,6 +10,6 @@
 #define V4ROOT_H
 
 extern int v4root_needed;
-extern void v4root_set(void);
+extern void v4root_set(int *mountpoints_checked);
 
 #endif /* V4ROOT_H */

utils/mountd/auth.c +27 -2

index 0881d9a6edba..5bd7e6622307 100644
@@ -77,6 +77,29 @@ check_useipaddr(void)
 		cache_flush(1);
 }
 
+static int mountpoints_changed(void)
+{
+	static int last_size = 0;
+	int size;
+	int fd;
+	char buf[4096];
+	int n;
+
+	fd = open("/proc/mounts", O_RDONLY);
+	if (fd < 0)
+		/* ignore mountpoint changes if we cannot read /proc/mounts */
+		return 0;
+	size = 0;
+	while ((n = read(fd, buf, sizeof(buf))) > 0)
+		size += n;
+	if (n < 0)
+		return 0;
+	if (size == last_size)
+		return 0;
+	last_size = size;
+	return 1;
+}
+
 unsigned int
 auth_reload()
 {

@@ -84,6 +107,7 @@ auth_reload()
 	static ino_t		last_inode;
 	static int		last_fd = -1;
 	static unsigned int	counter;
+	static int		mountpoints_checked = 0;
 	int			fd;
 
 	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {

@@ -91,7 +115,8 @@ auth_reload()
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
 		close(fd);
-	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+	} else if (last_fd != -1 && stb.st_ino == last_inode &&
+		   (!mountpoints_checked || !mountpoints_changed())) {
 		/* We opened the etab file before, and its inode
 		 * number hasn't changed since then.
 		 */

@@ -114,7 +139,7 @@ auth_reload()
 	memset(&my_client, 0, sizeof(my_client));
 	xtab_export_read();
 	check_useipaddr();
-	v4root_set();
+	v4root_set(&mountpoints_checked);
 
 	++counter;
 

utils/mountd/v4root.c +10 -2

index d52172592823..1a5778f9c7de 100644
@@ -183,7 +183,7 @@ static int v4root_add_parents(nfs_export *exp)
  * looking for components of the v4 mount.
  */
 void
-v4root_set()
+v4root_set(int *mountpoints_checked)
 {
 	nfs_export	*exp;
 	int	i;

@@ -193,6 +193,7 @@ v4root_set()
 	if (!v4root_support())
 		return;
 
+	*mountpoints_checked = 0;
 	for (i = 0; i < MCL_MAXTYPES; i++) {
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (exp->m_export.e_flags & NFSEXP_V4ROOT)

@@ -202,6 +203,14 @@ v4root_set()
 				 */
 				continue;
 
+			if (exp->m_export.e_mountpoint) {
+				*mountpoints_checked = 1;
+				if (!is_mountpoint(exp->m_export.e_mountpoint[0]?
+						   exp->m_export.e_mountpoint:
+						   exp->m_export.e_path))
+					continue;
+			}
+
 			if (strcmp(exp->m_export.e_path, "/") == 0 &&
 			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
 				/* Force '/' to be exported as fsid == 0*/
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · J. Bruce Fields

Quoted 5 lines · NeilBrown

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

Quoted 7 lines

Quoted 4 lines

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Surely there's some more reliable measurement--could we track some data about the mountpoint itself, maybe?

We could. But it would be more complex code for very little gain. I did consider using select() on /proc/mounts to get a notification whenever anything changes. What we be more reliable but more difficult. I also considered calculating an SHA1, or maybe just a crc32 on the contents of /proc/mounts. But then I realised that the size was very easy and very nearly as reliable.

Quoted 3 lines

But I'd still like some more justification for this change in logic. Does anyone currently use the "mp" option? If not, could we just deprecate it? If so, can we really get away with changing it this way?

I have a customer complaining that it doesn't work as advertised for NFSv4. So presumably they have a use-case, though I haven't asked for details on exactly why they want it.

I actually think this is the most useful of the changes. It means that if a filesystem isn't mounted, it isn't even visible over NFSv4.

After all, the reality is that people export filesystems, not names in their namespace. NFSv4 tries to make it all look like the same thing, and there is some justification for that. But I think a lot of people think about it as filesystems being exported, and the mountpoint option allows that thought to be expressed in the configuration.

NeilBrown

Quoted 126 lines

--b.

Quoted 123 lines

Signed-off-by NeilBrown <[email protected]>

---
 support/include/v4root.h |    2 +-
 utils/mountd/auth.c      |   29 +++++++++++++++++++++++++++--
 utils/mountd/v4root.c    |   11 ++++++++++-
 3 files changed, 38 insertions(+), 4 deletions(-)

support/include/v4root.h +1 -1

index 706c15c70d95..406fd4e43e5a 100644
@@ -10,6 +10,6 @@
 #define V4ROOT_H
 
 extern int v4root_needed;
-extern void v4root_set(void);
+extern void v4root_set(int *mountpoints_checked);
 
 #endif /* V4ROOT_H */

utils/mountd/auth.c +27 -2

index 0881d9a6edba..5bd7e6622307 100644
@@ -77,6 +77,29 @@ check_useipaddr(void)
 		cache_flush(1);
 }
 
+static int mountpoints_changed(void)
+{
+	static int last_size = 0;
+	int size;
+	int fd;
+	char buf[4096];
+	int n;
+
+	fd = open("/proc/mounts", O_RDONLY);
+	if (fd < 0)
+		/* ignore mountpoint changes if we cannot read /proc/mounts */
+		return 0;
+	size = 0;
+	while ((n = read(fd, buf, sizeof(buf))) > 0)
+		size += n;
+	if (n < 0)
+		return 0;
+	if (size == last_size)
+		return 0;
+	last_size = size;
+	return 1;
+}
+
 unsigned int
 auth_reload()
 {

@@ -84,6 +107,7 @@ auth_reload()
 	static ino_t		last_inode;
 	static int		last_fd = -1;
 	static unsigned int	counter;
+	static int		mountpoints_checked = 0;
 	int			fd;
 
 	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {

@@ -91,7 +115,8 @@ auth_reload()
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
 		close(fd);
-	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+	} else if (last_fd != -1 && stb.st_ino == last_inode &&
+		   (!mountpoints_checked || !mountpoints_changed())) {
 		/* We opened the etab file before, and its inode
 		 * number hasn't changed since then.
 		 */

@@ -114,7 +139,7 @@ auth_reload()
 	memset(&my_client, 0, sizeof(my_client));
 	xtab_export_read();
 	check_useipaddr();
-	v4root_set();
+	v4root_set(&mountpoints_checked);
 
 	++counter;
 

utils/mountd/v4root.c +10 -2

index d52172592823..1a5778f9c7de 100644
@@ -183,7 +183,7 @@ static int v4root_add_parents(nfs_export *exp)
  * looking for components of the v4 mount.
  */
 void
-v4root_set()
+v4root_set(int *mountpoints_checked)
 {
 	nfs_export	*exp;
 	int	i;

@@ -193,6 +193,7 @@ v4root_set()
 	if (!v4root_support())
 		return;
 
+	*mountpoints_checked = 0;
 	for (i = 0; i < MCL_MAXTYPES; i++) {
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (exp->m_export.e_flags & NFSEXP_V4ROOT)

@@ -202,6 +203,14 @@ v4root_set()
 				 */
 				continue;
 
+			if (exp->m_export.e_mountpoint) {
+				*mountpoints_checked = 1;
+				if (!is_mountpoint(exp->m_export.e_mountpoint[0]?
+						   exp->m_export.e_mountpoint:
+						   exp->m_export.e_path))
+					continue;
+			}
+
 			if (strcmp(exp->m_export.e_path, "/") == 0 &&
 			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
 				/* Force '/' to be exported as fsid == 0*/
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · NeilBrown

Quoted 6 lines · J. Bruce Fields

Quoted 5 lines · NeilBrown

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

Quoted 7 lines

Quoted 4 lines

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Surely there's some more reliable measurement--could we track some data about the mountpoint itself, maybe?

Quoted 6 lines

We could. But it would be more complex code for very little gain. I did consider using select() on /proc/mounts to get a notification whenever anything changes. What we be more reliable but more difficult. I also considered calculating an SHA1, or maybe just a crc32 on the contents of /proc/mounts. But then I realised that the size was very easy and very nearly as reliable.

So we don't care enough about the mountpoint option enough to make it work 100% reliably?

If we expect too few users for there to be a real chance of hitting the bad case here, then I wonder again whether the whole feature is worth the trouble.

Quoted 4 lines

Quoted 3 lines

But I'd still like some more justification for this change in logic. Does anyone currently use the "mp" option? If not, could we just deprecate it? If so, can we really get away with changing it this way?

Quoted 3 lines

I have a customer complaining that it doesn't work as advertised for NFSv4. So presumably they have a use-case, though I haven't asked for details on exactly why they want it.

I'd be inclined to ask for more details about the use case before continuing.

As it is I'm inclined towards some plan like documenting "mountpoint" as deprecated, warning on use, and taking it out eventually.

--b.

Quoted 138 lines

I actually think this is the most useful of the changes. It means that if a filesystem isn't mounted, it isn't even visible over NFSv4.

After all, the reality is that people export filesystems, not names in their namespace. NFSv4 tries to make it all look like the same thing, and there is some justification for that. But I think a lot of people think about it as filesystems being exported, and the mountpoint option allows that thought to be expressed in the configuration.

NeilBrown

Quoted 126 lines

--b.

Quoted 123 lines

Signed-off-by NeilBrown <[email protected]>

---
 support/include/v4root.h |    2 +-
 utils/mountd/auth.c      |   29 +++++++++++++++++++++++++++--
 utils/mountd/v4root.c    |   11 ++++++++++-
 3 files changed, 38 insertions(+), 4 deletions(-)

support/include/v4root.h +1 -1

index 706c15c70d95..406fd4e43e5a 100644
@@ -10,6 +10,6 @@
 #define V4ROOT_H
 
 extern int v4root_needed;
-extern void v4root_set(void);
+extern void v4root_set(int *mountpoints_checked);
 
 #endif /* V4ROOT_H */

utils/mountd/auth.c +27 -2

index 0881d9a6edba..5bd7e6622307 100644
@@ -77,6 +77,29 @@ check_useipaddr(void)
 		cache_flush(1);
 }
 
+static int mountpoints_changed(void)
+{
+	static int last_size = 0;
+	int size;
+	int fd;
+	char buf[4096];
+	int n;
+
+	fd = open("/proc/mounts", O_RDONLY);
+	if (fd < 0)
+		/* ignore mountpoint changes if we cannot read /proc/mounts */
+		return 0;
+	size = 0;
+	while ((n = read(fd, buf, sizeof(buf))) > 0)
+		size += n;
+	if (n < 0)
+		return 0;
+	if (size == last_size)
+		return 0;
+	last_size = size;
+	return 1;
+}
+
 unsigned int
 auth_reload()
 {

@@ -84,6 +107,7 @@ auth_reload()
 	static ino_t		last_inode;
 	static int		last_fd = -1;
 	static unsigned int	counter;
+	static int		mountpoints_checked = 0;
 	int			fd;
 
 	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {

@@ -91,7 +115,8 @@ auth_reload()
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
 		close(fd);
-	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+	} else if (last_fd != -1 && stb.st_ino == last_inode &&
+		   (!mountpoints_checked || !mountpoints_changed())) {
 		/* We opened the etab file before, and its inode
 		 * number hasn't changed since then.
 		 */

@@ -114,7 +139,7 @@ auth_reload()
 	memset(&my_client, 0, sizeof(my_client));
 	xtab_export_read();
 	check_useipaddr();
-	v4root_set();
+	v4root_set(&mountpoints_checked);
 
 	++counter;
 

utils/mountd/v4root.c +10 -2

index d52172592823..1a5778f9c7de 100644
@@ -183,7 +183,7 @@ static int v4root_add_parents(nfs_export *exp)
  * looking for components of the v4 mount.
  */
 void
-v4root_set()
+v4root_set(int *mountpoints_checked)
 {
 	nfs_export	*exp;
 	int	i;

@@ -193,6 +193,7 @@ v4root_set()
 	if (!v4root_support())
 		return;
 
+	*mountpoints_checked = 0;
 	for (i = 0; i < MCL_MAXTYPES; i++) {
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (exp->m_export.e_flags & NFSEXP_V4ROOT)

@@ -202,6 +203,14 @@ v4root_set()
 				 */
 				continue;
 
+			if (exp->m_export.e_mountpoint) {
+				*mountpoints_checked = 1;
+				if (!is_mountpoint(exp->m_export.e_mountpoint[0]?
+						   exp->m_export.e_mountpoint:
+						   exp->m_export.e_path))
+					continue;
+			}
+
 			if (strcmp(exp->m_export.e_path, "/") == 0 &&
 			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
 				/* Force '/' to be exported as fsid == 0*/
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 35 lines · J. Bruce Fields

Quoted 16 lines · NeilBrown

Quoted 6 lines · J. Bruce Fields

Quoted 5 lines · NeilBrown

export points with the "mountpoint" flag should not be exported if they aren't mounted.
They shouldn't even appear in the pseudo-root filesystem. So add an appropriate check to v4root_set().

Quoted 7 lines

Quoted 4 lines

This means that the v4root might need to be recomputed whenever a filesystem is mounted or unmounted. So when there are export points with the "mountpoint" flag, check for changes in the mount table. This is done be measuring the size of /proc/mounts.

Surely there's some more reliable measurement--could we track some data about the mountpoint itself, maybe?

Quoted 6 lines

We could. But it would be more complex code for very little gain. I did consider using select() on /proc/mounts to get a notification whenever anything changes. What we be more reliable but more difficult. I also considered calculating an SHA1, or maybe just a crc32 on the contents of /proc/mounts. But then I realised that the size was very easy and very nearly as reliable.

So we don't care enough about the mountpoint option enough to make it work 100% reliably?

If we expect too few users for there to be a real chance of hitting the bad case here, then I wonder again whether the whole feature is worth the trouble.

Quoted 4 lines

Quoted 3 lines

But I'd still like some more justification for this change in logic. Does anyone currently use the "mp" option? If not, could we just deprecate it? If so, can we really get away with changing it this way?

Quoted 6 lines

Quoted 3 lines

I have a customer complaining that it doesn't work as advertised for NFSv4. So presumably they have a use-case, though I haven't asked for details on exactly why they want it.

I'd be inclined to ask for more details about the use case before continuing.

I asked, and found the answer quite helpful.

So thanks for prompting that.

For NFSv2/3, If I list "/export/foo" in /etc/exports, but /export/foo fails to mount during boot, then a client which tries to mount "/export/foo" will get a file handle on /export (probably the root filesystem).
Unless subtree_check is set (which we don't like) this effectively means that the whole root filesystem is potentially exported, if the client can determine the filehandles.
Once the problem is fixed, the filesystem is mounted, and "exportfs -r" is run (possibly by reboot), the root filesystem will no longer be exported, so that filehandle that the client has becomes stale (this is the particlar symptom the customer mentioned).

I think it is safe to argue that having 'mount' fail is safer than having it succeed, present an empty directory, and then have that directory suddenly become stale at some later time.

For NFSv4 the root filesystem is always exported, but usually as the 'pseudo-root', being read-only and files being completely unavailable. If /export/foo is not mounted but /export/foo is exported, then at least part of the root filesystem will be exported (potentially) r/w. I'm not sure what happens with filehandles. A filehandle from the pseudo-root filesystem has fsid=0. A filehandle from a properly exported directory on the root filesystem might not - I'd have to check another day. So you might not get the 'stale file handles', but would might still get unexpected access to the root filesystem.

So I think this justifies maintaining (and maybe even encouraging) the 'mountpoint' export option.

For NFSv4 it is probably OK for the to-be-mounted-on directory to be visible, but firmly 'pseudo'. So I can probably drop my elegant /proc/mounts change detector which you aren't fond of.

When we get a filehandle for a filesystem which isn't currently mounted the current code sends no response, so clients hang. My latest patch sends ESTALE, so client gets an error.
I wonder if we could arrange to make just the exported root look like an empty (pseudo-root-style) directory. Then when the filesystem gets mounted the directory morphs into the real thing..
For files on the filesystem I could probably be convinced either way, unless testing shows some unpleasant behaviour.

Are you convinced? At all?

If getaddrinfo() returns EAI_AGAIN, we shouldn't just give up, but should continue normal retries as the nameserver may be unavailable for the same reason as the NFS server.

So move the getaddrinfo() call from nfs_validate_options() into nfs_try_mounts() which is always called soon after, except in the 'remount' case when we don't want it anyway.

If EAI_AGAIN is returned, set errno to EAGAIN and allow this to be a temporary failure. Otherwise report error and set errno to EALREADY so no further message is given.

Signed-off-by NeilBrown <[email protected]>