[PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[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/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.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsd/nfsd.c   |    3 +++
 utils/nfsd/nfssvc.c |   21 +++++++++++++++++++++
 utils/nfsd/nfssvc.h |    1 +
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 1cda1e5..6bbf697 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -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;
diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
index 34c67ca..9ea3a1f 100644
--- a/utils/nfsd/nfssvc.c
+++ b/utils/nfsd/nfssvc.c
@@ -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.
  *
diff --git a/utils/nfsd/nfssvc.h b/utils/nfsd/nfssvc.h
index 0c69bd6..ff81165 100644
--- a/utils/nfsd/nfssvc.h
+++ b/utils/nfsd/nfssvc.h
@@ -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);
-- 
1.7.1

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Neil Brown]

On Sat, 28 Aug 2010 07:35:14 -0400
Jeff Layton <jlayton@redhat.com> 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/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.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/nfsd/nfsd.c   |    3 +++
>  utils/nfsd/nfssvc.c |   21 +++++++++++++++++++++
>  utils/nfsd/nfssvc.h |    1 +
>  3 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..6bbf697 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -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;
> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> index 34c67ca..9ea3a1f 100644
> --- a/utils/nfsd/nfssvc.c
> +++ b/utils/nfsd/nfssvc.c
> @@ -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.
>   *
> diff --git a/utils/nfsd/nfssvc.h b/utils/nfsd/nfssvc.h
> index 0c69bd6..ff81165 100644
> --- a/utils/nfsd/nfssvc.h
> +++ b/utils/nfsd/nfssvc.h
> @@ -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);

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Neil Brown]

(I might have just sent an empty reply to this - sorry 'bout that).

On Sat, 28 Aug 2010 07:35:14 -0400
Jeff Layton <jlayton@redhat.com> 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/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.

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

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Sun, 29 Aug 2010 08:38:53 +1000
Neil Brown <neilb@suse.de> wrote:

> 
> (I might have just sent an empty reply to this - sorry 'bout that).
> 
> On Sat, 28 Aug 2010 07:35:14 -0400
> Jeff Layton <jlayton@redhat.com> 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/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.
> 
> 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.
> 

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.

> Suggestions:
> 
>  - just don't do that.  Use /etc/init.d/nfsserver start (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...

>  - Make /proc/fs/nfsd and 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.

>  - 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.

>  - 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.
> 

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.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Sat, Aug 28, 2010 at 10:24:50PM -0400, Jeff Layton wrote:
> 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.

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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Sat, Aug 28, 2010 at 10:24:50PM -0400, Jeff Layton wrote:
> On Sun, 29 Aug 2010 08:38:53 +1000
> Neil Brown <neilb@suse.de> wrote:
> > 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.
> > 
> 
> 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.
> 
> > Suggestions:
> > 
> >  - just don't do that.  Use /etc/init.d/nfsserver start (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...
> 
> >  - Make /proc/fs/nfsd and 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Neil Brown]

On Sun, 29 Aug 2010 15:37:17 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Sat, Aug 28, 2010 at 10:24:50PM -0400, Jeff Layton wrote:
> > On Sun, 29 Aug 2010 08:38:53 +1000
> > Neil Brown <neilb@suse.de> wrote:
> > > 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.
> > > 
> > 
> > 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.
> > 
> > > Suggestions:
> > > 
> > >  - just don't do that.  Use /etc/init.d/nfsserver start (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...
> > 
> > >  - Make /proc/fs/nfsd and 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.

> 
> 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

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/28/2010 07:35 AM, Jeff Layton 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/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.
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.

> 
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/nfsd/nfsd.c   |    3 +++
>  utils/nfsd/nfssvc.c |   21 +++++++++++++++++++++
>  utils/nfsd/nfssvc.h |    1 +
>  3 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..6bbf697 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -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;
> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> index 34c67ca..9ea3a1f 100644
> --- a/utils/nfsd/nfssvc.c
> +++ b/utils/nfsd/nfssvc.c
> @@ -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.
>   *
> diff --git a/utils/nfsd/nfssvc.h b/utils/nfsd/nfssvc.h
> index 0c69bd6..ff81165 100644
> --- a/utils/nfsd/nfssvc.h
> +++ b/utils/nfsd/nfssvc.h
> @@ -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);

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Mon, 30 Aug 2010 11:51:48 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 08/28/2010 07:35 AM, Jeff Layton 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/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.
> 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.
> 

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?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/30/2010 12:16 PM, Jeff Layton wrote:
> On Mon, 30 Aug 2010 11:51:48 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 08/28/2010 07:35 AM, Jeff Layton 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/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.
>> 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.
>>
> 
> 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? 

> 
> 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... 


> 
> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Mon, Aug 30, 2010 at 12:53:16PM -0400, Steve Dickson wrote:
> On 08/30/2010 12:16 PM, Jeff Layton wrote:
> > 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.

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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Mon, 30 Aug 2010 13:04:12 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Aug 30, 2010 at 12:53:16PM -0400, Steve Dickson wrote:
> > On 08/30/2010 12:16 PM, Jeff Layton wrote:
> > > 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... 
> > 

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.

> > 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.)
> 

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.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Mon, 30 Aug 2010 12:53:16 -0400
Steve Dickson <SteveD@redhat.com> wrote:
> > 
> > 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.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/30/2010 01:04 PM, J. Bruce Fields wrote:
> On Mon, Aug 30, 2010 at 12:53:16PM -0400, Steve Dickson wrote:
>> On 08/30/2010 12:16 PM, Jeff Layton wrote:
>>> 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.
> 
> 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.

> 
> On the other hand, the nfsctl interface is pretty old (when did the new
> stuff go in, exactly?).
Its been a while... 

> 
> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/30/2010 01:48 PM, Jeff Layton wrote:
> On Mon, 30 Aug 2010 12:53:16 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
>>>
>>> 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.
> 
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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Tue, 31 Aug 2010 08:24:57 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 08/30/2010 01:48 PM, Jeff Layton wrote:
> > On Mon, 30 Aug 2010 12:53:16 -0400
> > Steve Dickson <SteveD@redhat.com> wrote:
> >>>
> >>> 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.
> > 
> 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) {

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...

>     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...
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/31/2010 08:43 AM, Jeff Layton wrote:
> On Tue, 31 Aug 2010 08:24:57 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 08/30/2010 01:48 PM, Jeff Layton wrote:
>>> On Mon, 30 Aug 2010 12:53:16 -0400
>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>>>
>>>>> 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.
>>>
>> 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) {
> 
> 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...
> 
>>     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.
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... 

> 
> 
> 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. 
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... 

> The mount will then fail because
> it's now already mounted, and the system() 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? 

> 
> 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... 
 
> 
> 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.

> 
> 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...

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Tue, 31 Aug 2010 10:49:56 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> > 
> > 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...

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?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
> 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?

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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/31/2010 11:13 AM, J. Bruce Fields wrote:
> On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
>> 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?
> 
> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Tue, Aug 31, 2010 at 11:18:19AM -0400, Steve Dickson wrote:
> 
> 
> On 08/31/2010 11:13 AM, J. Bruce Fields wrote:
> > On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
> >> 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?
> > 
> > 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/31/2010 11:51 AM, J. Bruce Fields wrote:
> On Tue, Aug 31, 2010 at 11:18:19AM -0400, Steve Dickson wrote:
>>
>>
>> On 08/31/2010 11:13 AM, J. Bruce Fields wrote:
>>> On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
>>>> 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?
>>>
>>> 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. 


> 	2. Add code to turn off the legacy interface if the mount
> 	   doesn't work.
> 
> They're two separate issues with separate justifications.
> 
> --b.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Tue, Aug 31, 2010 at 12:13:20PM -0400, Steve Dickson wrote:
> 
> 
> On 08/31/2010 11:51 AM, J. Bruce Fields wrote:
> > On Tue, Aug 31, 2010 at 11:18:19AM -0400, Steve Dickson wrote:
> >>
> >>
> >> On 08/31/2010 11:13 AM, J. Bruce Fields wrote:
> >>> On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
> >>>> 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?
> >>>
> >>> 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... 

Yes.

> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/31/2010 12:15 PM, J. Bruce Fields wrote:
> On Tue, Aug 31, 2010 at 12:13:20PM -0400, Steve Dickson wrote:
>>
>>
>> On 08/31/2010 11:51 AM, J. Bruce Fields wrote:
>>> On Tue, Aug 31, 2010 at 11:18:19AM -0400, Steve Dickson wrote:
>>>>
>>>>
>>>> On 08/31/2010 11:13 AM, J. Bruce Fields wrote:
>>>>> On Tue, Aug 31, 2010 at 11:10:08AM -0400, Jeff Layton wrote:
>>>>>> 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?
>>>>>
>>>>> 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... 
> 
> 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? 

> 
>> 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.

> 
> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[J. Bruce Fields]

On Tue, Aug 31, 2010 at 01:18:46PM -0400, Steve Dickson wrote:
> 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.

> > 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Steve Dickson]

On 08/31/2010 02:07 PM, J. Bruce Fields wrote:
> On Tue, Aug 31, 2010 at 01:18:46PM -0400, Steve Dickson wrote:
>> 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.
> 
>>> 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.
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.....

> 
> 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.

Re: [PATCH] rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet

[Jeff Layton]

On Tue, 31 Aug 2010 14:59:35 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 08/31/2010 02:07 PM, J. Bruce Fields wrote:
> > On Tue, Aug 31, 2010 at 01:18:46PM -0400, Steve Dickson wrote:
> >> 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.
> > 
> >>> 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.
> 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.....
> 
> > 
> > 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,
-- 
Jeff Layton <jlayton@redhat.com>