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

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

[Jeff Layton]

This patch is the second attempt at fixing this problem. It's basically
the same as the first patch, but adds some xlog() calls to log info
when there are problems. I also changed the NFSD_*_FILE definitions
around so that they are defined based on another constant that holds
the nfsdfs mountpoint.

---------------------------[snip]-------------------------
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 |   53 ++++++++++++++++++++++++++++++++++++++++++++++++--
 utils/nfsd/nfssvc.h |    1 +
 3 files changed, 54 insertions(+), 3 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..636b403 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"
@@ -31,9 +33,10 @@
  */
 #undef IPV6_SUPPORTED
 
-#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
-#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
-#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
+#define NFSD_FS_DIR	  "/proc/fs/nfsd"
+#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
+#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
+#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
 
 /*
  * declaring a common static scratch buffer here keeps us from having to
@@ -44,6 +47,50 @@
 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)
+		return;
+	else if (errno != ENOENT) {
+		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
+				NFSD_THREAD_FILE, errno);
+		return;
+	}
+
+	/*
+	 * this call can return an error if modprobe is set up to automatically
+	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
+	 * code from it and just check for the "threads" file afterward.
+	 */
+	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
+
+	err = stat(NFSD_THREAD_FILE, &statbuf);
+	if (err == 0)
+		return;
+
+	if (errno == ENOENT)
+		xlog(L_ERROR, "Unable to mount nfsdfs on %s. Falling back "
+			      "to legacy nfsctl() interface. Some command "
+			      "line options may not work correctly.",
+			      NFSD_FS_DIR);
+	else
+		xlog(L_ERROR, "Unable to stat %s after attempting to mount "
+			      "%s. Falling back to legacy nfsctl() interface: "
+			      "errno %d (%m)",
+				NFSD_THREAD_FILE, NFSD_FS_DIR, errno);
+}
+
+/*
  * 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 (try #2)

[J. Bruce Fields]

On Tue, Aug 31, 2010 at 03:32:40PM -0400, Jeff Layton wrote:
> This patch is the second attempt at fixing this problem. It's basically
> the same as the first patch, but adds some xlog() calls to log info
> when there are problems. I also changed the NFSD_*_FILE definitions
> around so that they are defined based on another constant that holds
> the nfsdfs mountpoint.
> 
> ---------------------------[snip]-------------------------
> 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.

Seems OK to me.  If it's running /bin/mount instead of doing the mount
by hand that bothers you, we could just decide not to care about
/etc/mtab.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/nfsd/nfsd.c   |    3 ++
>  utils/nfsd/nfssvc.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  utils/nfsd/nfssvc.h |    1 +
>  3 files changed, 54 insertions(+), 3 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..636b403 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"
> @@ -31,9 +33,10 @@
>   */
>  #undef IPV6_SUPPORTED
>  
> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>  
>  /*
>   * declaring a common static scratch buffer here keeps us from having to
> @@ -44,6 +47,50 @@
>  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)
> +		return;
> +	else if (errno != ENOENT) {
> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> +				NFSD_THREAD_FILE, errno);
> +		return;
> +	}
> +
> +	/*
> +	 * this call can return an error if modprobe is set up to automatically
> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> +	 * code from it and just check for the "threads" file afterward.
> +	 */
> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +
> +	if (errno == ENOENT)
> +		xlog(L_ERROR, "Unable to mount nfsdfs on %s. Falling back "
> +			      "to legacy nfsctl() interface. Some command "
> +			      "line options may not work correctly.",
> +			      NFSD_FS_DIR);
> +	else
> +		xlog(L_ERROR, "Unable to stat %s after attempting to mount "
> +			      "%s. Falling back to legacy nfsctl() interface: "
> +			      "errno %d (%m)",
> +				NFSD_THREAD_FILE, NFSD_FS_DIR, errno);
> +}
> +
> +/*
>   * 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 (try #2)

[Jeff Layton]

On Wed, 1 Sep 2010 16:48:18 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Aug 31, 2010 at 03:32:40PM -0400, Jeff Layton wrote:
> > This patch is the second attempt at fixing this problem. It's basically
> > the same as the first patch, but adds some xlog() calls to log info
> > when there are problems. I also changed the NFSD_*_FILE definitions
> > around so that they are defined based on another constant that holds
> > the nfsdfs mountpoint.
> > 
> > ---------------------------[snip]-------------------------
> > 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.
> 
> Seems OK to me.  If it's running /bin/mount instead of doing the mount
> by hand that bothers you, we could just decide not to care about
> /etc/mtab.
> 
> --b.
> 
Thanks.

Sorry, I just cut and pasted that from the original email...I should
have probably trimmed that out.

I'm less leery of it now that we've discussed it. I think the patch is
probably ok as-is. After all, modprobe calls the exact same command.

While /etc/mtab isn't what I'd call reliable info, it's probably best
to try and keep it updated.

That said, Steve mentioned to me privately that he thought the log
messages ought to be reworded. He was going to send a new patch that
does that. I'm sort of waiting on that at this point...

> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  utils/nfsd/nfsd.c   |    3 ++
> >  utils/nfsd/nfssvc.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  utils/nfsd/nfssvc.h |    1 +
> >  3 files changed, 54 insertions(+), 3 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..636b403 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"
> > @@ -31,9 +33,10 @@
> >   */
> >  #undef IPV6_SUPPORTED
> >  
> > -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> > -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> > -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> > +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> > +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> > +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> > +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
> >  
> >  /*
> >   * declaring a common static scratch buffer here keeps us from having to
> > @@ -44,6 +47,50 @@
> >  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)
> > +		return;
> > +	else if (errno != ENOENT) {
> > +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> > +				NFSD_THREAD_FILE, errno);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * this call can return an error if modprobe is set up to automatically
> > +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> > +	 * code from it and just check for the "threads" file afterward.
> > +	 */
> > +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> > +
> > +	err = stat(NFSD_THREAD_FILE, &statbuf);
> > +	if (err == 0)
> > +		return;
> > +
> > +	if (errno == ENOENT)
> > +		xlog(L_ERROR, "Unable to mount nfsdfs on %s. Falling back "
> > +			      "to legacy nfsctl() interface. Some command "
> > +			      "line options may not work correctly.",
> > +			      NFSD_FS_DIR);
> > +	else
> > +		xlog(L_ERROR, "Unable to stat %s after attempting to mount "
> > +			      "%s. Falling back to legacy nfsctl() interface: "
> > +			      "errno %d (%m)",
> > +				NFSD_THREAD_FILE, NFSD_FS_DIR, errno);
> > +}
> > +
> > +/*
> >   * 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
> > 


-- 
Jeff Layton <jlayton@redhat.com>

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

[Neil Brown]

Hi,
 I was musing about this and thought I would share my musings - not to be
 taken too seriously unless they resonate with you.

 If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
 which should be running before nfsd is started?
 Should it 'exportfs -av' too?

 Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
 It already runs statd as requimred, which in-turn runs sm-notify, though it
 is really best to run that much earlier.

 How far do we really want to go with this "just do the right thing" approach?

 Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
 whatever your favourite distro calls it?  Or should it just be a tool for
 managing the nfsd threads?


 I could imagine that it would be OK to add a '-L' flag to say 'use legacy
 interface' and have rpc.nfsd fail noisily if nfsdfs wasn't mounted, and -L
 wasn't given.

 I could imagine that if protocol options were specified with -L, that would
 be an error.

 And if protocol options are specified which differ from what is currently
 in-force, and there are active threads, rpc.nfsd could:
    reduce the number of threads to 0
    change the protocol options
    restore the number of threads



 I think it is (long past) time to deprecate the legacy support in the kernel
 at least.  I suggest we add a printk to sys_nfsservctl to warn of pending
 deprecation, and probably add an entry to
 Documentation/feature-removal-schedule.txt.
 Aim for removing it all in 2.6.40 ?? and make it a CONFIG option before that.

 BTW, the 'new' cache has was added during 2.5 - definitely before Jan 2003.
 So the old stuff really is quite old now.

NeilBrown

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

[Jeff Layton]

On Thu, 2 Sep 2010 07:31:56 +1000
Neil Brown <neilb@suse.de> wrote:

> 
> Hi,
>  I was musing about this and thought I would share my musings - not to be
>  taken too seriously unless they resonate with you.
> 
>  If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
>  which should be running before nfsd is started?
>  Should it 'exportfs -av' too?
> 
>  Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
>  It already runs statd as requimred, which in-turn runs sm-notify, though it
>  is really best to run that much earlier.
> 
>  How far do we really want to go with this "just do the right thing" approach?
> 
>  Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
>  whatever your favourite distro calls it?  Or should it just be a tool for
>  managing the nfsd threads?
> 

That's the way I envision it. rpc.nfsd is a tool for managing the nfsd
threads, and the protocol versions and sockets that they use.

Personally I see a clear delineation here. It's ok for rpc.nfsd to
mount up /proc/fs/nfsd since it needs that interface to properly do its
job. Doing things like starting statd or mounting up pipefs are outside
of its scope.

It just seems a little weird to me that we ought to mandate that
someone run "modprobe nfsd" or whatever before they can actually do
anything with rpc.nfsd.

> 
>  I could imagine that it would be OK to add a '-L' flag to say 'use legacy
>  interface' and have rpc.nfsd fail noisily if nfsdfs wasn't mounted, and -L
>  wasn't given.
> 
>  I could imagine that if protocol options were specified with -L, that would
>  be an error.
> 
>  And if protocol options are specified which differ from what is currently
>  in-force, and there are active threads, rpc.nfsd could:
>     reduce the number of threads to 0
>     change the protocol options
>     restore the number of threads
> 
> 
> 
>  I think it is (long past) time to deprecate the legacy support in the kernel
>  at least.  I suggest we add a printk to sys_nfsservctl to warn of pending
>  deprecation, and probably add an entry to
>  Documentation/feature-removal-schedule.txt.
>  Aim for removing it all in 2.6.40 ?? and make it a CONFIG option before that.
> 
>  BTW, the 'new' cache has was added during 2.5 - definitely before Jan 2003.
>  So the old stuff really is quite old now.
> 

Deprecating it in kernel via your plan sounds good to me.

Failing noisily if /proc/fs/nfsd isn't mounted was what Steve was
suggesting. What would be the benefit there of not attempting to mount
nfsdfs first?

It would certainly be simpler, but is it likely to lead to any harm? If
not, it seems like a reasonable way to limit user confusion. It's hard
to understate the value of having it "just work".

The '-L' flag was sort of what I was thinking might be a good long
term plan, though I was leaning toward a compile-time switch. For
instance, "./configure --with-legacy-nfsd-interface".

We could continue to support nfsctl() for a period of time in mainline
nfs-utils via that switch. The actual mechanics of how to do that would
need to be worked out though.

-- 
Jeff Layton <jlayton@redhat.com>

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

[J. Bruce Fields]

On Thu, Sep 02, 2010 at 07:31:56AM +1000, Neil Brown wrote:
>  BTW, the 'new' cache has was added during 2.5 - definitely before Jan 2003.
>  So the old stuff really is quite old now.

Aren't a lot of the home-NAS-box-hackers stuck on 2.4 kernels?  (Thanks
to proprietary drivers.)

I suppose it's a small niche, and they probably don't need the latest
nfs-utils.

--b.

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

[Steve Dickson]

Hey,

On 09/01/2010 05:31 PM, Neil Brown wrote:
> 
> Hi,
>  I was musing about this and thought I would share my musings - not to be
>  taken too seriously unless they resonate with you.
> 
>  If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
>  which should be running before nfsd is started?
>  Should it 'exportfs -av' too?
> 
>  Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
>  It already runs statd as requimred, which in-turn runs sm-notify, though it
>  is really best to run that much earlier.
> 
>  How far do we really want to go with this "just do the right thing" approach?
Good point... My original thoughts were just to exit if /proc/fs/nfsd was
not mounted, which would be the simplest way... But in the name of "things
just working" I feel having rpc.nfsd trying the mount makes some sense... 

> 
>  Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
>  whatever your favourite distro calls it?  Or should it just be a tool for
>  managing the nfsd threads?
No and yes.. 

I also see rpc.nfsd becoming the real daemon which will take care of 
all the upcalls from the kernel. Basically ripping out all the upcall code
out of rpc.mountd and putting it into rpc.nfsd. That way rpc.mountd will
not have to run in V4-only environments.

> 
> 
>  I could imagine that it would be OK to add a '-L' flag to say 'use legacy
>  interface' and have rpc.nfsd fail noisily if nfsdfs wasn't mounted, and -L
>  wasn't given.
> 
>  I could imagine that if protocol options were specified with -L, that would
>  be an error.
Hmm... The first thing that comes to my mind is slipper slope... I think
we just deprecate interface and move on...

> 
>  And if protocol options are specified which differ from what is currently
>  in-force, and there are active threads, rpc.nfsd could:
>     reduce the number of threads to 0
>     change the protocol options
>     restore the number of threads
> 
> 
> 
>  I think it is (long past) time to deprecate the legacy support in the kernel
>  at least.  I suggest we add a printk to sys_nfsservctl to warn of pending
>  deprecation, and probably add an entry to
>  Documentation/feature-removal-schedule.txt.
>  Aim for removing it all in 2.6.40 ?? and make it a CONFIG option before that.
I second this...

> 
>  BTW, the 'new' cache has was added during 2.5 - definitely before Jan 2003.
>  So the old stuff really is quite old now.
Wow... it is time...

steved.

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

[Jeff Layton]

On Thu, 02 Sep 2010 07:29:04 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> Hey,
> 
> On 09/01/2010 05:31 PM, Neil Brown wrote:
> > 
> > Hi,
> >  I was musing about this and thought I would share my musings - not to be
> >  taken too seriously unless they resonate with you.
> > 
> >  If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
> >  which should be running before nfsd is started?
> >  Should it 'exportfs -av' too?
> > 
> >  Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
> >  It already runs statd as requimred, which in-turn runs sm-notify, though it
> >  is really best to run that much earlier.
> > 
> >  How far do we really want to go with this "just do the right thing" approach?
> Good point... My original thoughts were just to exit if /proc/fs/nfsd was
> not mounted, which would be the simplest way... But in the name of "things
> just working" I feel having rpc.nfsd trying the mount makes some sense... 
> 
> > 
> >  Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
> >  whatever your favourite distro calls it?  Or should it just be a tool for
> >  managing the nfsd threads?
> No and yes.. 
> 
> I also see rpc.nfsd becoming the real daemon which will take care of 
> all the upcalls from the kernel. Basically ripping out all the upcall code
> out of rpc.mountd and putting it into rpc.nfsd. That way rpc.mountd will
> not have to run in V4-only environments.
> 

That seems like a lot of work for little gain. rpc.nfsd (despite its
'd' suffix) is currently just a "control program". It does its thing
and then exits.

If you do the above then it'll have to live as a full-fledged daemon.
Won't we just be trading rpc.mountd for rpc.nfsd at that point? What
would be the benefit?

If you're concerned about the sockets that mountd listens on, I think
that Chuck had a patch at one point that made it possible to run mountd
as an "upcall-only" daemon for v4-only servers.

> > 
> > 
> >  I could imagine that it would be OK to add a '-L' flag to say 'use legacy
> >  interface' and have rpc.nfsd fail noisily if nfsdfs wasn't mounted, and -L
> >  wasn't given.
> > 
> >  I could imagine that if protocol options were specified with -L, that would
> >  be an error.
> Hmm... The first thing that comes to my mind is slipper slope... I think
> we just deprecate interface and move on...
> 

Agreed. Let's go ahead and start deprecating the kernel interface. We
can decide afterward what to do in userspace.

-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

On Sep 2, 2010, at 7:55 AM, Jeff Layton wrote:

> On Thu, 02 Sep 2010 07:29:04 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Hey,
>> 
>> On 09/01/2010 05:31 PM, Neil Brown wrote:
>>> 
>>> Hi,
>>> I was musing about this and thought I would share my musings - not to be
>>> taken too seriously unless they resonate with you.
>>> 
>>> If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
>>> which should be running before nfsd is started?
>>> Should it 'exportfs -av' too?
>>> 
>>> Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
>>> It already runs statd as requimred, which in-turn runs sm-notify, though it
>>> is really best to run that much earlier.
>>> 
>>> How far do we really want to go with this "just do the right thing" approach?
>> Good point... My original thoughts were just to exit if /proc/fs/nfsd was
>> not mounted, which would be the simplest way... But in the name of "things
>> just working" I feel having rpc.nfsd trying the mount makes some sense... 
>> 
>>> 
>>> Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
>>> whatever your favourite distro calls it?  Or should it just be a tool for
>>> managing the nfsd threads?
>> No and yes.. 
>> 
>> I also see rpc.nfsd becoming the real daemon which will take care of 
>> all the upcalls from the kernel. Basically ripping out all the upcall code
>> out of rpc.mountd and putting it into rpc.nfsd. That way rpc.mountd will
>> not have to run in V4-only environments.
>> 
> 
> That seems like a lot of work for little gain. rpc.nfsd (despite its
> 'd' suffix) is currently just a "control program". It does its thing
> and then exits.
> 
> If you do the above then it'll have to live as a full-fledged daemon.
> Won't we just be trading rpc.mountd for rpc.nfsd at that point? What
> would be the benefit?
> 
> If you're concerned about the sockets that mountd listens on, I think
> that Chuck had a patch at one point that made it possible to run mountd
> as an "upcall-only" daemon for v4-only servers.

Neil's patches are now in upstream nfs-utils.  You can disable all of the network listeners with command line options on mountd.

It's been said that people complain that they don't like running rpc.mountd on NFSv4-only servers.  I'm not sure why that's a problem we have to fix with a code change.  Better documentation, better automatic configuration detection in the NFS start-up scripts, or simply renaming rpc.mountd could solve this issue without the need for rip-and-replace of well-tested code.

-- 
chuck[dot]lever[at]oracle[dot]com

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

[J. Bruce Fields]

On Thu, Sep 02, 2010 at 10:04:19AM -0400, Chuck Lever wrote:
> Neil's patches are now in upstream nfs-utils.  You can disable all of
> the network listeners with command line options on mountd.
> 
> It's been said that people complain that they don't like running
> rpc.mountd on NFSv4-only servers.  I'm not sure why that's a problem
> we have to fix with a code change.  Better documentation, better
> automatic configuration detection in the NFS start-up scripts, or
> simply renaming rpc.mountd could solve this issue without the need for
> rip-and-replace of well-tested code.

Yeah, I've suggested a separate upcall-handling daemon before, but agree
that for now we should leave well enough alone.

--b.

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

[Chuck Lever]

On Sep 1, 2010, at 5:31 PM, Neil Brown wrote:

> 
> Hi,
> I was musing about this and thought I would share my musings - not to be
> taken too seriously unless they resonate with you.
> 
> If rpc.nfsd is mounting /proc/fs/nfsd, should it also be starting rpc.statd,
> which should be running before nfsd is started?
> Should it 'exportfs -av' too?
> 
> Should mount.nfs be mounting /var/lib/nfs/rpc_pipefs too?
> It already runs statd as requimred, which in-turn runs sm-notify, though it
> is really best to run that much earlier.
> 
> How far do we really want to go with this "just do the right thing" approach?
> 
> Should "rpc.nfsd" be a replacement for /etc/init.d/nfs-kernel-server or
> whatever your favourite distro calls it?  Or should it just be a tool for
> managing the nfsd threads?
> 
> 
> I could imagine that it would be OK to add a '-L' flag to say 'use legacy
> interface' and have rpc.nfsd fail noisily if nfsdfs wasn't mounted, and -L
> wasn't given.
> 
> I could imagine that if protocol options were specified with -L, that would
> be an error.
> 
> And if protocol options are specified which differ from what is currently
> in-force, and there are active threads, rpc.nfsd could:
>    reduce the number of threads to 0
>    change the protocol options
>    restore the number of threads
> 
> 
> 
> I think it is (long past) time to deprecate the legacy support in the kernel
> at least.  I suggest we add a printk to sys_nfsservctl to warn of pending
> deprecation, and probably add an entry to
> Documentation/feature-removal-schedule.txt.
> Aim for removing it all in 2.6.40 ?? and make it a CONFIG option before that.

Based on my experience with crabby packagers of nfs-utils, I think a long term plan is a good idea.  Warnings of deprecation, for now, is an appropriate first step.  And, leaving ample time for people to take the hint is good.  .40 gives us a year.  .44 gives us two years.  Unless it becomes terribly unwieldy, two years is probably a little better, given the glacial rate at which some of the outlying distributions pick up new features.

-- 
chuck[dot]lever[at]oracle[dot]com

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

[Steve Dickson]

On 09/02/2010 10:25 AM, J. Bruce Fields wrote:
> On Thu, Sep 02, 2010 at 10:04:19AM -0400, Chuck Lever wrote:
>> Neil's patches are now in upstream nfs-utils.  You can disable all of
>> the network listeners with command line options on mountd.
>>
>> It's been said that people complain that they don't like running
>> rpc.mountd on NFSv4-only servers.  I'm not sure why that's a problem
>> we have to fix with a code change.  Better documentation, better
>> automatic configuration detection in the NFS start-up scripts, or
>> simply renaming rpc.mountd could solve this issue without the need for
>> rip-and-replace of well-tested code.
I think a lot of the problems come from people having to open up 
their firewalls and such... basically security issues..  was well 
as having "extra" daemons stealing cpu cycles... 

> 
> Yeah, I've suggested a separate upcall-handling daemon before, but agree
> that for now we should leave well enough alone.
What would be the deciding factor to start this work? Maybe when
Trond done splitting up kernel code into separate version modules? 

steved.

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

[J. Bruce Fields]

On Thu, Sep 02, 2010 at 12:41:53PM -0400, Steve Dickson wrote:
> 
> 
> On 09/02/2010 10:25 AM, J. Bruce Fields wrote:
> > On Thu, Sep 02, 2010 at 10:04:19AM -0400, Chuck Lever wrote:
> >> Neil's patches are now in upstream nfs-utils.  You can disable all of
> >> the network listeners with command line options on mountd.
> >>
> >> It's been said that people complain that they don't like running
> >> rpc.mountd on NFSv4-only servers.  I'm not sure why that's a problem
> >> we have to fix with a code change.  Better documentation, better
> >> automatic configuration detection in the NFS start-up scripts, or
> >> simply renaming rpc.mountd could solve this issue without the need for
> >> rip-and-replace of well-tested code.
> I think a lot of the problems come from people having to open up 
> their firewalls and such... basically security issues..  was well 
> as having "extra" daemons stealing cpu cycles... 
> 
> > 
> > Yeah, I've suggested a separate upcall-handling daemon before, but agree
> > that for now we should leave well enough alone.
> What would be the deciding factor to start this work? Maybe when
> Trond done splitting up kernel code into separate version modules? 

I think it's at most an idea to keep in the back of our minds in case it
looks useful some day--not a todo we should schedule.

--b.

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

[Jeff Layton]

On Tue, 31 Aug 2010 15:32:40 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> This patch is the second attempt at fixing this problem. It's basically
> the same as the first patch, but adds some xlog() calls to log info
> when there are problems. I also changed the NFSD_*_FILE definitions
> around so that they are defined based on another constant that holds
> the nfsdfs mountpoint.
> 
> ---------------------------[snip]-------------------------
> 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 |   53 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  utils/nfsd/nfssvc.h |    1 +
>  3 files changed, 54 insertions(+), 3 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..636b403 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"
> @@ -31,9 +33,10 @@
>   */
>  #undef IPV6_SUPPORTED
>  
> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>  
>  /*
>   * declaring a common static scratch buffer here keeps us from having to
> @@ -44,6 +47,50 @@
>  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)
> +		return;
> +	else if (errno != ENOENT) {
> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> +				NFSD_THREAD_FILE, errno);
> +		return;
> +	}
> +
> +	/*
> +	 * this call can return an error if modprobe is set up to automatically
> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> +	 * code from it and just check for the "threads" file afterward.
> +	 */
> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +
> +	if (errno == ENOENT)
> +		xlog(L_ERROR, "Unable to mount nfsdfs on %s. Falling back "
> +			      "to legacy nfsctl() interface. Some command "
> +			      "line options may not work correctly.",
> +			      NFSD_FS_DIR);
> +	else
> +		xlog(L_ERROR, "Unable to stat %s after attempting to mount "
> +			      "%s. Falling back to legacy nfsctl() interface: "
> +			      "errno %d (%m)",
> +				NFSD_THREAD_FILE, NFSD_FS_DIR, errno);
> +}
> +
> +/*
>   * 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);

Hi Steve,

It's been a couple of weeks and this latest patch hasn't been committed...

IIRC, you wanted changes to the xlog messages and were going to send an
alternate patch. I'd like to see this in the next release of nfs-utils.
What do we need to do to move this to resolution?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve Dickson]

On 09/14/2010 09:23 AM, Jeff Layton wrote:
> On Tue, 31 Aug 2010 15:32:40 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> Hi Steve,
> 
> It's been a couple of weeks and this latest patch hasn't been committed...
> 
> IIRC, you wanted changes to the xlog messages and were going to send an
> alternate patch. I'd like to see this in the next release of nfs-utils.
> What do we need to do to move this to resolution?
Sorry for taking so long to get to this..  

What I was thinking is to simply make the error messages more definitive 
as to what the problem is and how to fix it.. Logging that we are 
falling back to the legacy probably would not mean much to mere 
mortals... IMHO... 

I still think at some point nfsd should fail to start up, but logging
an error message at this point is fine..

Comments?

steved.

commit 8a78cb2e651d55fc1aaacf1e4ca9c009d1cf9113
Author: Jeff Layton <jlayton@redhat.com>
Date:   Wed Sep 15 15:54:36 2010 -0400

    rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet (try #3)
    
    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.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 1cda1e5..658b8fa 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(progname);
+
 	/* 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..f7edcba 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"
@@ -31,9 +33,13 @@
  */
 #undef IPV6_SUPPORTED
 
-#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
-#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
-#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
+#ifndef NFSD_FS_DIR
+#define NFSD_FS_DIR	  "/proc/fs/nfsd"
+#endif
+
+#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
+#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
+#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
 
 /*
  * declaring a common static scratch buffer here keeps us from having to
@@ -44,6 +50,45 @@
 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(char *progname)
+{
+	int err;
+	struct stat statbuf;
+
+	err = stat(NFSD_THREAD_FILE, &statbuf);
+	if (err == 0)
+		return;
+	else if (errno != ENOENT) {
+		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
+				NFSD_THREAD_FILE, errno);
+		return;
+	}
+
+	/*
+	 * this call can return an error if modprobe is set up to automatically
+	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
+	 * code from it and just check for the "threads" file afterward.
+	 */
+	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
+
+	err = stat(NFSD_THREAD_FILE, &statbuf);
+	if (err == 0)
+		return;
+
+	xlog(L_ERROR, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
+		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
+		"' and then restart %s to correct the problem", errno, progname);
+
+	return;
+}
+
+/*
  * 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..1a01cec 100644
--- a/utils/nfsd/nfssvc.h
+++ b/utils/nfsd/nfssvc.h
@@ -20,6 +20,7 @@
  *
  */
 
+void	nfssvc_mount_nfsdfs(char *progname);
 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 (try #2)

[Jeff Layton]

On Wed, 15 Sep 2010 16:09:02 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> On 09/14/2010 09:23 AM, Jeff Layton wrote:
> > On Tue, 31 Aug 2010 15:32:40 -0400
> > Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > Hi Steve,
> > 
> > It's been a couple of weeks and this latest patch hasn't been committed...
> > 
> > IIRC, you wanted changes to the xlog messages and were going to send an
> > alternate patch. I'd like to see this in the next release of nfs-utils.
> > What do we need to do to move this to resolution?
> Sorry for taking so long to get to this..  
> 
> What I was thinking is to simply make the error messages more definitive 
> as to what the problem is and how to fix it.. Logging that we are 
> falling back to the legacy probably would not mean much to mere 
> mortals... IMHO... 
> 
> I still think at some point nfsd should fail to start up, but logging
> an error message at this point is fine..
> 
> Comments?
> 
> steved.
> 
> commit 8a78cb2e651d55fc1aaacf1e4ca9c009d1cf9113
> Author: Jeff Layton <jlayton@redhat.com>
> Date:   Wed Sep 15 15:54:36 2010 -0400
> 
>     rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet (try #3)
>     
>     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.
>     
>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..658b8fa 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(progname);
> +
>  	/* 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..f7edcba 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"
> @@ -31,9 +33,13 @@
>   */
>  #undef IPV6_SUPPORTED
>  
> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> +#ifndef NFSD_FS_DIR
> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> +#endif
> +
> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>  
>  /*
>   * declaring a common static scratch buffer here keeps us from having to
> @@ -44,6 +50,45 @@
>  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(char *progname)
> +{
> +	int err;
> +	struct stat statbuf;
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +	else if (errno != ENOENT) {
> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> +				NFSD_THREAD_FILE, errno);
> +		return;
> +	}
> +
> +	/*
> +	 * this call can return an error if modprobe is set up to automatically
> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> +	 * code from it and just check for the "threads" file afterward.
> +	 */
> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +
> +	xlog(L_ERROR, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
> +		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
> +		"' and then restart %s to correct the problem", errno, progname);
> +

A few of problems with that log message...

First, it looks like something failed, but nfsd will probably end up
getting started anyway. We recommend starting nfsd to correct the
problem, but it's not clear what the problem actually is. nfsd got
started right?

Second, it says to restart nfsd to correct the problem, but restarting
nfsd isn't exactly straightforward (you need to run "nfsd 0" and then
nfsd <some positive int>).

Third, there could be other problems than just the fact that nfsdfs
didn't get mounted. Maybe there was some weird SELinux permissions
problem? In that case, trying to mount nfsdfs won't help anything and
that message will just confuse the issue.

I think it's going to be really difficult to explain the problem and
solution in a terse xlog message. My expectation was mainly that we
should pop up a message to give the user a heads up that "something" is
wrong. I think it's tough to offer specific solutions in such a message
though.


> +	return;
> +}
> +
> +/*
>   * 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..1a01cec 100644
> --- a/utils/nfsd/nfssvc.h
> +++ b/utils/nfsd/nfssvc.h
> @@ -20,6 +20,7 @@
>   *
>   */
>  
> +void	nfssvc_mount_nfsdfs(char *progname);
>  int	nfssvc_inuse(void);
>  int	nfssvc_set_sockets(const int family, const unsigned int protobits,
>  			   const char *host, const char *port);
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve Dickson]

On 09/15/2010 06:31 PM, Jeff Layton wrote:
> On Wed, 15 Sep 2010 16:09:02 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> On 09/14/2010 09:23 AM, Jeff Layton wrote:
>>> On Tue, 31 Aug 2010 15:32:40 -0400
>>> Jeff Layton <jlayton@redhat.com> wrote:
>>>
>>> Hi Steve,
>>>
>>> It's been a couple of weeks and this latest patch hasn't been committed...
>>>
>>> IIRC, you wanted changes to the xlog messages and were going to send an
>>> alternate patch. I'd like to see this in the next release of nfs-utils.
>>> What do we need to do to move this to resolution?
>> Sorry for taking so long to get to this..  
>>
>> What I was thinking is to simply make the error messages more definitive 
>> as to what the problem is and how to fix it.. Logging that we are 
>> falling back to the legacy probably would not mean much to mere 
>> mortals... IMHO... 
>>
>> I still think at some point nfsd should fail to start up, but logging
>> an error message at this point is fine..
>>
>> Comments?
>>
>> steved.
>>
>> commit 8a78cb2e651d55fc1aaacf1e4ca9c009d1cf9113
>> Author: Jeff Layton <jlayton@redhat.com>
>> Date:   Wed Sep 15 15:54:36 2010 -0400
>>
>>     rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet (try #3)
>>     
>>     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.
>>     
>>     Signed-off-by: Steve Dickson <steved@redhat.com>
>>
>> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
>> index 1cda1e5..658b8fa 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(progname);
>> +
>>  	/* 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..f7edcba 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"
>> @@ -31,9 +33,13 @@
>>   */
>>  #undef IPV6_SUPPORTED
>>  
>> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
>> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
>> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
>> +#ifndef NFSD_FS_DIR
>> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
>> +#endif
>> +
>> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
>> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
>> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>>  
>>  /*
>>   * declaring a common static scratch buffer here keeps us from having to
>> @@ -44,6 +50,45 @@
>>  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(char *progname)
>> +{
>> +	int err;
>> +	struct stat statbuf;
>> +
>> +	err = stat(NFSD_THREAD_FILE, &statbuf);
>> +	if (err == 0)
>> +		return;
>> +	else if (errno != ENOENT) {
>> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
>> +				NFSD_THREAD_FILE, errno);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * this call can return an error if modprobe is set up to automatically
>> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
>> +	 * code from it and just check for the "threads" file afterward.
>> +	 */
>> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
>> +
>> +	err = stat(NFSD_THREAD_FILE, &statbuf);
>> +	if (err == 0)
>> +		return;
>> +
>> +	xlog(L_ERROR, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
>> +		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
>> +		"' and then restart %s to correct the problem", errno, progname);
>> +
> 
> A few of problems with that log message...
> 
> First, it looks like something failed, but nfsd will probably end up
> getting started anyway. We recommend starting nfsd to correct the
> problem, but it's not clear what the problem actually is. nfsd got
> started right?
Hmm... I was thinking this part of the message
    "Unable to access " NFSD_FS_DIR " errno %d (%m)."
made it clear as to what the problem is. 

We are making note of a non fatal error... and since
the message is being written to stderr, instead if 
syslog()-ed, the actual message look more like a note 
then an error since the 'ERROR' string is not be added.
We could change the message to a L_WARNING... 

> 
> Second, it says to restart nfsd to correct the problem, but restarting
> nfsd isn't exactly straightforward (you need to run "nfsd 0" and then
> nfsd <some positive int>).
Well I'm making the assumption that people who do see this
message did not use the normal init-script way of starting rpc.nfsd,
because if they did they would not be seeing this message. So,
again I'm assuming, that these same people are starting rpc.nfsd
my hand, which means they know how to restart rpc.nfsd.

> 
> Third, there could be other problems than just the fact that nfsdfs
> didn't get mounted. Maybe there was some weird SELinux permissions
> problem? In that case, trying to mount nfsdfs won't help anything and
> that message will just confuse the issue.
If its an SELinux error or some issue that causing either the stat()
and mount() to fail, hopefully those type of issues will be logged. 
Plus having them try the mount by hand could possibly lead them in the 
right direction.

> 
> I think it's going to be really difficult to explain the problem and
> solution in a terse xlog message. My expectation was mainly that we
> should pop up a message to give the user a heads up that "something" is
> wrong. I think it's tough to offer specific solutions in such a message
> though.
All I'm trying to is log an non-fatal error,  give a reason
as to why the error happen and a possible resolution to that error.
Since were are not logging the fact the system() call failed, I
thought it was a fairly reasonable thing to do....


steved.

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

[Jeff Layton]

On Thu, 16 Sep 2010 07:06:20 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> >> +
> >> +	/*
> >> +	 * this call can return an error if modprobe is set up to automatically
> >> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> >> +	 * code from it and just check for the "threads" file afterward.
> >> +	 */
> >> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> >> +
> >> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> >> +	if (err == 0)
> >> +		return;
> >> +
> >> +	xlog(L_ERROR, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
> >> +		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
> >> +		"' and then restart %s to correct the problem", errno, progname);
> >> +
> > 
> > A few of problems with that log message...
> > 
> > First, it looks like something failed, but nfsd will probably end up
> > getting started anyway. We recommend starting nfsd to correct the
> > problem, but it's not clear what the problem actually is. nfsd got
> > started right?
> Hmm... I was thinking this part of the message
>     "Unable to access " NFSD_FS_DIR " errno %d (%m)."
> made it clear as to what the problem is. 
> 
> We are making note of a non fatal error... and since
> the message is being written to stderr, instead if 
> syslog()-ed, the actual message look more like a note 
> then an error since the 'ERROR' string is not be added.
> We could change the message to a L_WARNING... 
> 

L_WARNING would be more appropriate I think. But the typical user is
going to "huh?" with this error message. It says something about a
"problem" but we're not being clear as to what the problem is. After
all, nfsd got started so it's not likely to be immediately clear to the
uninitiated person.

> > 
> > Second, it says to restart nfsd to correct the problem, but restarting
> > nfsd isn't exactly straightforward (you need to run "nfsd 0" and then
> > nfsd <some positive int>).
> Well I'm making the assumption that people who do see this
> message did not use the normal init-script way of starting rpc.nfsd,
> because if they did they would not be seeing this message. So,
> again I'm assuming, that these same people are starting rpc.nfsd
> my hand, which means they know how to restart rpc.nfsd.
> 

That depends very much on the init script. We know that Fedora and
RHEL, and probably others too, will plug in nfsd.ko before running
rpc.nfsd. Based on the report by Tetsuo a couple of months ago, I think
it's safe to assume that at least some Debian userspace doesn't do
that. By extension, that probably means that Ubuntu doesn't either. It's
also quite plausible that many smaller, distros (OpenWRT, for instance)
won't. Personally, I expect that once we do this we're going to find
that a large number of distros will need to fix their init scripts.

> > 
> > Third, there could be other problems than just the fact that nfsdfs
> > didn't get mounted. Maybe there was some weird SELinux permissions
> > problem? In that case, trying to mount nfsdfs won't help anything and
> > that message will just confuse the issue.
> If its an SELinux error or some issue that causing either the stat()
> and mount() to fail, hopefully those type of issues will be logged. 
> Plus having them try the mount by hand could possibly lead them in the 
> right direction.
> 

Fair enough.

> > 
> > I think it's going to be really difficult to explain the problem and
> > solution in a terse xlog message. My expectation was mainly that we
> > should pop up a message to give the user a heads up that "something" is
> > wrong. I think it's tough to offer specific solutions in such a message
> > though.
> All I'm trying to is log an non-fatal error,  give a reason
> as to why the error happen and a possible resolution to that error.
> Since were are not logging the fact the system() call failed, I
> thought it was a fairly reasonable thing to do....
> 

It is. The main issue is that we have a somewhat complex situation here
if the mount ended up not working. I attempted to deal with it by being
somewhat vague. That said...we're getting into bikeshed paint colors
here. If it helps move things along, let's go with your version and
call it a day...

-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve Dickson]

On 09/16/2010 07:32 AM, Jeff Layton wrote:
> On Thu, 16 Sep 2010 07:06:20 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>>> +
>>>> +	/*
>>>> +	 * this call can return an error if modprobe is set up to automatically
>>>> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
>>>> +	 * code from it and just check for the "threads" file afterward.
>>>> +	 */
>>>> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
>>>> +
>>>> +	err = stat(NFSD_THREAD_FILE, &statbuf);
>>>> +	if (err == 0)
>>>> +		return;
>>>> +
>>>> +	xlog(L_ERROR, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
>>>> +		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
>>>> +		"' and then restart %s to correct the problem", errno, progname);
>>>> +
>>>
>>> A few of problems with that log message...
>>>
>>> First, it looks like something failed, but nfsd will probably end up
>>> getting started anyway. We recommend starting nfsd to correct the
>>> problem, but it's not clear what the problem actually is. nfsd got
>>> started right?
>> Hmm... I was thinking this part of the message
>>     "Unable to access " NFSD_FS_DIR " errno %d (%m)."
>> made it clear as to what the problem is. 
>>
>> We are making note of a non fatal error... and since
>> the message is being written to stderr, instead if 
>> syslog()-ed, the actual message look more like a note 
>> then an error since the 'ERROR' string is not be added.
>> We could change the message to a L_WARNING... 
>>
> 
> L_WARNING would be more appropriate I think. But the typical user is
> going to "huh?" with this error message. It says something about a
> "problem" but we're not being clear as to what the problem is. After
> all, nfsd got started so it's not likely to be immediately clear to the
> uninitiated person.
L_WARNING it is...

> 
>>>
>>> Second, it says to restart nfsd to correct the problem, but restarting
>>> nfsd isn't exactly straightforward (you need to run "nfsd 0" and then
>>> nfsd <some positive int>).
>> Well I'm making the assumption that people who do see this
>> message did not use the normal init-script way of starting rpc.nfsd,
>> because if they did they would not be seeing this message. So,
>> again I'm assuming, that these same people are starting rpc.nfsd
>> my hand, which means they know how to restart rpc.nfsd.
>>
> 
> That depends very much on the init script. We know that Fedora and
> RHEL, and probably others too, will plug in nfsd.ko before running
> rpc.nfsd. Based on the report by Tetsuo a couple of months ago, I think
> it's safe to assume that at least some Debian userspace doesn't do
> that. By extension, that probably means that Ubuntu doesn't either. It's
> also quite plausible that many smaller, distros (OpenWRT, for instance)
> won't. Personally, I expect that once we do this we're going to find
> that a large number of distros will need to fix their init scripts.
Agreed...

> 
>>>
>>> Third, there could be other problems than just the fact that nfsdfs
>>> didn't get mounted. Maybe there was some weird SELinux permissions
>>> problem? In that case, trying to mount nfsdfs won't help anything and
>>> that message will just confuse the issue.
>> If its an SELinux error or some issue that causing either the stat()
>> and mount() to fail, hopefully those type of issues will be logged. 
>> Plus having them try the mount by hand could possibly lead them in the 
>> right direction.
>>
> 
> Fair enough.
> 
>>>
>>> I think it's going to be really difficult to explain the problem and
>>> solution in a terse xlog message. My expectation was mainly that we
>>> should pop up a message to give the user a heads up that "something" is
>>> wrong. I think it's tough to offer specific solutions in such a message
>>> though.
>> All I'm trying to is log an non-fatal error,  give a reason
>> as to why the error happen and a possible resolution to that error.
>> Since were are not logging the fact the system() call failed, I
>> thought it was a fairly reasonable thing to do....
>>
> 
> It is. The main issue is that we have a somewhat complex situation here
> if the mount ended up not working. I attempted to deal with it by being
> somewhat vague. That said...we're getting into bikeshed paint colors
> here. If it helps move things along, let's go with your version and
> call it a day...
Cool... thanks taking the time...

steved.

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

[Steve Dickson]

On 09/15/2010 04:09 PM, Steve Dickson wrote:
> On 09/14/2010 09:23 AM, Jeff Layton wrote:
>> On Tue, 31 Aug 2010 15:32:40 -0400
>> Jeff Layton <jlayton@redhat.com> wrote:
>>
>> Hi Steve,
>>
>> It's been a couple of weeks and this latest patch hasn't been committed...
>>
>> IIRC, you wanted changes to the xlog messages and were going to send an
>> alternate patch. I'd like to see this in the next release of nfs-utils.
>> What do we need to do to move this to resolution?
> Sorry for taking so long to get to this..  
> 
> What I was thinking is to simply make the error messages more definitive 
> as to what the problem is and how to fix it.. Logging that we are 
> falling back to the legacy probably would not mean much to mere 
> mortals... IMHO... 
> 
> I still think at some point nfsd should fail to start up, but logging
> an error message at this point is fine..
> 
> Comments?
> 
> steved.
> 
> commit 8a78cb2e651d55fc1aaacf1e4ca9c009d1cf9113
> Author: Jeff Layton <jlayton@redhat.com>
> Date:   Wed Sep 15 15:54:36 2010 -0400
> 
>     rpc.nfsd: mount up nfsdfs is it doesn't appear to be mounted yet (try #3)
>     
>     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.
>     
>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..658b8fa 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(progname);
> +
>  	/* 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..f7edcba 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"
> @@ -31,9 +33,13 @@
>   */
>  #undef IPV6_SUPPORTED
>  
> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> +#ifndef NFSD_FS_DIR
> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> +#endif
> +
> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>  
>  /*
>   * declaring a common static scratch buffer here keeps us from having to
> @@ -44,6 +50,45 @@
>  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(char *progname)
> +{
> +	int err;
> +	struct stat statbuf;
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +	else if (errno != ENOENT) {
> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> +				NFSD_THREAD_FILE, errno);
> +		return;
> +	}
One minor nit I just noticed...  we really don't need
that else if clause... it can be just an if clause...
unless I'm missing something... 

steved.

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

[Steve Dickson]

Hey Jeff,

Here is the patch with the L_WARNING change and the 
s/else if/if change I just mentioned... 

steved.

Author: Jeff Layton <jlayton@redhat.com>
Date:   Thu Sep 16 08:58:02 2010 -0400
    
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.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 1cda1e5..658b8fa 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(progname);
+
 	/* 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..7693626 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"
@@ -31,9 +33,13 @@
  */
 #undef IPV6_SUPPORTED
 
-#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
-#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
-#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
+#ifndef NFSD_FS_DIR
+#define NFSD_FS_DIR	  "/proc/fs/nfsd"
+#endif
+
+#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
+#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
+#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
 
 /*
  * declaring a common static scratch buffer here keeps us from having to
@@ -44,6 +50,46 @@
 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(char *progname)
+{
+	int err;
+	struct stat statbuf;
+
+	err = stat(NFSD_THREAD_FILE, &statbuf);
+	if (err == 0)
+		return;
+
+	if (errno != ENOENT) {
+		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
+				NFSD_THREAD_FILE, errno);
+		return;
+	}
+
+	/*
+	 * this call can return an error if modprobe is set up to automatically
+	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
+	 * code from it and just check for the "threads" file afterward.
+	 */
+	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
+
+	err = stat(NFSD_THREAD_FILE, &statbuf);
+	if (err == 0)
+		return;
+
+	xlog(L_WARNING, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
+		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
+		"' and then restart %s to correct the problem", errno, progname);
+
+	return;
+}
+
+/*
  * 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..1a01cec 100644
--- a/utils/nfsd/nfssvc.h
+++ b/utils/nfsd/nfssvc.h
@@ -20,6 +20,7 @@
  *
  */
 
+void	nfssvc_mount_nfsdfs(char *progname);
 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 (try #4)

[Jeff Layton]

On Thu, 16 Sep 2010 09:02:56 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> Hey Jeff,
> 
> Here is the patch with the L_WARNING change and the 
> s/else if/if change I just mentioned... 
> 
> steved.
> 
> Author: Jeff Layton <jlayton@redhat.com>
> Date:   Thu Sep 16 08:58:02 2010 -0400
>     
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 1cda1e5..658b8fa 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(progname);
> +
>  	/* 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..7693626 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"
> @@ -31,9 +33,13 @@
>   */
>  #undef IPV6_SUPPORTED
>  
> -#define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> -#define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> -#define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> +#ifndef NFSD_FS_DIR
> +#define NFSD_FS_DIR	  "/proc/fs/nfsd"
> +#endif
> +
> +#define NFSD_PORTS_FILE   NFSD_FS_DIR "/portlist"
> +#define NFSD_VERS_FILE    NFSD_FS_DIR "/versions"
> +#define NFSD_THREAD_FILE  NFSD_FS_DIR "/threads"
>  
>  /*
>   * declaring a common static scratch buffer here keeps us from having to
> @@ -44,6 +50,46 @@
>  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(char *progname)
> +{
> +	int err;
> +	struct stat statbuf;
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +
> +	if (errno != ENOENT) {
> +		xlog(L_ERROR, "Unable to stat %s: errno %d (%m)",
> +				NFSD_THREAD_FILE, errno);
> +		return;
> +	}
> +
> +	/*
> +	 * this call can return an error if modprobe is set up to automatically
> +	 * mount nfsdfs when nfsd.ko is plugged in. So, ignore the return
> +	 * code from it and just check for the "threads" file afterward.
> +	 */
> +	system("/bin/mount -t nfsd nfsd " NFSD_FS_DIR " >/dev/null 2>&1");
> +
> +	err = stat(NFSD_THREAD_FILE, &statbuf);
> +	if (err == 0)
> +		return;
> +
> +	xlog(L_WARNING, "Unable to access " NFSD_FS_DIR " errno %d (%m)." 
> +		"\nPlease try, as root, 'mount -t nfsd nfsd " NFSD_FS_DIR 
> +		"' and then restart %s to correct the problem", errno, progname);
> +
> +	return;
> +}
> +
> +/*
>   * 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..1a01cec 100644
> --- a/utils/nfsd/nfssvc.h
> +++ b/utils/nfsd/nfssvc.h
> @@ -20,6 +20,7 @@
>   *
>   */
>  
> +void	nfssvc_mount_nfsdfs(char *progname);
>  int	nfssvc_inuse(void);
>  int	nfssvc_set_sockets(const int family, const unsigned int protobits,
>  			   const char *host, const char *port);
> 

Looks good to me.
-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve Dickson]

On 09/16/2010 09:02 AM, Steve Dickson wrote:
> Hey Jeff,
> 
> Here is the patch with the L_WARNING change and the 
> s/else if/if change I just mentioned... 
> 
> steved.
> 
> Author: Jeff Layton <jlayton@redhat.com>
> Date:   Thu Sep 16 08:58:02 2010 -0400
>     
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Steve Dickson <steved@redhat.com>
Committed...

steved.