rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet
Subsystems (2)
PATCH 1/1 rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least, /proc/fs/nfsd is mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.
If someone runs rpc.nfsd without plugging in nfsd.ko first, /proc/fs/nfsd won't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.
This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
To avoid this situation, have rpc.nfsd check to see if the "threads" file is already present. If it's not, then make an attempt to mount /proc/fs/nfsd. This is a "best-effort" sort of thing, however so we just ignore the return code from the mount attempt and fall back to using nfsctl() if it fails.
Full disclosure: I'm not 100% thrilled with this patch. It seems ugly and kludgey, but I don't see a better way to handle this problem. Suggestions welcome.
--- utils/nfsd/nfsd.c | 3 +++ utils/nfsd/nfssvc.c | 21 +++++++++++++++++++++ utils/nfsd/nfssvc.h | 1 + 3 files changed, 25 insertions(+), 0 deletions(-)
utils/nfsd/nfsd.c +3 -0
@@ -246,6 +246,9 @@ main(int argc, char **argv)exit (1 ); } +/* make sure nfsdfs is mounted if it's available */ +nfssvc_mount_nfsdfs (); +/* can only change number of threads if nfsd is already up */ if (nfssvc_inuse ()) { socket_up =1 ;
utils/nfsd/nfssvc.c +21 -0
@@ -15,9 +15,11 @@#include <netdb.h> #include <netinet/in.h> #include <arpa/inet.h> +#include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <errno.h> +#include <stdlib.h> #include "nfslib.h" #include "xlog.h"
@@ -44,6 +46,25 @@char buf[128 ];/* +* Using the "new" interfaces for nfsd requires that /proc/fs/nfsd is +* actually mounted. Make an attempt to mount it here if it doesn't appear +* to be. If the mount attempt fails, no big deal -- fall back to using nfsctl +* instead. +*/ +void +nfssvc_mount_nfsdfs (void ) +{ +int err; +struct stat statbuf; + + err =stat (NFSD_THREAD_FILE , &statbuf); +if (err ==0 || errno !=ENOENT ) +return ; + +system ("/bin/mount -t nfsd nfsd /proc/fs/nfsd >/dev/null 2>&1" ); +} + +/* * Are there already sockets configured? If not, then it is safe to try to * open some and pass them through. *
utils/nfsd/nfssvc.h +1 -0
@@ -20,6 +20,7 @@* */ +void nfssvc_mount_nfsdfs (void );int nfssvc_inuse (void );int nfssvc_set_sockets (const int family,const unsigned int protobits,const char *host,const char *port);
Signature · 1 line
1.7.1
Quoted 99 lines · On Sat, 28 Aug 2010 07:35:14 -0400 Jeff Layton <[email protected]> wrote:
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
To avoid this situation, have rpc.nfsd check to see if the "threads" file is already present. If it's not, then make an attempt to mount
/proc/fs/nfsd. This is a "best-effort" sort of thing, however so we just ignore the return code from the mount attempt and fall back to usingnfsctl()if it fails.Full disclosure: I'm not 100% thrilled with this patch. It seems ugly and kludgey, but I don't see a better way to handle this problem. Suggestions welcome.
Signed-off-by Jeff Layton <[email protected]>--- utils/nfsd/nfsd.c | 3 +++ utils/nfsd/nfssvc.c | 21 +++++++++++++++++++++ utils/nfsd/nfssvc.h | 1 + 3 files changed, 25 insertions(+), 0 deletions(-)utils/nfsd/nfsd.c +3
-0@@ -246,6 +246,9 @@ main(int argc, char **argv)exit (1 ); } +/* make sure nfsdfs is mounted if it's available */ +nfssvc_mount_nfsdfs (); +/* can only change number of threads if nfsd is already up */ if (nfssvc_inuse ()) { socket_up =1 ;utils/nfsd/nfssvc.c +21
-0@@ -15,9 +15,11 @@#include <netdb.h> #include <netinet/in.h> #include <arpa/inet.h> +#include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <errno.h> +#include <stdlib.h> #include "nfslib.h" #include "xlog.h" @@ -44,6 +46,25 @@char buf[128 ];/* +* Using the "new" interfaces for nfsd requires that /proc/fs/nfsd is +* actually mounted. Make an attempt to mount it here if it doesn't appear +* to be. If the mount attempt fails, no big deal -- fall back to using nfsctl +* instead. +*/ +void +nfssvc_mount_nfsdfs (void ) +{ +int err; +struct stat statbuf; + + err =stat (NFSD_THREAD_FILE , &statbuf); +if (err ==0 || errno !=ENOENT ) +return ; + +system ("/bin/mount -t nfsd nfsd /proc/fs/nfsd >/dev/null 2>&1" ); +} + +/* * Are there already sockets configured? If not, then it is safe to try to * open some and pass them through. * utils/nfsd/nfssvc.h +1
-0@@ -20,6 +20,7 @@* */ +void nfssvc_mount_nfsdfs (void );int nfssvc_inuse (void );int nfssvc_set_sockets (const int family,const unsigned int protobits,const char *host,const char *port);
(I might have just sent an empty reply to this - sorry 'bout that).
Quoted 20 lines · On Sat, 28 Aug 2010 07:35:14 -0400 Jeff Layton <[email protected]> wrote:
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
To avoid this situation, have rpc.nfsd check to see if the "threads" file is already present. If it's not, then make an attempt to mount
/proc/fs/nfsd. This is a "best-effort" sort of thing, however so we just ignore the return code from the mount attempt and fall back to usingnfsctl()if it fails.
Quoted 3 lines
Full disclosure: I'm not 100% thrilled with this patch. It seems ugly and kludgey, but I don't see a better way to handle this problem. Suggestions welcome.
I don't think it is all that bad. It is a shame you have to use system() rather than just calling mount() directly but I guess we need that to update /etc/mtab.
Suggestions:
- just don't do that. Use /etc/init.d/nfsserver start (or whatever the distro uses).
- Make /proc/fs/nfsd and auto-mount point. That sounds like the systemd approach.
- Print a warning message if the kernel is newer than 2.6 and nfsd-fs isn't mounted - see "don't do that" above.
- If nfsdfs isn't mounted, try a no-op via the nfsd syscall, like NFSCTL_GETFD with invalid parameters. This will trigger a mod-probe which will trigger the mount.
Then check for the mounted filesystem again. Yes, I think I like that one the best.
NeilBrown
Quoted 26 lines · On Sun, 29 Aug 2010 08:38:53 +1000 Neil Brown <[email protected]> wrote:
(I might have just sent an empty reply to this - sorry 'bout that).
Quoted 20 lines · On Sat, 28 Aug 2010 07:35:14 -0400 Jeff Layton <[email protected]> wrote:
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
To avoid this situation, have rpc.nfsd check to see if the "threads" file is already present. If it's not, then make an attempt to mount
/proc/fs/nfsd. This is a "best-effort" sort of thing, however so we just ignore the return code from the mount attempt and fall back to usingnfsctl()if it fails.
Quoted 8 lines
Quoted 3 lines
Full disclosure: I'm not 100% thrilled with this patch. It seems ugly and kludgey, but I don't see a better way to handle this problem. Suggestions welcome.
I don't think it is all that bad. It is a shame you have to use
system()rather than just callingmount()directly but I guess we need that to update/etc/mtab.
Yeah, that's the main reason I went with system(). It's worthwhile to note that I'm using the exact same command that's in modprobe.conf on fedora/RHEL.
Quoted 4 lines
Suggestions:
- just don't do that. Use
/etc/init.d/nfsserverstart (or whatever the distro uses).
I don't think we should count on that. Most people will use that, but it seems like we shouldn't require that for this to work as expected...
Quoted 2 lines
- Make
/proc/fs/nfsdand auto-mount point. That sounds like the systemd approach.
Yes, I think systemd will take care of this eventually, but I think we need something for existing distros that aren't using it yet.
Quoted 2 lines
- Print a warning message if the kernel is newer than 2.6 and nfsd-fs isn't mounted - see "don't do that" above.
That's certainly an option, but it's less "automatic" than doing something to try and get it mounted before we fall back to nfsctl(). For that reason I'm less enthusiastic about it.
Quoted 6 lines
- If nfsdfs isn't mounted, try a no-op via the nfsd syscall, like
NFSCTL_GETFDwith invalid parameters. This will trigger a mod-probe which will trigger the mount.
Then check for the mounted filesystem again. Yes, I think I like that one the best.
I considered that when I did this, but didn't know if we could count on nfsdfs automatically being mounted up when nfsd.ko is plugged in. I went with calling system() since I figured we'd be making less assumptions about modprobe.conf configuration.
FWIW, at least on fedora the /bin/mount here usually returns an error. /bin/mount calls mount() and that triggers a request_module() for nfsdfs. That triggers modprobe to mount the fs, and /bin/mount then returns an error since it's already mounted. The error though is harmless in this case since the fs gets mounted up anyway.
If the consensus is that using a no-op nfsctl() call is better than shelling out to /bin/mount here, then I'll go with that. It seems though like the patch I've already proposed would allow for nfsdfs to end up mounted even when modprobe.conf isn't set up to do it.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 11 lines · Jeff Layton
I considered that when I did this, but didn't know if we could count on nfsdfs automatically being mounted up when nfsd.ko is plugged in. I went with calling
system()since I figured we'd be making less assumptions about modprobe.conf configuration.FWIW, at least on fedora the
/bin/mounthere usually returns an error./bin/mountcallsmount()and that triggers arequest_module()for nfsdfs. That triggers modprobe to mount the fs, and/bin/mountthen returns an error since it's already mounted. The error though is harmless in this case since the fs gets mounted up anyway.
Quoted 4 lines
If the consensus is that using a no-op
nfsctl()call is better than shelling out to/bin/mounthere, then I'll go with that. It seems though like the patch I've already proposed would allow for nfsdfs to end up mounted even when modprobe.conf isn't set up to do it.
I'd rather not rely on modprobe. It's partly a personal grudge: I usually build nfsd into kernels when I test, and that always requires a minor scuffle or two with init scripts that assume nfsd should be a module.
--b.
Quoted 16 lines · Jeff Layton
Quoted 4 lines · On Sun, 29 Aug 2010 08:38:53 +1000 Neil Brown <[email protected]> wrote:
I don't think it is all that bad. It is a shame you have to use
system()rather than just callingmount()directly but I guess we need that to update/etc/mtab.Yeah, that's the main reason I went with
system(). It's worthwhile to note that I'm using the exact same command that's in modprobe.conf on fedora/RHEL.Quoted 4 lines
Suggestions:
- just don't do that. Use
/etc/init.d/nfsserverstart (or whatever the distro uses).
Quoted 8 lines
I don't think we should count on that. Most people will use that, but it seems like we shouldn't require that for this to work as expected...
Quoted 2 lines
- Make
/proc/fs/nfsdand auto-mount point. That sounds like the systemd approach.Yes, I think systemd will take care of this eventually, but I think we need something for existing distros that aren't using it yet.
I thought Neil was saying that this would be a "systemd-like" approach, not that it would actually require systemd. Am I missing something?
I can't come up with an immediate objection. I'm also not opposed to your patch as it stands.
--b.
Quoted 20 lines · On Sun, 29 Aug 2010 15:37:17 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 19 lines · Jeff Layton
Quoted 4 lines · On Sun, 29 Aug 2010 08:38:53 +1000 Neil Brown <[email protected]> wrote:
I don't think it is all that bad. It is a shame you have to use
system()rather than just callingmount()directly but I guess we need that to update/etc/mtab.Yeah, that's the main reason I went with
system(). It's worthwhile to note that I'm using the exact same command that's in modprobe.conf on fedora/RHEL.Quoted 4 lines
Suggestions:
- just don't do that. Use
/etc/init.d/nfsserverstart (or whatever the distro uses).I don't think we should count on that. Most people will use that, but it seems like we shouldn't require that for this to work as expected...
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
- Make
/proc/fs/nfsdand auto-mount point. That sounds like the systemd approach.Yes, I think systemd will take care of this eventually, but I think we need something for existing distros that aren't using it yet.
I thought Neil was saying that this would be a "systemd-like" approach, not that it would actually require systemd. Am I missing something?
That is what I was saying, but making autofs or am-utils a pre-requisite for nfsd doesn't sound like a good idea. I would "like" it to work like this, but I don't think it is really practical.
Quoted 3 lines
I can't come up with an immediate objection. I'm also not opposed to your patch as it stands.
I agree, the 'no-op NFSD request' is very dependent on specific user-space setup so we don't want to burn that into rpc.nfsd.
I support that patch as it stands.
NeilBrown
Quoted 9 lines · Jeff Layton
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.
Quoted 4 lines
This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
Well maybe its time we stop supporting the legacy interface... I would rather stop supporting something nobody uses then added some questionable code... Lets just error out when /proc/fs/nfsd is not mounted and log what needs to happen...
steved.
Quoted 86 lines
To avoid this situation, have rpc.nfsd check to see if the "threads" file is already present. If it's not, then make an attempt to mount
/proc/fs/nfsd. This is a "best-effort" sort of thing, however so we just ignore the return code from the mount attempt and fall back to usingnfsctl()if it fails.Full disclosure: I'm not 100% thrilled with this patch. It seems ugly and kludgey, but I don't see a better way to handle this problem. Suggestions welcome.
Signed-off-by Jeff Layton <[email protected]>--- utils/nfsd/nfsd.c | 3 +++ utils/nfsd/nfssvc.c | 21 +++++++++++++++++++++ utils/nfsd/nfssvc.h | 1 + 3 files changed, 25 insertions(+), 0 deletions(-)utils/nfsd/nfsd.c +3
-0@@ -246,6 +246,9 @@ main(int argc, char **argv)exit (1 ); } +/* make sure nfsdfs is mounted if it's available */ +nfssvc_mount_nfsdfs (); +/* can only change number of threads if nfsd is already up */ if (nfssvc_inuse ()) { socket_up =1 ;utils/nfsd/nfssvc.c +21
-0@@ -15,9 +15,11 @@#include <netdb.h> #include <netinet/in.h> #include <arpa/inet.h> +#include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <errno.h> +#include <stdlib.h> #include "nfslib.h" #include "xlog.h" @@ -44,6 +46,25 @@char buf[128 ];/* +* Using the "new" interfaces for nfsd requires that /proc/fs/nfsd is +* actually mounted. Make an attempt to mount it here if it doesn't appear +* to be. If the mount attempt fails, no big deal -- fall back to using nfsctl +* instead. +*/ +void +nfssvc_mount_nfsdfs (void ) +{ +int err; +struct stat statbuf; + + err =stat (NFSD_THREAD_FILE , &statbuf); +if (err ==0 || errno !=ENOENT ) +return ; + +system ("/bin/mount -t nfsd nfsd /proc/fs/nfsd >/dev/null 2>&1" ); +} + +/* * Are there already sockets configured? If not, then it is safe to try to * open some and pass them through. * utils/nfsd/nfssvc.h +1
-0@@ -20,6 +20,7 @@* */ +void nfssvc_mount_nfsdfs (void );int nfssvc_inuse (void );int nfssvc_set_sockets (const int family,const unsigned int protobits,const char *host,const char *port);
Quoted 16 lines · On Mon, 30 Aug 2010 11:51:48 -0400 Steve Dickson <[email protected]> wrote:
Quoted 9 lines · Jeff Layton
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.Quoted 4 lines
This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
Quoted 7 lines
Well maybe its time we stop supporting the legacy interface... I would rather stop supporting something nobody uses then added some questionable code... Lets just error out when
/proc/fs/nfsdis not mounted and log what needs to happen...steved.
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
I'm not sure I like throwing up our hands and bailing out with a log message. We'll be going from an at least somewhat carefully considered fallback mechanism to an outright failure to start in this situation. That doesn't really seem like an improvement to me...
How this as an alternate proposal?
We attempt to mount up nfsdfs. If the "threads" file still isn't present after the attempt, we then log a warning and go with the nfsctl() interface?
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 24 lines · Jeff Layton
Quoted 21 lines · On Mon, 30 Aug 2010 11:51:48 -0400 Steve Dickson <[email protected]> wrote:
Quoted 9 lines · Jeff Layton
There's a bit of a chicken and egg problem when nfsd is run the first time. On Fedora/RHEL at least,
/proc/fs/nfsdis mounted up whenever nfsd is plugged in via a modprobe.conf "install" directive.If someone runs rpc.nfsd without plugging in nfsd.ko first,
/proc/fs/nfsdwon't be mounted and rpc.nfsd will end up using the legacy nfsctl interface. After that, nfsd will be plugged in and subsequent rpc.nfsd invocations will use that instead.Quoted 4 lines
This is a problem as some nfsd command-line options are ignored when the legacy interface is used. It'll also be a problem for people who want IPv6 enabled servers. The upshot is that we really don't want to use the legacy interface unless there is no other option.
Well maybe its time we stop supporting the legacy interface... I would rather stop supporting something nobody uses then added some questionable code... Lets just error out when
/proc/fs/nfsdis not mounted and log what needs to happen...
Quoted 6 lines
Quoted 2 lines
steved.
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
I thought you said the legacy interface would not work with IPV6 or did I misunderstand you?
Quoted 5 lines
I'm not sure I like throwing up our hands and bailing out with a log message. We'll be going from an at least somewhat carefully considered fallback mechanism to an outright failure to start in this situation. That doesn't really seem like an improvement to me...
This case will only happen when people start rpc.nfsd by hand (i.e. not via some start-up script) which %99.999 of the users do not do .
But the few that do start rpc.nfsd by hand, they are probably debugging something, so logging a message saying /proc/fs/nfsd is not mount would be quite handy... IMHO...
Quoted 6 lines
How this as an alternate proposal?
We attempt to mount up nfsdfs. If the "threads" file still isn't present after the attempt, we then log a warning and go with the
nfsctl()interface?
Has anybody test this legacy interface lately?? Does anybody anybody depend on the existence of this interface??? I would guess the answer would be no to both questions... So I see this as an opportunity so simplify the code... which is always a good thing...
So I would have no problem saying from the next release of nfs-utils, the legacy interface is no longer supported... especially if there are issues with IPV6.
steved.
Quoted 6 lines · Steve Dickson
Quoted 5 lines · Jeff Layton
How this as an alternate proposal?
We attempt to mount up nfsdfs. If the "threads" file still isn't present after the attempt, we then log a warning and go with the
nfsctl()interface?
Quoted 8 lines
Has anybody test this legacy interface lately?? Does anybody anybody depend on the existence of this interface??? I would guess the answer would be no to both questions... So I see this as an opportunity so simplify the code... which is always a good thing...
So I would have no problem saying from the next release of nfs-utils, the legacy interface is no longer supported... especially if there are issues with IPV6.
In general I'd like backwards-compatibility to be a very high priority, in both directions. (Both continuing to support old nfs-utils in new kernels, and supporting old kernels with new nfs-utils.) Among other advantages, it makes it easier to troubleshoot user problems if we don't have to ask them to upgrade multiple packages at once to test a fix.
On the other hand, the nfsctl interface is pretty old (when did the new stuff go in, exactly?).
On the other other hand, Jeff's patch isn't really very complicated. (Though the amount of additional code we could delete might be large.)
--b.
Quoted 4 lines · On Mon, 30 Aug 2010 13:04:12 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 3 lines · Steve Dickson
Quoted 2 lines · Jeff Layton
How this as an alternate proposal?
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
We attempt to mount up nfsdfs. If the "threads" file still isn't present after the attempt, we then log a warning and go with the
nfsctl()interface?Has anybody test this legacy interface lately?? Does anybody anybody depend on the existence of this interface??? I would guess the answer would be no to both questions... So I see this as an opportunity so simplify the code... which is always a good thing...
People do still use it because we get bug reports on it when it doesn't work correctly. That said, mostly my guess is that people don't use it knowingly -- they end up using it because /proc/fs/nfsd isn't mounted at the time that they run rpc.nfsd.
Quoted 10 lines
Quoted 3 lines
So I would have no problem saying from the next release of nfs-utils, the legacy interface is no longer supported... especially if there are issues with IPV6.
In general I'd like backwards-compatibility to be a very high priority, in both directions. (Both continuing to support old nfs-utils in new kernels, and supporting old kernels with new nfs-utils.) Among other advantages, it makes it easier to troubleshoot user problems if we don't have to ask them to upgrade multiple packages at once to test a fix.
Quoted 6 lines
On the other hand, the nfsctl interface is pretty old (when did the new stuff go in, exactly?).
On the other other hand, Jeff's patch isn't really very complicated. (Though the amount of additional code we could delete might be large.)
Not sure when it went in. I know that RHEL4 has it, so it was pre-2.6.9.
I like solutions that don't require extra steps by the user. If we just log a message and error out then we're going to be requiring them to do "extra stuff" to get a working NFS server. That seems less optimal to me and will probably piss at least a few people off.
I also suspect that a lot of distros are going to break if we do that. Most don't explicitly plug in nfsd.ko before they start calling nfs-utils programs.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 16 lines · J. Bruce Fields
Quoted 6 lines · Steve Dickson
Quoted 5 lines · Jeff Layton
How this as an alternate proposal?
We attempt to mount up nfsdfs. If the "threads" file still isn't present after the attempt, we then log a warning and go with the
nfsctl()interface?Quoted 8 lines
Has anybody test this legacy interface lately?? Does anybody anybody depend on the existence of this interface??? I would guess the answer would be no to both questions... So I see this as an opportunity so simplify the code... which is always a good thing...
So I would have no problem saying from the next release of nfs-utils, the legacy interface is no longer supported... especially if there are issues with IPV6.
Quoted 5 lines
In general I'd like backwards-compatibility to be a very high priority, in both directions. (Both continuing to support old nfs-utils in new kernels, and supporting old kernels with new nfs-utils.) Among other advantages, it makes it easier to troubleshoot user problems if we don't have to ask them to upgrade multiple packages at once to test a fix.
While I agree backwards-compatibility extremely important, but taking a look at the code, if things like /proc/fs/nfsd/portlist and /proc/fs/nfsd/versions do not exist, I don't see how anything will work.
Quoted 3 lines
On the other hand, the nfsctl interface is pretty old (when did the new stuff go in, exactly?).
Its been a while...
Quoted 3 lines
On the other other hand, Jeff's patch isn't really very complicated. (Though the amount of additional code we could delete might be large.)
Well what guarantees /proc/nfs/nfsd will be mounted after the system() call? There is none plus there is no error checking so if the mount does (silently) fail we are in the same position we are today... I just don't see how the patch moves us forward...
steved.
Quoted 7 lines · On Mon, 30 Aug 2010 12:53:16 -0400 Steve Dickson <[email protected]> wrote:
Quoted 4 lines
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
I thought you said the legacy interface would not work with IPV6 or did I misunderstand you?
Sorry, forgot to answer this part. IPv6 indeed will not work when using the legacy interface. In order to get the IPv6 code in though, I ended up rewriting a large swath of rpc.nfsd. That task would have been easier had I not needed to deal with the legacy nfsctl() interface.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 6 lines · Jeff Layton
Quoted 4 lines · On Mon, 30 Aug 2010 12:53:16 -0400 Steve Dickson <[email protected]> wrote:
Quoted 4 lines
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
Quoted 9 lines
Quoted 3 lines
I thought you said the legacy interface would not work with IPV6 or did I misunderstand you?
Sorry, forgot to answer this part. IPv6 indeed will not work when using the legacy interface. In order to get the IPv6 code in though, I ended up rewriting a large swath of rpc.nfsd. That task would have been easier had I not needed to deal with the legacy
nfsctl()interface.
So I'm thinking this the final nail in the coffin of the legacy interface at least in rpc.nfsd. So if /proc/fs/nfsd filesystem is not or can not be mounted then rpc.nfsd should fail... IMHO...
Maybe something like:
if (stat("/proc/fs/nfsd") < 0) {
if (system("mount /proc/fs/nfsd") < 0) {
xlog(L_ERROR, "Unable to mount /proc/fs/nfsd");
exit 0;
}
}steved.
Quoted 18 lines · On Tue, 31 Aug 2010 08:24:57 -0400 Steve Dickson <[email protected]> wrote:
Quoted 6 lines · Jeff Layton
Quoted 4 lines · On Mon, 30 Aug 2010 12:53:16 -0400 Steve Dickson <[email protected]> wrote:
Quoted 4 lines
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
Quoted 9 lines
Quoted 3 lines
I thought you said the legacy interface would not work with IPV6 or did I misunderstand you?
Sorry, forgot to answer this part. IPv6 indeed will not work when using the legacy interface. In order to get the IPv6 code in though, I ended up rewriting a large swath of rpc.nfsd. That task would have been easier had I not needed to deal with the legacy
nfsctl()interface.
Quoted 7 lines
So I'm thinking this the final nail in the coffin of the legacy interface at least in rpc.nfsd. So if
/proc/fs/nfsdfilesystem is not or can not be mounted then rpc.nfsd should fail... IMHO...Maybe something like:
if (
stat("/proc/fs/nfsd")< 0) {
This needs to check a file within /proc/fs/nfsd. The nfsd "dir" may already exist even if it's not mounted. I chose "threads" but any file in there would do...
Quoted 7 lines
if (system("mount /proc/fs/nfsd") < 0) { xlog(L_ERROR, "Unable to mount /proc/fs/nfsd"); exit 0; } }steved.
That will universally fail, at least on most distros.
When you go to mount /proc/fs/nfsd, the system does a request_module() for nfsd.ko. On most distros (e.g. Fedora or RHEL), modprobe will then automatically mount up /proc/fs/nfsd. request_module will then return and the mount attempt will proceed. The mount will then fail because it's now already mounted, and the system() call will return non-zero.
That's the main reason I ignored the return code from system() in the original patch. What you could do is recheck whether the file exists after the mount attempt, and then fail if it doesn't.
Is that really what we want to do here though? How does ditching the legacy interface move us forward now? If we had decided to do this a year ago when I was overhauling rpc.nfsd, it might have made sense. The code exists now though and it works. Ripping this out seems more disruptive than just leaving it in.
To keep this on track, I think we need to treat this as 2 separate discussions:
1) how to we ensure that /proc/fs/nfsd is actually mounted when rpc.nfsd is run?
2) what do we do about the legacy nfsctl() interface?
These are separate but related questions...
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 39 lines · Jeff Layton
Quoted 18 lines · On Tue, 31 Aug 2010 08:24:57 -0400 Steve Dickson <[email protected]> wrote:
Quoted 6 lines · Jeff Layton
Quoted 4 lines · On Mon, 30 Aug 2010 12:53:16 -0400 Steve Dickson <[email protected]> wrote:
Quoted 4 lines
Hmmm...if I had known that it was ok to stop supporting old kernels, the IPv6 support for rpc.nfsd would have been a heck of a lot easier to implement.
Quoted 9 lines
Quoted 3 lines
I thought you said the legacy interface would not work with IPV6 or did I misunderstand you?
Sorry, forgot to answer this part. IPv6 indeed will not work when using the legacy interface. In order to get the IPv6 code in though, I ended up rewriting a large swath of rpc.nfsd. That task would have been easier had I not needed to deal with the legacy
nfsctl()interface.Quoted 7 lines
So I'm thinking this the final nail in the coffin of the legacy interface at least in rpc.nfsd. So if
/proc/fs/nfsdfilesystem is not or can not be mounted then rpc.nfsd should fail... IMHO...Maybe something like:
if (
stat("/proc/fs/nfsd")< 0) {This needs to check a file within
/proc/fs/nfsd. The nfsd "dir" may already exist even if it's not mounted. I chose "threads" but any file in there would do...Quoted 6 lines
if (system("mount /proc/fs/nfsd") < 0) { xlog(L_ERROR, "Unable to mount /proc/fs/nfsd"); exit 0; } }
Quoted 3 lines
Quoted 1 line
steved.
That will universally fail, at least on most distros.
By no means was I meaning this was working code...
I was just trying to introduced logic of: check to see if /proc/fs/nfsd exists, if not, mount it. If the mount fails, exit. Detail to be worked out later...
Quoted 6 lines
When you go to mount
/proc/fs/nfsd, the system does arequest_module()for nfsd.ko. On most distros (e.g. Fedora or RHEL), modprobe will then automatically mount up/proc/fs/nfsd.request_modulewill then return and the mount attempt will proceed.
Yeah I know... I am one that taught modprobe how to that... at least in the RH distros... ;-) Although I believe the actual mechanics have changed over the years...
Quoted 2 lines
The mount will then fail because
it's now already mounted, and thesystem()call will return non-zero.
Isn't this reason to do the stat() before the mount? So you will not try to mount the FS if is already mounted?
Quoted 4 lines
That's the main reason I ignored the return code from
system()in the original patch. What you could do is recheck whether the file exists after the mount attempt, and then fail if it doesn't.
My point was we need to know if the mount did or did not work to take the appropriate action...
Quoted 6 lines
Is that really what we want to do here though? How does ditching the legacy interface move us forward now? If we had decided to do this a year ago when I was overhauling rpc.nfsd, it might have made sense. The code exists now though and it works. Ripping this out seems more disruptive than just leaving it in.
I believe we are talking about 4 lines of code at the bottom of nfssvc_threads() I don't think that would be too disruptive...
steved.
Quoted 10 lines
To keep this on track, I think we need to treat this as 2 separate discussions:
1) how to we ensure that
/proc/fs/nfsdis actually mounted when rpc.nfsd is run?2) what do we do about the legacy
nfsctl()interface?These are separate but related questions...
Quoted 10 lines · On Tue, 31 Aug 2010 10:49:56 -0400 Steve Dickson <[email protected]> wrote:
Quoted 10 lines
To keep this on track, I think we need to treat this as 2 separate discussions:
1) how to we ensure that
/proc/fs/nfsdis actually mounted when rpc.nfsd is run?2) what do we do about the legacy
nfsctl()interface?These are separate but related questions...
I was just pointing out that checking the return code from the system() call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.
Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.
OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using nfsctl() for now.
In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 14 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.
Quoted 7 lines
OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
--b.
Quoted 19 lines · J. Bruce Fields
Quoted 18 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.
Quoted 6 lines
Quoted 3 lines
In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
When the mount fails, I think we should exit... basically eliminating the legacy interface code
steved.
Quoted 22 lines · Steve Dickson
Quoted 19 lines · J. Bruce Fields
Quoted 18 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
When the mount fails, I think we should exit... basically eliminating the legacy interface code
Maybe. But as I say, make it two separate steps:
1. Add code to attempt the mount.
2. Add code to turn off the legacy interface if the mount doesn't work.
They're two separate issues with separate justifications.
--b.
Quoted 27 lines · J. Bruce Fields
Quoted 26 lines · Steve Dickson
Quoted 23 lines · J. Bruce Fields
Quoted 21 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
When the mount fails, I think we should exit... basically eliminating the legacy interface code
Maybe. But as I say, make it two separate steps:
1. Add code to attempt the mount.
But the question comes do to, what do we do when the mount fails? It sounds like you are advocating ignoring the error and allow the nfsd threads to be started via the nfsctl(NFSCTL_SVC) call...
I'm advocating that we exit on the mount error, because even thought the nfsd threads may be set up correctly, the protocols and versions will not be set up correctly because there is no nfsctl() calls to set them up correctly... especially with IPV6 enabled...
steved.
Quoted 6 lines
2. Add code to turn off the legacy interface if the mount doesn't work.
They're two separate issues with separate justifications.
--b.
Quoted 35 lines · Steve Dickson
Quoted 32 lines · J. Bruce Fields
Quoted 30 lines · Steve Dickson
Quoted 19 lines · J. Bruce Fields
Quoted 18 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.Quoted 6 lines
Quoted 3 lines
In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
When the mount fails, I think we should exit... basically eliminating the legacy interface code
Quoted 7 lines
Quoted 3 lines
Maybe. But as I say, make it two separate steps:
1. Add code to attempt the mount.
But the question comes do to, what do we do when the mount fails? It sounds like you are advocating ignoring the error and allow the nfsd threads to be started via the
nfsctl(NFSCTL_SVC)call...
Yes.
Quoted 4 lines
I'm advocating that we exit on the mount error, because even thought the nfsd threads may be set up correctly, the protocols and versions will not be set up correctly because there is no
nfsctl()calls to set them up correctly... especially with IPV6 enabled...
Perhaps so. But that should be done as a *separate* follow-up patch that is clearly labelled "drop support for kernel versions before x.y.z".
Dropping backwards compatibility may be a reasonable thing to do, but it's something that we should be very clear about, and that we should put in a patch that does that and nothing else.
--b.
Quoted 38 lines · J. Bruce Fields
Quoted 37 lines · Steve Dickson
Quoted 34 lines · J. Bruce Fields
Quoted 22 lines · Steve Dickson
Quoted 19 lines · J. Bruce Fields
Quoted 18 lines · Jeff Layton
I was just pointing out that checking the return code from the
system()call isn't sufficient. Because of the way most people have modprobe set up, it can return an error even though nfsdfs ended up being mounted anyway. Checking for the presence of the file after attempting the mount would be a more reliable test.Assuming we're in agreement there, we have another question to settle...If the mount attempt fails, what should we do about it?
With my original patch, we fall back to using
nfsctl(). You're suggesting that we should error out there. I'm not opposed to that, but it does mean dropping support for some really old kernels. It also means that we can remove some dead code in rpc.nfsd.OTOH, the fallback might allow nfsd to keep working for some people. Maybe it would be better to just log a scary warning and fall back to using
nfsctl()for now.Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
In a couple of releases, we could start returning an error there and rip out the legacy interface code, or compile it out by default and allow people to compile it in via a configure option?
Yes, let's just add the additional mount attempt for now, and figure out what to do about the legacy interface as a next step.
When the mount fails, I think we should exit... basically eliminating the legacy interface code
Maybe. But as I say, make it two separate steps:
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
1. Add code to attempt the mount.
But the question comes do to, what do we do when the mount fails? It sounds like you are advocating ignoring the error and allow the nfsd threads to be started via the
nfsctl(NFSCTL_SVC)call...Yes.
Even if not failing could bring instability... Do we really know what will happen when the portlist and version files are not set up correctly?? I know I do not test that configuration.... Does anybody?
Quoted 9 lines
Quoted 5 lines
I'm advocating that we exit on the mount error, because even thought the nfsd threads may be set up correctly, the protocols and versions will not be set up correctly because there is no
nfsctl()calls to set them up correctly... especially with IPV6 enabled...Perhaps so. But that should be done as a *separate* follow-up patch that is clearly labelled "drop support for kernel versions before x.y.z".
Fine... put it in a separate patch but include it in this patch series... I really don't think we need to rip the guts out of mountd and exportfs just to remove this one nfsctl() in rpc.nfsd.
Quoted 4 lines
Dropping backwards compatibility may be a reasonable thing to do, but it's something that we should be very clear about, and that we should put in a patch that does that and nothing else.
So you are talking about doing a complete rewrite of mountd and exportfs just to take out that one call from rpc.nfsd... I just don't see the justification... If we are going to do that, let just think about rewriting all the tools to make the more v4 friendly... something we talked about at the last bakeathon...
steved.
Quoted 2 lines · Steve Dickson
I really don't think we need to rip the guts out of mountd and exportfs just to remove this one
nfsctl()in rpc.nfsd.
I agree.
Quoted 8 lines
Quoted 3 lines
Dropping backwards compatibility may be a reasonable thing to do, but it's something that we should be very clear about, and that we should put in a patch that does that and nothing else.
So you are talking about doing a complete rewrite of mountd and exportfs just to take out that one call from rpc.nfsd... I just don't see the justification... If we are going to do that, let just think about rewriting all the tools to make the more v4 friendly... something we talked about at the last bakeathon...
All I'm asking is that any patch which disables support for older kernels (by ceasing to do the nfsctl) be a *separate* patch that clearly states which kernels we're dropping support for.
Once we decide to drop such support (if we do), that might allow us to rip out a lot of additional code. That can be done later.
--b.
Quoted 15 lines · J. Bruce Fields
Quoted 2 lines · Steve Dickson
I really don't think we need to rip the guts out of mountd and exportfs just to remove this one
nfsctl()in rpc.nfsd.I agree.
Quoted 8 lines
Quoted 3 lines
Dropping backwards compatibility may be a reasonable thing to do, but it's something that we should be very clear about, and that we should put in a patch that does that and nothing else.
So you are talking about doing a complete rewrite of mountd and exportfs just to take out that one call from rpc.nfsd... I just don't see the justification... If we are going to do that, let just think about rewriting all the tools to make the more v4 friendly... something we talked about at the last bakeathon...
Quoted 3 lines
All I'm asking is that any patch which disables support for older kernels (by ceasing to do the nfsctl) be a *separate* patch that clearly states which kernels we're dropping support for.
Fine. The taking out the nfsclt() and/or the adding of the error check on the mount (basically causing nfsclt() to never be called) will be in a separate clearly documented patch.....
Quoted 3 lines
Once we decide to drop such support (if we do), that might allow us to rip out a lot of additional code. That can be done later.
I agree...
steved.
Quoted 26 lines · On Tue, 31 Aug 2010 14:59:35 -0400 Steve Dickson <[email protected]> wrote:
Quoted 15 lines · J. Bruce Fields
Quoted 2 lines · Steve Dickson
I really don't think we need to rip the guts out of mountd and exportfs just to remove this one
nfsctl()in rpc.nfsd.I agree.
Quoted 8 lines
Quoted 3 lines
Dropping backwards compatibility may be a reasonable thing to do, but it's something that we should be very clear about, and that we should put in a patch that does that and nothing else.
So you are talking about doing a complete rewrite of mountd and exportfs just to take out that one call from rpc.nfsd... I just don't see the justification... If we are going to do that, let just think about rewriting all the tools to make the more v4 friendly... something we talked about at the last bakeathon...
Quoted 3 lines
All I'm asking is that any patch which disables support for older kernels (by ceasing to do the nfsctl) be a *separate* patch that clearly states which kernels we're dropping support for.
Fine. The taking out the
nfsclt()and/or the adding of the error check on the mount (basically causingnfsclt()to never be called) will be in a separate clearly documented patch.....Quoted 1 line
Quoted 5 lines
Quoted 2 lines
Once we decide to drop such support (if we do), that might allow us to rip out a lot of additional code. That can be done later.
I agree...
steved.
Sounds good -- doing this as two patches makes much more sense.
I think Steve had a good point though -- we should log a message or something if we tried to mount up /proc/fs/nfsd and it didn't work. That'll help clue in people that they have a problem and things might not work as expected.
I'll respin the patch to do that and repost once I've tested it.
Thanks,
Signature · 1 line
Jeff Layton <[email protected]>