[PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage
@ 2012-01-23 20:02 Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 01/11] nfsdcld: add client tracking daemon stub Jeff Layton
                   ` (10 more replies)
  0 siblings, 11 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

This patchset is the userspace portion of the knfsd client name tracking
overhaul. See this patch series for an explanation:

    nfsd: overhaul the client name tracking code

The main changes from the last set are:

- the daemon will now reopen the pipe if it's deleted and recreated. This
  can happen if knfsd is restarted.

- The daemon now implements an "init" upcall. When knfsd starts, it will
  upcall to userspace to ask for a "boot_generation" value. nfsdcld will
  fetch that value out of the DB, increment it and reinsert it (atomically).
  That guarantees uniqueness of the boot generation value even when multiple
  machines are sharing the same nfsdcld db.

- the "create" and "check" upcalls now send the rowid of the record in the
  downcall. Eventually the kernel will use this to track some info about
  lockowners in some situations. For now the kernel ignores this value,
  but the daemon passes it anyway.

- "remove" functionality has been removed from the daemon. The upcall that
  previously requested that has been removed, so we have no need for that
  functionality in the daemon. Client records are now only removed when
  the grace period ends.

Jeff Layton (11):
  nfsdcld: add client tracking daemon stub
  nfsdcld: add autoconf goop for sqlite
  nfsdcld: add routines for a sqlite backend database
  nfsdcld: add check/update functionality
  nfsdcld: add function to remove unreclaimed client records
  nfsdcld: have daemon pass client row index back to kernel
  nfsdcld: implement an init upcall
  nfsdcld: allow daemon to wait for pipe to show up
  nfsdcld: reopen pipe if it's deleted and recreated
  nfsdcld: add a manpage for nfsdcld
  nfsdcld: update the README

 README                    |   25 +++-
 aclocal/libsqlite3.m4     |   33 +++
 configure.ac              |   21 ++
 utils/Makefile.am         |    4 +
 utils/nfsdcld/Makefile.am |   14 ++
 utils/nfsdcld/nfsdcld.c   |  478 +++++++++++++++++++++++++++++++++++++++++++++
 utils/nfsdcld/nfsdcld.man |  180 +++++++++++++++++
 utils/nfsdcld/nfsdcld.pod |   67 +++++++
 utils/nfsdcld/sqlite.c    |  461 +++++++++++++++++++++++++++++++++++++++++++
 utils/nfsdcld/sqlite.h    |   31 +++
 10 files changed, 1312 insertions(+), 2 deletions(-)
 create mode 100644 aclocal/libsqlite3.m4
 create mode 100644 utils/nfsdcld/Makefile.am
 create mode 100644 utils/nfsdcld/nfsdcld.c
 create mode 100644 utils/nfsdcld/nfsdcld.man
 create mode 100644 utils/nfsdcld/nfsdcld.pod
 create mode 100644 utils/nfsdcld/sqlite.c
 create mode 100644 utils/nfsdcld/sqlite.h

-- 
1.7.7.5


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v4 01/11] nfsdcld: add client tracking daemon stub
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 02/11] nfsdcld: add autoconf goop for sqlite Jeff Layton
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

This program opens and "listens" on the new nfsd/cld rpc_pipefs pipe.
The code here doesn't actually do anything on stable storage yet. That
will be added in a later patch.

The patch also adds a autoconf enable switch for the new daemon that
defaults to "no", and a test for the upcall description header file.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 configure.ac              |   14 +++
 utils/Makefile.am         |    4 +
 utils/nfsdcld/Makefile.am |   14 +++
 utils/nfsdcld/nfsdcld.c   |  264 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 296 insertions(+), 0 deletions(-)
 create mode 100644 utils/nfsdcld/Makefile.am
 create mode 100644 utils/nfsdcld/nfsdcld.c

diff --git a/configure.ac b/configure.ac
index 920e8da..d50e54e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -185,6 +185,12 @@ else
 	AM_CONDITIONAL(MOUNT_CONFIG, [test "$enable_mount" = "yes"])
 fi
 
+AC_ARG_ENABLE(nfsdcld,
+	[AC_HELP_STRING([--enable-nfsdcld],
+			[Create nfsdcld NFSv4 clientid tracking daemon. <:@default=no@:>@])],
+	enable_nfsdcld=$enableval,
+	enable_nfsdcld="no")
+
 dnl Check for TI-RPC library and headers
 AC_LIBTIRPC
 
@@ -260,6 +266,13 @@ if test "$enable_nfsv4" = yes; then
   dnl check for the keyutils libraries and headers
   AC_KEYUTILS
 
+  if test "$enable_nfsdcld" = "yes"; then
+    AC_CHECK_HEADERS([linux/nfsd/cld.h], ,
+		     AC_MSG_ERROR([Cannot find header needed for nfsdcld]))
+  fi
+
+  AM_CONDITIONAL(CONFIG_NFSDCLD, [test "$enable_nfsdcld" = "yes" ])
+
   dnl librpcsecgss already has a dependency on libgssapi,
   dnl but we need to make sure we get the right version
   if test "$enable_gss" = yes; then
@@ -460,6 +473,7 @@ AC_CONFIG_FILES([
 	tools/nfs-iostat/Makefile
 	utils/Makefile
 	utils/blkmapd/Makefile
+	utils/nfsdcld/Makefile
 	utils/exportfs/Makefile
 	utils/gssd/Makefile
 	utils/idmapd/Makefile
diff --git a/utils/Makefile.am b/utils/Makefile.am
index d074b85..5df7ca7 100644
--- a/utils/Makefile.am
+++ b/utils/Makefile.am
@@ -21,6 +21,10 @@ if CONFIG_MOUNT
 OPTDIRS += mount
 endif
 
+if CONFIG_NFSDCLD
+OPTDIRS += nfsdcld
+endif
+
 SUBDIRS = \
 	exportfs \
 	mountd \
diff --git a/utils/nfsdcld/Makefile.am b/utils/nfsdcld/Makefile.am
new file mode 100644
index 0000000..ed7ed42
--- /dev/null
+++ b/utils/nfsdcld/Makefile.am
@@ -0,0 +1,14 @@
+## Process this file with automake to produce Makefile.in
+
+#man8_MANS	= nfsdcld.man
+#EXTRA_DIST = $(man8_MANS)
+
+AM_CFLAGS	+= -D_LARGEFILE64_SOURCE
+sbin_PROGRAMS	= nfsdcld
+
+nfsdcld_SOURCES = nfsdcld.c
+
+nfsdcld_LDADD = ../../support/nfs/libnfs.a $(LIBEVENT)
+
+MAINTAINERCLEANFILES = Makefile.in
+
diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
new file mode 100644
index 0000000..fab35ae
--- /dev/null
+++ b/utils/nfsdcld/nfsdcld.c
@@ -0,0 +1,264 @@
+/*
+ * nfsdcld.c -- NFSv4 client name tracking daemon
+ *
+ * Copyright (C) 2011  Red Hat, Jeff Layton <jlayton@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif /* HAVE_CONFIG_H */
+
+#include <errno.h>
+#include <event.h>
+#include <stdbool.h>
+#include <getopt.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <linux/nfsd/cld.h>
+
+#include "xlog.h"
+#include "nfslib.h"
+
+#ifndef PIPEFS_DIR
+#define PIPEFS_DIR NFS_STATEDIR "/rpc_pipefs"
+#endif
+
+#define DEFAULT_CLD_PATH	PIPEFS_DIR "/nfsd/cld"
+
+#define UPCALL_VERSION		1
+
+/* private data structures */
+struct cld_client {
+	int			cl_fd;
+	struct event		cl_event;
+	struct cld_msg	cl_msg;
+};
+
+/* global variables */
+static char *pipepath = DEFAULT_CLD_PATH;
+
+static struct option longopts[] =
+{
+	{ "help", 0, NULL, 'h' },
+	{ "foreground", 0, NULL, 'F' },
+	{ "debug", 0, NULL, 'd' },
+	{ "pipe", 1, NULL, 'p' },
+	{ NULL, 0, 0, 0 },
+};
+
+/* forward declarations */
+static void cldcb(int UNUSED(fd), short which, void *data);
+
+static void
+usage(char *progname)
+{
+	printf("Usage:\n");
+	printf("%s [ -hFd ] [ -p pipe ]\n", progname);
+}
+
+static int
+cld_pipe_open(struct cld_client *clnt)
+{
+	clnt->cl_fd = open(pipepath, O_RDWR, 0);
+	if (clnt->cl_fd < 0) {
+		xlog(L_ERROR, "%s: unable to open %s: %m", __func__, pipepath);
+		return clnt->cl_fd;
+	}
+
+	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
+	event_add(&clnt->cl_event, NULL);
+
+	return clnt->cl_fd;
+}
+
+static void
+cld_pipe_reopen(struct cld_client *clnt)
+{
+	int fd;
+
+	fd = open(pipepath, O_RDWR, 0);
+	if (fd < 0) {
+		xlog_warn("%s: Re-opening of %s failed: %m",
+				__func__, pipepath);
+		return;
+	}
+
+	if (clnt->cl_event.ev_flags & EVLIST_INIT)
+		event_del(&clnt->cl_event);
+	if (clnt->cl_fd > 0)
+		close(clnt->cl_fd);
+
+	clnt->cl_fd = fd;
+	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
+	/* event_add is done by the caller */
+}
+
+static void
+cld_create(struct cld_client *clnt)
+{
+	ssize_t bsize, wsize;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	xlog(D_GENERAL, "%s: create client record", __func__);
+
+	/* FIXME: create client record on storage here */
+
+	/* set up reply */
+	cmsg->cm_status = 0;
+
+	bsize = sizeof(*cmsg);
+
+	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
+	if (wsize != bsize) {
+		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
+			 __func__, wsize);
+		cld_pipe_reopen(clnt);
+	}
+}
+
+static void
+cld_not_implemented(struct cld_client *clnt)
+{
+	ssize_t bsize, wsize;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	xlog(D_GENERAL, "%s: downcalling with not implemented error", __func__);
+
+	/* set up reply */
+	cmsg->cm_status = -EOPNOTSUPP;
+
+	bsize = sizeof(*cmsg);
+
+	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
+	if (wsize != bsize)
+		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
+			 __func__, wsize);
+
+	/* reopen pipe, just to be sure */
+	cld_pipe_reopen(clnt);
+}
+
+static void
+cldcb(int UNUSED(fd), short which, void *data)
+{
+	ssize_t len;
+	struct cld_client *clnt = data;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	if (which != EV_READ)
+		goto out;
+
+	len = atomicio(read, clnt->cl_fd, cmsg, sizeof(*cmsg));
+	if (len <= 0) {
+		xlog_warn("%s: pipe read failed: %m", __func__);
+		cld_pipe_reopen(clnt);
+		goto out;
+	}
+
+	if (cmsg->cm_vers != UPCALL_VERSION) {
+		xlog_warn("%s: unsupported upcall version: %hu", cmsg->cm_vers);
+		cld_pipe_reopen(clnt);
+		goto out;
+	}
+
+	switch(cmsg->cm_cmd) {
+	case Cld_Create:
+		cld_create(clnt);
+		break;
+	default:
+		xlog_warn("%s: command %u is not yet implemented", __func__,
+			  cmsg->cm_cmd);
+		cld_not_implemented(clnt);
+	}
+out:
+	event_add(&clnt->cl_event, NULL);
+}
+
+int
+main(int argc, char **argv)
+{
+	char arg;
+	int rc = 0, fd;
+	bool foreground = false;
+	char *progname;
+	struct cld_client clnt;
+
+	memset(&clnt, 0, sizeof(clnt));
+
+	progname = strdup(basename(argv[0]));
+	if (!progname) {
+		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
+		return 1;
+	}
+
+	event_init();
+	xlog_syslog(0);
+	xlog_stderr(1);
+
+	/* process command-line options */
+	while ((arg = getopt_long(argc, argv, "hdFp:", longopts,
+				  NULL)) != EOF) {
+		switch (arg) {
+		case 'd':
+			xlog_config(D_ALL, 1);
+			break;
+		case 'F':
+			foreground = true;
+			break;
+		case 'p':
+			pipepath = optarg;
+			break;
+		default:
+			usage(progname);
+			return 0;
+		}
+	}
+
+
+	xlog_open(progname);
+	if (!foreground) {
+		xlog_syslog(1);
+		xlog_stderr(0);
+		rc = daemon(0, 0);
+		if (rc) {
+			xlog(L_ERROR, "Unable to daemonize: %m");
+			goto out;
+		}
+	}
+
+	/* set up storage db */
+
+	/* set up event handler */
+	fd = cld_pipe_open(&clnt);
+	if (fd < 0) {
+		rc = fd;
+		goto out;
+	}
+
+	rc = event_dispatch();
+	if (rc < 0)
+		xlog(L_ERROR, "%s: event_dispatch failed: %m", __func__);
+
+	close(clnt.cl_fd);
+out:
+	free(progname);
+	return rc;
+}
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 02/11] nfsdcld: add autoconf goop for sqlite
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 01/11] nfsdcld: add client tracking daemon stub Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 03/11] nfsdcld: add routines for a sqlite backend database Jeff Layton
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

Mostly cribbed from Chuck Lever's new-statd rewrite a few years ago...

This adds an autoconf test for the sqlite3 library and includes. If
they're not working properly and nfsdcld was enabled, then configure
will error out.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 aclocal/libsqlite3.m4 |   33 +++++++++++++++++++++++++++++++++
 configure.ac          |    7 +++++++
 2 files changed, 40 insertions(+), 0 deletions(-)
 create mode 100644 aclocal/libsqlite3.m4

diff --git a/aclocal/libsqlite3.m4 b/aclocal/libsqlite3.m4
new file mode 100644
index 0000000..73d1e46
--- /dev/null
+++ b/aclocal/libsqlite3.m4
@@ -0,0 +1,33 @@
+dnl Checks for matching sqlite3 header and library, and
+dnl sufficient sqlite3 version.
+dnl
+AC_DEFUN([AC_SQLITE3_VERS], [
+  AC_CHECK_HEADERS([sqlite3.h], ,)
+
+  dnl look for the library; do not add to LIBS if found
+  AC_CHECK_LIB([sqlite3], [sqlite3_libversion_number], [LIBSQLITE=-lsqlite3], ,)
+  AC_SUBST(LIBSQLITE)
+
+  AC_MSG_CHECKING(for suitable sqlite3 version)
+
+  AC_CACHE_VAL([libsqlite3_cv_is_recent],
+   [
+    saved_LIBS="$LIBS"
+    LIBS=-lsqlite3
+    AC_TRY_RUN([
+	#include <stdio.h>
+	#include <sqlite3.h>
+	int main()
+	{
+		int vers = sqlite3_libversion_number();
+
+		return vers != SQLITE_VERSION_NUMBER ||
+			vers < 3003000;
+	}
+       ], [libsqlite3_cv_is_recent=yes], [libsqlite3_cv_is_recent=no],
+       [libsqlite3_cv_is_recent=unknown])
+    LIBS="$saved_LIBS"])
+
+  AC_MSG_RESULT($libsqlite3_cv_is_recent)
+  AM_CONDITIONAL(CONFIG_SQLITE3, [test "$libsqlite3_cv_is_recent" = "yes"])
+])dnl
diff --git a/configure.ac b/configure.ac
index d50e54e..134b609 100644
--- a/configure.ac
+++ b/configure.ac
@@ -266,9 +266,16 @@ if test "$enable_nfsv4" = yes; then
   dnl check for the keyutils libraries and headers
   AC_KEYUTILS
 
+  dnl Check for sqlite3
+  AC_SQLITE3_VERS
+
   if test "$enable_nfsdcld" = "yes"; then
     AC_CHECK_HEADERS([linux/nfsd/cld.h], ,
 		     AC_MSG_ERROR([Cannot find header needed for nfsdcld]))
+
+    if test "$libsqlite3_cv_is_recent" != "yes" ; then
+      AC_MSG_ERROR([nfsdcld requires sqlite3])
+    fi
   fi
 
   AM_CONDITIONAL(CONFIG_NFSDCLD, [test "$enable_nfsdcld" = "yes" ])
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 03/11] nfsdcld: add routines for a sqlite backend database
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 01/11] nfsdcld: add client tracking daemon stub Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 02/11] nfsdcld: add autoconf goop for sqlite Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 04/11] nfsdcld: add check/update functionality Jeff Layton
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

Rather than roll our own "storage engine", use sqlite instead. It fits
the bill nicely as it does:

- durable on-disk storage
- the ability to constrain record uniqueness
- a facility for collating and searching the host records

...it does add a build dependency to nfs-utils, but almost all modern
distros provide those packages.

The current incarnation of this code dynamically links against a
provided sqlite library, but we could also consider including their
single-file "amalgamation" to reduce dependencies (though with all
the caveats that that entails).

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/Makefile.am |    4 +-
 utils/nfsdcld/nfsdcld.c   |   25 ++++-
 utils/nfsdcld/sqlite.c    |  252 +++++++++++++++++++++++++++++++++++++++++++++
 utils/nfsdcld/sqlite.h    |   26 +++++
 4 files changed, 299 insertions(+), 8 deletions(-)
 create mode 100644 utils/nfsdcld/sqlite.c
 create mode 100644 utils/nfsdcld/sqlite.h

diff --git a/utils/nfsdcld/Makefile.am b/utils/nfsdcld/Makefile.am
index ed7ed42..8e4f2ab 100644
--- a/utils/nfsdcld/Makefile.am
+++ b/utils/nfsdcld/Makefile.am
@@ -6,9 +6,9 @@
 AM_CFLAGS	+= -D_LARGEFILE64_SOURCE
 sbin_PROGRAMS	= nfsdcld
 
-nfsdcld_SOURCES = nfsdcld.c
+nfsdcld_SOURCES = nfsdcld.c sqlite.c
 
-nfsdcld_LDADD = ../../support/nfs/libnfs.a $(LIBEVENT)
+nfsdcld_LDADD = ../../support/nfs/libnfs.a $(LIBEVENT) $(LIBSQLITE)
 
 MAINTAINERCLEANFILES = Makefile.in
 
diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index fab35ae..d6bd041 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -36,6 +36,7 @@
 
 #include "xlog.h"
 #include "nfslib.h"
+#include "sqlite.h"
 
 #ifndef PIPEFS_DIR
 #define PIPEFS_DIR NFS_STATEDIR "/rpc_pipefs"
@@ -61,6 +62,7 @@ static struct option longopts[] =
 	{ "foreground", 0, NULL, 'F' },
 	{ "debug", 0, NULL, 'd' },
 	{ "pipe", 1, NULL, 'p' },
+	{ "storagedir", 1, NULL, 's' },
 	{ NULL, 0, 0, 0 },
 };
 
@@ -71,7 +73,7 @@ static void
 usage(char *progname)
 {
 	printf("Usage:\n");
-	printf("%s [ -hFd ] [ -p pipe ]\n", progname);
+	printf("%s [ -hFd ] [ -p pipe ] [ -s dir ]\n", progname);
 }
 
 static int
@@ -114,18 +116,20 @@ cld_pipe_reopen(struct cld_client *clnt)
 static void
 cld_create(struct cld_client *clnt)
 {
+	int ret;
 	ssize_t bsize, wsize;
 	struct cld_msg *cmsg = &clnt->cl_msg;
 
-	xlog(D_GENERAL, "%s: create client record", __func__);
+	xlog(D_GENERAL, "%s: create client record.", __func__);
 
-	/* FIXME: create client record on storage here */
+	ret = sqlite_insert_client(cmsg->cm_u.cm_name.cn_id,
+				   cmsg->cm_u.cm_name.cn_len);
 
-	/* set up reply */
-	cmsg->cm_status = 0;
+	cmsg->cm_status = ret ? -EREMOTEIO : ret;
 
 	bsize = sizeof(*cmsg);
 
+	xlog(D_GENERAL, "Doing downcall with status %d", cmsg->cm_status);
 	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
 	if (wsize != bsize) {
 		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
@@ -199,6 +203,7 @@ main(int argc, char **argv)
 	int rc = 0, fd;
 	bool foreground = false;
 	char *progname;
+	char *storagedir = NULL;
 	struct cld_client clnt;
 
 	memset(&clnt, 0, sizeof(clnt));
@@ -214,7 +219,7 @@ main(int argc, char **argv)
 	xlog_stderr(1);
 
 	/* process command-line options */
-	while ((arg = getopt_long(argc, argv, "hdFp:", longopts,
+	while ((arg = getopt_long(argc, argv, "hdFp:s:", longopts,
 				  NULL)) != EOF) {
 		switch (arg) {
 		case 'd':
@@ -226,6 +231,9 @@ main(int argc, char **argv)
 		case 'p':
 			pipepath = optarg;
 			break;
+		case 's':
+			storagedir = optarg;
+			break;
 		default:
 			usage(progname);
 			return 0;
@@ -245,6 +253,11 @@ main(int argc, char **argv)
 	}
 
 	/* set up storage db */
+	rc = sqlite_maindb_init(storagedir);
+	if (rc) {
+		xlog(L_ERROR, "Failed to open main database: %d", rc);
+		goto out;
+	}
 
 	/* set up event handler */
 	fd = cld_pipe_open(&clnt);
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
new file mode 100644
index 0000000..606d714
--- /dev/null
+++ b/utils/nfsdcld/sqlite.c
@@ -0,0 +1,252 @@
+/*
+ * Copyright (C) 2011  Red Hat, Jeff Layton <jlayton@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+/*
+ * Explanation:
+ *
+ * This file contains the code to manage the sqlite backend database for the
+ * clstated upcall daemon.
+ *
+ * The main database is called main.sqlite and contains the following tables:
+ *
+ * parameters: simple key/value pairs for storing database info
+ *
+ * clients: one column containing a BLOB with the as sent by the client
+ * 	    and a timestamp (in epoch seconds) of when the record was
+ * 	    established
+ *
+ * FIXME: should we also record the fsid being accessed?
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif /* HAVE_CONFIG_H */
+
+#include <errno.h>
+#include <event.h>
+#include <stdbool.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sqlite3.h>
+#include <linux/limits.h>
+
+#include "xlog.h"
+
+#define CLD_SQLITE_SCHEMA_VERSION 1
+
+#ifndef CLD_SQLITE_TOPDIR
+#define CLD_SQLITE_TOPDIR NFS_STATEDIR "/nfsdcld"
+#endif
+
+/* in milliseconds */
+#define CLD_SQLITE_BUSY_TIMEOUT 10000
+
+/* private data structures */
+
+/* global variables */
+
+/* top level DB directory */
+static char *sqlite_topdir;
+
+/* reusable pathname and sql command buffer */
+static char buf[PATH_MAX];
+
+/* global database handle */
+static sqlite3 *dbh;
+
+/* forward declarations */
+
+/* make a directory, ignoring EEXIST errors unless it's not a directory */
+static int
+mkdir_if_not_exist(char *dirname)
+{
+	int ret;
+	struct stat statbuf;
+
+	ret = mkdir(dirname, S_IRWXU);
+	if (ret && errno != EEXIST)
+		return -errno;
+
+	ret = stat(dirname, &statbuf);
+	if (ret)
+		return -errno;
+
+	if (!S_ISDIR(statbuf.st_mode))
+		ret = -ENOTDIR;
+
+	return ret;
+}
+
+/*
+ * Open the "main" database, and attempt to initialize it by creating the
+ * parameters table and inserting the schema version into it. Ignore any errors
+ * from that, and then attempt to select the version out of it again. If the
+ * version appears wrong, then assume that the DB is corrupt or has been
+ * upgraded, and return an error. If all of that works, then attempt to create
+ * the "clients" table.
+ */
+int
+sqlite_maindb_init(char *topdir)
+{
+	int ret;
+	char *err = NULL;
+	sqlite3_stmt *stmt = NULL;
+
+	sqlite_topdir = topdir ? topdir : CLD_SQLITE_TOPDIR;
+
+	ret = mkdir_if_not_exist(sqlite_topdir);
+	if (ret)
+		return ret;
+
+	ret = snprintf(buf, PATH_MAX - 1, "%s/main.sqlite", sqlite_topdir);
+	if (ret < 0)
+		return ret;
+
+	buf[PATH_MAX - 1] = '\0';
+
+	ret = sqlite3_open(buf, &dbh);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to open main database: %d", ret);
+		return ret;
+	}
+
+	ret = sqlite3_busy_timeout(dbh, CLD_SQLITE_BUSY_TIMEOUT);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to set sqlite busy timeout: %d", ret);
+		goto out_err;
+	}
+
+	/* Try to create table */
+	ret = sqlite3_exec(dbh, "CREATE TABLE IF NOT EXISTS parameters "
+				"(key TEXT PRIMARY KEY, value TEXT);",
+				NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to create parameter table: %d", ret);
+		goto out_err;
+	}
+
+	/* insert version into table -- ignore error if it fails */
+	ret = snprintf(buf, sizeof(buf),
+		       "INSERT OR IGNORE INTO parameters values (\"version\", "
+		       "\"%d\");", CLD_SQLITE_SCHEMA_VERSION);
+	if (ret < 0) {
+		goto out_err;
+	} else if ((size_t)ret >= sizeof(buf)) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	ret = sqlite3_exec(dbh, (const char *)buf, NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to insert into parameter table: %d",
+				ret);
+		goto out_err;
+	}
+
+	ret = sqlite3_prepare_v2(dbh,
+		"SELECT value FROM parameters WHERE key == \"version\";",
+		 -1, &stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to prepare select statement: %d", ret);
+		goto out_err;
+	}
+
+	/* check schema version */
+	ret = sqlite3_step(stmt);
+	if (ret != SQLITE_ROW) {
+		xlog(D_GENERAL, "Select statement execution failed: %s",
+				sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	/* process SELECT result */
+	ret = sqlite3_column_int(stmt, 0);
+	if (ret != CLD_SQLITE_SCHEMA_VERSION) {
+		xlog(L_ERROR, "Unsupported database schema version! "
+			"Expected %d, got %d.",
+			CLD_SQLITE_SCHEMA_VERSION, ret);
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	/* now create the "clients" table */
+	ret = sqlite3_exec(dbh, "CREATE TABLE IF NOT EXISTS clients "
+				"(id BLOB PRIMARY KEY, time INTEGER);",
+				NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(L_ERROR, "Unable to create clients table: %s", err);
+		goto out_err;
+	}
+
+	sqlite3_free(err);
+	sqlite3_finalize(stmt);
+	return 0;
+
+out_err:
+	if (err) {
+		xlog(L_ERROR, "sqlite error: %s", err);
+		sqlite3_free(err);
+	}
+	sqlite3_finalize(stmt);
+	sqlite3_close(dbh);
+	return ret;
+}
+
+/*
+ * Create a client record
+ *
+ * Returns a non-zero sqlite error code, or SQLITE_OK (aka 0)
+ */
+int
+sqlite_insert_client(const unsigned char *clname, const size_t namelen)
+{
+	int ret;
+	sqlite3_stmt *stmt = NULL;
+
+	ret = sqlite3_prepare_v2(dbh, "INSERT OR REPLACE INTO clients VALUES "
+				      "(?, strftime('%s', 'now'));", -1,
+					&stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Insert statement prepare failed: %s",
+			sqlite3_errmsg(dbh));
+		return ret;
+	}
+
+	ret = sqlite3_bind_blob(stmt, 1, (const void *)clname, namelen,
+				SQLITE_STATIC);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Bind blob failed: %d", sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_step(stmt);
+	if (ret == SQLITE_DONE)
+		ret = SQLITE_OK;
+	else
+		xlog(D_GENERAL, "Unexpected return code from insert: %s",
+				sqlite3_errmsg(dbh));
+
+out_err:
+	xlog(D_GENERAL, "%s: returning %d", __func__, ret);
+	sqlite3_finalize(stmt);
+	return ret;
+}
diff --git a/utils/nfsdcld/sqlite.h b/utils/nfsdcld/sqlite.h
new file mode 100644
index 0000000..ba4c213
--- /dev/null
+++ b/utils/nfsdcld/sqlite.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011  Red Hat, Jeff Layton <jlayton@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#ifndef _SQLITE_H_
+#define _SQLITE_H_
+
+int sqlite_maindb_init(char *topdir);
+int sqlite_insert_client(const unsigned char *clname, const size_t namelen);
+
+#endif /* _SQLITE_H */
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 04/11] nfsdcld: add check/update functionality
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (2 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 03/11] nfsdcld: add routines for a sqlite backend database Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 05/11] nfsdcld: add function to remove unreclaimed client records Jeff Layton
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

Add functions to check whether a client is allowed to reclaim, and
update its timestamp in the DB if so. If either the query or update
fails, then the host is not allowed to reclaim state.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/nfsdcld.c |   29 +++++++++++++++++++
 utils/nfsdcld/sqlite.c  |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 utils/nfsdcld/sqlite.h  |    1 +
 3 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index d6bd041..2930de4 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -161,6 +161,32 @@ cld_not_implemented(struct cld_client *clnt)
 }
 
 static void
+cld_check(struct cld_client *clnt)
+{
+	int ret;
+	ssize_t bsize, wsize;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	xlog(D_GENERAL, "%s: check client record", __func__);
+
+	ret = sqlite_check_client(cmsg->cm_u.cm_name.cn_id,
+				  cmsg->cm_u.cm_name.cn_len);
+
+	/* set up reply */
+	cmsg->cm_status = ret ? -EACCES : ret;
+
+	bsize = sizeof(*cmsg);
+
+	xlog(D_GENERAL, "Doing downcall with status %d", cmsg->cm_status);
+	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
+	if (wsize != bsize) {
+		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
+			 __func__, wsize);
+		cld_pipe_reopen(clnt);
+	}
+}
+
+static void
 cldcb(int UNUSED(fd), short which, void *data)
 {
 	ssize_t len;
@@ -187,6 +213,9 @@ cldcb(int UNUSED(fd), short which, void *data)
 	case Cld_Create:
 		cld_create(clnt);
 		break;
+	case Cld_Allow:
+		cld_check(clnt);
+		break;
 	default:
 		xlog_warn("%s: command %u is not yet implemented", __func__,
 			  cmsg->cm_cmd);
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
index 606d714..814e421 100644
--- a/utils/nfsdcld/sqlite.c
+++ b/utils/nfsdcld/sqlite.c
@@ -250,3 +250,73 @@ out_err:
 	sqlite3_finalize(stmt);
 	return ret;
 }
+
+/*
+ * Is the given clname in the clients table? If so, then update its timestamp
+ * and return success. If the record isn't present, or the update fails, then
+ * return an error.
+ */
+int
+sqlite_check_client(const unsigned char *clname, const size_t namelen)
+{
+	int ret;
+	sqlite3_stmt *stmt = NULL;
+
+	ret = sqlite3_prepare_v2(dbh, "SELECT count(*) FROM clients WHERE "
+				      "id==?", -1, &stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to prepare update statement: %s",
+				sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_bind_blob(stmt, 1, (const void *)clname, namelen,
+				SQLITE_STATIC);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Bind blob failed: %s", sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_step(stmt);
+	if (ret != SQLITE_ROW) {
+		xlog(D_GENERAL, "Unexpected return code from select: %d", ret);
+		goto out_err;
+	}
+
+	ret = sqlite3_column_int(stmt, 0);
+	xlog(D_GENERAL, "Select returned %d rows", ret);
+	if (ret != 1) {
+		ret = -EACCES;
+		goto out_err;
+	}
+
+	sqlite3_finalize(stmt);
+	stmt = NULL;
+	ret = sqlite3_prepare_v2(dbh, "UPDATE OR FAIL clients SET "
+				      "time=strftime('%s', 'now') WHERE id==?",
+				 -1, &stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to prepare update statement: %s",
+				sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_bind_blob(stmt, 1, (const void *)clname, namelen,
+				SQLITE_STATIC);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Bind blob failed: %s", sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_step(stmt);
+	if (ret == SQLITE_DONE)
+		ret = SQLITE_OK;
+	else
+		xlog(D_GENERAL, "Unexpected return code from update: %s",
+				sqlite3_errmsg(dbh));
+
+out_err:
+	xlog(D_GENERAL, "%s: returning %d", __func__, ret);
+	sqlite3_finalize(stmt);
+	return ret;
+}
diff --git a/utils/nfsdcld/sqlite.h b/utils/nfsdcld/sqlite.h
index ba4c213..7fc5ea4 100644
--- a/utils/nfsdcld/sqlite.h
+++ b/utils/nfsdcld/sqlite.h
@@ -22,5 +22,6 @@
 
 int sqlite_maindb_init(char *topdir);
 int sqlite_insert_client(const unsigned char *clname, const size_t namelen);
+int sqlite_check_client(const unsigned char *clname, const size_t namelen);
 
 #endif /* _SQLITE_H */
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 05/11] nfsdcld: add function to remove unreclaimed client records
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (3 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 04/11] nfsdcld: add check/update functionality Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 06/11] nfsdcld: have daemon pass client row index back to kernel Jeff Layton
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

This should remove any client record that has a timestamp prior to
the given time.

Eventually, this call will need to be made cluster aware when this is
run in a clustered configuration. For now, this is only suitable for
single-host configurations.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/nfsdcld.c |   29 +++++++++++++++++++++++++++++
 utils/nfsdcld/sqlite.c  |   28 ++++++++++++++++++++++++++++
 utils/nfsdcld/sqlite.h  |    1 +
 3 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index 2930de4..151dd16 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -187,6 +187,32 @@ cld_check(struct cld_client *clnt)
 }
 
 static void
+cld_gracedone(struct cld_client *clnt)
+{
+	int ret;
+	ssize_t bsize, wsize;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	xlog(D_GENERAL, "%s: grace done. cm_gracetime=%ld", __func__,
+			cmsg->cm_u.cm_gracetime);
+
+	ret = sqlite_remove_unreclaimed(cmsg->cm_u.cm_gracetime);
+
+	/* set up reply: downcall with 0 status */
+	cmsg->cm_status = ret ? -EREMOTEIO : ret;
+
+	bsize = sizeof(*cmsg);
+
+	xlog(D_GENERAL, "Doing downcall with status %d", cmsg->cm_status);
+	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
+	if (wsize != bsize) {
+		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
+			 __func__, wsize);
+		cld_pipe_reopen(clnt);
+	}
+}
+
+static void
 cldcb(int UNUSED(fd), short which, void *data)
 {
 	ssize_t len;
@@ -216,6 +242,9 @@ cldcb(int UNUSED(fd), short which, void *data)
 	case Cld_Allow:
 		cld_check(clnt);
 		break;
+	case Cld_GraceDone:
+		cld_gracedone(clnt);
+		break;
 	default:
 		xlog_warn("%s: command %u is not yet implemented", __func__,
 			  cmsg->cm_cmd);
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
index 814e421..a0bcf2f 100644
--- a/utils/nfsdcld/sqlite.c
+++ b/utils/nfsdcld/sqlite.c
@@ -38,6 +38,7 @@
 #include "config.h"
 #endif /* HAVE_CONFIG_H */
 
+#include <dirent.h>
 #include <errno.h>
 #include <event.h>
 #include <stdbool.h>
@@ -320,3 +321,30 @@ out_err:
 	sqlite3_finalize(stmt);
 	return ret;
 }
+
+/*
+ * remove any client records that were not reclaimed since grace_start.
+ */
+int
+sqlite_remove_unreclaimed(time_t grace_start)
+{
+	int ret;
+	char *err = NULL;
+
+	ret = snprintf(buf, sizeof(buf), "DELETE FROM clients WHERE time < %ld",
+			grace_start);
+	if (ret < 0) {
+		return ret;
+	} else if ((size_t)ret >= sizeof(buf)) {
+		ret = -EINVAL;
+		return ret;
+	}
+
+	ret = sqlite3_exec(dbh, buf, NULL, NULL, &err);
+	if (ret != SQLITE_OK)
+		xlog(D_GENERAL, "Delete failed: %s", err);
+
+	xlog(D_GENERAL, "%s: returning %d", __func__, ret);
+	sqlite3_free(err);
+	return ret;
+}
diff --git a/utils/nfsdcld/sqlite.h b/utils/nfsdcld/sqlite.h
index 7fc5ea4..f6ed348 100644
--- a/utils/nfsdcld/sqlite.h
+++ b/utils/nfsdcld/sqlite.h
@@ -23,5 +23,6 @@
 int sqlite_maindb_init(char *topdir);
 int sqlite_insert_client(const unsigned char *clname, const size_t namelen);
 int sqlite_check_client(const unsigned char *clname, const size_t namelen);
+int sqlite_remove_unreclaimed(const time_t grace_start);
 
 #endif /* _SQLITE_H */
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 06/11] nfsdcld: have daemon pass client row index back to kernel
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (4 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 05/11] nfsdcld: add function to remove unreclaimed client records Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 07/11] nfsdcld: implement an init upcall Jeff Layton
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

In order to deal with some potential problems with lock recovery, we'll
need a way to tag clustered locks in such a way that we know what client
owns them. In order to do that, we need a way to uniquely the client.

We could use the nfs_client_id4 value that the client sends, but it's
too long to be reasonable for this purpose. We could also hash that
down, but there's always the potential for hash collisions. What would
be best is a value that's unique for a particular clientid at a
particular time.

Add an autoincrement index field to the clients table, and have the
check and insert routines grab that and pass it back to the kernel.

For now, the kernel ignores this value, but eventually the locking code
can use this to designate ownership of a lock.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/nfsdcld.c |    8 +++++-
 utils/nfsdcld/sqlite.c  |   53 ++++++++++++++++++++++++++++++++++------------
 utils/nfsdcld/sqlite.h  |    6 +++-
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index 151dd16..bd643b0 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -117,15 +117,17 @@ static void
 cld_create(struct cld_client *clnt)
 {
 	int ret;
+	int64_t index;
 	ssize_t bsize, wsize;
 	struct cld_msg *cmsg = &clnt->cl_msg;
 
 	xlog(D_GENERAL, "%s: create client record.", __func__);
 
 	ret = sqlite_insert_client(cmsg->cm_u.cm_name.cn_id,
-				   cmsg->cm_u.cm_name.cn_len);
+				   cmsg->cm_u.cm_name.cn_len, &index);
 
 	cmsg->cm_status = ret ? -EREMOTEIO : ret;
+	cmsg->cm_u.cm_index = index;
 
 	bsize = sizeof(*cmsg);
 
@@ -164,16 +166,18 @@ static void
 cld_check(struct cld_client *clnt)
 {
 	int ret;
+	int64_t index;
 	ssize_t bsize, wsize;
 	struct cld_msg *cmsg = &clnt->cl_msg;
 
 	xlog(D_GENERAL, "%s: check client record", __func__);
 
 	ret = sqlite_check_client(cmsg->cm_u.cm_name.cn_id,
-				  cmsg->cm_u.cm_name.cn_len);
+				  cmsg->cm_u.cm_name.cn_len, &index);
 
 	/* set up reply */
 	cmsg->cm_status = ret ? -EACCES : ret;
+	cmsg->cm_u.cm_index = index;
 
 	bsize = sizeof(*cmsg);
 
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
index a0bcf2f..85e76a1 100644
--- a/utils/nfsdcld/sqlite.c
+++ b/utils/nfsdcld/sqlite.c
@@ -191,7 +191,8 @@ sqlite_maindb_init(char *topdir)
 
 	/* now create the "clients" table */
 	ret = sqlite3_exec(dbh, "CREATE TABLE IF NOT EXISTS clients "
-				"(id BLOB PRIMARY KEY, time INTEGER);",
+				"(idx INTEGER PRIMARY KEY AUTOINCREMENT, "
+				"id BLOB UNIQUE, time INTEGER);",
 				NULL, NULL, &err);
 	if (ret != SQLITE_OK) {
 		xlog(L_ERROR, "Unable to create clients table: %s", err);
@@ -218,12 +219,14 @@ out_err:
  * Returns a non-zero sqlite error code, or SQLITE_OK (aka 0)
  */
 int
-sqlite_insert_client(const unsigned char *clname, const size_t namelen)
+sqlite_insert_client(const unsigned char *clname, const size_t namelen,
+		     int64_t *index)
 {
 	int ret;
 	sqlite3_stmt *stmt = NULL;
 
-	ret = sqlite3_prepare_v2(dbh, "INSERT OR REPLACE INTO clients VALUES "
+	ret = sqlite3_prepare_v2(dbh, "INSERT OR REPLACE INTO clients "
+				      "(id, time) VALUES "
 				      "(?, strftime('%s', 'now'));", -1,
 					&stmt, NULL);
 	if (ret != SQLITE_OK) {
@@ -240,12 +243,38 @@ sqlite_insert_client(const unsigned char *clname, const size_t namelen)
 	}
 
 	ret = sqlite3_step(stmt);
-	if (ret == SQLITE_DONE)
-		ret = SQLITE_OK;
-	else
+	if (ret != SQLITE_DONE) {
 		xlog(D_GENERAL, "Unexpected return code from insert: %s",
 				sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	sqlite3_finalize(stmt);
+	stmt = NULL;
+
+	ret = sqlite3_prepare_v2(dbh, "SELECT index FROM clients WHERE "
+				      "id==?", -1, &stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to prepare update statement: %s",
+				sqlite3_errmsg(dbh));
+		goto out_err;
+	}
 
+	ret = sqlite3_bind_blob(stmt, 1, (const void *)clname, namelen,
+				SQLITE_STATIC);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Bind blob failed: %s", sqlite3_errmsg(dbh));
+		goto out_err;
+	}
+
+	ret = sqlite3_step(stmt);
+	if (ret != SQLITE_ROW) {
+		xlog(D_GENERAL, "Unexpected return code from select: %d", ret);
+		goto out_err;
+	}
+
+	ret = SQLITE_OK;
+	*index = (int64_t)sqlite3_column_int64(stmt, 0);
 out_err:
 	xlog(D_GENERAL, "%s: returning %d", __func__, ret);
 	sqlite3_finalize(stmt);
@@ -258,12 +287,13 @@ out_err:
  * return an error.
  */
 int
-sqlite_check_client(const unsigned char *clname, const size_t namelen)
+sqlite_check_client(const unsigned char *clname, const size_t namelen,
+		    int64_t *index)
 {
 	int ret;
 	sqlite3_stmt *stmt = NULL;
 
-	ret = sqlite3_prepare_v2(dbh, "SELECT count(*) FROM clients WHERE "
+	ret = sqlite3_prepare_v2(dbh, "SELECT index FROM clients WHERE "
 				      "id==?", -1, &stmt, NULL);
 	if (ret != SQLITE_OK) {
 		xlog(D_GENERAL, "Unable to prepare update statement: %s",
@@ -284,12 +314,7 @@ sqlite_check_client(const unsigned char *clname, const size_t namelen)
 		goto out_err;
 	}
 
-	ret = sqlite3_column_int(stmt, 0);
-	xlog(D_GENERAL, "Select returned %d rows", ret);
-	if (ret != 1) {
-		ret = -EACCES;
-		goto out_err;
-	}
+	*index = (int64_t)sqlite3_column_int64(stmt, 0);
 
 	sqlite3_finalize(stmt);
 	stmt = NULL;
diff --git a/utils/nfsdcld/sqlite.h b/utils/nfsdcld/sqlite.h
index f6ed348..0476bef 100644
--- a/utils/nfsdcld/sqlite.h
+++ b/utils/nfsdcld/sqlite.h
@@ -21,8 +21,10 @@
 #define _SQLITE_H_
 
 int sqlite_maindb_init(char *topdir);
-int sqlite_insert_client(const unsigned char *clname, const size_t namelen);
-int sqlite_check_client(const unsigned char *clname, const size_t namelen);
+int sqlite_insert_client(const unsigned char *clname, const size_t namelen,
+			 int64_t *index);
+int sqlite_check_client(const unsigned char *clname, const size_t namelen,
+			int64_t *index);
 int sqlite_remove_unreclaimed(const time_t grace_start);
 
 #endif /* _SQLITE_H */
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 07/11] nfsdcld: implement an init upcall
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (5 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 06/11] nfsdcld: have daemon pass client row index back to kernel Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 08/11] nfsdcld: allow daemon to wait for pipe to show up Jeff Layton
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

The init upcall gets and increments the generation number in the
parameters table atomically with respect to other clients. We do this by
starting an immediate transaction, selecting the current value and then
incrementing it on stable storage before ending the transaction. That
ensures that accesses to the generation counter are serialized.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/nfsdcld.c |   29 +++++++++++++++
 utils/nfsdcld/sqlite.c  |   90 +++++++++++++++++++++++++++++++++++++++++++++-
 utils/nfsdcld/sqlite.h  |    1 +
 3 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index bd643b0..afc53d2 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -114,6 +114,32 @@ cld_pipe_reopen(struct cld_client *clnt)
 }
 
 static void
+cld_init(struct cld_client *clnt)
+{
+	int ret;
+	uint32_t generation;
+	ssize_t bsize, wsize;
+	struct cld_msg *cmsg = &clnt->cl_msg;
+
+	xlog(D_GENERAL, "%s: server initialization", __func__);
+
+	ret = sqlite_get_generation(&generation);
+
+	cmsg->cm_status = ret ? -EREMOTEIO : ret;
+	cmsg->cm_u.cm_generation = generation;
+
+	bsize = sizeof(*cmsg);
+
+	xlog(D_GENERAL, "Doing downcall with status %d", cmsg->cm_status);
+	wsize = atomicio((void *)write, clnt->cl_fd, cmsg, bsize);
+	if (wsize != bsize) {
+		xlog(L_ERROR, "%s: problem writing to cld pipe (%ld): %m",
+			 __func__, wsize);
+		cld_pipe_reopen(clnt);
+	}
+}
+
+static void
 cld_create(struct cld_client *clnt)
 {
 	int ret;
@@ -240,6 +266,9 @@ cldcb(int UNUSED(fd), short which, void *data)
 	}
 
 	switch(cmsg->cm_cmd) {
+	case Cld_Init:
+		cld_init(clnt);
+		break;
 	case Cld_Create:
 		cld_create(clnt);
 		break;
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
index 85e76a1..47a3a19 100644
--- a/utils/nfsdcld/sqlite.c
+++ b/utils/nfsdcld/sqlite.c
@@ -189,6 +189,13 @@ sqlite_maindb_init(char *topdir)
 		goto out_err;
 	}
 
+	ret = sqlite3_exec(dbh, "INSERT OR IGNORE INTO parameters VALUES "
+				"(\"generation\", 0);", NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(L_ERROR, "Unable to initialize generation.");
+		goto out_err;
+	}
+
 	/* now create the "clients" table */
 	ret = sqlite3_exec(dbh, "CREATE TABLE IF NOT EXISTS clients "
 				"(idx INTEGER PRIMARY KEY AUTOINCREMENT, "
@@ -213,6 +220,85 @@ out_err:
 	return ret;
 }
 
+int
+sqlite_get_generation(uint32_t *generation)
+{
+	int ret, ret2;
+	char *err = NULL;
+	sqlite3_stmt *stmt = NULL;
+
+	ret = sqlite3_exec(dbh, "BEGIN IMMEDIATE TRANSACTION;",
+				NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(L_ERROR, "Unable to start transaction: %s", err);
+		goto out_err;
+	}
+
+	ret = sqlite3_prepare_v2(dbh,
+		"SELECT value FROM parameters WHERE key==\"generation\";",
+		-1, &stmt, NULL);
+	if (ret != SQLITE_OK) {
+		xlog(D_GENERAL, "Unable to prepare select statement: %d", ret);
+		goto out_err;
+	}
+
+	ret = sqlite3_step(stmt);
+	if (ret != SQLITE_ROW) {
+		xlog(D_GENERAL, "Select statement execution failed (%d): %s",
+				ret, sqlite3_errmsg(dbh));
+		goto out_rollback;
+	}
+
+	/* pass the generation number back to caller */
+	*generation = (uint32_t)sqlite3_column_int(stmt, 0);
+
+	ret = snprintf(buf, sizeof(buf),
+		       "UPDATE parameters set value=%d where key=="
+		       "\"generation\";", *generation + 1);
+	if (ret < 0) {
+		goto out_rollback;
+	} else if ((size_t)ret >= sizeof(buf)) {
+		ret = -EINVAL;
+		goto out_rollback;
+	}
+
+	ret = sqlite3_exec(dbh, (const char *)buf, NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(L_ERROR, "Unable to update generation number(%d): %s",
+				ret, err);
+		goto out_rollback;
+	}
+
+	ret = sqlite3_exec(dbh, "COMMIT TRANSACTION;", NULL, NULL, &err);
+	if (ret != SQLITE_OK) {
+		xlog(L_ERROR, "Unable to commit transaction(%d): %s",
+				ret, err);
+		goto out_rollback;
+	}
+
+out_err:
+	if (err) {
+		xlog(L_ERROR, "sqlite error: %s", err);
+		sqlite3_free(err);
+	}
+	sqlite3_finalize(stmt);
+	return ret;
+
+out_rollback:
+	/* attempt to rollback on any error once the transaction is started */
+	sqlite3_free(err);
+	err = NULL;
+	ret2 = sqlite3_exec(dbh, "ROLLBACK TRANSACTION;", NULL, NULL, &err);
+	if (ret2 != SQLITE_OK) {
+		/*
+		 * FIXME: is transaction still active? Should we re-init the
+		 * dbh here or have the daemon exit?
+		 */
+		xlog(L_ERROR, "Transaction rollback failed.");
+	}
+	goto out_err;
+}
+
 /*
  * Create a client record
  *
@@ -252,7 +338,7 @@ sqlite_insert_client(const unsigned char *clname, const size_t namelen,
 	sqlite3_finalize(stmt);
 	stmt = NULL;
 
-	ret = sqlite3_prepare_v2(dbh, "SELECT index FROM clients WHERE "
+	ret = sqlite3_prepare_v2(dbh, "SELECT idx FROM clients WHERE "
 				      "id==?", -1, &stmt, NULL);
 	if (ret != SQLITE_OK) {
 		xlog(D_GENERAL, "Unable to prepare update statement: %s",
@@ -293,7 +379,7 @@ sqlite_check_client(const unsigned char *clname, const size_t namelen,
 	int ret;
 	sqlite3_stmt *stmt = NULL;
 
-	ret = sqlite3_prepare_v2(dbh, "SELECT index FROM clients WHERE "
+	ret = sqlite3_prepare_v2(dbh, "SELECT idx FROM clients WHERE "
 				      "id==?", -1, &stmt, NULL);
 	if (ret != SQLITE_OK) {
 		xlog(D_GENERAL, "Unable to prepare update statement: %s",
diff --git a/utils/nfsdcld/sqlite.h b/utils/nfsdcld/sqlite.h
index 0476bef..6a77b80 100644
--- a/utils/nfsdcld/sqlite.h
+++ b/utils/nfsdcld/sqlite.h
@@ -21,6 +21,7 @@
 #define _SQLITE_H_
 
 int sqlite_maindb_init(char *topdir);
+int sqlite_get_generation(uint32_t *generation);
 int sqlite_insert_client(const unsigned char *clname, const size_t namelen,
 			 int64_t *index);
 int sqlite_check_client(const unsigned char *clname, const size_t namelen,
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 08/11] nfsdcld: allow daemon to wait for pipe to show up
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (6 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 07/11] nfsdcld: implement an init upcall Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated Jeff Layton
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

Use inotify to set a watch for IN_CREATE events in the nfsd rpc_pipefs
directory. Then, try to open the pipe. If that fails with -ENOENT, then
wait for an inotify event. When one comes in, then retry opening the
pipe.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 configure.ac            |    2 +-
 utils/nfsdcld/nfsdcld.c |   54 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 134b609..191a72a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -270,7 +270,7 @@ if test "$enable_nfsv4" = yes; then
   AC_SQLITE3_VERS
 
   if test "$enable_nfsdcld" = "yes"; then
-    AC_CHECK_HEADERS([linux/nfsd/cld.h], ,
+    AC_CHECK_HEADERS([linux/nfsd/cld.h libgen.h sys/inotify.h], ,
 		     AC_MSG_ERROR([Cannot find header needed for nfsdcld]))
 
     if test "$libsqlite3_cv_is_recent" != "yes" ; then
diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index afc53d2..b0c08e2 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -32,6 +32,8 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <libgen.h>
+#include <sys/inotify.h>
 #include <linux/nfsd/cld.h>
 
 #include "xlog.h"
@@ -76,18 +78,62 @@ usage(char *progname)
 	printf("%s [ -hFd ] [ -p pipe ] [ -s dir ]\n", progname);
 }
 
+#define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
+
 static int
 cld_pipe_open(struct cld_client *clnt)
 {
-	clnt->cl_fd = open(pipepath, O_RDWR, 0);
-	if (clnt->cl_fd < 0) {
-		xlog(L_ERROR, "%s: unable to open %s: %m", __func__, pipepath);
-		return clnt->cl_fd;
+	int ifd, wat;
+	ssize_t ret;
+	char *dirc, *dname;
+	char event[INOTIFY_EVENT_MAX];
+
+	clnt->cl_fd = -1;
+	dirc = strndup(pipepath, PATH_MAX);
+	if (!dirc) {
+		xlog_err("%s: unable to allocate memory", __func__);
+		goto out_free;
+	}
+
+	dname = dirname(dirc);
+
+	ifd = inotify_init();
+	if (ifd < 0) {
+		xlog_err("%s: inotify_init failed: %m", __func__);
+		goto out_free;
+	}
+
+	wat = inotify_add_watch(ifd, dname, IN_CREATE);
+	if (wat < 0) {
+		xlog_err("%s: inotify_add_watch failed: %m", __func__);
+		goto out;
+	}
+
+	for (;;) {
+		errno = 0;
+		clnt->cl_fd = open(pipepath, O_RDWR, 0);
+		if (clnt->cl_fd >= 0)
+			break;
+		else if (errno != ENOENT) {
+			xlog_err("%s: unable to open %s: %m", __func__,
+					pipepath);
+			goto out;
+		}
+		ret = read(ifd, event, INOTIFY_EVENT_MAX);
+		if (ret < 0) {
+			xlog_err("%s: read from inotify fd failed: %m",
+					__func__);
+			goto out;
+		}
 	}
 
 	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
 	event_add(&clnt->cl_event, NULL);
 
+out:
+	close(ifd);
+out_free:
+	free(dirc);
 	return clnt->cl_fd;
 }
 
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (7 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 08/11] nfsdcld: allow daemon to wait for pipe to show up Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-25 18:16   ` Steve Dickson
  2012-01-23 20:02 ` [PATCH v4 10/11] nfsdcld: add a manpage for nfsdcld Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 11/11] nfsdcld: update the README Jeff Layton
  10 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

This can happen if nfsd is shut down and restarted. If that occurs,
then reopen the pipe so we're not waiting for data on the defunct
pipe.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
index b0c08e2..0dc5b37 100644
--- a/utils/nfsdcld/nfsdcld.c
+++ b/utils/nfsdcld/nfsdcld.c
@@ -57,6 +57,8 @@ struct cld_client {
 
 /* global variables */
 static char *pipepath = DEFAULT_CLD_PATH;
+static int 		inotify_fd = -1;
+static struct event	pipedir_event;
 
 static struct option longopts[] =
 {
@@ -68,8 +70,10 @@ static struct option longopts[] =
 	{ NULL, 0, 0, 0 },
 };
 
+
 /* forward declarations */
 static void cldcb(int UNUSED(fd), short which, void *data);
+static void cld_pipe_reopen(struct cld_client *clnt);
 
 static void
 usage(char *progname)
@@ -80,10 +84,62 @@ usage(char *progname)
 
 #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
 
+static void
+cld_inotify_cb(int UNUSED(fd), short which, void *data)
+{
+	int ret, oldfd;
+	char evbuf[INOTIFY_EVENT_MAX];
+	char *dirc = NULL, *pname;
+	struct inotify_event *event = (struct inotify_event *)evbuf;
+	struct cld_client *clnt = data;
+
+	if (which != EV_READ)
+		return;
+
+	dirc = strndup(pipepath, PATH_MAX);
+	if (!dirc) {
+		xlog_err("%s: unable to allocate memory", __func__);
+		goto out;
+	}
+
+	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
+	if (ret < 0) {
+		xlog_err("%s: read from inotify fd failed: %m", __func__);
+		goto out;
+	}
+
+	/* check to see if we have a filename in the evbuf */
+	if (!event->len)
+		goto out;
+
+	pname = basename(dirc);
+
+	/* does the filename match our pipe? */
+	if (strncmp(pname, event->name, event->len))
+		goto out;
+
+	/*
+	 * reopen the pipe. The old fd is not closed until the new one is
+	 * opened, so we know they should be different if the reopen is
+	 * successful.
+	 */
+	oldfd = clnt->cl_fd;
+	do {
+		cld_pipe_reopen(clnt);
+	} while (oldfd == clnt->cl_fd);
+
+	/* readd the event for the cl_event pipe */
+	event_add(&clnt->cl_event, NULL);
+
+out:
+	event_add(&pipedir_event, NULL);
+	free(dirc);
+}
+
 static int
 cld_pipe_open(struct cld_client *clnt)
 {
-	int ifd, wat;
+	int wat;
 	ssize_t ret;
 	char *dirc, *dname;
 	char event[INOTIFY_EVENT_MAX];
@@ -97,16 +153,16 @@ cld_pipe_open(struct cld_client *clnt)
 
 	dname = dirname(dirc);
 
-	ifd = inotify_init();
-	if (ifd < 0) {
+	inotify_fd = inotify_init();
+	if (inotify_fd < 0) {
 		xlog_err("%s: inotify_init failed: %m", __func__);
 		goto out_free;
 	}
 
-	wat = inotify_add_watch(ifd, dname, IN_CREATE);
+	wat = inotify_add_watch(inotify_fd, dname, IN_CREATE);
 	if (wat < 0) {
 		xlog_err("%s: inotify_add_watch failed: %m", __func__);
-		goto out;
+		goto out_err;
 	}
 
 	for (;;) {
@@ -117,24 +173,31 @@ cld_pipe_open(struct cld_client *clnt)
 		else if (errno != ENOENT) {
 			xlog_err("%s: unable to open %s: %m", __func__,
 					pipepath);
-			goto out;
+			goto out_err;
 		}
-		ret = read(ifd, event, INOTIFY_EVENT_MAX);
+		ret = read(inotify_fd, event, INOTIFY_EVENT_MAX);
 		if (ret < 0) {
 			xlog_err("%s: read from inotify fd failed: %m",
 					__func__);
-			goto out;
+			goto out_err;
 		}
 	}
 
+	/* set event for the pipe reads */
 	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
 	event_add(&clnt->cl_event, NULL);
 
-out:
-	close(ifd);
+	/* another event for inotify read */
+	event_set(&pipedir_event, inotify_fd, EV_READ, cld_inotify_cb, clnt);
+	event_add(&pipedir_event, NULL);
+
 out_free:
 	free(dirc);
 	return clnt->cl_fd;
+
+out_err:
+	close(inotify_fd);
+	goto out_free;
 }
 
 static void
@@ -142,6 +205,7 @@ cld_pipe_reopen(struct cld_client *clnt)
 {
 	int fd;
 
+	xlog(D_GENERAL, "%s: reopening pipe", __func__);
 	fd = open(pipepath, O_RDWR, 0);
 	if (fd < 0) {
 		xlog_warn("%s: Re-opening of %s failed: %m",
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 10/11] nfsdcld: add a manpage for nfsdcld
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (8 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  2012-01-23 20:02 ` [PATCH v4 11/11] nfsdcld: update the README Jeff Layton
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

...for now I'm including the POD source for the manpage. We can drop
from the repo if desired though.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsdcld/Makefile.am |    4 +-
 utils/nfsdcld/nfsdcld.man |  180 +++++++++++++++++++++++++++++++++++++++++++++
 utils/nfsdcld/nfsdcld.pod |   67 +++++++++++++++++
 3 files changed, 249 insertions(+), 2 deletions(-)
 create mode 100644 utils/nfsdcld/nfsdcld.man
 create mode 100644 utils/nfsdcld/nfsdcld.pod

diff --git a/utils/nfsdcld/Makefile.am b/utils/nfsdcld/Makefile.am
index 8e4f2ab..f320dff 100644
--- a/utils/nfsdcld/Makefile.am
+++ b/utils/nfsdcld/Makefile.am
@@ -1,7 +1,7 @@
 ## Process this file with automake to produce Makefile.in
 
-#man8_MANS	= nfsdcld.man
-#EXTRA_DIST = $(man8_MANS)
+man8_MANS	= nfsdcld.man
+EXTRA_DIST	= $(man8_MANS)
 
 AM_CFLAGS	+= -D_LARGEFILE64_SOURCE
 sbin_PROGRAMS	= nfsdcld
diff --git a/utils/nfsdcld/nfsdcld.man b/utils/nfsdcld/nfsdcld.man
new file mode 100644
index 0000000..bad5f34
--- /dev/null
+++ b/utils/nfsdcld/nfsdcld.man
@@ -0,0 +1,180 @@
+.\" Automatically generated by Pod::Man 2.22 (Pod::Simple 3.13)
+.\"
+.\" Standard preamble:
+.\" ========================================================================
+.de Sp \" Vertical space (when we can't use .PP)
+.if t .sp .5v
+.if n .sp
+..
+.de Vb \" Begin verbatim text
+.ft CW
+.nf
+.ne \\$1
+..
+.de Ve \" End verbatim text
+.ft R
+.fi
+..
+.\" Set up some character translations and predefined strings.  \*(-- will
+.\" give an unbreakable dash, \*(PI will give pi, \*(L" will give a left
+.\" double quote, and \*(R" will give a right double quote.  \*(C+ will
+.\" give a nicer C++.  Capital omega is used to do unbreakable dashes and
+.\" therefore won't be available.  \*(C` and \*(C' expand to `' in nroff,
+.\" nothing in troff, for use with C<>.
+.tr \(*W-
+.ds C+ C\v'-.1v'\h'-1p'\s-2+\h'-1p'+\s0\v'.1v'\h'-1p'
+.ie n \{\
+.    ds -- \(*W-
+.    ds PI pi
+.    if (\n(.H=4u)&(1m=24u) .ds -- \(*W\h'-12u'\(*W\h'-12u'-\" diablo 10 pitch
+.    if (\n(.H=4u)&(1m=20u) .ds -- \(*W\h'-12u'\(*W\h'-8u'-\"  diablo 12 pitch
+.    ds L" ""
+.    ds R" ""
+.    ds C` ""
+.    ds C' ""
+'br\}
+.el\{\
+.    ds -- \|\(em\|
+.    ds PI \(*p
+.    ds L" ``
+.    ds R" ''
+'br\}
+.\"
+.\" Escape single quotes in literal strings from groff's Unicode transform.
+.ie \n(.g .ds Aq \(aq
+.el       .ds Aq '
+.\"
+.\" If the F register is turned on, we'll generate index entries on stderr for
+.\" titles (.TH), headers (.SH), subsections (.SS), items (.Ip), and index
+.\" entries marked with X<> in POD.  Of course, you'll have to process the
+.\" output yourself in some meaningful fashion.
+.ie \nF \{\
+.    de IX
+.    tm Index:\\$1\t\\n%\t"\\$2"
+..
+.    nr % 0
+.    rr F
+.\}
+.el \{\
+.    de IX
+..
+.\}
+.\"
+.\" Accent mark definitions (@(#)ms.acc 1.5 88/02/08 SMI; from UCB 4.2).
+.\" Fear.  Run.  Save yourself.  No user-serviceable parts.
+.    \" fudge factors for nroff and troff
+.if n \{\
+.    ds #H 0
+.    ds #V .8m
+.    ds #F .3m
+.    ds #[ \f1
+.    ds #] \fP
+.\}
+.if t \{\
+.    ds #H ((1u-(\\\\n(.fu%2u))*.13m)
+.    ds #V .6m
+.    ds #F 0
+.    ds #[ \&
+.    ds #] \&
+.\}
+.    \" simple accents for nroff and troff
+.if n \{\
+.    ds ' \&
+.    ds ` \&
+.    ds ^ \&
+.    ds , \&
+.    ds ~ ~
+.    ds /
+.\}
+.if t \{\
+.    ds ' \\k:\h'-(\\n(.wu*8/10-\*(#H)'\'\h"|\\n:u"
+.    ds ` \\k:\h'-(\\n(.wu*8/10-\*(#H)'\`\h'|\\n:u'
+.    ds ^ \\k:\h'-(\\n(.wu*10/11-\*(#H)'^\h'|\\n:u'
+.    ds , \\k:\h'-(\\n(.wu*8/10)',\h'|\\n:u'
+.    ds ~ \\k:\h'-(\\n(.wu-\*(#H-.1m)'~\h'|\\n:u'
+.    ds / \\k:\h'-(\\n(.wu*8/10-\*(#H)'\z\(sl\h'|\\n:u'
+.\}
+.    \" troff and (daisy-wheel) nroff accents
+.ds : \\k:\h'-(\\n(.wu*8/10-\*(#H+.1m+\*(#F)'\v'-\*(#V'\z.\h'.2m+\*(#F'.\h'|\\n:u'\v'\*(#V'
+.ds 8 \h'\*(#H'\(*b\h'-\*(#H'
+.ds o \\k:\h'-(\\n(.wu+\w'\(de'u-\*(#H)/2u'\v'-.3n'\*(#[\z\(de\v'.3n'\h'|\\n:u'\*(#]
+.ds d- \h'\*(#H'\(pd\h'-\w'~'u'\v'-.25m'\f2\(hy\fP\v'.25m'\h'-\*(#H'
+.ds D- D\\k:\h'-\w'D'u'\v'-.11m'\z\(hy\v'.11m'\h'|\\n:u'
+.ds th \*(#[\v'.3m'\s+1I\s-1\v'-.3m'\h'-(\w'I'u*2/3)'\s-1o\s+1\*(#]
+.ds Th \*(#[\s+2I\s-2\h'-\w'I'u*3/5'\v'-.3m'o\v'.3m'\*(#]
+.ds ae a\h'-(\w'a'u*4/10)'e
+.ds Ae A\h'-(\w'A'u*4/10)'E
+.    \" corrections for vroff
+.if v .ds ~ \\k:\h'-(\\n(.wu*9/10-\*(#H)'\s-2\u~\d\s+2\h'|\\n:u'
+.if v .ds ^ \\k:\h'-(\\n(.wu*10/11-\*(#H)'\v'-.4m'^\v'.4m'\h'|\\n:u'
+.    \" for low resolution devices (crt and lpr)
+.if \n(.H>23 .if \n(.V>19 \
+\{\
+.    ds : e
+.    ds 8 ss
+.    ds o a
+.    ds d- d\h'-1'\(ga
+.    ds D- D\h'-1'\(hy
+.    ds th \o'bp'
+.    ds Th \o'LP'
+.    ds ae ae
+.    ds Ae AE
+.\}
+.rm #[ #] #H #V #F C
+.\" ========================================================================
+.\"
+.IX Title "NFSDCLD 8"
+.TH NFSDCLD 8 "2011-12-21" "" ""
+.\" For nroff, turn off justification.  Always turn off hyphenation; it makes
+.\" way too many mistakes in technical documents.
+.if n .ad l
+.nh
+.SH "NAME"
+nfsdcld \- NFSv4 Client Tracking Daemon
+.SH "SYNOPSIS"
+.IX Header "SYNOPSIS"
+nfsdcld [\-d] [\-F] [\-p path] [\-s stable storage dir]
+.SH "DESCRIPTION"
+.IX Header "DESCRIPTION"
+nfsdcld is the NFSv4 client tracking daemon. It is not necessary to run
+this daemon on machines that are not acting as NFSv4 servers.
+.PP
+When a network partition is combined with a server reboot, there are
+edge conditions that can cause the server to grant lock reclaims when
+other clients have taken conflicting locks in the interim. A more detailed
+explanation of this issue is described in \s-1RFC\s0 3530, section 8.6.3.
+.PP
+In order to prevent these problems, the server must track a small amount
+of per-client information on stable storage. This daemon provides the
+userspace piece of that functionality.
+.SH "OPTIONS"
+.IX Header "OPTIONS"
+.IP "\fB\-d\fR, \fB\-\-debug\fR" 4
+.IX Item "-d, --debug"
+Enable debug level logging.
+.IP "\fB\-F\fR, \fB\-\-foreground\fR" 4
+.IX Item "-F, --foreground"
+Runs the daemon in the foreground and prints all output to stderr
+.IP "\fB\-p\fR \fIpipe\fR, \fB\-\-pipe\fR=\fIpipe\fR" 4
+.IX Item "-p pipe, --pipe=pipe"
+Location of the \*(L"cld\*(R" upcall pipe. The default value is
+\&\fI/var/lib/nfs/rpc_pipefs/nfsd/cld\fR. If the pipe does not exist when the
+daemon starts then it will wait for it to be created.
+.IP "\fB\-s\fR \fIstoragedir\fR, \fB\-\-storagedir\fR=\fIstorage_dir\fR" 4
+.IX Item "-s storagedir, --storagedir=storage_dir"
+Directory where stable storage information should be kept. The default
+value is \fI/var/lib/nfs/nfsdcld\fR.
+.SH "NOTES"
+.IX Header "NOTES"
+The Linux kernel NFSv4 server has historically tracked this information
+on stable storage by manipulating information on the filesystem
+directly, in the directory to which \fI/proc/fs/nfsd/nfsv4recoverydir\fR
+points.
+.PP
+This daemon requires a kernel that supports the nfsdcld upcall. If the
+kernel does not support the new upcall, or is using the legacy client
+name tracking code then it will not create the pipe that nfsdcld uses to
+talk to the kernel.
+.SH "AUTHORS"
+.IX Header "AUTHORS"
+The nfsdcld daemon was developed by Jeff Layton <jlayton@redhat.com>.
diff --git a/utils/nfsdcld/nfsdcld.pod b/utils/nfsdcld/nfsdcld.pod
new file mode 100644
index 0000000..2ff1d3d
--- /dev/null
+++ b/utils/nfsdcld/nfsdcld.pod
@@ -0,0 +1,67 @@
+# turn into a manpage with the following command:
+#
+# pod2man -s 8 -c '' -r '' --stderr nfsdcld.pod > nfsdcld.man
+#
+
+=head1 NAME
+
+nfsdcld - NFSv4 Client Tracking Daemon
+
+=head1 SYNOPSIS
+
+nfsdcld [-d] [-F] [-p path] [-s stable storage dir]
+
+=head1 DESCRIPTION
+
+nfsdcld is the NFSv4 client tracking daemon. It is not necessary to run
+this daemon on machines that are not acting as NFSv4 servers.
+
+When a network partition is combined with a server reboot, there are
+edge conditions that can cause the server to grant lock reclaims when
+other clients have taken conflicting locks in the interim. A more detailed
+explanation of this issue is described in RFC 3530, section 8.6.3.
+
+In order to prevent these problems, the server must track a small amount
+of per-client information on stable storage. This daemon provides the
+userspace piece of that functionality.
+
+=head1 OPTIONS
+
+=over
+
+=item B<-d>, B<--debug>
+
+Enable debug level logging.
+
+=item B<-F>, B<--foreground>
+
+Runs the daemon in the foreground and prints all output to stderr
+
+=item B<-p> I<pipe>, B<--pipe>=I<pipe>
+
+Location of the "cld" upcall pipe. The default value is
+I</var/lib/nfs/rpc_pipefs/nfsd/cld>. If the pipe does not exist when the
+daemon starts then it will wait for it to be created.
+
+=item B<-s> I<storagedir>, B<--storagedir>=I<storage_dir>
+
+Directory where stable storage information should be kept. The default
+value is I</var/lib/nfs/nfsdcld>.
+
+=back
+
+=head1 NOTES
+
+The Linux kernel NFSv4 server has historically tracked this information
+on stable storage by manipulating information on the filesystem
+directly, in the directory to which I</proc/fs/nfsd/nfsv4recoverydir>
+points.
+
+This daemon requires a kernel that supports the nfsdcld upcall. If the
+kernel does not support the new upcall, or is using the legacy client
+name tracking code then it will not create the pipe that nfsdcld uses to
+talk to the kernel.
+
+=head1 AUTHORS
+
+The nfsdcld daemon was developed by Jeff Layton <jlayton@redhat.com>.
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v4 11/11] nfsdcld: update the README
  2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
                   ` (9 preceding siblings ...)
  2012-01-23 20:02 ` [PATCH v4 10/11] nfsdcld: add a manpage for nfsdcld Jeff Layton
@ 2012-01-23 20:02 ` Jeff Layton
  10 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2012-01-23 20:02 UTC (permalink / raw)
  To: steved; +Cc: linux-nfs

Add info to the README about when this daemon should be started, and
its build and runtime requirements.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 README |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/README b/README
index e7588cf..cb1d2f5 100644
--- a/README
+++ b/README
@@ -15,6 +15,8 @@ libraries.  They are available from
    http://www.citi.umich.edu/projects/nfsv4/linux/libnfsidmap/
 Otherwise use --disable-nfsv4
 
+To use the nfsdcld tracking daemon, nfsv4 support must be enabled,
+and the libsqlite3 development libraries must be installed.
 
 1. COMPILING
 
@@ -106,12 +108,31 @@ scripts can be written to work correctly.
        the lock.
        rpc.statd is only needed for NFSv2 and NFSv3 support.
 
-   E/ rpc.nfsd
+   E/ nfsdcld
+       This daemon is only needed on kernels that support the nfsdcld
+       upcall, and only if the legacy client ID tracking isn't used. It
+       is also not needed if the server does not support NFSv4.
+
+       To determine whether you need this or not, do the following:
+
+           # cat /proc/fs/nfsd/versions
+
+       That should yield a list of NFS versions that this kernel supports,
+       if "4" or later is not in that list, or they are prefixed with a "-"
+       then you don't need to run this daemon. Next:
+
+           # cat /proc/fs/nfsd/nfsv4recoverydir
+
+       If that file is not present, or the directory that the above command
+       outputs is not present, then this daemon is required in order to
+       support lock recovery by the clients when the server reboots.
+
+   F/ rpc.nfsd
        Starting nfsd will automatically start lockd.  The nfs server
        will now be fully active and respond to any requests from
        clients.
        
-   F/ sm-notify
+   G/ sm-notify
        This will notify any client which might have locks from before
        a reboot to try to reclaim their locks.  This should start
        immediately after rpc.nfsd is started so that clients have a
-- 
1.7.7.5


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-23 20:02 ` [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated Jeff Layton
@ 2012-01-25 18:16   ` Steve Dickson
  2012-01-25 19:09     ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2012-01-25 18:16 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-nfs

Hey Jeff,

Commit inline... 

On 01/23/2012 03:02 PM, Jeff Layton wrote:
> This can happen if nfsd is shut down and restarted. If that occurs,
> then reopen the pipe so we're not waiting for data on the defunct
> pipe.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> index b0c08e2..0dc5b37 100644
> --- a/utils/nfsdcld/nfsdcld.c
> +++ b/utils/nfsdcld/nfsdcld.c
> @@ -57,6 +57,8 @@ struct cld_client {
>  
>  /* global variables */
>  static char *pipepath = DEFAULT_CLD_PATH;
> +static int 		inotify_fd = -1;
> +static struct event	pipedir_event;
>  
>  static struct option longopts[] =
>  {
> @@ -68,8 +70,10 @@ static struct option longopts[] =
>  	{ NULL, 0, 0, 0 },
>  };
>  
> +
>  /* forward declarations */
>  static void cldcb(int UNUSED(fd), short which, void *data);
> +static void cld_pipe_reopen(struct cld_client *clnt);
>  
>  static void
>  usage(char *progname)
> @@ -80,10 +84,62 @@ usage(char *progname)
>  
>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
>  
> +static void
> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> +{
> +	int ret, oldfd;
> +	char evbuf[INOTIFY_EVENT_MAX];
> +	char *dirc = NULL, *pname;
> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> +	struct cld_client *clnt = data;
> +
> +	if (which != EV_READ)
> +		return;
> +
> +	dirc = strndup(pipepath, PATH_MAX);
> +	if (!dirc) {
> +		xlog_err("%s: unable to allocate memory", __func__);
> +		goto out;
> +	}
> +
> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> +	if (ret < 0) {
> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> +		goto out;
> +	}
> +
> +	/* check to see if we have a filename in the evbuf */
> +	if (!event->len)
> +		goto out;
> +
> +	pname = basename(dirc);
> +
> +	/* does the filename match our pipe? */
> +	if (strncmp(pname, event->name, event->len))
> +		goto out;
> +
> +	/*
> +	 * reopen the pipe. The old fd is not closed until the new one is
> +	 * opened, so we know they should be different if the reopen is
> +	 * successful.
> +	 */
> +	oldfd = clnt->cl_fd;
> +	do {
> +		cld_pipe_reopen(clnt);
> +	} while (oldfd == clnt->cl_fd);
Doesn't this have a potential for an infinite loop? 

steved.  
> +
> +	/* readd the event for the cl_event pipe */
> +	event_add(&clnt->cl_event, NULL);
> +
> +out:
> +	event_add(&pipedir_event, NULL);
> +	free(dirc);
> +}
> +
>  static int
>  cld_pipe_open(struct cld_client *clnt)
>  {
> -	int ifd, wat;
> +	int wat;
>  	ssize_t ret;
>  	char *dirc, *dname;
>  	char event[INOTIFY_EVENT_MAX];
> @@ -97,16 +153,16 @@ cld_pipe_open(struct cld_client *clnt)
>  
>  	dname = dirname(dirc);
>  
> -	ifd = inotify_init();
> -	if (ifd < 0) {
> +	inotify_fd = inotify_init();
> +	if (inotify_fd < 0) {
>  		xlog_err("%s: inotify_init failed: %m", __func__);
>  		goto out_free;
>  	}
>  
> -	wat = inotify_add_watch(ifd, dname, IN_CREATE);
> +	wat = inotify_add_watch(inotify_fd, dname, IN_CREATE);
>  	if (wat < 0) {
>  		xlog_err("%s: inotify_add_watch failed: %m", __func__);
> -		goto out;
> +		goto out_err;
>  	}
>  
>  	for (;;) {
> @@ -117,24 +173,31 @@ cld_pipe_open(struct cld_client *clnt)
>  		else if (errno != ENOENT) {
>  			xlog_err("%s: unable to open %s: %m", __func__,
>  					pipepath);
> -			goto out;
> +			goto out_err;
>  		}
> -		ret = read(ifd, event, INOTIFY_EVENT_MAX);
> +		ret = read(inotify_fd, event, INOTIFY_EVENT_MAX);
>  		if (ret < 0) {
>  			xlog_err("%s: read from inotify fd failed: %m",
>  					__func__);
> -			goto out;
> +			goto out_err;
>  		}
>  	}
>  
> +	/* set event for the pipe reads */
>  	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
>  	event_add(&clnt->cl_event, NULL);
>  
> -out:
> -	close(ifd);
> +	/* another event for inotify read */
> +	event_set(&pipedir_event, inotify_fd, EV_READ, cld_inotify_cb, clnt);
> +	event_add(&pipedir_event, NULL);
> +
>  out_free:
>  	free(dirc);
>  	return clnt->cl_fd;
> +
> +out_err:
> +	close(inotify_fd);
> +	goto out_free;
>  }
>  
>  static void
> @@ -142,6 +205,7 @@ cld_pipe_reopen(struct cld_client *clnt)
>  {
>  	int fd;
>  
> +	xlog(D_GENERAL, "%s: reopening pipe", __func__);
>  	fd = open(pipepath, O_RDWR, 0);
>  	if (fd < 0) {
>  		xlog_warn("%s: Re-opening of %s failed: %m",
> -- 1.7.7.5

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 18:16   ` Steve Dickson
@ 2012-01-25 19:09     ` Jeff Layton
  2012-01-25 19:31       ` Steve Dickson
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-25 19:09 UTC (permalink / raw)
  To: Steve Dickson; +Cc: linux-nfs

On Wed, 25 Jan 2012 13:16:24 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> Hey Jeff,
> 
> Commit inline... 
> 
> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> > This can happen if nfsd is shut down and restarted. If that occurs,
> > then reopen the pipe so we're not waiting for data on the defunct
> > pipe.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> >  1 files changed, 74 insertions(+), 10 deletions(-)
> > 
> > diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> > index b0c08e2..0dc5b37 100644
> > --- a/utils/nfsdcld/nfsdcld.c
> > +++ b/utils/nfsdcld/nfsdcld.c
> > @@ -57,6 +57,8 @@ struct cld_client {
> >  
> >  /* global variables */
> >  static char *pipepath = DEFAULT_CLD_PATH;
> > +static int 		inotify_fd = -1;
> > +static struct event	pipedir_event;
> >  
> >  static struct option longopts[] =
> >  {
> > @@ -68,8 +70,10 @@ static struct option longopts[] =
> >  	{ NULL, 0, 0, 0 },
> >  };
> >  
> > +
> >  /* forward declarations */
> >  static void cldcb(int UNUSED(fd), short which, void *data);
> > +static void cld_pipe_reopen(struct cld_client *clnt);
> >  
> >  static void
> >  usage(char *progname)
> > @@ -80,10 +84,62 @@ usage(char *progname)
> >  
> >  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> >  
> > +static void
> > +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> > +{
> > +	int ret, oldfd;
> > +	char evbuf[INOTIFY_EVENT_MAX];
> > +	char *dirc = NULL, *pname;
> > +	struct inotify_event *event = (struct inotify_event *)evbuf;
> > +	struct cld_client *clnt = data;
> > +
> > +	if (which != EV_READ)
> > +		return;
> > +
> > +	dirc = strndup(pipepath, PATH_MAX);
> > +	if (!dirc) {
> > +		xlog_err("%s: unable to allocate memory", __func__);
> > +		goto out;
> > +	}
> > +
> > +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> > +	if (ret < 0) {
> > +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> > +		goto out;
> > +	}
> > +
> > +	/* check to see if we have a filename in the evbuf */
> > +	if (!event->len)
> > +		goto out;
> > +
> > +	pname = basename(dirc);
> > +
> > +	/* does the filename match our pipe? */
> > +	if (strncmp(pname, event->name, event->len))
> > +		goto out;
> > +
> > +	/*
> > +	 * reopen the pipe. The old fd is not closed until the new one is
> > +	 * opened, so we know they should be different if the reopen is
> > +	 * successful.
> > +	 */
> > +	oldfd = clnt->cl_fd;
> > +	do {
> > +		cld_pipe_reopen(clnt);
> > +	} while (oldfd == clnt->cl_fd);
> Doesn't this have a potential for an infinite loop? 
> 
> steved.  


Yes. If reopening the new pipe continually fails then it will loop
forever.

> > +
> > +	/* readd the event for the cl_event pipe */
> > +	event_add(&clnt->cl_event, NULL);
> > +
> > +out:
> > +	event_add(&pipedir_event, NULL);
> > +	free(dirc);
> > +}
> > +
> >  static int
> >  cld_pipe_open(struct cld_client *clnt)
> >  {
> > -	int ifd, wat;
> > +	int wat;
> >  	ssize_t ret;
> >  	char *dirc, *dname;
> >  	char event[INOTIFY_EVENT_MAX];
> > @@ -97,16 +153,16 @@ cld_pipe_open(struct cld_client *clnt)
> >  
> >  	dname = dirname(dirc);
> >  
> > -	ifd = inotify_init();
> > -	if (ifd < 0) {
> > +	inotify_fd = inotify_init();
> > +	if (inotify_fd < 0) {
> >  		xlog_err("%s: inotify_init failed: %m", __func__);
> >  		goto out_free;
> >  	}
> >  
> > -	wat = inotify_add_watch(ifd, dname, IN_CREATE);
> > +	wat = inotify_add_watch(inotify_fd, dname, IN_CREATE);
> >  	if (wat < 0) {
> >  		xlog_err("%s: inotify_add_watch failed: %m", __func__);
> > -		goto out;
> > +		goto out_err;
> >  	}
> >  
> >  	for (;;) {
> > @@ -117,24 +173,31 @@ cld_pipe_open(struct cld_client *clnt)
> >  		else if (errno != ENOENT) {
> >  			xlog_err("%s: unable to open %s: %m", __func__,
> >  					pipepath);
> > -			goto out;
> > +			goto out_err;
> >  		}
> > -		ret = read(ifd, event, INOTIFY_EVENT_MAX);
> > +		ret = read(inotify_fd, event, INOTIFY_EVENT_MAX);
> >  		if (ret < 0) {
> >  			xlog_err("%s: read from inotify fd failed: %m",
> >  					__func__);
> > -			goto out;
> > +			goto out_err;
> >  		}
> >  	}
> >  
> > +	/* set event for the pipe reads */
> >  	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
> >  	event_add(&clnt->cl_event, NULL);
> >  
> > -out:
> > -	close(ifd);
> > +	/* another event for inotify read */
> > +	event_set(&pipedir_event, inotify_fd, EV_READ, cld_inotify_cb, clnt);
> > +	event_add(&pipedir_event, NULL);
> > +
> >  out_free:
> >  	free(dirc);
> >  	return clnt->cl_fd;
> > +
> > +out_err:
> > +	close(inotify_fd);
> > +	goto out_free;
> >  }
> >  
> >  static void
> > @@ -142,6 +205,7 @@ cld_pipe_reopen(struct cld_client *clnt)
> >  {
> >  	int fd;
> >  
> > +	xlog(D_GENERAL, "%s: reopening pipe", __func__);
> >  	fd = open(pipepath, O_RDWR, 0);
> >  	if (fd < 0) {
> >  		xlog_warn("%s: Re-opening of %s failed: %m",
> > -- 1.7.7.5


-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 19:09     ` Jeff Layton
@ 2012-01-25 19:31       ` Steve Dickson
  2012-01-25 20:28         ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2012-01-25 19:31 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-nfs



On 01/25/2012 02:09 PM, Jeff Layton wrote:
> On Wed, 25 Jan 2012 13:16:24 -0500
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Hey Jeff,
>>
>> Commit inline... 
>>
>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
>>> This can happen if nfsd is shut down and restarted. If that occurs,
>>> then reopen the pipe so we're not waiting for data on the defunct
>>> pipe.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
>>>  1 files changed, 74 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
>>> index b0c08e2..0dc5b37 100644
>>> --- a/utils/nfsdcld/nfsdcld.c
>>> +++ b/utils/nfsdcld/nfsdcld.c
>>> @@ -57,6 +57,8 @@ struct cld_client {
>>>  
>>>  /* global variables */
>>>  static char *pipepath = DEFAULT_CLD_PATH;
>>> +static int 		inotify_fd = -1;
>>> +static struct event	pipedir_event;
>>>  
>>>  static struct option longopts[] =
>>>  {
>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
>>>  	{ NULL, 0, 0, 0 },
>>>  };
>>>  
>>> +
>>>  /* forward declarations */
>>>  static void cldcb(int UNUSED(fd), short which, void *data);
>>> +static void cld_pipe_reopen(struct cld_client *clnt);
>>>  
>>>  static void
>>>  usage(char *progname)
>>> @@ -80,10 +84,62 @@ usage(char *progname)
>>>  
>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
>>>  
>>> +static void
>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
>>> +{
>>> +	int ret, oldfd;
>>> +	char evbuf[INOTIFY_EVENT_MAX];
>>> +	char *dirc = NULL, *pname;
>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
>>> +	struct cld_client *clnt = data;
>>> +
>>> +	if (which != EV_READ)
>>> +		return;
>>> +
>>> +	dirc = strndup(pipepath, PATH_MAX);
>>> +	if (!dirc) {
>>> +		xlog_err("%s: unable to allocate memory", __func__);
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
>>> +	if (ret < 0) {
>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* check to see if we have a filename in the evbuf */
>>> +	if (!event->len)
>>> +		goto out;
>>> +
>>> +	pname = basename(dirc);
>>> +
>>> +	/* does the filename match our pipe? */
>>> +	if (strncmp(pname, event->name, event->len))
>>> +		goto out;
>>> +
>>> +	/*
>>> +	 * reopen the pipe. The old fd is not closed until the new one is
>>> +	 * opened, so we know they should be different if the reopen is
>>> +	 * successful.
>>> +	 */
>>> +	oldfd = clnt->cl_fd;
>>> +	do {
>>> +		cld_pipe_reopen(clnt);
>>> +	} while (oldfd == clnt->cl_fd);
>> Doesn't this have a potential for an infinite loop? 
>>
>> steved.  
> 
> 
> Yes. If reopening the new pipe continually fails then it will loop
> forever.
Would it be more accurate to say it would be spinning forever? 
Since there is no sleep or delay in cld_pipe_reopen, what's
going to stop the daemon from spinning in a CPU bound loop?

steved.
> 
>>> +
>>> +	/* readd the event for the cl_event pipe */
>>> +	event_add(&clnt->cl_event, NULL);
>>> +
>>> +out:
>>> +	event_add(&pipedir_event, NULL);
>>> +	free(dirc);
>>> +}
>>> +
>>>  static int
>>>  cld_pipe_open(struct cld_client *clnt)
>>>  {
>>> -	int ifd, wat;
>>> +	int wat;
>>>  	ssize_t ret;
>>>  	char *dirc, *dname;
>>>  	char event[INOTIFY_EVENT_MAX];
>>> @@ -97,16 +153,16 @@ cld_pipe_open(struct cld_client *clnt)
>>>  
>>>  	dname = dirname(dirc);
>>>  
>>> -	ifd = inotify_init();
>>> -	if (ifd < 0) {
>>> +	inotify_fd = inotify_init();
>>> +	if (inotify_fd < 0) {
>>>  		xlog_err("%s: inotify_init failed: %m", __func__);
>>>  		goto out_free;
>>>  	}
>>>  
>>> -	wat = inotify_add_watch(ifd, dname, IN_CREATE);
>>> +	wat = inotify_add_watch(inotify_fd, dname, IN_CREATE);
>>>  	if (wat < 0) {
>>>  		xlog_err("%s: inotify_add_watch failed: %m", __func__);
>>> -		goto out;
>>> +		goto out_err;
>>>  	}
>>>  
>>>  	for (;;) {
>>> @@ -117,24 +173,31 @@ cld_pipe_open(struct cld_client *clnt)
>>>  		else if (errno != ENOENT) {
>>>  			xlog_err("%s: unable to open %s: %m", __func__,
>>>  					pipepath);
>>> -			goto out;
>>> +			goto out_err;
>>>  		}
>>> -		ret = read(ifd, event, INOTIFY_EVENT_MAX);
>>> +		ret = read(inotify_fd, event, INOTIFY_EVENT_MAX);
>>>  		if (ret < 0) {
>>>  			xlog_err("%s: read from inotify fd failed: %m",
>>>  					__func__);
>>> -			goto out;
>>> +			goto out_err;
>>>  		}
>>>  	}
>>>  
>>> +	/* set event for the pipe reads */
>>>  	event_set(&clnt->cl_event, clnt->cl_fd, EV_READ, cldcb, clnt);
>>>  	event_add(&clnt->cl_event, NULL);
>>>  
>>> -out:
>>> -	close(ifd);
>>> +	/* another event for inotify read */
>>> +	event_set(&pipedir_event, inotify_fd, EV_READ, cld_inotify_cb, clnt);
>>> +	event_add(&pipedir_event, NULL);
>>> +
>>>  out_free:
>>>  	free(dirc);
>>>  	return clnt->cl_fd;
>>> +
>>> +out_err:
>>> +	close(inotify_fd);
>>> +	goto out_free;
>>>  }
>>>  
>>>  static void
>>> @@ -142,6 +205,7 @@ cld_pipe_reopen(struct cld_client *clnt)
>>>  {
>>>  	int fd;
>>>  
>>> +	xlog(D_GENERAL, "%s: reopening pipe", __func__);
>>>  	fd = open(pipepath, O_RDWR, 0);
>>>  	if (fd < 0) {
>>>  		xlog_warn("%s: Re-opening of %s failed: %m",
>>> -- 1.7.7.5
> 
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 19:31       ` Steve Dickson
@ 2012-01-25 20:28         ` Jeff Layton
  2012-01-25 22:04           ` Steve Dickson
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-25 20:28 UTC (permalink / raw)
  To: Steve Dickson; +Cc: linux-nfs

On Wed, 25 Jan 2012 14:31:10 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 01/25/2012 02:09 PM, Jeff Layton wrote:
> > On Wed, 25 Jan 2012 13:16:24 -0500
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >> Hey Jeff,
> >>
> >> Commit inline... 
> >>
> >> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> >>> This can happen if nfsd is shut down and restarted. If that occurs,
> >>> then reopen the pipe so we're not waiting for data on the defunct
> >>> pipe.
> >>>
> >>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>> ---
> >>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> >>>  1 files changed, 74 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> >>> index b0c08e2..0dc5b37 100644
> >>> --- a/utils/nfsdcld/nfsdcld.c
> >>> +++ b/utils/nfsdcld/nfsdcld.c
> >>> @@ -57,6 +57,8 @@ struct cld_client {
> >>>  
> >>>  /* global variables */
> >>>  static char *pipepath = DEFAULT_CLD_PATH;
> >>> +static int 		inotify_fd = -1;
> >>> +static struct event	pipedir_event;
> >>>  
> >>>  static struct option longopts[] =
> >>>  {
> >>> @@ -68,8 +70,10 @@ static struct option longopts[] =
> >>>  	{ NULL, 0, 0, 0 },
> >>>  };
> >>>  
> >>> +
> >>>  /* forward declarations */
> >>>  static void cldcb(int UNUSED(fd), short which, void *data);
> >>> +static void cld_pipe_reopen(struct cld_client *clnt);
> >>>  
> >>>  static void
> >>>  usage(char *progname)
> >>> @@ -80,10 +84,62 @@ usage(char *progname)
> >>>  
> >>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> >>>  
> >>> +static void
> >>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> >>> +{
> >>> +	int ret, oldfd;
> >>> +	char evbuf[INOTIFY_EVENT_MAX];
> >>> +	char *dirc = NULL, *pname;
> >>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> >>> +	struct cld_client *clnt = data;
> >>> +
> >>> +	if (which != EV_READ)
> >>> +		return;
> >>> +
> >>> +	dirc = strndup(pipepath, PATH_MAX);
> >>> +	if (!dirc) {
> >>> +		xlog_err("%s: unable to allocate memory", __func__);
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> >>> +	if (ret < 0) {
> >>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	/* check to see if we have a filename in the evbuf */
> >>> +	if (!event->len)
> >>> +		goto out;
> >>> +
> >>> +	pname = basename(dirc);
> >>> +
> >>> +	/* does the filename match our pipe? */
> >>> +	if (strncmp(pname, event->name, event->len))
> >>> +		goto out;
> >>> +
> >>> +	/*
> >>> +	 * reopen the pipe. The old fd is not closed until the new one is
> >>> +	 * opened, so we know they should be different if the reopen is
> >>> +	 * successful.
> >>> +	 */
> >>> +	oldfd = clnt->cl_fd;
> >>> +	do {
> >>> +		cld_pipe_reopen(clnt);
> >>> +	} while (oldfd == clnt->cl_fd);
> >> Doesn't this have a potential for an infinite loop? 
> >>
> >> steved.  
> > 
> > 
> > Yes. If reopening the new pipe continually fails then it will loop
> > forever.
> Would it be more accurate to say it would be spinning forever? 
> Since there is no sleep or delay in cld_pipe_reopen, what's
> going to stop the daemon from spinning in a CPU bound loop?
> 

Well, not spinning in a userspace loop...it'll continually be cycling on
an open() call that's not working for whatever reason. We sort of have
to loop on that though. I think the best we can do is add a sleep(1) in
there or something. Would that be sufficient?

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 20:28         ` Jeff Layton
@ 2012-01-25 22:04           ` Steve Dickson
  2012-01-25 23:32             ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2012-01-25 22:04 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-nfs



On 01/25/2012 03:28 PM, Jeff Layton wrote:
> On Wed, 25 Jan 2012 14:31:10 -0500
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
>>> On Wed, 25 Jan 2012 13:16:24 -0500
>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>
>>>> Hey Jeff,
>>>>
>>>> Commit inline... 
>>>>
>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
>>>>> then reopen the pipe so we're not waiting for data on the defunct
>>>>> pipe.
>>>>>
>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>>>> ---
>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
>>>>> index b0c08e2..0dc5b37 100644
>>>>> --- a/utils/nfsdcld/nfsdcld.c
>>>>> +++ b/utils/nfsdcld/nfsdcld.c
>>>>> @@ -57,6 +57,8 @@ struct cld_client {
>>>>>  
>>>>>  /* global variables */
>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
>>>>> +static int 		inotify_fd = -1;
>>>>> +static struct event	pipedir_event;
>>>>>  
>>>>>  static struct option longopts[] =
>>>>>  {
>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
>>>>>  	{ NULL, 0, 0, 0 },
>>>>>  };
>>>>>  
>>>>> +
>>>>>  /* forward declarations */
>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
>>>>>  
>>>>>  static void
>>>>>  usage(char *progname)
>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
>>>>>  
>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
>>>>>  
>>>>> +static void
>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
>>>>> +{
>>>>> +	int ret, oldfd;
>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
>>>>> +	char *dirc = NULL, *pname;
>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
>>>>> +	struct cld_client *clnt = data;
>>>>> +
>>>>> +	if (which != EV_READ)
>>>>> +		return;
>>>>> +
>>>>> +	dirc = strndup(pipepath, PATH_MAX);
>>>>> +	if (!dirc) {
>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
>>>>> +	if (ret < 0) {
>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	/* check to see if we have a filename in the evbuf */
>>>>> +	if (!event->len)
>>>>> +		goto out;
>>>>> +
>>>>> +	pname = basename(dirc);
>>>>> +
>>>>> +	/* does the filename match our pipe? */
>>>>> +	if (strncmp(pname, event->name, event->len))
>>>>> +		goto out;
>>>>> +
>>>>> +	/*
>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
>>>>> +	 * opened, so we know they should be different if the reopen is
>>>>> +	 * successful.
>>>>> +	 */
>>>>> +	oldfd = clnt->cl_fd;
>>>>> +	do {
>>>>> +		cld_pipe_reopen(clnt);
>>>>> +	} while (oldfd == clnt->cl_fd);
>>>> Doesn't this have a potential for an infinite loop? 
>>>>
>>>> steved.  
>>>
>>>
>>> Yes. If reopening the new pipe continually fails then it will loop
>>> forever.
>> Would it be more accurate to say it would be spinning forever? 
>> Since there is no sleep or delay in cld_pipe_reopen, what's
>> going to stop the daemon from spinning in a CPU bound loop?
>>
> 
> Well, not spinning in a userspace loop...it'll continually be cycling on
> an open() call that's not working for whatever reason. We sort of have
> to loop on that though. I think the best we can do is add a sleep(1) in
> there or something. Would that be sufficient?
> 
I still think it going to needlessly suck up CPU cycles... 

The way I handled this in the rpc.idmapd daemon was to do the
reopen on a SIGHUP signal. Then in NFS server initscript 
I did the following:
    /usr/bin/pkill -HUP rpc.idmapd

Thoughts?

steved.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 22:04           ` Steve Dickson
@ 2012-01-25 23:32             ` Jeff Layton
  2012-01-26 12:47               ` Steve Dickson
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-25 23:32 UTC (permalink / raw)
  To: Steve Dickson; +Cc: linux-nfs

On Wed, 25 Jan 2012 17:04:44 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 01/25/2012 03:28 PM, Jeff Layton wrote:
> > On Wed, 25 Jan 2012 14:31:10 -0500
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >>
> >>
> >> On 01/25/2012 02:09 PM, Jeff Layton wrote:
> >>> On Wed, 25 Jan 2012 13:16:24 -0500
> >>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>
> >>>> Hey Jeff,
> >>>>
> >>>> Commit inline... 
> >>>>
> >>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> >>>>> This can happen if nfsd is shut down and restarted. If that occurs,
> >>>>> then reopen the pipe so we're not waiting for data on the defunct
> >>>>> pipe.
> >>>>>
> >>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>>>> ---
> >>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> >>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> >>>>> index b0c08e2..0dc5b37 100644
> >>>>> --- a/utils/nfsdcld/nfsdcld.c
> >>>>> +++ b/utils/nfsdcld/nfsdcld.c
> >>>>> @@ -57,6 +57,8 @@ struct cld_client {
> >>>>>  
> >>>>>  /* global variables */
> >>>>>  static char *pipepath = DEFAULT_CLD_PATH;
> >>>>> +static int 		inotify_fd = -1;
> >>>>> +static struct event	pipedir_event;
> >>>>>  
> >>>>>  static struct option longopts[] =
> >>>>>  {
> >>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
> >>>>>  	{ NULL, 0, 0, 0 },
> >>>>>  };
> >>>>>  
> >>>>> +
> >>>>>  /* forward declarations */
> >>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
> >>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
> >>>>>  
> >>>>>  static void
> >>>>>  usage(char *progname)
> >>>>> @@ -80,10 +84,62 @@ usage(char *progname)
> >>>>>  
> >>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> >>>>>  
> >>>>> +static void
> >>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> >>>>> +{
> >>>>> +	int ret, oldfd;
> >>>>> +	char evbuf[INOTIFY_EVENT_MAX];
> >>>>> +	char *dirc = NULL, *pname;
> >>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> >>>>> +	struct cld_client *clnt = data;
> >>>>> +
> >>>>> +	if (which != EV_READ)
> >>>>> +		return;
> >>>>> +
> >>>>> +	dirc = strndup(pipepath, PATH_MAX);
> >>>>> +	if (!dirc) {
> >>>>> +		xlog_err("%s: unable to allocate memory", __func__);
> >>>>> +		goto out;
> >>>>> +	}
> >>>>> +
> >>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> >>>>> +	if (ret < 0) {
> >>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> >>>>> +		goto out;
> >>>>> +	}
> >>>>> +
> >>>>> +	/* check to see if we have a filename in the evbuf */
> >>>>> +	if (!event->len)
> >>>>> +		goto out;
> >>>>> +
> >>>>> +	pname = basename(dirc);
> >>>>> +
> >>>>> +	/* does the filename match our pipe? */
> >>>>> +	if (strncmp(pname, event->name, event->len))
> >>>>> +		goto out;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
> >>>>> +	 * opened, so we know they should be different if the reopen is
> >>>>> +	 * successful.
> >>>>> +	 */
> >>>>> +	oldfd = clnt->cl_fd;
> >>>>> +	do {
> >>>>> +		cld_pipe_reopen(clnt);
> >>>>> +	} while (oldfd == clnt->cl_fd);
> >>>> Doesn't this have a potential for an infinite loop? 
> >>>>
> >>>> steved.  
> >>>
> >>>
> >>> Yes. If reopening the new pipe continually fails then it will loop
> >>> forever.
> >> Would it be more accurate to say it would be spinning forever? 
> >> Since there is no sleep or delay in cld_pipe_reopen, what's
> >> going to stop the daemon from spinning in a CPU bound loop?
> >>
> > 
> > Well, not spinning in a userspace loop...it'll continually be cycling on
> > an open() call that's not working for whatever reason. We sort of have
> > to loop on that though. I think the best we can do is add a sleep(1) in
> > there or something. Would that be sufficient?
> > 
> I still think it going to needlessly suck up CPU cycles... 
> 
> The way I handled this in the rpc.idmapd daemon was to do the
> reopen on a SIGHUP signal. Then in NFS server initscript 
> I did the following:
>     /usr/bin/pkill -HUP rpc.idmapd
> 
> Thoughts?
> 

Ugh, that requires manual intervention if the pipe is removed and
recreated. If someone restarts nfsd and doesn't send the signal, then
they won't get the upcalls. I'd prefer something that "just works".

Seriously, is it that big a deal to just loop here? One open(2) call
every second doesn't seem that bad, and honestly if a new pipe pops up
and the daemon can't open it then a few CPU cycles is the least of your
worries.

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-25 23:32             ` Jeff Layton
@ 2012-01-26 12:47               ` Steve Dickson
  2012-01-26 13:28                 ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2012-01-26 12:47 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-nfs



On 01/25/2012 06:32 PM, Jeff Layton wrote:
> On Wed, 25 Jan 2012 17:04:44 -0500
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 01/25/2012 03:28 PM, Jeff Layton wrote:
>>> On Wed, 25 Jan 2012 14:31:10 -0500
>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>
>>>>
>>>>
>>>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
>>>>> On Wed, 25 Jan 2012 13:16:24 -0500
>>>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>>>
>>>>>> Hey Jeff,
>>>>>>
>>>>>> Commit inline... 
>>>>>>
>>>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
>>>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
>>>>>>> then reopen the pipe so we're not waiting for data on the defunct
>>>>>>> pipe.
>>>>>>>
>>>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>>>>>> ---
>>>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
>>>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
>>>>>>> index b0c08e2..0dc5b37 100644
>>>>>>> --- a/utils/nfsdcld/nfsdcld.c
>>>>>>> +++ b/utils/nfsdcld/nfsdcld.c
>>>>>>> @@ -57,6 +57,8 @@ struct cld_client {
>>>>>>>  
>>>>>>>  /* global variables */
>>>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
>>>>>>> +static int 		inotify_fd = -1;
>>>>>>> +static struct event	pipedir_event;
>>>>>>>  
>>>>>>>  static struct option longopts[] =
>>>>>>>  {
>>>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
>>>>>>>  	{ NULL, 0, 0, 0 },
>>>>>>>  };
>>>>>>>  
>>>>>>> +
>>>>>>>  /* forward declarations */
>>>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
>>>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
>>>>>>>  
>>>>>>>  static void
>>>>>>>  usage(char *progname)
>>>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
>>>>>>>  
>>>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
>>>>>>>  
>>>>>>> +static void
>>>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
>>>>>>> +{
>>>>>>> +	int ret, oldfd;
>>>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
>>>>>>> +	char *dirc = NULL, *pname;
>>>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
>>>>>>> +	struct cld_client *clnt = data;
>>>>>>> +
>>>>>>> +	if (which != EV_READ)
>>>>>>> +		return;
>>>>>>> +
>>>>>>> +	dirc = strndup(pipepath, PATH_MAX);
>>>>>>> +	if (!dirc) {
>>>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
>>>>>>> +		goto out;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
>>>>>>> +	if (ret < 0) {
>>>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
>>>>>>> +		goto out;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/* check to see if we have a filename in the evbuf */
>>>>>>> +	if (!event->len)
>>>>>>> +		goto out;
>>>>>>> +
>>>>>>> +	pname = basename(dirc);
>>>>>>> +
>>>>>>> +	/* does the filename match our pipe? */
>>>>>>> +	if (strncmp(pname, event->name, event->len))
>>>>>>> +		goto out;
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
>>>>>>> +	 * opened, so we know they should be different if the reopen is
>>>>>>> +	 * successful.
>>>>>>> +	 */
>>>>>>> +	oldfd = clnt->cl_fd;
>>>>>>> +	do {
>>>>>>> +		cld_pipe_reopen(clnt);
>>>>>>> +	} while (oldfd == clnt->cl_fd);
>>>>>> Doesn't this have a potential for an infinite loop? 
>>>>>>
>>>>>> steved.  
>>>>>
>>>>>
>>>>> Yes. If reopening the new pipe continually fails then it will loop
>>>>> forever.
>>>> Would it be more accurate to say it would be spinning forever? 
>>>> Since there is no sleep or delay in cld_pipe_reopen, what's
>>>> going to stop the daemon from spinning in a CPU bound loop?
>>>>
>>>
>>> Well, not spinning in a userspace loop...it'll continually be cycling on
>>> an open() call that's not working for whatever reason. We sort of have
>>> to loop on that though. I think the best we can do is add a sleep(1) in
>>> there or something. Would that be sufficient?
>>>
>> I still think it going to needlessly suck up CPU cycles... 
>>
>> The way I handled this in the rpc.idmapd daemon was to do the
>> reopen on a SIGHUP signal. Then in NFS server initscript 
>> I did the following:
>>     /usr/bin/pkill -HUP rpc.idmapd
>>
>> Thoughts?
>>
> 
> Ugh, that requires manual intervention if the pipe is removed and
> recreated. If someone restarts nfsd and doesn't send the signal, then
> they won't get the upcalls. I'd prefer something that "just works".
I have not seen any bz open saying rpc.idmapd doesn't just work... 

> 
> Seriously, is it that big a deal to just loop here? One open(2) call
> every second doesn't seem that bad, and honestly if a new pipe pops up
> and the daemon can't open it then a few CPU cycles is the least of your
> worries.
> 
Put the daemon in that loop and then run the top command in another 
window.. If the daemon is at the top of the list then it is a big
deal because that daemon will on the top forever for no reason, in
the cast of the NFS server not coming back. 

tia,

steved.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 12:47               ` Steve Dickson
@ 2012-01-26 13:28                 ` Jeff Layton
  2012-01-26 14:30                   ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-26 13:28 UTC (permalink / raw)
  To: Steve Dickson; +Cc: linux-nfs

On Thu, 26 Jan 2012 07:47:51 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 01/25/2012 06:32 PM, Jeff Layton wrote:
> > On Wed, 25 Jan 2012 17:04:44 -0500
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >>
> >>
> >> On 01/25/2012 03:28 PM, Jeff Layton wrote:
> >>> On Wed, 25 Jan 2012 14:31:10 -0500
> >>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
> >>>>> On Wed, 25 Jan 2012 13:16:24 -0500
> >>>>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>>>
> >>>>>> Hey Jeff,
> >>>>>>
> >>>>>> Commit inline... 
> >>>>>>
> >>>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> >>>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
> >>>>>>> then reopen the pipe so we're not waiting for data on the defunct
> >>>>>>> pipe.
> >>>>>>>
> >>>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>>>>>> ---
> >>>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> >>>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> >>>>>>> index b0c08e2..0dc5b37 100644
> >>>>>>> --- a/utils/nfsdcld/nfsdcld.c
> >>>>>>> +++ b/utils/nfsdcld/nfsdcld.c
> >>>>>>> @@ -57,6 +57,8 @@ struct cld_client {
> >>>>>>>  
> >>>>>>>  /* global variables */
> >>>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
> >>>>>>> +static int 		inotify_fd = -1;
> >>>>>>> +static struct event	pipedir_event;
> >>>>>>>  
> >>>>>>>  static struct option longopts[] =
> >>>>>>>  {
> >>>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
> >>>>>>>  	{ NULL, 0, 0, 0 },
> >>>>>>>  };
> >>>>>>>  
> >>>>>>> +
> >>>>>>>  /* forward declarations */
> >>>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
> >>>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
> >>>>>>>  
> >>>>>>>  static void
> >>>>>>>  usage(char *progname)
> >>>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
> >>>>>>>  
> >>>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> >>>>>>>  
> >>>>>>> +static void
> >>>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> >>>>>>> +{
> >>>>>>> +	int ret, oldfd;
> >>>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
> >>>>>>> +	char *dirc = NULL, *pname;
> >>>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> >>>>>>> +	struct cld_client *clnt = data;
> >>>>>>> +
> >>>>>>> +	if (which != EV_READ)
> >>>>>>> +		return;
> >>>>>>> +
> >>>>>>> +	dirc = strndup(pipepath, PATH_MAX);
> >>>>>>> +	if (!dirc) {
> >>>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
> >>>>>>> +		goto out;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> >>>>>>> +	if (ret < 0) {
> >>>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> >>>>>>> +		goto out;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	/* check to see if we have a filename in the evbuf */
> >>>>>>> +	if (!event->len)
> >>>>>>> +		goto out;
> >>>>>>> +
> >>>>>>> +	pname = basename(dirc);
> >>>>>>> +
> >>>>>>> +	/* does the filename match our pipe? */
> >>>>>>> +	if (strncmp(pname, event->name, event->len))
> >>>>>>> +		goto out;
> >>>>>>> +
> >>>>>>> +	/*
> >>>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
> >>>>>>> +	 * opened, so we know they should be different if the reopen is
> >>>>>>> +	 * successful.
> >>>>>>> +	 */
> >>>>>>> +	oldfd = clnt->cl_fd;
> >>>>>>> +	do {
> >>>>>>> +		cld_pipe_reopen(clnt);
> >>>>>>> +	} while (oldfd == clnt->cl_fd);
> >>>>>> Doesn't this have a potential for an infinite loop? 
> >>>>>>
> >>>>>> steved.  
> >>>>>
> >>>>>
> >>>>> Yes. If reopening the new pipe continually fails then it will loop
> >>>>> forever.
> >>>> Would it be more accurate to say it would be spinning forever? 
> >>>> Since there is no sleep or delay in cld_pipe_reopen, what's
> >>>> going to stop the daemon from spinning in a CPU bound loop?
> >>>>
> >>>
> >>> Well, not spinning in a userspace loop...it'll continually be cycling on
> >>> an open() call that's not working for whatever reason. We sort of have
> >>> to loop on that though. I think the best we can do is add a sleep(1) in
> >>> there or something. Would that be sufficient?
> >>>
> >> I still think it going to needlessly suck up CPU cycles... 
> >>
> >> The way I handled this in the rpc.idmapd daemon was to do the
> >> reopen on a SIGHUP signal. Then in NFS server initscript 
> >> I did the following:
> >>     /usr/bin/pkill -HUP rpc.idmapd
> >>
> >> Thoughts?
> >>
> > 
> > Ugh, that requires manual intervention if the pipe is removed and
> > recreated. If someone restarts nfsd and doesn't send the signal, then
> > they won't get the upcalls. I'd prefer something that "just works".
> I have not seen any bz open saying rpc.idmapd doesn't just work... 
> 
> > 
> > Seriously, is it that big a deal to just loop here? One open(2) call
> > every second doesn't seem that bad, and honestly if a new pipe pops up
> > and the daemon can't open it then a few CPU cycles is the least of your
> > worries.
> > 
> Put the daemon in that loop and then run the top command in another 
> window.. If the daemon is at the top of the list then it is a big
> deal because that daemon will on the top forever for no reason, in
> the cast of the NFS server not coming back. 
> 

This situation is really unlikely. The daemon does not reopen the pipe
when the old one goes away. It reopens it when a new one with the same
name is recreated in the directory.

That's an important distinction because in order to get into this loop,
you'd need to:

1/ remove the old pipe -- this happens when the daemon is shut down

2/ create a new pipe -- this happens when the daemon is restarted

3/ not be able to open the new pipe for some reason, even though you
were able to open the old one

The reason I put this in a loop is because it's possible (though not
likely) that you'd hit condition #3 temporarily. In that event, looping
and retrying an open(2) call every second seems entirely reasonable and
is more fault tolerant than just dying here. The open of a pipe takes
much less than 1s, so there's plenty of time between open attempts for
the machine to get other things done.

If it turns out that there's a problem, the admin can shut down the
daemon at that point. They may need to do so anyway in order to resolve
the situation if the thing preventing the opening of the pipe isn't
temporary.

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 13:28                 ` Jeff Layton
@ 2012-01-26 14:30                   ` Jeff Layton
  2012-01-26 15:31                     ` Steve Dickson
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-26 14:30 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Steve Dickson, linux-nfs

On Thu, 26 Jan 2012 08:28:30 -0500
Jeff Layton <jlayton@redhat.com> wrote:

> On Thu, 26 Jan 2012 07:47:51 -0500
> Steve Dickson <SteveD@redhat.com> wrote:
> 
> > 
> > 
> > On 01/25/2012 06:32 PM, Jeff Layton wrote:
> > > On Wed, 25 Jan 2012 17:04:44 -0500
> > > Steve Dickson <SteveD@redhat.com> wrote:
> > > 
> > >>
> > >>
> > >> On 01/25/2012 03:28 PM, Jeff Layton wrote:
> > >>> On Wed, 25 Jan 2012 14:31:10 -0500
> > >>> Steve Dickson <SteveD@redhat.com> wrote:
> > >>>
> > >>>>
> > >>>>
> > >>>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
> > >>>>> On Wed, 25 Jan 2012 13:16:24 -0500
> > >>>>> Steve Dickson <SteveD@redhat.com> wrote:
> > >>>>>
> > >>>>>> Hey Jeff,
> > >>>>>>
> > >>>>>> Commit inline... 
> > >>>>>>
> > >>>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> > >>>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
> > >>>>>>> then reopen the pipe so we're not waiting for data on the defunct
> > >>>>>>> pipe.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > >>>>>>> ---
> > >>>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> > >>>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
> > >>>>>>>
> > >>>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> > >>>>>>> index b0c08e2..0dc5b37 100644
> > >>>>>>> --- a/utils/nfsdcld/nfsdcld.c
> > >>>>>>> +++ b/utils/nfsdcld/nfsdcld.c
> > >>>>>>> @@ -57,6 +57,8 @@ struct cld_client {
> > >>>>>>>  
> > >>>>>>>  /* global variables */
> > >>>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
> > >>>>>>> +static int 		inotify_fd = -1;
> > >>>>>>> +static struct event	pipedir_event;
> > >>>>>>>  
> > >>>>>>>  static struct option longopts[] =
> > >>>>>>>  {
> > >>>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
> > >>>>>>>  	{ NULL, 0, 0, 0 },
> > >>>>>>>  };
> > >>>>>>>  
> > >>>>>>> +
> > >>>>>>>  /* forward declarations */
> > >>>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
> > >>>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
> > >>>>>>>  
> > >>>>>>>  static void
> > >>>>>>>  usage(char *progname)
> > >>>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
> > >>>>>>>  
> > >>>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> > >>>>>>>  
> > >>>>>>> +static void
> > >>>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> > >>>>>>> +{
> > >>>>>>> +	int ret, oldfd;
> > >>>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
> > >>>>>>> +	char *dirc = NULL, *pname;
> > >>>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> > >>>>>>> +	struct cld_client *clnt = data;
> > >>>>>>> +
> > >>>>>>> +	if (which != EV_READ)
> > >>>>>>> +		return;
> > >>>>>>> +
> > >>>>>>> +	dirc = strndup(pipepath, PATH_MAX);
> > >>>>>>> +	if (!dirc) {
> > >>>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
> > >>>>>>> +		goto out;
> > >>>>>>> +	}
> > >>>>>>> +
> > >>>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> > >>>>>>> +	if (ret < 0) {
> > >>>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> > >>>>>>> +		goto out;
> > >>>>>>> +	}
> > >>>>>>> +
> > >>>>>>> +	/* check to see if we have a filename in the evbuf */
> > >>>>>>> +	if (!event->len)
> > >>>>>>> +		goto out;
> > >>>>>>> +
> > >>>>>>> +	pname = basename(dirc);
> > >>>>>>> +
> > >>>>>>> +	/* does the filename match our pipe? */
> > >>>>>>> +	if (strncmp(pname, event->name, event->len))
> > >>>>>>> +		goto out;
> > >>>>>>> +
> > >>>>>>> +	/*
> > >>>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
> > >>>>>>> +	 * opened, so we know they should be different if the reopen is
> > >>>>>>> +	 * successful.
> > >>>>>>> +	 */
> > >>>>>>> +	oldfd = clnt->cl_fd;
> > >>>>>>> +	do {
> > >>>>>>> +		cld_pipe_reopen(clnt);
> > >>>>>>> +	} while (oldfd == clnt->cl_fd);
> > >>>>>> Doesn't this have a potential for an infinite loop? 
> > >>>>>>
> > >>>>>> steved.  
> > >>>>>
> > >>>>>
> > >>>>> Yes. If reopening the new pipe continually fails then it will loop
> > >>>>> forever.
> > >>>> Would it be more accurate to say it would be spinning forever? 
> > >>>> Since there is no sleep or delay in cld_pipe_reopen, what's
> > >>>> going to stop the daemon from spinning in a CPU bound loop?
> > >>>>
> > >>>
> > >>> Well, not spinning in a userspace loop...it'll continually be cycling on
> > >>> an open() call that's not working for whatever reason. We sort of have
> > >>> to loop on that though. I think the best we can do is add a sleep(1) in
> > >>> there or something. Would that be sufficient?
> > >>>
> > >> I still think it going to needlessly suck up CPU cycles... 
> > >>
> > >> The way I handled this in the rpc.idmapd daemon was to do the
> > >> reopen on a SIGHUP signal. Then in NFS server initscript 
> > >> I did the following:
> > >>     /usr/bin/pkill -HUP rpc.idmapd
> > >>
> > >> Thoughts?
> > >>
> > > 
> > > Ugh, that requires manual intervention if the pipe is removed and
> > > recreated. If someone restarts nfsd and doesn't send the signal, then
> > > they won't get the upcalls. I'd prefer something that "just works".
> > I have not seen any bz open saying rpc.idmapd doesn't just work... 
> > 
> > > 
> > > Seriously, is it that big a deal to just loop here? One open(2) call
> > > every second doesn't seem that bad, and honestly if a new pipe pops up
> > > and the daemon can't open it then a few CPU cycles is the least of your
> > > worries.
> > > 
> > Put the daemon in that loop and then run the top command in another 
> > window.. If the daemon is at the top of the list then it is a big
> > deal because that daemon will on the top forever for no reason, in
> > the cast of the NFS server not coming back. 
> > 
> 
> This situation is really unlikely. The daemon does not reopen the pipe
> when the old one goes away. It reopens it when a new one with the same
> name is recreated in the directory.
> 
> That's an important distinction because in order to get into this loop,
> you'd need to:
> 
> 1/ remove the old pipe -- this happens when the daemon is shut down
> 
> 2/ create a new pipe -- this happens when the daemon is restarted
> 

To clarify, the above happen when knfsd are stopped and started...

> 3/ not be able to open the new pipe for some reason, even though you
> were able to open the old one
> 
> The reason I put this in a loop is because it's possible (though not
> likely) that you'd hit condition #3 temporarily. In that event, looping
> and retrying an open(2) call every second seems entirely reasonable and
> is more fault tolerant than just dying here. The open of a pipe takes
> much less than 1s, so there's plenty of time between open attempts for
> the machine to get other things done.
> 
> If it turns out that there's a problem, the admin can shut down the
> daemon at that point. They may need to do so anyway in order to resolve
> the situation if the thing preventing the opening of the pipe isn't
> temporary.
> 


-- 
Jeff Layton <jlayton@poochiereds.net>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 14:30                   ` Jeff Layton
@ 2012-01-26 15:31                     ` Steve Dickson
  2012-01-26 15:41                       ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2012-01-26 15:31 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Jeff Layton, linux-nfs



On 01/26/2012 09:30 AM, Jeff Layton wrote:
> On Thu, 26 Jan 2012 08:28:30 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> 
>> On Thu, 26 Jan 2012 07:47:51 -0500
>> Steve Dickson <SteveD@redhat.com> wrote:
>>
>>>
>>>
>>> On 01/25/2012 06:32 PM, Jeff Layton wrote:
>>>> On Wed, 25 Jan 2012 17:04:44 -0500
>>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> On 01/25/2012 03:28 PM, Jeff Layton wrote:
>>>>>> On Wed, 25 Jan 2012 14:31:10 -0500
>>>>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
>>>>>>>> On Wed, 25 Jan 2012 13:16:24 -0500
>>>>>>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> Hey Jeff,
>>>>>>>>>
>>>>>>>>> Commit inline... 
>>>>>>>>>
>>>>>>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
>>>>>>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
>>>>>>>>>> then reopen the pipe so we're not waiting for data on the defunct
>>>>>>>>>> pipe.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
>>>>>>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
>>>>>>>>>> index b0c08e2..0dc5b37 100644
>>>>>>>>>> --- a/utils/nfsdcld/nfsdcld.c
>>>>>>>>>> +++ b/utils/nfsdcld/nfsdcld.c
>>>>>>>>>> @@ -57,6 +57,8 @@ struct cld_client {
>>>>>>>>>>  
>>>>>>>>>>  /* global variables */
>>>>>>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
>>>>>>>>>> +static int 		inotify_fd = -1;
>>>>>>>>>> +static struct event	pipedir_event;
>>>>>>>>>>  
>>>>>>>>>>  static struct option longopts[] =
>>>>>>>>>>  {
>>>>>>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
>>>>>>>>>>  	{ NULL, 0, 0, 0 },
>>>>>>>>>>  };
>>>>>>>>>>  
>>>>>>>>>> +
>>>>>>>>>>  /* forward declarations */
>>>>>>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
>>>>>>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
>>>>>>>>>>  
>>>>>>>>>>  static void
>>>>>>>>>>  usage(char *progname)
>>>>>>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
>>>>>>>>>>  
>>>>>>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
>>>>>>>>>>  
>>>>>>>>>> +static void
>>>>>>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	int ret, oldfd;
>>>>>>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
>>>>>>>>>> +	char *dirc = NULL, *pname;
>>>>>>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
>>>>>>>>>> +	struct cld_client *clnt = data;
>>>>>>>>>> +
>>>>>>>>>> +	if (which != EV_READ)
>>>>>>>>>> +		return;
>>>>>>>>>> +
>>>>>>>>>> +	dirc = strndup(pipepath, PATH_MAX);
>>>>>>>>>> +	if (!dirc) {
>>>>>>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
>>>>>>>>>> +		goto out;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
>>>>>>>>>> +	if (ret < 0) {
>>>>>>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
>>>>>>>>>> +		goto out;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	/* check to see if we have a filename in the evbuf */
>>>>>>>>>> +	if (!event->len)
>>>>>>>>>> +		goto out;
>>>>>>>>>> +
>>>>>>>>>> +	pname = basename(dirc);
>>>>>>>>>> +
>>>>>>>>>> +	/* does the filename match our pipe? */
>>>>>>>>>> +	if (strncmp(pname, event->name, event->len))
>>>>>>>>>> +		goto out;
>>>>>>>>>> +
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
>>>>>>>>>> +	 * opened, so we know they should be different if the reopen is
>>>>>>>>>> +	 * successful.
>>>>>>>>>> +	 */
>>>>>>>>>> +	oldfd = clnt->cl_fd;
>>>>>>>>>> +	do {
>>>>>>>>>> +		cld_pipe_reopen(clnt);
>>>>>>>>>> +	} while (oldfd == clnt->cl_fd);
>>>>>>>>> Doesn't this have a potential for an infinite loop? 
>>>>>>>>>
>>>>>>>>> steved.  
>>>>>>>>
>>>>>>>>
>>>>>>>> Yes. If reopening the new pipe continually fails then it will loop
>>>>>>>> forever.
>>>>>>> Would it be more accurate to say it would be spinning forever? 
>>>>>>> Since there is no sleep or delay in cld_pipe_reopen, what's
>>>>>>> going to stop the daemon from spinning in a CPU bound loop?
>>>>>>>
>>>>>>
>>>>>> Well, not spinning in a userspace loop...it'll continually be cycling on
>>>>>> an open() call that's not working for whatever reason. We sort of have
>>>>>> to loop on that though. I think the best we can do is add a sleep(1) in
>>>>>> there or something. Would that be sufficient?
>>>>>>
>>>>> I still think it going to needlessly suck up CPU cycles... 
>>>>>
>>>>> The way I handled this in the rpc.idmapd daemon was to do the
>>>>> reopen on a SIGHUP signal. Then in NFS server initscript 
>>>>> I did the following:
>>>>>     /usr/bin/pkill -HUP rpc.idmapd
>>>>>
>>>>> Thoughts?
>>>>>
>>>>
>>>> Ugh, that requires manual intervention if the pipe is removed and
>>>> recreated. If someone restarts nfsd and doesn't send the signal, then
>>>> they won't get the upcalls. I'd prefer something that "just works".
>>> I have not seen any bz open saying rpc.idmapd doesn't just work... 
>>>
>>>>
>>>> Seriously, is it that big a deal to just loop here? One open(2) call
>>>> every second doesn't seem that bad, and honestly if a new pipe pops up
>>>> and the daemon can't open it then a few CPU cycles is the least of your
>>>> worries.
>>>>
>>> Put the daemon in that loop and then run the top command in another 
>>> window.. If the daemon is at the top of the list then it is a big
>>> deal because that daemon will on the top forever for no reason, in
>>> the cast of the NFS server not coming back. 
>>>
>>
>> This situation is really unlikely. The daemon does not reopen the pipe
>> when the old one goes away. It reopens it when a new one with the same
>> name is recreated in the directory.
>>
>> That's an important distinction because in order to get into this loop,
>> you'd need to:
>>
>> 1/ remove the old pipe -- this happens when the daemon is shut down
Just to be clear, when this happens, that while loop is *not* executed

>>
>> 2/ create a new pipe -- this happens when the daemon is restarted
Then when this happens that while loop is *not* executed.

>>
> 
> To clarify, the above happen when knfsd are stopped and started...
> 
>> 3/ not be able to open the new pipe for some reason, even though you
>> were able to open the old one
Only when 1,2,3 happens synchronously will that while loop be execute, correct?

More the to the point, stopping the server will *not* cause this while 
to be execute until the server is restarted, correct?

>>
>> The reason I put this in a loop is because it's possible (though not
>> likely) that you'd hit condition #3 temporarily. In that event, looping
>> and retrying an open(2) call every second seems entirely reasonable and
>> is more fault tolerant than just dying here. The open of a pipe takes
>> much less than 1s, so there's plenty of time between open attempts for
>> the machine to get other things done
By no means am I saying not to make it fault tolerant... Please do! I'm
just worried about the daemon spinning out of control.. :-)

>>
>> If it turns out that there's a problem, the admin can shut down the
>> daemon at that point. They may need to do so anyway in order to resolve
>> the situation if the thing preventing the opening of the pipe isn't
>> temporary.
I guess I would rather figure this out now, during the design, than
after the bits hit the street... 

steved.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 15:31                     ` Steve Dickson
@ 2012-01-26 15:41                       ` Jeff Layton
  2012-01-26 18:58                         ` J. Bruce Fields
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-26 15:41 UTC (permalink / raw)
  To: Steve Dickson; +Cc: Jeff Layton, linux-nfs

On Thu, 26 Jan 2012 10:31:01 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 01/26/2012 09:30 AM, Jeff Layton wrote:
> > On Thu, 26 Jan 2012 08:28:30 -0500
> > Jeff Layton <jlayton@redhat.com> wrote:
> > 
> >> On Thu, 26 Jan 2012 07:47:51 -0500
> >> Steve Dickson <SteveD@redhat.com> wrote:
> >>
> >>>
> >>>
> >>> On 01/25/2012 06:32 PM, Jeff Layton wrote:
> >>>> On Wed, 25 Jan 2012 17:04:44 -0500
> >>>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>> On 01/25/2012 03:28 PM, Jeff Layton wrote:
> >>>>>> On Wed, 25 Jan 2012 14:31:10 -0500
> >>>>>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 01/25/2012 02:09 PM, Jeff Layton wrote:
> >>>>>>>> On Wed, 25 Jan 2012 13:16:24 -0500
> >>>>>>>> Steve Dickson <SteveD@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>>> Hey Jeff,
> >>>>>>>>>
> >>>>>>>>> Commit inline... 
> >>>>>>>>>
> >>>>>>>>> On 01/23/2012 03:02 PM, Jeff Layton wrote:
> >>>>>>>>>> This can happen if nfsd is shut down and restarted. If that occurs,
> >>>>>>>>>> then reopen the pipe so we're not waiting for data on the defunct
> >>>>>>>>>> pipe.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  utils/nfsdcld/nfsdcld.c |   84 +++++++++++++++++++++++++++++++++++++++++-----
> >>>>>>>>>>  1 files changed, 74 insertions(+), 10 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
> >>>>>>>>>> index b0c08e2..0dc5b37 100644
> >>>>>>>>>> --- a/utils/nfsdcld/nfsdcld.c
> >>>>>>>>>> +++ b/utils/nfsdcld/nfsdcld.c
> >>>>>>>>>> @@ -57,6 +57,8 @@ struct cld_client {
> >>>>>>>>>>  
> >>>>>>>>>>  /* global variables */
> >>>>>>>>>>  static char *pipepath = DEFAULT_CLD_PATH;
> >>>>>>>>>> +static int 		inotify_fd = -1;
> >>>>>>>>>> +static struct event	pipedir_event;
> >>>>>>>>>>  
> >>>>>>>>>>  static struct option longopts[] =
> >>>>>>>>>>  {
> >>>>>>>>>> @@ -68,8 +70,10 @@ static struct option longopts[] =
> >>>>>>>>>>  	{ NULL, 0, 0, 0 },
> >>>>>>>>>>  };
> >>>>>>>>>>  
> >>>>>>>>>> +
> >>>>>>>>>>  /* forward declarations */
> >>>>>>>>>>  static void cldcb(int UNUSED(fd), short which, void *data);
> >>>>>>>>>> +static void cld_pipe_reopen(struct cld_client *clnt);
> >>>>>>>>>>  
> >>>>>>>>>>  static void
> >>>>>>>>>>  usage(char *progname)
> >>>>>>>>>> @@ -80,10 +84,62 @@ usage(char *progname)
> >>>>>>>>>>  
> >>>>>>>>>>  #define INOTIFY_EVENT_MAX (sizeof(struct inotify_event) + NAME_MAX)
> >>>>>>>>>>  
> >>>>>>>>>> +static void
> >>>>>>>>>> +cld_inotify_cb(int UNUSED(fd), short which, void *data)
> >>>>>>>>>> +{
> >>>>>>>>>> +	int ret, oldfd;
> >>>>>>>>>> +	char evbuf[INOTIFY_EVENT_MAX];
> >>>>>>>>>> +	char *dirc = NULL, *pname;
> >>>>>>>>>> +	struct inotify_event *event = (struct inotify_event *)evbuf;
> >>>>>>>>>> +	struct cld_client *clnt = data;
> >>>>>>>>>> +
> >>>>>>>>>> +	if (which != EV_READ)
> >>>>>>>>>> +		return;
> >>>>>>>>>> +
> >>>>>>>>>> +	dirc = strndup(pipepath, PATH_MAX);
> >>>>>>>>>> +	if (!dirc) {
> >>>>>>>>>> +		xlog_err("%s: unable to allocate memory", __func__);
> >>>>>>>>>> +		goto out;
> >>>>>>>>>> +	}
> >>>>>>>>>> +
> >>>>>>>>>> +	ret = read(inotify_fd, evbuf, INOTIFY_EVENT_MAX);
> >>>>>>>>>> +	if (ret < 0) {
> >>>>>>>>>> +		xlog_err("%s: read from inotify fd failed: %m", __func__);
> >>>>>>>>>> +		goto out;
> >>>>>>>>>> +	}
> >>>>>>>>>> +
> >>>>>>>>>> +	/* check to see if we have a filename in the evbuf */
> >>>>>>>>>> +	if (!event->len)
> >>>>>>>>>> +		goto out;
> >>>>>>>>>> +
> >>>>>>>>>> +	pname = basename(dirc);
> >>>>>>>>>> +
> >>>>>>>>>> +	/* does the filename match our pipe? */
> >>>>>>>>>> +	if (strncmp(pname, event->name, event->len))
> >>>>>>>>>> +		goto out;
> >>>>>>>>>> +
> >>>>>>>>>> +	/*
> >>>>>>>>>> +	 * reopen the pipe. The old fd is not closed until the new one is
> >>>>>>>>>> +	 * opened, so we know they should be different if the reopen is
> >>>>>>>>>> +	 * successful.
> >>>>>>>>>> +	 */
> >>>>>>>>>> +	oldfd = clnt->cl_fd;
> >>>>>>>>>> +	do {
> >>>>>>>>>> +		cld_pipe_reopen(clnt);
> >>>>>>>>>> +	} while (oldfd == clnt->cl_fd);
> >>>>>>>>> Doesn't this have a potential for an infinite loop? 
> >>>>>>>>>
> >>>>>>>>> steved.  
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Yes. If reopening the new pipe continually fails then it will loop
> >>>>>>>> forever.
> >>>>>>> Would it be more accurate to say it would be spinning forever? 
> >>>>>>> Since there is no sleep or delay in cld_pipe_reopen, what's
> >>>>>>> going to stop the daemon from spinning in a CPU bound loop?
> >>>>>>>
> >>>>>>
> >>>>>> Well, not spinning in a userspace loop...it'll continually be cycling on
> >>>>>> an open() call that's not working for whatever reason. We sort of have
> >>>>>> to loop on that though. I think the best we can do is add a sleep(1) in
> >>>>>> there or something. Would that be sufficient?
> >>>>>>
> >>>>> I still think it going to needlessly suck up CPU cycles... 
> >>>>>
> >>>>> The way I handled this in the rpc.idmapd daemon was to do the
> >>>>> reopen on a SIGHUP signal. Then in NFS server initscript 
> >>>>> I did the following:
> >>>>>     /usr/bin/pkill -HUP rpc.idmapd
> >>>>>
> >>>>> Thoughts?
> >>>>>
> >>>>
> >>>> Ugh, that requires manual intervention if the pipe is removed and
> >>>> recreated. If someone restarts nfsd and doesn't send the signal, then
> >>>> they won't get the upcalls. I'd prefer something that "just works".
> >>> I have not seen any bz open saying rpc.idmapd doesn't just work... 
> >>>
> >>>>
> >>>> Seriously, is it that big a deal to just loop here? One open(2) call
> >>>> every second doesn't seem that bad, and honestly if a new pipe pops up
> >>>> and the daemon can't open it then a few CPU cycles is the least of your
> >>>> worries.
> >>>>
> >>> Put the daemon in that loop and then run the top command in another 
> >>> window.. If the daemon is at the top of the list then it is a big
> >>> deal because that daemon will on the top forever for no reason, in
> >>> the cast of the NFS server not coming back. 
> >>>
> >>
> >> This situation is really unlikely. The daemon does not reopen the pipe
> >> when the old one goes away. It reopens it when a new one with the same
> >> name is recreated in the directory.
> >>
> >> That's an important distinction because in order to get into this loop,
> >> you'd need to:
> >>
> >> 1/ remove the old pipe -- this happens when the daemon is shut down
> Just to be clear, when this happens, that while loop is *not* executed
> 
> >>
> >> 2/ create a new pipe -- this happens when the daemon is restarted
> Then when this happens that while loop is *not* executed.
> 
> >>
> > 
> > To clarify, the above happen when knfsd are stopped and started...
> > 
> >> 3/ not be able to open the new pipe for some reason, even though you
> >> were able to open the old one
> Only when 1,2,3 happens synchronously will that while loop be execute, correct?
> 
> More the to the point, stopping the server will *not* cause this while 
> to be execute until the server is restarted, correct?
> 

Correct. When knfsd starts back up, a new pipe will be created. At that
point, the daemon will try to open the new one and will then close the
old if the open succeeds. It will only loop here if that open fails for
some reason.

> >>
> >> The reason I put this in a loop is because it's possible (though not
> >> likely) that you'd hit condition #3 temporarily. In that event, looping
> >> and retrying an open(2) call every second seems entirely reasonable and
> >> is more fault tolerant than just dying here. The open of a pipe takes
> >> much less than 1s, so there's plenty of time between open attempts for
> >> the machine to get other things done
> By no means am I saying not to make it fault tolerant... Please do! I'm
> just worried about the daemon spinning out of control.. :-)
> 

Agreed. Adding a small sleep between open attempts should keep it from
going crazy if reopening the pipe continually fails. I should probably
also ratelimit the log message when the reopen fails too. I'll plan to
add that in the next iteration.

> >>
> >> If it turns out that there's a problem, the admin can shut down the
> >> daemon at that point. They may need to do so anyway in order to resolve
> >> the situation if the thing preventing the opening of the pipe isn't
> >> temporary.
> I guess I would rather figure this out now, during the design, than
> after the bits hit the street... 
> 

Yep, definitely.

Thanks for the review!
-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 15:41                       ` Jeff Layton
@ 2012-01-26 18:58                         ` J. Bruce Fields
  2012-01-26 19:36                           ` Jeff Layton
  0 siblings, 1 reply; 27+ messages in thread
From: J. Bruce Fields @ 2012-01-26 18:58 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Steve Dickson, linux-nfs

On Thu, Jan 26, 2012 at 10:41:57AM -0500, Jeff Layton wrote:
> Correct. When knfsd starts back up, a new pipe will be created. At that
> point, the daemon will try to open the new one and will then close the
> old if the open succeeds. It will only loop here if that open fails for
> some reason.

I'm a little confused--how could this happen?

If it's because of something like knfsd stopping and restarting again
(and the pipe diseappearing and reappering), then we should get another
inotify event, and can handle the problem then.

--b.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 18:58                         ` J. Bruce Fields
@ 2012-01-26 19:36                           ` Jeff Layton
  2012-01-26 20:18                             ` J. Bruce Fields
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2012-01-26 19:36 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Steve Dickson, linux-nfs

On Thu, 26 Jan 2012 13:58:59 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jan 26, 2012 at 10:41:57AM -0500, Jeff Layton wrote:
> > Correct. When knfsd starts back up, a new pipe will be created. At that
> > point, the daemon will try to open the new one and will then close the
> > old if the open succeeds. It will only loop here if that open fails for
> > some reason.
> 
> I'm a little confused--how could this happen?
> 

Bugs?

> If it's because of something like knfsd stopping and restarting again
> (and the pipe diseappearing and reappering), then we should get another
> inotify event, and can handle the problem then.
> 

That's exactly what happens already. When the pipe is recreated, we get
an inotify event, and then attempt to reopen the pipe. I have that
attempt inside of a while loop so that if it fails, we'll reattempt
until it works.

It's highly unlikely that that would fail if the original open worked,
but it's best to account for the possibility and attempt to handle it
gracefully.

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 19:36                           ` Jeff Layton
@ 2012-01-26 20:18                             ` J. Bruce Fields
  2012-01-26 21:58                               ` Steve Dickson
  0 siblings, 1 reply; 27+ messages in thread
From: J. Bruce Fields @ 2012-01-26 20:18 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Steve Dickson, linux-nfs

On Thu, Jan 26, 2012 at 02:36:59PM -0500, Jeff Layton wrote:
> On Thu, 26 Jan 2012 13:58:59 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Thu, Jan 26, 2012 at 10:41:57AM -0500, Jeff Layton wrote:
> > > Correct. When knfsd starts back up, a new pipe will be created. At that
> > > point, the daemon will try to open the new one and will then close the
> > > old if the open succeeds. It will only loop here if that open fails for
> > > some reason.
> > 
> > I'm a little confused--how could this happen?
> > 
> 
> Bugs?

If that's really the only case then "crash and log an error" might
really be more robust in the long term than spinning.

--b.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated
  2012-01-26 20:18                             ` J. Bruce Fields
@ 2012-01-26 21:58                               ` Steve Dickson
  0 siblings, 0 replies; 27+ messages in thread
From: Steve Dickson @ 2012-01-26 21:58 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Jeff Layton, linux-nfs



On 01/26/2012 03:18 PM, J. Bruce Fields wrote:
> On Thu, Jan 26, 2012 at 02:36:59PM -0500, Jeff Layton wrote:
>> On Thu, 26 Jan 2012 13:58:59 -0500
>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>
>>> On Thu, Jan 26, 2012 at 10:41:57AM -0500, Jeff Layton wrote:
>>>> Correct. When knfsd starts back up, a new pipe will be created. At that
>>>> point, the daemon will try to open the new one and will then close the
>>>> old if the open succeeds. It will only loop here if that open fails for
>>>> some reason.
>>>
>>> I'm a little confused--how could this happen?
>>>
>>
>> Bugs?
> 
> If that's really the only case then "crash and log an error" might
> really be more robust in the long term than spinning.
log and crash would make the bug that much more noticeable... IMHO...

steved.
 

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2012-01-26 21:58 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-23 20:02 [PATCH v4 00/11] nfsdcld: add a daemon to track NFSv4 client names on stable storage Jeff Layton
2012-01-23 20:02 ` [PATCH v4 01/11] nfsdcld: add client tracking daemon stub Jeff Layton
2012-01-23 20:02 ` [PATCH v4 02/11] nfsdcld: add autoconf goop for sqlite Jeff Layton
2012-01-23 20:02 ` [PATCH v4 03/11] nfsdcld: add routines for a sqlite backend database Jeff Layton
2012-01-23 20:02 ` [PATCH v4 04/11] nfsdcld: add check/update functionality Jeff Layton
2012-01-23 20:02 ` [PATCH v4 05/11] nfsdcld: add function to remove unreclaimed client records Jeff Layton
2012-01-23 20:02 ` [PATCH v4 06/11] nfsdcld: have daemon pass client row index back to kernel Jeff Layton
2012-01-23 20:02 ` [PATCH v4 07/11] nfsdcld: implement an init upcall Jeff Layton
2012-01-23 20:02 ` [PATCH v4 08/11] nfsdcld: allow daemon to wait for pipe to show up Jeff Layton
2012-01-23 20:02 ` [PATCH v4 09/11] nfsdcld: reopen pipe if it's deleted and recreated Jeff Layton
2012-01-25 18:16   ` Steve Dickson
2012-01-25 19:09     ` Jeff Layton
2012-01-25 19:31       ` Steve Dickson
2012-01-25 20:28         ` Jeff Layton
2012-01-25 22:04           ` Steve Dickson
2012-01-25 23:32             ` Jeff Layton
2012-01-26 12:47               ` Steve Dickson
2012-01-26 13:28                 ` Jeff Layton
2012-01-26 14:30                   ` Jeff Layton
2012-01-26 15:31                     ` Steve Dickson
2012-01-26 15:41                       ` Jeff Layton
2012-01-26 18:58                         ` J. Bruce Fields
2012-01-26 19:36                           ` Jeff Layton
2012-01-26 20:18                             ` J. Bruce Fields
2012-01-26 21:58                               ` Steve Dickson
2012-01-23 20:02 ` [PATCH v4 10/11] nfsdcld: add a manpage for nfsdcld Jeff Layton
2012-01-23 20:02 ` [PATCH v4 11/11] nfsdcld: update the README Jeff Layton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.