[PATCH 0/3] nfs-utils: multithreaded mountd fixes

[PATCH 0/3] nfs-utils: multithreaded mountd fixes

[Ben Myers]

Hey Steve,

The following fixes a problem with mountd where you get backed up behind the
rmtab lock.  It may have happened that an xflock timed out and stat failed so
the filedescriptor was leaked in mountlist_list and the lock never released.
Skipping the fdatasync has also helped performance-wise.

Tested very lightly on TOT and more heavily on 1.0.7 where this bug was
discovered.

Thanks,
Ben

---

Ben Myers (3):
      nfs-utils: remove xflock timeout
      nfs-utils: dont leak fd in mountlist_list
      nfs-utils: don't fdatasync the rmtab


 support/nfs/cacheio.c |    5 ++---
 support/nfs/rmtab.c   |   23 ++++++++++++++++++-----
 support/nfs/xio.c     |   18 +-----------------
 utils/mountd/rmtab.c  |    5 ++++-
 4 files changed, 25 insertions(+), 26 deletions(-)

-- 

[PATCH 1/3] nfs-utils: remove xflock timeout

[Ben Myers]

Remove this 10 second timeout which can cause unexpected behavior and
corruption in the rmtab when hit.
---
 support/nfs/xio.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/support/nfs/xio.c b/support/nfs/xio.c
index 5e2e1e9..e3d27d2 100644
--- a/support/nfs/xio.c
+++ b/support/nfs/xio.c
@@ -44,16 +44,9 @@ xfclose(XFILE *xfp)
 	xfree(xfp);
 }
 
-static void
-doalarm(int sig)
-{
-	return;
-}
-
 int
 xflock(char *fname, char *type)
 {
-	struct sigaction sa, oldsa;
 	int		readonly = !strcmp(type, "r");
 	struct flock	fl = { readonly? F_RDLCK : F_WRLCK, SEEK_SET, 0, 0, 0 };
 	int		fd;
@@ -68,21 +61,12 @@ xflock(char *fname, char *type)
 		return -1;
 	}
 
-	sa.sa_handler = doalarm;
-	sa.sa_flags = 0;
-	sigemptyset(&sa.sa_mask);
-	sigaction(SIGALRM, &sa, &oldsa);
-	alarm(10);
 	if (fcntl(fd, F_SETLKW, &fl) < 0) {
-		alarm(0);
 		xlog(L_WARNING, "failed to lock %s: errno %d (%s)",
 				fname, errno, strerror(errno));
 		close(fd);
-		fd = 0;
-	} else {
-		alarm(0);
+		fd = -1;
 	}
-	sigaction(SIGALRM, &oldsa, NULL);
 
 	return fd;
 }

[PATCH 2/3] nfs-utils: dont leak fd in mountlist_list

[Ben Myers]

Don't leak this file descriptor if stat should fail.
---
 utils/mountd/rmtab.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/utils/mountd/rmtab.c b/utils/mountd/rmtab.c
index b028529..19b22ee 100644
--- a/utils/mountd/rmtab.c
+++ b/utils/mountd/rmtab.c
@@ -24,6 +24,7 @@
 #include "ha-callout.h"
 
 #include <limits.h> /* PATH_MAX */
+#include <errno.h>
 
 extern int reverse_resolve;
 
@@ -187,7 +188,9 @@ mountlist_list(void)
 	if ((lockid = xflock(_PATH_RMTABLCK, "r")) < 0)
 		return NULL;
 	if (stat(_PATH_RMTAB, &stb) < 0) {
-		xlog(L_ERROR, "can't stat %s", _PATH_RMTAB);
+		xlog(L_ERROR, "can't stat %s: %s",
+				_PATH_RMTAB, strerror(errno));
+		xfunlock(lockid);
 		return NULL;
 	}
 	if (stb.st_mtime != last_mtime) {

[PATCH 3/3] nfs-utils: don't fdatasync the rmtab

[Ben Myers]

If we're using the new caching interface the rmtab will be ignored by exportfs
so there is no need to fdatasync.  This improves mountd performance.
---
 support/nfs/cacheio.c |    5 ++---
 support/nfs/rmtab.c   |   23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
index 6a6ed5a..bdf5d84 100644
--- a/support/nfs/cacheio.c
+++ b/support/nfs/cacheio.c
@@ -285,9 +285,8 @@ int readline(int fd, char **buf, int *lenp)
 int
 check_new_cache(void)
 {
-	struct stat stb;
-	return	(stat("/proc/fs/nfs/filehandle", &stb) == 0) ||
-		(stat("/proc/fs/nfsd/filehandle", &stb) == 0);
+	return	(access("/proc/fs/nfs/filehandle", F_OK) == 0) ||
+		(access("/proc/fs/nfsd/filehandle", F_OK) == 0);
 }	
 
 
diff --git a/support/nfs/rmtab.c b/support/nfs/rmtab.c
index 4cbd285..a28abf3 100644
--- a/support/nfs/rmtab.c
+++ b/support/nfs/rmtab.c
@@ -117,11 +117,24 @@ void
 fendrmtabent(FILE *fp)
 {
 	if (fp) {
-		/* If it was written to, we really want
-		 * to flush to disk before returning
-		 */
-		fflush(fp);
-		fdatasync(fileno(fp));
+		static int have_new_cache = -1;
+		if (have_new_cache == -1) /* check only once */
+			have_new_cache = check_new_cache();
+
+		if (!have_new_cache) {
+			/*
+			 * If we are using the old caching interface: exportfs
+			 * uses the rmtab to determine what should be exported,
+			 * so it is important that it be up-to-date.
+			 *
+			 * If we are using the new caching interface: the rmtab
+			 * is ignored by exportfs and the fdatasync only serves
+			 * to slow us down.
+			 */
+			fflush(fp);
+			fdatasync(fileno(fp));
+		}
+
 		fclose(fp);
 	}
 }

Re: [PATCH 0/3] nfs-utils: multithreaded mountd fixes

[Steve Dickson]

On 01/28/2010 04:26 PM, Ben Myers wrote:
> Hey Steve,
> 
> The following fixes a problem with mountd where you get backed up behind the
> rmtab lock.  It may have happened that an xflock timed out and stat failed so
> the filedescriptor was leaked in mountlist_list and the lock never released.
> Skipping the fdatasync has also helped performance-wise.
> 
> Tested very lightly on TOT and more heavily on 1.0.7 where this bug was
> discovered.
Committed... 

steved.