[PATCH v2 0/5] nfsd: support for lifting grace period early

[PATCH v2 0/5] nfsd: support for lifting grace period early

[Jeff Layton]

v2:
- move grace period handling into its own module

One of the huge annoyances in dealing with knfsd is the 90s grace period
that's imposed when the server reboots. This is not just an annoyance,
but means a siginificant amount of "downtime" in many production
environments.

This patchset aimed at reducing this pain. It adds a couple of /proc
knobs that tell the lockd and nfsd lock managers to lift the grace
period.

It also changes the UMH upcalls to pass a little bit of extra info in
the form of environment variables so that the upcall program can
determine whether there are still any clients that may be in the process
of reclaiming.

There are also a couple of cleanup patches in here that are not strictly
required. In particular, making a separate grace.ko module doesn't have
to be done, but I think it's a good idea.

Jeff Layton (5):
  lockd: move lockd's grace period handling into its own module
  lockd: add a /proc/fs/lockd/nlm_end_grace file
  nfsd: add a v4_end_grace file to /proc/fs/nfsd
  nfsd: remove redundant boot_time parm from grace_done client tracking
    op
  nfsd: pass extra info in env vars to upcalls to allow for early grace
    period end

 fs/Kconfig                       |   6 ++-
 fs/lockd/Makefile                |   3 +-
 fs/lockd/netns.h                 |   1 -
 fs/lockd/procfs.c                |  76 +++++++++++++++++++++++++++
 fs/lockd/procfs.h                |  28 ++++++++++
 fs/lockd/svc.c                   |  10 +++-
 fs/nfs_common/Makefile           |   3 +-
 fs/{lockd => nfs_common}/grace.c |  68 +++++++++++++++++++++----
 fs/nfsd/Kconfig                  |   1 +
 fs/nfsd/nfs4recover.c            | 107 +++++++++++++++++++++++++++++++--------
 fs/nfsd/nfs4state.c              |   8 +--
 fs/nfsd/nfsctl.c                 |  35 +++++++++++++
 fs/nfsd/state.h                  |   5 +-
 include/linux/proc_fs.h          |   2 +
 14 files changed, 312 insertions(+), 41 deletions(-)
 create mode 100644 fs/lockd/procfs.c
 create mode 100644 fs/lockd/procfs.h
 rename fs/{lockd => nfs_common}/grace.c (50%)

-- 
1.9.3

[PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[Jeff Layton]

Currently, all of the grace period handling is part of lockd. Eventually
though we'd like to be able to build v4-only servers, at which point
we'll need to put all of this elsewhere.

Move the code itself into fs/nfs_common and have it build a grace.ko
module. Then, rejigger the Kconfig options so that both nfsd and lockd
enable it automatically.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/Kconfig                       |  6 +++-
 fs/lockd/Makefile                |  2 +-
 fs/lockd/netns.h                 |  1 -
 fs/lockd/svc.c                   |  1 -
 fs/nfs_common/Makefile           |  3 +-
 fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
 fs/nfsd/Kconfig                  |  1 +
 include/linux/proc_fs.h          |  2 ++
 8 files changed, 69 insertions(+), 15 deletions(-)
 rename fs/{lockd => nfs_common}/grace.c (50%)

diff --git a/fs/Kconfig b/fs/Kconfig
index 312393f32948..db5dc1598716 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
 source "fs/nfs/Kconfig"
 source "fs/nfsd/Kconfig"
 
+config GRACE_PERIOD
+	tristate
+
 config LOCKD
 	tristate
 	depends on FILE_LOCKING
+	select GRACE_PERIOD
 
 config LOCKD_V4
 	bool
@@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
 
 config NFS_COMMON
 	bool
-	depends on NFSD || NFS_FS
+	depends on NFSD || NFS_FS || LOCKD
 	default y
 
 source "net/sunrpc/Kconfig"
diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
index ca58d64374ca..6a0b351ce30e 100644
--- a/fs/lockd/Makefile
+++ b/fs/lockd/Makefile
@@ -5,6 +5,6 @@
 obj-$(CONFIG_LOCKD) += lockd.o
 
 lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
-	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
+	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
 lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
 lockd-objs		      := $(lockd-objs-y)
diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
index 5010b55628b4..097bfa3adb1c 100644
--- a/fs/lockd/netns.h
+++ b/fs/lockd/netns.h
@@ -11,7 +11,6 @@ struct lockd_net {
 
 	struct delayed_work grace_period_end;
 	struct lock_manager lockd_manager;
-	struct list_head grace_list;
 
 	spinlock_t nsm_clnt_lock;
 	unsigned int nsm_users;
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 8f27c93f8d2e..3599c9ca28ae 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
 	struct lockd_net *ln = net_generic(net, lockd_net_id);
 
 	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
-	INIT_LIST_HEAD(&ln->grace_list);
 	spin_lock_init(&ln->nsm_clnt_lock);
 	return 0;
 }
diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
index f689ed82af3a..d153ca3ea577 100644
--- a/fs/nfs_common/Makefile
+++ b/fs/nfs_common/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
-
 nfs_acl-objs := nfsacl.o
+
+obj-$(CONFIG_GRACE_PERIOD) += grace.o
diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
similarity index 50%
rename from fs/lockd/grace.c
rename to fs/nfs_common/grace.c
index 6d1ee7204c88..ae6e58ea4de5 100644
--- a/fs/lockd/grace.c
+++ b/fs/nfs_common/grace.c
@@ -1,17 +1,20 @@
 /*
  * Common code for control of lockd and nfsv4 grace periods.
+ *
+ * Transplanted from lockd code
  */
 
 #include <linux/module.h>
-#include <linux/lockd/bind.h>
 #include <net/net_namespace.h>
+#include <net/netns/generic.h>
+#include <linux/fs.h>
 
-#include "netns.h"
-
+static int grace_net_id;
 static DEFINE_SPINLOCK(grace_lock);
 
 /**
  * locks_start_grace
+ * @net: net namespace that this lock manager belongs to
  * @lm: who this grace period is for
  *
  * A grace period is a period during which locks should not be given
@@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
  *
  * This function is called to start a grace period.
  */
-void locks_start_grace(struct net *net, struct lock_manager *lm)
+void
+locks_start_grace(struct net *net, struct lock_manager *lm)
 {
-	struct lockd_net *ln = net_generic(net, lockd_net_id);
+	struct list_head *grace_list = net_generic(net, grace_net_id);
 
 	spin_lock(&grace_lock);
-	list_add(&lm->list, &ln->grace_list);
+	list_add(&lm->list, grace_list);
 	spin_unlock(&grace_lock);
 }
 EXPORT_SYMBOL_GPL(locks_start_grace);
 
 /**
  * locks_end_grace
+ * @net: net namespace that this lock manager belongs to
  * @lm: who this grace period is for
  *
  * Call this function to state that the given lock manager is ready to
@@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
  * Note that callers count on it being safe to call this more than once,
  * and the second call should be a no-op.
  */
-void locks_end_grace(struct lock_manager *lm)
+void
+locks_end_grace(struct lock_manager *lm)
 {
 	spin_lock(&grace_lock);
 	list_del_init(&lm->list);
@@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
  * to answer ordinary lock requests, and when they should accept only
  * lock reclaims.
  */
-int locks_in_grace(struct net *net)
+int
+locks_in_grace(struct net *net)
 {
-	struct lockd_net *ln = net_generic(net, lockd_net_id);
+	struct list_head *grace_list = net_generic(net, grace_net_id);
 
-	return !list_empty(&ln->grace_list);
+	return !list_empty(grace_list);
 }
 EXPORT_SYMBOL_GPL(locks_in_grace);
+
+static int __net_init
+grace_init_net(struct net *net)
+{
+	struct list_head *grace_list = net_generic(net, grace_net_id);
+
+	INIT_LIST_HEAD(grace_list);
+	return 0;
+}
+
+static void __net_exit
+grace_exit_net(struct net *net)
+{
+	struct list_head *grace_list = net_generic(net, grace_net_id);
+
+	BUG_ON(!list_empty(grace_list));
+}
+
+static struct pernet_operations grace_net_ops = {
+	.init = grace_init_net,
+	.exit = grace_exit_net,
+	.id   = &grace_net_id,
+	.size = sizeof(struct list_head),
+};
+
+static int __init
+init_grace(void)
+{
+	return register_pernet_subsys(&grace_net_ops);
+}
+
+static void __exit
+exit_grace(void)
+{
+	unregister_pernet_subsys(&grace_net_ops);
+}
+
+MODULE_AUTHOR("Jeff Layton <jlayton@primarydata.com>");
+MODULE_LICENSE("GPL");
+module_init(init_grace)
+module_exit(exit_grace)
diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
index f994e750e0d1..4fa98764de21 100644
--- a/fs/nfsd/Kconfig
+++ b/fs/nfsd/Kconfig
@@ -71,6 +71,7 @@ config NFSD_V4
 	select FS_POSIX_ACL
 	select SUNRPC_GSS
 	select CRYPTO
+	select GRACE_PERIOD
 	help
 	  This option enables support in your system's NFS server for
 	  version 4 of the NFS protocol (RFC 3530).
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 9d117f61d976..b97bf2ef996e 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
 
 #endif /* CONFIG_PROC_FS */
 
+struct net;
+
 static inline struct proc_dir_entry *proc_net_mkdir(
 	struct net *net, const char *name, struct proc_dir_entry *parent)
 {
-- 
1.9.3

[PATCH v2 2/5] lockd: add a /proc/fs/lockd/nlm_end_grace file

[Jeff Layton]

Add a new procfile that will allow a (privileged) userland process to
end the NLM grace period early. The basic idea here will be to have
sm-notify write to this file, if it sent out no NOTIFY requests when
it runs. In that situation, we can generally expect that there will be
no reclaim requests so the grace period can be lifted early.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/lockd/Makefile |  1 +
 fs/lockd/procfs.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/lockd/procfs.h | 28 ++++++++++++++++++++
 fs/lockd/svc.c    |  9 +++++++
 4 files changed, 114 insertions(+)
 create mode 100644 fs/lockd/procfs.c
 create mode 100644 fs/lockd/procfs.h

diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
index 6a0b351ce30e..9b320cc2a8cf 100644
--- a/fs/lockd/Makefile
+++ b/fs/lockd/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_LOCKD) += lockd.o
 lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
 	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
 lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
+lockd-objs-$(CONFIG_PROC_FS) += procfs.o
 lockd-objs		      := $(lockd-objs-y)
diff --git a/fs/lockd/procfs.c b/fs/lockd/procfs.c
new file mode 100644
index 000000000000..dfd708fa43a3
--- /dev/null
+++ b/fs/lockd/procfs.c
@@ -0,0 +1,76 @@
+/*
+ * Procfs support for lockd
+ *
+ * Copyright (c) 2014 Jeff Layton <jlayton@primarydata.com>
+ */
+
+#include <linux/fs.h>
+#include <linux/proc_fs.h>
+#include <linux/module.h>
+#include <linux/nsproxy.h>
+#include <net/net_namespace.h>
+
+#include "netns.h"
+#include "procfs.h"
+
+/*
+ * Just check to see if the data written is longer than zero length. We
+ * don't really care what's in the data, just that someone wrote something
+ * to the file.
+ */
+static ssize_t
+nlm_end_grace_write(struct file *file, const char __user *buf, size_t size,
+		    loff_t *pos)
+{
+	struct lockd_net *ln = net_generic(current->nsproxy->net_ns, lockd_net_id);
+
+	if (size > 0)
+		locks_end_grace(&ln->lockd_manager);
+
+	return size;
+}
+
+static ssize_t
+nlm_end_grace_read(struct file *file, char __user *buf, size_t size,
+		   loff_t *pos)
+{
+	struct lockd_net *ln = net_generic(current->nsproxy->net_ns, lockd_net_id);
+	char resp[3];
+
+	resp[0] = list_empty(&ln->lockd_manager.list) ? 'Y' : 'N';
+	resp[1] = '\n';
+	resp[2] = '\0';
+
+	return simple_read_from_buffer(buf, size, pos, resp, sizeof(resp));
+}
+
+static const struct file_operations lockd_end_grace_operations = {
+	.write		= nlm_end_grace_write,
+	.read		= nlm_end_grace_read,
+	.llseek		= default_llseek,
+	.owner		= THIS_MODULE,
+};
+
+int __init
+lockd_create_procfs(void)
+{
+	struct proc_dir_entry *entry;
+
+	entry = proc_mkdir("fs/lockd", NULL);
+	if (!entry)
+		return -ENOMEM;
+	entry = proc_create("nlm_end_grace", S_IRUGO|S_IWUSR, entry,
+				 &lockd_end_grace_operations);
+	if (!entry) {
+		remove_proc_entry("fs/lockd", NULL);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+void __exit
+lockd_remove_procfs(void)
+{
+	remove_proc_entry("fs/lockd/nlm_end_grace", NULL);
+	remove_proc_entry("fs/lockd", NULL);
+}
diff --git a/fs/lockd/procfs.h b/fs/lockd/procfs.h
new file mode 100644
index 000000000000..2257a1311027
--- /dev/null
+++ b/fs/lockd/procfs.h
@@ -0,0 +1,28 @@
+/*
+ * Procfs support for lockd
+ *
+ * Copyright (c) 2014 Jeff Layton <jlayton@primarydata.com>
+ */
+#ifndef _LOCKD_PROCFS_H
+#define _LOCKD_PROCFS_H
+
+#include <linux/kconfig.h>
+
+#if IS_ENABLED(CONFIG_PROC_FS)
+int lockd_create_procfs(void);
+void lockd_remove_procfs(void);
+#else
+static inline int
+lockd_create_procfs(void)
+{
+	return 0;
+}
+
+static inline void
+lockd_remove_procfs(void)
+{
+	return;
+}
+#endif /* IS_ENABLED(CONFIG_PROC_FS) */
+
+#endif /* _LOCKD_PROCFS_H */
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 3599c9ca28ae..f01a9aa01dff 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -36,6 +36,7 @@
 #include <linux/nfs.h>
 
 #include "netns.h"
+#include "procfs.h"
 
 #define NLMDBG_FACILITY		NLMDBG_SVC
 #define LOCKD_BUFSIZE		(1024 + NLMSVC_XDRSIZE)
@@ -616,8 +617,15 @@ static int __init init_nlm(void)
 	err = register_pernet_subsys(&lockd_net_ops);
 	if (err)
 		goto err_pernet;
+
+	err = lockd_create_procfs();
+	if (err)
+		goto err_procfs;
+
 	return 0;
 
+err_procfs:
+	unregister_pernet_subsys(&lockd_net_ops);
 err_pernet:
 #ifdef CONFIG_SYSCTL
 	unregister_sysctl_table(nlm_sysctl_table);
@@ -630,6 +638,7 @@ static void __exit exit_nlm(void)
 {
 	/* FIXME: delete all NLM clients */
 	nlm_shutdown_hosts();
+	lockd_remove_procfs();
 	unregister_pernet_subsys(&lockd_net_ops);
 #ifdef CONFIG_SYSCTL
 	unregister_sysctl_table(nlm_sysctl_table);
-- 
1.9.3

[PATCH v2 3/5] nfsd: add a v4_end_grace file to /proc/fs/nfsd

[Jeff Layton]

Allow a privileged userland process to end the v4 grace period early.
Any write to the file will cause the v4 grace period to be lifted.
The basic idea with this will be to allow the userland client tracking
program to lift the grace period once it knows that no more clients
will be reclaiming state.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/nfsctl.c    | 35 +++++++++++++++++++++++++++++++++++
 fs/nfsd/state.h     |  3 +++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59e7e91..711280e0e4ac 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4107,7 +4107,7 @@ out:
 	return status;
 }
 
-static void
+void
 nfsd4_end_grace(struct nfsd_net *nn)
 {
 	/* do nothing if grace period already ended */
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 4e042105fb6e..99044e237861 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -49,6 +49,7 @@ enum {
 	NFSD_Leasetime,
 	NFSD_Gracetime,
 	NFSD_RecoveryDir,
+	NFSD_V4EndGrace,
 #endif
 };
 
@@ -68,6 +69,7 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size);
 static ssize_t write_leasetime(struct file *file, char *buf, size_t size);
 static ssize_t write_gracetime(struct file *file, char *buf, size_t size);
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
+static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size);
 #endif
 
 static ssize_t (*write_op[])(struct file *, char *, size_t) = {
@@ -84,6 +86,7 @@ static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_Leasetime] = write_leasetime,
 	[NFSD_Gracetime] = write_gracetime,
 	[NFSD_RecoveryDir] = write_recoverydir,
+	[NFSD_V4EndGrace] = write_v4_end_grace,
 #endif
 };
 
@@ -1077,6 +1080,37 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
 	return rv;
 }
 
+/**
+ * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
+ *
+ * Input:
+ *			buf:		ignored
+ *			size:		zero
+ * OR
+ *
+ * Input:
+ * 			buf:		any value
+ *			size:		non-zero length of C string in @buf
+ * Output:
+ *			passed-in buffer filled with "Y" or "N" with a newline
+ *			and NULL-terminated C string. This indicates whether
+ *			the grace period has ended in the current net
+ *			namespace. Return code is the size in bytes of the
+ *			string. Writing to the file will end the grace period
+ *			for nfsd's v4 lock manager.
+ */
+static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
+{
+	struct net *net = file->f_dentry->d_sb->s_fs_info;
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	if (size > 0)
+		nfsd4_end_grace(nn);
+
+	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
+			 nn->grace_ended ? 'Y' : 'N');
+}
+
 #endif
 
 /*----------------------------------------------------------------------------*/
@@ -1110,6 +1144,7 @@ static int nfsd_fill_super(struct super_block * sb, void * data, int silent)
 		[NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Gracetime] = {"nfsv4gracetime", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
 #endif
 		/* last one */ {""}
 	};
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4a89e00d7461..ecf579904892 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -545,6 +545,9 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
 
+/* grace period management */
+void nfsd4_end_grace(struct nfsd_net *nn);
+
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);
 extern void nfsd4_client_tracking_exit(struct net *net);
-- 
1.9.3

[PATCH v2 4/5] nfsd: remove redundant boot_time parm from grace_done client tracking op

[Jeff Layton]

Since it's stored in nfsd_net, we don't need to pass it in separately.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4recover.c | 17 ++++++++---------
 fs/nfsd/nfs4state.c   |  2 +-
 fs/nfsd/state.h       |  2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 9c271f42604a..a0d2ba956a3f 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -58,7 +58,7 @@ struct nfsd4_client_tracking_ops {
 	void (*create)(struct nfs4_client *);
 	void (*remove)(struct nfs4_client *);
 	int (*check)(struct nfs4_client *);
-	void (*grace_done)(struct nfsd_net *, time_t);
+	void (*grace_done)(struct nfsd_net *);
 };
 
 /* Globals */
@@ -392,7 +392,7 @@ purge_old(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
 }
 
 static void
-nfsd4_recdir_purge_old(struct nfsd_net *nn, time_t boot_time)
+nfsd4_recdir_purge_old(struct nfsd_net *nn)
 {
 	int status;
 
@@ -1016,7 +1016,7 @@ nfsd4_cld_check(struct nfs4_client *clp)
 }
 
 static void
-nfsd4_cld_grace_done(struct nfsd_net *nn, time_t boot_time)
+nfsd4_cld_grace_done(struct nfsd_net *nn)
 {
 	int ret;
 	struct cld_upcall *cup;
@@ -1029,7 +1029,7 @@ nfsd4_cld_grace_done(struct nfsd_net *nn, time_t boot_time)
 	}
 
 	cup->cu_msg.cm_cmd = Cld_GraceDone;
-	cup->cu_msg.cm_u.cm_gracetime = (int64_t)boot_time;
+	cup->cu_msg.cm_u.cm_gracetime = (int64_t)nn->boot_time;
 	ret = cld_pipe_upcall(cn->cn_pipe, &cup->cu_msg);
 	if (!ret)
 		ret = cup->cu_msg.cm_status;
@@ -1245,13 +1245,12 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 }
 
 static void
-nfsd4_umh_cltrack_grace_done(struct nfsd_net __attribute__((unused)) *nn,
-				time_t boot_time)
+nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
 {
 	char *legacy;
 	char timestr[22]; /* FIXME: better way to determine max size? */
 
-	sprintf(timestr, "%ld", boot_time);
+	sprintf(timestr, "%ld", nn->boot_time);
 	legacy = nfsd4_cltrack_legacy_topdir();
 	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
 	kfree(legacy);
@@ -1356,10 +1355,10 @@ nfsd4_client_record_check(struct nfs4_client *clp)
 }
 
 void
-nfsd4_record_grace_done(struct nfsd_net *nn, time_t boot_time)
+nfsd4_record_grace_done(struct nfsd_net *nn)
 {
 	if (nn->client_tracking_ops)
-		nn->client_tracking_ops->grace_done(nn, boot_time);
+		nn->client_tracking_ops->grace_done(nn);
 }
 
 static int
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 711280e0e4ac..21becb29dae1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4116,7 +4116,7 @@ nfsd4_end_grace(struct nfsd_net *nn)
 
 	dprintk("NFSD: end of grace period\n");
 	nn->grace_ended = true;
-	nfsd4_record_grace_done(nn, nn->boot_time);
+	nfsd4_record_grace_done(nn);
 	locks_end_grace(&nn->nfsd4_manager);
 	/*
 	 * Now that every NFSv4 client has had the chance to recover and
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index ecf579904892..854f0c574ccf 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -554,7 +554,7 @@ extern void nfsd4_client_tracking_exit(struct net *net);
 extern void nfsd4_client_record_create(struct nfs4_client *clp);
 extern void nfsd4_client_record_remove(struct nfs4_client *clp);
 extern int nfsd4_client_record_check(struct nfs4_client *clp);
-extern void nfsd4_record_grace_done(struct nfsd_net *nn, time_t boot_time);
+extern void nfsd4_record_grace_done(struct nfsd_net *nn);
 
 /* nfs fault injection functions */
 #ifdef CONFIG_NFSD_FAULT_INJECTION
-- 
1.9.3

[PATCH v2 5/5] nfsd: pass extra info in env vars to upcalls to allow for early grace period end

[Jeff Layton]

In order to support lifting the grace period early, we must tell
nfsdcltrack what sort of client the "create" upcall is for. We can't
reliably tell if a v4.0 client has completed reclaiming, so we can only
lift the grace period once all the v4.1+ clients have issued a
RECLAIM_COMPLETE and if there are no v4.0 clients.

Also, in order to lift the grace period, we have to tell userland when
the grace period started so that it can tell whether a RECLAIM_COMPLETE
has been issued for each client since then.

Since this is all optional info, we pass it along in environment
variables to the "init" and "create" upcalls. By doing this, we don't
need to revise the upcall format. The UMH upcall can simply make use of
this info if it happens to be present. If it's not then it can just
avoid lifting the grace period early.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-------
 fs/nfsd/nfs4state.c   |  4 +--
 2 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index a0d2ba956a3f..2b61bcd92b58 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1062,6 +1062,8 @@ MODULE_PARM_DESC(cltrack_legacy_disable,
 
 #define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
 #define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
+#define CLIENT_MINORVERS_ENV_PREFIX "NFSDCLTRACK_CLIENT_MINORVERSION="
+#define GRACE_START_ENV_PREFIX "NFSDCLTRACK_GRACE_START="
 
 static char *
 nfsd4_cltrack_legacy_topdir(void)
@@ -1126,10 +1128,60 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
 	return result;
 }
 
+static char *
+nfsd4_cltrack_client_minorversion(struct nfs4_client *clp)
+{
+	int copied;
+	size_t len;
+	char *result;
+
+	/* prefix + max width of integer string + terminating NULL */
+	len = strlen(CLIENT_MINORVERS_ENV_PREFIX) + 10 + 1;
+
+	result = kmalloc(len, GFP_KERNEL);
+	if (!result)
+		return result;
+
+	copied = snprintf(result, len, CLIENT_MINORVERS_ENV_PREFIX "%u",
+				clp->cl_minorversion);
+	if (copied >= len) {
+		/* just return nothing if output was truncated */
+		kfree(result);
+		return NULL;
+	}
+
+	return result;
+}
+
+static char *
+nfsd4_cltrack_grace_start(time_t grace_start)
+{
+	int copied;
+	size_t len;
+	char *result;
+
+	/* prefix + max width of int64_t string + terminating NULL */
+	len = strlen(GRACE_START_ENV_PREFIX) + 22 + 1;
+
+	result = kmalloc(len, GFP_KERNEL);
+	if (!result)
+		return result;
+
+	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%ld",
+				grace_start);
+	if (copied >= len) {
+		/* just return nothing if output was truncated */
+		kfree(result);
+		return NULL;
+	}
+
+	return result;
+}
+
 static int
-nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
+nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
 {
-	char *envp[2];
+	char *envp[3];
 	char *argv[4];
 	int ret;
 
@@ -1140,10 +1192,12 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
 
 	dprintk("%s: cmd: %s\n", __func__, cmd);
 	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
-	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
+	dprintk("%s: env0: %s\n", __func__, env0 ? env0 : "(null)");
+	dprintk("%s: env1: %s\n", __func__, env1 ? env1 : "(null)");
 
-	envp[0] = legacy;
-	envp[1] = NULL;
+	envp[0] = env0;
+	envp[1] = env1;
+	envp[2] = NULL;
 
 	argv[0] = (char *)cltrack_prog;
 	argv[1] = cmd;
@@ -1187,28 +1241,40 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
 }
 
 static int
-nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
+nfsd4_umh_cltrack_init(struct net *net)
 {
+	int ret;
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
+
 	/* XXX: The usermode helper s not working in container yet. */
 	if (net != &init_net) {
 		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
 			"tracking in a container!\n");
 		return -EINVAL;
 	}
-	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
+
+	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
+	kfree(grace_start);
+	return ret;
 }
 
 static void
 nfsd4_umh_cltrack_create(struct nfs4_client *clp)
 {
-	char *hexid;
+	char *hexid, *minorvers, *grace_start;
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
 	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
 	if (!hexid) {
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return;
 	}
-	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
+	minorvers = nfsd4_cltrack_client_minorversion(clp);
+	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
+	nfsd4_umh_cltrack_upcall("create", hexid, minorvers, grace_start);
+	kfree(minorvers);
+	kfree(grace_start);
 	kfree(hexid);
 }
 
@@ -1222,7 +1288,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return;
 	}
-	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
+	nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL);
 	kfree(hexid);
 }
 
@@ -1238,7 +1304,7 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 		return -ENOMEM;
 	}
 	legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name);
-	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
+	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL);
 	kfree(legacy);
 	kfree(hexid);
 	return ret;
@@ -1252,7 +1318,7 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
 
 	sprintf(timestr, "%ld", nn->boot_time);
 	legacy = nfsd4_cltrack_legacy_topdir();
-	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
+	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
 	kfree(legacy);
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 21becb29dae1..5b04353bf2ec 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6342,10 +6342,10 @@ nfs4_state_start_net(struct net *net)
 	ret = nfs4_state_create_net(net);
 	if (ret)
 		return ret;
-	nfsd4_client_tracking_init(net);
 	nn->boot_time = get_seconds();
-	locks_start_grace(net, &nn->nfsd4_manager);
 	nn->grace_ended = false;
+	locks_start_grace(net, &nn->nfsd4_manager);
+	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
 	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
-- 
1.9.3

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> Currently, all of the grace period handling is part of lockd. Eventually
> though we'd like to be able to build v4-only servers, at which point
> we'll need to put all of this elsewhere.
> 
> Move the code itself into fs/nfs_common and have it build a grace.ko
> module. Then, rejigger the Kconfig options so that both nfsd and lockd
> enable it automatically.

Thanks, applying this one for 3.18 indepedently of the others.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/Kconfig                       |  6 +++-
>  fs/lockd/Makefile                |  2 +-
>  fs/lockd/netns.h                 |  1 -
>  fs/lockd/svc.c                   |  1 -
>  fs/nfs_common/Makefile           |  3 +-
>  fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
>  fs/nfsd/Kconfig                  |  1 +
>  include/linux/proc_fs.h          |  2 ++
>  8 files changed, 69 insertions(+), 15 deletions(-)
>  rename fs/{lockd => nfs_common}/grace.c (50%)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 312393f32948..db5dc1598716 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
>  source "fs/nfs/Kconfig"
>  source "fs/nfsd/Kconfig"
>  
> +config GRACE_PERIOD
> +	tristate
> +
>  config LOCKD
>  	tristate
>  	depends on FILE_LOCKING
> +	select GRACE_PERIOD
>  
>  config LOCKD_V4
>  	bool
> @@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
>  
>  config NFS_COMMON
>  	bool
> -	depends on NFSD || NFS_FS
> +	depends on NFSD || NFS_FS || LOCKD
>  	default y
>  
>  source "net/sunrpc/Kconfig"
> diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
> index ca58d64374ca..6a0b351ce30e 100644
> --- a/fs/lockd/Makefile
> +++ b/fs/lockd/Makefile
> @@ -5,6 +5,6 @@
>  obj-$(CONFIG_LOCKD) += lockd.o
>  
>  lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
> -	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
> +	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
>  lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
>  lockd-objs		      := $(lockd-objs-y)
> diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> index 5010b55628b4..097bfa3adb1c 100644
> --- a/fs/lockd/netns.h
> +++ b/fs/lockd/netns.h
> @@ -11,7 +11,6 @@ struct lockd_net {
>  
>  	struct delayed_work grace_period_end;
>  	struct lock_manager lockd_manager;
> -	struct list_head grace_list;
>  
>  	spinlock_t nsm_clnt_lock;
>  	unsigned int nsm_users;
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 8f27c93f8d2e..3599c9ca28ae 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
>  	struct lockd_net *ln = net_generic(net, lockd_net_id);
>  
>  	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> -	INIT_LIST_HEAD(&ln->grace_list);
>  	spin_lock_init(&ln->nsm_clnt_lock);
>  	return 0;
>  }
> diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> index f689ed82af3a..d153ca3ea577 100644
> --- a/fs/nfs_common/Makefile
> +++ b/fs/nfs_common/Makefile
> @@ -3,5 +3,6 @@
>  #
>  
>  obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> -
>  nfs_acl-objs := nfsacl.o
> +
> +obj-$(CONFIG_GRACE_PERIOD) += grace.o
> diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
> similarity index 50%
> rename from fs/lockd/grace.c
> rename to fs/nfs_common/grace.c
> index 6d1ee7204c88..ae6e58ea4de5 100644
> --- a/fs/lockd/grace.c
> +++ b/fs/nfs_common/grace.c
> @@ -1,17 +1,20 @@
>  /*
>   * Common code for control of lockd and nfsv4 grace periods.
> + *
> + * Transplanted from lockd code
>   */
>  
>  #include <linux/module.h>
> -#include <linux/lockd/bind.h>
>  #include <net/net_namespace.h>
> +#include <net/netns/generic.h>
> +#include <linux/fs.h>
>  
> -#include "netns.h"
> -
> +static int grace_net_id;
>  static DEFINE_SPINLOCK(grace_lock);
>  
>  /**
>   * locks_start_grace
> + * @net: net namespace that this lock manager belongs to
>   * @lm: who this grace period is for
>   *
>   * A grace period is a period during which locks should not be given
> @@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
>   *
>   * This function is called to start a grace period.
>   */
> -void locks_start_grace(struct net *net, struct lock_manager *lm)
> +void
> +locks_start_grace(struct net *net, struct lock_manager *lm)
>  {
> -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> +	struct list_head *grace_list = net_generic(net, grace_net_id);
>  
>  	spin_lock(&grace_lock);
> -	list_add(&lm->list, &ln->grace_list);
> +	list_add(&lm->list, grace_list);
>  	spin_unlock(&grace_lock);
>  }
>  EXPORT_SYMBOL_GPL(locks_start_grace);
>  
>  /**
>   * locks_end_grace
> + * @net: net namespace that this lock manager belongs to
>   * @lm: who this grace period is for
>   *
>   * Call this function to state that the given lock manager is ready to
> @@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
>   * Note that callers count on it being safe to call this more than once,
>   * and the second call should be a no-op.
>   */
> -void locks_end_grace(struct lock_manager *lm)
> +void
> +locks_end_grace(struct lock_manager *lm)
>  {
>  	spin_lock(&grace_lock);
>  	list_del_init(&lm->list);
> @@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
>   * to answer ordinary lock requests, and when they should accept only
>   * lock reclaims.
>   */
> -int locks_in_grace(struct net *net)
> +int
> +locks_in_grace(struct net *net)
>  {
> -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> +	struct list_head *grace_list = net_generic(net, grace_net_id);
>  
> -	return !list_empty(&ln->grace_list);
> +	return !list_empty(grace_list);
>  }
>  EXPORT_SYMBOL_GPL(locks_in_grace);
> +
> +static int __net_init
> +grace_init_net(struct net *net)
> +{
> +	struct list_head *grace_list = net_generic(net, grace_net_id);
> +
> +	INIT_LIST_HEAD(grace_list);
> +	return 0;
> +}
> +
> +static void __net_exit
> +grace_exit_net(struct net *net)
> +{
> +	struct list_head *grace_list = net_generic(net, grace_net_id);
> +
> +	BUG_ON(!list_empty(grace_list));
> +}
> +
> +static struct pernet_operations grace_net_ops = {
> +	.init = grace_init_net,
> +	.exit = grace_exit_net,
> +	.id   = &grace_net_id,
> +	.size = sizeof(struct list_head),
> +};
> +
> +static int __init
> +init_grace(void)
> +{
> +	return register_pernet_subsys(&grace_net_ops);
> +}
> +
> +static void __exit
> +exit_grace(void)
> +{
> +	unregister_pernet_subsys(&grace_net_ops);
> +}
> +
> +MODULE_AUTHOR("Jeff Layton <jlayton@primarydata.com>");
> +MODULE_LICENSE("GPL");
> +module_init(init_grace)
> +module_exit(exit_grace)
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index f994e750e0d1..4fa98764de21 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -71,6 +71,7 @@ config NFSD_V4
>  	select FS_POSIX_ACL
>  	select SUNRPC_GSS
>  	select CRYPTO
> +	select GRACE_PERIOD
>  	help
>  	  This option enables support in your system's NFS server for
>  	  version 4 of the NFS protocol (RFC 3530).
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 9d117f61d976..b97bf2ef996e 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
>  
>  #endif /* CONFIG_PROC_FS */
>  
> +struct net;
> +
>  static inline struct proc_dir_entry *proc_net_mkdir(
>  	struct net *net, const char *name, struct proc_dir_entry *parent)
>  {
> -- 
> 1.9.3
> 

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > Currently, all of the grace period handling is part of lockd. Eventually
> > though we'd like to be able to build v4-only servers, at which point
> > we'll need to put all of this elsewhere.
> > 
> > Move the code itself into fs/nfs_common and have it build a grace.ko
> > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > enable it automatically.
> 
> Thanks, applying this one for 3.18 indepedently of the others.

This code should also be fixed, though.

Currently nfsd is recording the grace period as done when its own timer
runs out, but then it continuing to accept reclaims until lockd is also
done.

This is an unusual bug to actually hit in part because lockd's grace
period is by default less than nfsd's.

Your code introduces new cases when nfsd's grace period could end before
lockd's, but in all of them future reclaims would be denied (because the
only clients active on last shutdown would be 4.1 clients that have all
issued RECLAIM_COMPLETE, so reclaims are all going to be denied).

So maybe it's not urgent.

--b.

> 
> --b.
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/Kconfig                       |  6 +++-
> >  fs/lockd/Makefile                |  2 +-
> >  fs/lockd/netns.h                 |  1 -
> >  fs/lockd/svc.c                   |  1 -
> >  fs/nfs_common/Makefile           |  3 +-
> >  fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
> >  fs/nfsd/Kconfig                  |  1 +
> >  include/linux/proc_fs.h          |  2 ++
> >  8 files changed, 69 insertions(+), 15 deletions(-)
> >  rename fs/{lockd => nfs_common}/grace.c (50%)
> > 
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 312393f32948..db5dc1598716 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
> >  source "fs/nfs/Kconfig"
> >  source "fs/nfsd/Kconfig"
> >  
> > +config GRACE_PERIOD
> > +	tristate
> > +
> >  config LOCKD
> >  	tristate
> >  	depends on FILE_LOCKING
> > +	select GRACE_PERIOD
> >  
> >  config LOCKD_V4
> >  	bool
> > @@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
> >  
> >  config NFS_COMMON
> >  	bool
> > -	depends on NFSD || NFS_FS
> > +	depends on NFSD || NFS_FS || LOCKD
> >  	default y
> >  
> >  source "net/sunrpc/Kconfig"
> > diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
> > index ca58d64374ca..6a0b351ce30e 100644
> > --- a/fs/lockd/Makefile
> > +++ b/fs/lockd/Makefile
> > @@ -5,6 +5,6 @@
> >  obj-$(CONFIG_LOCKD) += lockd.o
> >  
> >  lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
> > -	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
> > +	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
> >  lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
> >  lockd-objs		      := $(lockd-objs-y)
> > diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> > index 5010b55628b4..097bfa3adb1c 100644
> > --- a/fs/lockd/netns.h
> > +++ b/fs/lockd/netns.h
> > @@ -11,7 +11,6 @@ struct lockd_net {
> >  
> >  	struct delayed_work grace_period_end;
> >  	struct lock_manager lockd_manager;
> > -	struct list_head grace_list;
> >  
> >  	spinlock_t nsm_clnt_lock;
> >  	unsigned int nsm_users;
> > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > index 8f27c93f8d2e..3599c9ca28ae 100644
> > --- a/fs/lockd/svc.c
> > +++ b/fs/lockd/svc.c
> > @@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
> >  	struct lockd_net *ln = net_generic(net, lockd_net_id);
> >  
> >  	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> > -	INIT_LIST_HEAD(&ln->grace_list);
> >  	spin_lock_init(&ln->nsm_clnt_lock);
> >  	return 0;
> >  }
> > diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> > index f689ed82af3a..d153ca3ea577 100644
> > --- a/fs/nfs_common/Makefile
> > +++ b/fs/nfs_common/Makefile
> > @@ -3,5 +3,6 @@
> >  #
> >  
> >  obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> > -
> >  nfs_acl-objs := nfsacl.o
> > +
> > +obj-$(CONFIG_GRACE_PERIOD) += grace.o
> > diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
> > similarity index 50%
> > rename from fs/lockd/grace.c
> > rename to fs/nfs_common/grace.c
> > index 6d1ee7204c88..ae6e58ea4de5 100644
> > --- a/fs/lockd/grace.c
> > +++ b/fs/nfs_common/grace.c
> > @@ -1,17 +1,20 @@
> >  /*
> >   * Common code for control of lockd and nfsv4 grace periods.
> > + *
> > + * Transplanted from lockd code
> >   */
> >  
> >  #include <linux/module.h>
> > -#include <linux/lockd/bind.h>
> >  #include <net/net_namespace.h>
> > +#include <net/netns/generic.h>
> > +#include <linux/fs.h>
> >  
> > -#include "netns.h"
> > -
> > +static int grace_net_id;
> >  static DEFINE_SPINLOCK(grace_lock);
> >  
> >  /**
> >   * locks_start_grace
> > + * @net: net namespace that this lock manager belongs to
> >   * @lm: who this grace period is for
> >   *
> >   * A grace period is a period during which locks should not be given
> > @@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
> >   *
> >   * This function is called to start a grace period.
> >   */
> > -void locks_start_grace(struct net *net, struct lock_manager *lm)
> > +void
> > +locks_start_grace(struct net *net, struct lock_manager *lm)
> >  {
> > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> >  
> >  	spin_lock(&grace_lock);
> > -	list_add(&lm->list, &ln->grace_list);
> > +	list_add(&lm->list, grace_list);
> >  	spin_unlock(&grace_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(locks_start_grace);
> >  
> >  /**
> >   * locks_end_grace
> > + * @net: net namespace that this lock manager belongs to
> >   * @lm: who this grace period is for
> >   *
> >   * Call this function to state that the given lock manager is ready to
> > @@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> >   * Note that callers count on it being safe to call this more than once,
> >   * and the second call should be a no-op.
> >   */
> > -void locks_end_grace(struct lock_manager *lm)
> > +void
> > +locks_end_grace(struct lock_manager *lm)
> >  {
> >  	spin_lock(&grace_lock);
> >  	list_del_init(&lm->list);
> > @@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> >   * to answer ordinary lock requests, and when they should accept only
> >   * lock reclaims.
> >   */
> > -int locks_in_grace(struct net *net)
> > +int
> > +locks_in_grace(struct net *net)
> >  {
> > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> >  
> > -	return !list_empty(&ln->grace_list);
> > +	return !list_empty(grace_list);
> >  }
> >  EXPORT_SYMBOL_GPL(locks_in_grace);
> > +
> > +static int __net_init
> > +grace_init_net(struct net *net)
> > +{
> > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +
> > +	INIT_LIST_HEAD(grace_list);
> > +	return 0;
> > +}
> > +
> > +static void __net_exit
> > +grace_exit_net(struct net *net)
> > +{
> > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +
> > +	BUG_ON(!list_empty(grace_list));
> > +}
> > +
> > +static struct pernet_operations grace_net_ops = {
> > +	.init = grace_init_net,
> > +	.exit = grace_exit_net,
> > +	.id   = &grace_net_id,
> > +	.size = sizeof(struct list_head),
> > +};
> > +
> > +static int __init
> > +init_grace(void)
> > +{
> > +	return register_pernet_subsys(&grace_net_ops);
> > +}
> > +
> > +static void __exit
> > +exit_grace(void)
> > +{
> > +	unregister_pernet_subsys(&grace_net_ops);
> > +}
> > +
> > +MODULE_AUTHOR("Jeff Layton <jlayton@primarydata.com>");
> > +MODULE_LICENSE("GPL");
> > +module_init(init_grace)
> > +module_exit(exit_grace)
> > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > index f994e750e0d1..4fa98764de21 100644
> > --- a/fs/nfsd/Kconfig
> > +++ b/fs/nfsd/Kconfig
> > @@ -71,6 +71,7 @@ config NFSD_V4
> >  	select FS_POSIX_ACL
> >  	select SUNRPC_GSS
> >  	select CRYPTO
> > +	select GRACE_PERIOD
> >  	help
> >  	  This option enables support in your system's NFS server for
> >  	  version 4 of the NFS protocol (RFC 3530).
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index 9d117f61d976..b97bf2ef996e 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> >  
> >  #endif /* CONFIG_PROC_FS */
> >  
> > +struct net;
> > +
> >  static inline struct proc_dir_entry *proc_net_mkdir(
> >  	struct net *net, const char *name, struct proc_dir_entry *parent)
> >  {
> > -- 
> > 1.9.3
> > 

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[Jeff Layton]

On Thu, 28 Aug 2014 16:24:43 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > Currently, all of the grace period handling is part of lockd. Eventually
> > > though we'd like to be able to build v4-only servers, at which point
> > > we'll need to put all of this elsewhere.
> > > 
> > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > enable it automatically.
> > 
> > Thanks, applying this one for 3.18 indepedently of the others.
> 
> This code should also be fixed, though.
> 
> Currently nfsd is recording the grace period as done when its own timer
> runs out, but then it continuing to accept reclaims until lockd is also
> done.
> 
> This is an unusual bug to actually hit in part because lockd's grace
> period is by default less than nfsd's.
> 
> Your code introduces new cases when nfsd's grace period could end before
> lockd's, but in all of them future reclaims would be denied (because the
> only clients active on last shutdown would be 4.1 clients that have all
> issued RECLAIM_COMPLETE, so reclaims are all going to be denied).
> 
> So maybe it's not urgent.
> 
> --b.
> 

Yeah, I don't really see a bug here...

We'll only lift the nfsd grace period once all the v4.1 clients have
sent a RECLAIM_COMPLETE and if there aren't any v4.0 ones. At that
point, no one should be sending any reclaims but we'll still allow them
if the lockd grace period hasn't been lifted yet.

I suppose that we could deny them in principle, but there doesn't seem
to be any real harm in allowing it to happen. The simple "fix" for it
would be to have nfs4_check_open_reclaim do a test for
NFSD4_CLIENT_RECLAIM_COMPLETE, and reject the reclaim request if it has
been set.

It also occurs to me that we probably ought to have the nfsdcltrack
upcall set and test NFSD4_CLIENT_STABLE to reduce upcalls. The fact
that we don't do that there seems inefficient (though it shouldn't harm
correctness).

> > 
> > --b.
> > 
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > > ---
> > >  fs/Kconfig                       |  6 +++-
> > >  fs/lockd/Makefile                |  2 +-
> > >  fs/lockd/netns.h                 |  1 -
> > >  fs/lockd/svc.c                   |  1 -
> > >  fs/nfs_common/Makefile           |  3 +-
> > >  fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
> > >  fs/nfsd/Kconfig                  |  1 +
> > >  include/linux/proc_fs.h          |  2 ++
> > >  8 files changed, 69 insertions(+), 15 deletions(-)
> > >  rename fs/{lockd => nfs_common}/grace.c (50%)
> > > 
> > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > index 312393f32948..db5dc1598716 100644
> > > --- a/fs/Kconfig
> > > +++ b/fs/Kconfig
> > > @@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
> > >  source "fs/nfs/Kconfig"
> > >  source "fs/nfsd/Kconfig"
> > >  
> > > +config GRACE_PERIOD
> > > +	tristate
> > > +
> > >  config LOCKD
> > >  	tristate
> > >  	depends on FILE_LOCKING
> > > +	select GRACE_PERIOD
> > >  
> > >  config LOCKD_V4
> > >  	bool
> > > @@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
> > >  
> > >  config NFS_COMMON
> > >  	bool
> > > -	depends on NFSD || NFS_FS
> > > +	depends on NFSD || NFS_FS || LOCKD
> > >  	default y
> > >  
> > >  source "net/sunrpc/Kconfig"
> > > diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
> > > index ca58d64374ca..6a0b351ce30e 100644
> > > --- a/fs/lockd/Makefile
> > > +++ b/fs/lockd/Makefile
> > > @@ -5,6 +5,6 @@
> > >  obj-$(CONFIG_LOCKD) += lockd.o
> > >  
> > >  lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
> > > -	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
> > > +	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
> > >  lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
> > >  lockd-objs		      := $(lockd-objs-y)
> > > diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> > > index 5010b55628b4..097bfa3adb1c 100644
> > > --- a/fs/lockd/netns.h
> > > +++ b/fs/lockd/netns.h
> > > @@ -11,7 +11,6 @@ struct lockd_net {
> > >  
> > >  	struct delayed_work grace_period_end;
> > >  	struct lock_manager lockd_manager;
> > > -	struct list_head grace_list;
> > >  
> > >  	spinlock_t nsm_clnt_lock;
> > >  	unsigned int nsm_users;
> > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > > index 8f27c93f8d2e..3599c9ca28ae 100644
> > > --- a/fs/lockd/svc.c
> > > +++ b/fs/lockd/svc.c
> > > @@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
> > >  	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > >  
> > >  	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> > > -	INIT_LIST_HEAD(&ln->grace_list);
> > >  	spin_lock_init(&ln->nsm_clnt_lock);
> > >  	return 0;
> > >  }
> > > diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> > > index f689ed82af3a..d153ca3ea577 100644
> > > --- a/fs/nfs_common/Makefile
> > > +++ b/fs/nfs_common/Makefile
> > > @@ -3,5 +3,6 @@
> > >  #
> > >  
> > >  obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> > > -
> > >  nfs_acl-objs := nfsacl.o
> > > +
> > > +obj-$(CONFIG_GRACE_PERIOD) += grace.o
> > > diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
> > > similarity index 50%
> > > rename from fs/lockd/grace.c
> > > rename to fs/nfs_common/grace.c
> > > index 6d1ee7204c88..ae6e58ea4de5 100644
> > > --- a/fs/lockd/grace.c
> > > +++ b/fs/nfs_common/grace.c
> > > @@ -1,17 +1,20 @@
> > >  /*
> > >   * Common code for control of lockd and nfsv4 grace periods.
> > > + *
> > > + * Transplanted from lockd code
> > >   */
> > >  
> > >  #include <linux/module.h>
> > > -#include <linux/lockd/bind.h>
> > >  #include <net/net_namespace.h>
> > > +#include <net/netns/generic.h>
> > > +#include <linux/fs.h>
> > >  
> > > -#include "netns.h"
> > > -
> > > +static int grace_net_id;
> > >  static DEFINE_SPINLOCK(grace_lock);
> > >  
> > >  /**
> > >   * locks_start_grace
> > > + * @net: net namespace that this lock manager belongs to
> > >   * @lm: who this grace period is for
> > >   *
> > >   * A grace period is a period during which locks should not be given
> > > @@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
> > >   *
> > >   * This function is called to start a grace period.
> > >   */
> > > -void locks_start_grace(struct net *net, struct lock_manager *lm)
> > > +void
> > > +locks_start_grace(struct net *net, struct lock_manager *lm)
> > >  {
> > > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > >  
> > >  	spin_lock(&grace_lock);
> > > -	list_add(&lm->list, &ln->grace_list);
> > > +	list_add(&lm->list, grace_list);
> > >  	spin_unlock(&grace_lock);
> > >  }
> > >  EXPORT_SYMBOL_GPL(locks_start_grace);
> > >  
> > >  /**
> > >   * locks_end_grace
> > > + * @net: net namespace that this lock manager belongs to
> > >   * @lm: who this grace period is for
> > >   *
> > >   * Call this function to state that the given lock manager is ready to
> > > @@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> > >   * Note that callers count on it being safe to call this more than once,
> > >   * and the second call should be a no-op.
> > >   */
> > > -void locks_end_grace(struct lock_manager *lm)
> > > +void
> > > +locks_end_grace(struct lock_manager *lm)
> > >  {
> > >  	spin_lock(&grace_lock);
> > >  	list_del_init(&lm->list);
> > > @@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> > >   * to answer ordinary lock requests, and when they should accept only
> > >   * lock reclaims.
> > >   */
> > > -int locks_in_grace(struct net *net)
> > > +int
> > > +locks_in_grace(struct net *net)
> > >  {
> > > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > >  
> > > -	return !list_empty(&ln->grace_list);
> > > +	return !list_empty(grace_list);
> > >  }
> > >  EXPORT_SYMBOL_GPL(locks_in_grace);
> > > +
> > > +static int __net_init
> > > +grace_init_net(struct net *net)
> > > +{
> > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > +
> > > +	INIT_LIST_HEAD(grace_list);
> > > +	return 0;
> > > +}
> > > +
> > > +static void __net_exit
> > > +grace_exit_net(struct net *net)
> > > +{
> > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > +
> > > +	BUG_ON(!list_empty(grace_list));
> > > +}
> > > +
> > > +static struct pernet_operations grace_net_ops = {
> > > +	.init = grace_init_net,
> > > +	.exit = grace_exit_net,
> > > +	.id   = &grace_net_id,
> > > +	.size = sizeof(struct list_head),
> > > +};
> > > +
> > > +static int __init
> > > +init_grace(void)
> > > +{
> > > +	return register_pernet_subsys(&grace_net_ops);
> > > +}
> > > +
> > > +static void __exit
> > > +exit_grace(void)
> > > +{
> > > +	unregister_pernet_subsys(&grace_net_ops);
> > > +}
> > > +
> > > +MODULE_AUTHOR("Jeff Layton <jlayton@primarydata.com>");
> > > +MODULE_LICENSE("GPL");
> > > +module_init(init_grace)
> > > +module_exit(exit_grace)
> > > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > > index f994e750e0d1..4fa98764de21 100644
> > > --- a/fs/nfsd/Kconfig
> > > +++ b/fs/nfsd/Kconfig
> > > @@ -71,6 +71,7 @@ config NFSD_V4
> > >  	select FS_POSIX_ACL
> > >  	select SUNRPC_GSS
> > >  	select CRYPTO
> > > +	select GRACE_PERIOD
> > >  	help
> > >  	  This option enables support in your system's NFS server for
> > >  	  version 4 of the NFS protocol (RFC 3530).
> > > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > > index 9d117f61d976..b97bf2ef996e 100644
> > > --- a/include/linux/proc_fs.h
> > > +++ b/include/linux/proc_fs.h
> > > @@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> > >  
> > >  #endif /* CONFIG_PROC_FS */
> > >  
> > > +struct net;
> > > +
> > >  static inline struct proc_dir_entry *proc_net_mkdir(
> > >  	struct net *net, const char *name, struct proc_dir_entry *parent)
> > >  {
> > > -- 
> > > 1.9.3
> > > 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Thu, Aug 28, 2014 at 07:53:26PM -0400, Jeff Layton wrote:
> On Thu, 28 Aug 2014 16:24:43 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > > Currently, all of the grace period handling is part of lockd. Eventually
> > > > though we'd like to be able to build v4-only servers, at which point
> > > > we'll need to put all of this elsewhere.
> > > > 
> > > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > > enable it automatically.
> > > 
> > > Thanks, applying this one for 3.18 indepedently of the others.
> > 
> > This code should also be fixed, though.
> > 
> > Currently nfsd is recording the grace period as done when its own timer
> > runs out, but then it continuing to accept reclaims until lockd is also
> > done.
> > 
> > This is an unusual bug to actually hit in part because lockd's grace
> > period is by default less than nfsd's.
> > 
> > Your code introduces new cases when nfsd's grace period could end before
> > lockd's, but in all of them future reclaims would be denied (because the
> > only clients active on last shutdown would be 4.1 clients that have all
> > issued RECLAIM_COMPLETE, so reclaims are all going to be denied).
> > 
> > So maybe it's not urgent.
> > 
> > --b.
> > 
> 
> Yeah, I don't really see a bug here...
> 
> We'll only lift the nfsd grace period once all the v4.1 clients have
> sent a RECLAIM_COMPLETE and if there aren't any v4.0 ones. At that
> point, no one should be sending any reclaims but we'll still allow them
> if the lockd grace period hasn't been lifted yet.
> 
> I suppose that we could deny them in principle, but there doesn't seem
> to be any real harm in allowing it to happen. The simple "fix" for it
> would be to have nfs4_check_open_reclaim do a test for
> NFSD4_CLIENT_RECLAIM_COMPLETE, and reject the reclaim request if it has
> been set.

Oops, I didn't realize we don't already do that.  I don't know that such
reclaims are really harmless.  But since only a very broken client would
send them maybe it's not a big deal that we don't catch them.  Still,
it's probably a bug not to check for that.

Anyway, we still do have the preexisting bug in the (non-default) case
where the nfsd grace period ends first.

--b.

> It also occurs to me that we probably ought to have the nfsdcltrack
> upcall set and test NFSD4_CLIENT_STABLE to reduce upcalls. The fact
> that we don't do that there seems inefficient (though it shouldn't harm
> correctness).
> 
> > > 
> > > --b.
> > > 
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > > > ---
> > > >  fs/Kconfig                       |  6 +++-
> > > >  fs/lockd/Makefile                |  2 +-
> > > >  fs/lockd/netns.h                 |  1 -
> > > >  fs/lockd/svc.c                   |  1 -
> > > >  fs/nfs_common/Makefile           |  3 +-
> > > >  fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
> > > >  fs/nfsd/Kconfig                  |  1 +
> > > >  include/linux/proc_fs.h          |  2 ++
> > > >  8 files changed, 69 insertions(+), 15 deletions(-)
> > > >  rename fs/{lockd => nfs_common}/grace.c (50%)
> > > > 
> > > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > > index 312393f32948..db5dc1598716 100644
> > > > --- a/fs/Kconfig
> > > > +++ b/fs/Kconfig
> > > > @@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
> > > >  source "fs/nfs/Kconfig"
> > > >  source "fs/nfsd/Kconfig"
> > > >  
> > > > +config GRACE_PERIOD
> > > > +	tristate
> > > > +
> > > >  config LOCKD
> > > >  	tristate
> > > >  	depends on FILE_LOCKING
> > > > +	select GRACE_PERIOD
> > > >  
> > > >  config LOCKD_V4
> > > >  	bool
> > > > @@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
> > > >  
> > > >  config NFS_COMMON
> > > >  	bool
> > > > -	depends on NFSD || NFS_FS
> > > > +	depends on NFSD || NFS_FS || LOCKD
> > > >  	default y
> > > >  
> > > >  source "net/sunrpc/Kconfig"
> > > > diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
> > > > index ca58d64374ca..6a0b351ce30e 100644
> > > > --- a/fs/lockd/Makefile
> > > > +++ b/fs/lockd/Makefile
> > > > @@ -5,6 +5,6 @@
> > > >  obj-$(CONFIG_LOCKD) += lockd.o
> > > >  
> > > >  lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
> > > > -	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
> > > > +	        svcshare.o svcproc.o svcsubs.o mon.o xdr.o
> > > >  lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
> > > >  lockd-objs		      := $(lockd-objs-y)
> > > > diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> > > > index 5010b55628b4..097bfa3adb1c 100644
> > > > --- a/fs/lockd/netns.h
> > > > +++ b/fs/lockd/netns.h
> > > > @@ -11,7 +11,6 @@ struct lockd_net {
> > > >  
> > > >  	struct delayed_work grace_period_end;
> > > >  	struct lock_manager lockd_manager;
> > > > -	struct list_head grace_list;
> > > >  
> > > >  	spinlock_t nsm_clnt_lock;
> > > >  	unsigned int nsm_users;
> > > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > > > index 8f27c93f8d2e..3599c9ca28ae 100644
> > > > --- a/fs/lockd/svc.c
> > > > +++ b/fs/lockd/svc.c
> > > > @@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
> > > >  	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > >  
> > > >  	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> > > > -	INIT_LIST_HEAD(&ln->grace_list);
> > > >  	spin_lock_init(&ln->nsm_clnt_lock);
> > > >  	return 0;
> > > >  }
> > > > diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> > > > index f689ed82af3a..d153ca3ea577 100644
> > > > --- a/fs/nfs_common/Makefile
> > > > +++ b/fs/nfs_common/Makefile
> > > > @@ -3,5 +3,6 @@
> > > >  #
> > > >  
> > > >  obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> > > > -
> > > >  nfs_acl-objs := nfsacl.o
> > > > +
> > > > +obj-$(CONFIG_GRACE_PERIOD) += grace.o
> > > > diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
> > > > similarity index 50%
> > > > rename from fs/lockd/grace.c
> > > > rename to fs/nfs_common/grace.c
> > > > index 6d1ee7204c88..ae6e58ea4de5 100644
> > > > --- a/fs/lockd/grace.c
> > > > +++ b/fs/nfs_common/grace.c
> > > > @@ -1,17 +1,20 @@
> > > >  /*
> > > >   * Common code for control of lockd and nfsv4 grace periods.
> > > > + *
> > > > + * Transplanted from lockd code
> > > >   */
> > > >  
> > > >  #include <linux/module.h>
> > > > -#include <linux/lockd/bind.h>
> > > >  #include <net/net_namespace.h>
> > > > +#include <net/netns/generic.h>
> > > > +#include <linux/fs.h>
> > > >  
> > > > -#include "netns.h"
> > > > -
> > > > +static int grace_net_id;
> > > >  static DEFINE_SPINLOCK(grace_lock);
> > > >  
> > > >  /**
> > > >   * locks_start_grace
> > > > + * @net: net namespace that this lock manager belongs to
> > > >   * @lm: who this grace period is for
> > > >   *
> > > >   * A grace period is a period during which locks should not be given
> > > > @@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
> > > >   *
> > > >   * This function is called to start a grace period.
> > > >   */
> > > > -void locks_start_grace(struct net *net, struct lock_manager *lm)
> > > > +void
> > > > +locks_start_grace(struct net *net, struct lock_manager *lm)
> > > >  {
> > > > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > >  
> > > >  	spin_lock(&grace_lock);
> > > > -	list_add(&lm->list, &ln->grace_list);
> > > > +	list_add(&lm->list, grace_list);
> > > >  	spin_unlock(&grace_lock);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(locks_start_grace);
> > > >  
> > > >  /**
> > > >   * locks_end_grace
> > > > + * @net: net namespace that this lock manager belongs to
> > > >   * @lm: who this grace period is for
> > > >   *
> > > >   * Call this function to state that the given lock manager is ready to
> > > > @@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> > > >   * Note that callers count on it being safe to call this more than once,
> > > >   * and the second call should be a no-op.
> > > >   */
> > > > -void locks_end_grace(struct lock_manager *lm)
> > > > +void
> > > > +locks_end_grace(struct lock_manager *lm)
> > > >  {
> > > >  	spin_lock(&grace_lock);
> > > >  	list_del_init(&lm->list);
> > > > @@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> > > >   * to answer ordinary lock requests, and when they should accept only
> > > >   * lock reclaims.
> > > >   */
> > > > -int locks_in_grace(struct net *net)
> > > > +int
> > > > +locks_in_grace(struct net *net)
> > > >  {
> > > > -	struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > >  
> > > > -	return !list_empty(&ln->grace_list);
> > > > +	return !list_empty(grace_list);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(locks_in_grace);
> > > > +
> > > > +static int __net_init
> > > > +grace_init_net(struct net *net)
> > > > +{
> > > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > > +
> > > > +	INIT_LIST_HEAD(grace_list);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void __net_exit
> > > > +grace_exit_net(struct net *net)
> > > > +{
> > > > +	struct list_head *grace_list = net_generic(net, grace_net_id);
> > > > +
> > > > +	BUG_ON(!list_empty(grace_list));
> > > > +}
> > > > +
> > > > +static struct pernet_operations grace_net_ops = {
> > > > +	.init = grace_init_net,
> > > > +	.exit = grace_exit_net,
> > > > +	.id   = &grace_net_id,
> > > > +	.size = sizeof(struct list_head),
> > > > +};
> > > > +
> > > > +static int __init
> > > > +init_grace(void)
> > > > +{
> > > > +	return register_pernet_subsys(&grace_net_ops);
> > > > +}
> > > > +
> > > > +static void __exit
> > > > +exit_grace(void)
> > > > +{
> > > > +	unregister_pernet_subsys(&grace_net_ops);
> > > > +}
> > > > +
> > > > +MODULE_AUTHOR("Jeff Layton <jlayton@primarydata.com>");
> > > > +MODULE_LICENSE("GPL");
> > > > +module_init(init_grace)
> > > > +module_exit(exit_grace)
> > > > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > > > index f994e750e0d1..4fa98764de21 100644
> > > > --- a/fs/nfsd/Kconfig
> > > > +++ b/fs/nfsd/Kconfig
> > > > @@ -71,6 +71,7 @@ config NFSD_V4
> > > >  	select FS_POSIX_ACL
> > > >  	select SUNRPC_GSS
> > > >  	select CRYPTO
> > > > +	select GRACE_PERIOD
> > > >  	help
> > > >  	  This option enables support in your system's NFS server for
> > > >  	  version 4 of the NFS protocol (RFC 3530).
> > > > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > > > index 9d117f61d976..b97bf2ef996e 100644
> > > > --- a/include/linux/proc_fs.h
> > > > +++ b/include/linux/proc_fs.h
> > > > @@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> > > >  
> > > >  #endif /* CONFIG_PROC_FS */
> > > >  
> > > > +struct net;
> > > > +
> > > >  static inline struct proc_dir_entry *proc_net_mkdir(
> > > >  	struct net *net, const char *name, struct proc_dir_entry *parent)
> > > >  {
> > > > -- 
> > > > 1.9.3
> > > > 
> 
> 
> -- 
> Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 2/5] lockd: add a /proc/fs/lockd/nlm_end_grace file

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 02:38:26PM -0400, Jeff Layton wrote:
> Add a new procfile that will allow a (privileged) userland process to
> end the NLM grace period early. The basic idea here will be to have
> sm-notify write to this file, if it sent out no NOTIFY requests when
> it runs. In that situation, we can generally expect that there will be
> no reclaim requests so the grace period can be lifted early.
> 
> +static ssize_t
> +nlm_end_grace_write(struct file *file, const char __user *buf, size_t size,
> +		    loff_t *pos)
> +{
> +	struct lockd_net *ln = net_generic(current->nsproxy->net_ns, lockd_net_id);
> +
> +	if (size > 0)

I'd rather we expect some particular string ("Y\n"?) and return -EINVAL
otherwise, just in case we want to extend this some day.

--b.

Re: [PATCH v2 3/5] nfsd: add a v4_end_grace file to /proc/fs/nfsd

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 02:38:27PM -0400, Jeff Layton wrote:
> Allow a privileged userland process to end the v4 grace period early.
> Any write to the file will cause the v4 grace period to be lifted.
> The basic idea with this will be to allow the userland client tracking
> program to lift the grace period once it knows that no more clients
> will be reclaiming state.
...
> +/**
> + * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
> + *
> + * Input:
> + *			buf:		ignored
> + *			size:		zero
> + * OR
> + *
> + * Input:
> + * 			buf:		any value
> + *			size:		non-zero length of C string in @buf
> + * Output:
> + *			passed-in buffer filled with "Y" or "N" with a newline
> + *			and NULL-terminated C string. This indicates whether
> + *			the grace period has ended in the current net
> + *			namespace. Return code is the size in bytes of the
> + *			string. Writing to the file will end the grace period
> + *			for nfsd's v4 lock manager.
> + */
> +static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
> +{
> +	struct net *net = file->f_dentry->d_sb->s_fs_info;
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	if (size > 0)
> +		nfsd4_end_grace(nn);

Ditto for this one.

Do we really need separate files for nlm and nfsd?

I think the separate nlm and nfsd grace periods may just be a historical
mistake.

--b.

Re: [PATCH v2 4/5] nfsd: remove redundant boot_time parm from grace_done client tracking op

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 02:38:28PM -0400, Jeff Layton wrote:
> Since it's stored in nfsd_net, we don't need to pass it in separately.

OK.  I may as well take this for 3.18 now, I guess.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4recover.c | 17 ++++++++---------
>  fs/nfsd/nfs4state.c   |  2 +-
>  fs/nfsd/state.h       |  2 +-
>  3 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 9c271f42604a..a0d2ba956a3f 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -58,7 +58,7 @@ struct nfsd4_client_tracking_ops {
>  	void (*create)(struct nfs4_client *);
>  	void (*remove)(struct nfs4_client *);
>  	int (*check)(struct nfs4_client *);
> -	void (*grace_done)(struct nfsd_net *, time_t);
> +	void (*grace_done)(struct nfsd_net *);
>  };
>  
>  /* Globals */
> @@ -392,7 +392,7 @@ purge_old(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
>  }
>  
>  static void
> -nfsd4_recdir_purge_old(struct nfsd_net *nn, time_t boot_time)
> +nfsd4_recdir_purge_old(struct nfsd_net *nn)
>  {
>  	int status;
>  
> @@ -1016,7 +1016,7 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  }
>  
>  static void
> -nfsd4_cld_grace_done(struct nfsd_net *nn, time_t boot_time)
> +nfsd4_cld_grace_done(struct nfsd_net *nn)
>  {
>  	int ret;
>  	struct cld_upcall *cup;
> @@ -1029,7 +1029,7 @@ nfsd4_cld_grace_done(struct nfsd_net *nn, time_t boot_time)
>  	}
>  
>  	cup->cu_msg.cm_cmd = Cld_GraceDone;
> -	cup->cu_msg.cm_u.cm_gracetime = (int64_t)boot_time;
> +	cup->cu_msg.cm_u.cm_gracetime = (int64_t)nn->boot_time;
>  	ret = cld_pipe_upcall(cn->cn_pipe, &cup->cu_msg);
>  	if (!ret)
>  		ret = cup->cu_msg.cm_status;
> @@ -1245,13 +1245,12 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
>  }
>  
>  static void
> -nfsd4_umh_cltrack_grace_done(struct nfsd_net __attribute__((unused)) *nn,
> -				time_t boot_time)
> +nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
>  {
>  	char *legacy;
>  	char timestr[22]; /* FIXME: better way to determine max size? */
>  
> -	sprintf(timestr, "%ld", boot_time);
> +	sprintf(timestr, "%ld", nn->boot_time);
>  	legacy = nfsd4_cltrack_legacy_topdir();
>  	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
>  	kfree(legacy);
> @@ -1356,10 +1355,10 @@ nfsd4_client_record_check(struct nfs4_client *clp)
>  }
>  
>  void
> -nfsd4_record_grace_done(struct nfsd_net *nn, time_t boot_time)
> +nfsd4_record_grace_done(struct nfsd_net *nn)
>  {
>  	if (nn->client_tracking_ops)
> -		nn->client_tracking_ops->grace_done(nn, boot_time);
> +		nn->client_tracking_ops->grace_done(nn);
>  }
>  
>  static int
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 711280e0e4ac..21becb29dae1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4116,7 +4116,7 @@ nfsd4_end_grace(struct nfsd_net *nn)
>  
>  	dprintk("NFSD: end of grace period\n");
>  	nn->grace_ended = true;
> -	nfsd4_record_grace_done(nn, nn->boot_time);
> +	nfsd4_record_grace_done(nn);
>  	locks_end_grace(&nn->nfsd4_manager);
>  	/*
>  	 * Now that every NFSv4 client has had the chance to recover and
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index ecf579904892..854f0c574ccf 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -554,7 +554,7 @@ extern void nfsd4_client_tracking_exit(struct net *net);
>  extern void nfsd4_client_record_create(struct nfs4_client *clp);
>  extern void nfsd4_client_record_remove(struct nfs4_client *clp);
>  extern int nfsd4_client_record_check(struct nfs4_client *clp);
> -extern void nfsd4_record_grace_done(struct nfsd_net *nn, time_t boot_time);
> +extern void nfsd4_record_grace_done(struct nfsd_net *nn);
>  
>  /* nfs fault injection functions */
>  #ifdef CONFIG_NFSD_FAULT_INJECTION
> -- 
> 1.9.3
> 

Re: [PATCH v2 5/5] nfsd: pass extra info in env vars to upcalls to allow for early grace period end

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 02:38:29PM -0400, Jeff Layton wrote:
> In order to support lifting the grace period early, we must tell
> nfsdcltrack what sort of client the "create" upcall is for. We can't
> reliably tell if a v4.0 client has completed reclaiming, so we can only
> lift the grace period once all the v4.1+ clients have issued a
> RECLAIM_COMPLETE and if there are no v4.0 clients.

We really only care about 4.0 vs 4.1 clients.  Maybe
NFSDCLTRACK_CLIENT_WILL_RECLAIM_COMPLETE=Y|N would be better.

--b.

> 
> Also, in order to lift the grace period, we have to tell userland when
> the grace period started so that it can tell whether a RECLAIM_COMPLETE
> has been issued for each client since then.
> 
> Since this is all optional info, we pass it along in environment
> variables to the "init" and "create" upcalls. By doing this, we don't
> need to revise the upcall format. The UMH upcall can simply make use of
> this info if it happens to be present. If it's not then it can just
> avoid lifting the grace period early.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-------
>  fs/nfsd/nfs4state.c   |  4 +--
>  2 files changed, 80 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index a0d2ba956a3f..2b61bcd92b58 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -1062,6 +1062,8 @@ MODULE_PARM_DESC(cltrack_legacy_disable,
>  
>  #define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
>  #define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
> +#define CLIENT_MINORVERS_ENV_PREFIX "NFSDCLTRACK_CLIENT_MINORVERSION="
> +#define GRACE_START_ENV_PREFIX "NFSDCLTRACK_GRACE_START="
>  
>  static char *
>  nfsd4_cltrack_legacy_topdir(void)
> @@ -1126,10 +1128,60 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
>  	return result;
>  }
>  
> +static char *
> +nfsd4_cltrack_client_minorversion(struct nfs4_client *clp)
> +{
> +	int copied;
> +	size_t len;
> +	char *result;
> +
> +	/* prefix + max width of integer string + terminating NULL */
> +	len = strlen(CLIENT_MINORVERS_ENV_PREFIX) + 10 + 1;
> +
> +	result = kmalloc(len, GFP_KERNEL);
> +	if (!result)
> +		return result;
> +
> +	copied = snprintf(result, len, CLIENT_MINORVERS_ENV_PREFIX "%u",
> +				clp->cl_minorversion);
> +	if (copied >= len) {
> +		/* just return nothing if output was truncated */
> +		kfree(result);
> +		return NULL;
> +	}
> +
> +	return result;
> +}
> +
> +static char *
> +nfsd4_cltrack_grace_start(time_t grace_start)
> +{
> +	int copied;
> +	size_t len;
> +	char *result;
> +
> +	/* prefix + max width of int64_t string + terminating NULL */
> +	len = strlen(GRACE_START_ENV_PREFIX) + 22 + 1;
> +
> +	result = kmalloc(len, GFP_KERNEL);
> +	if (!result)
> +		return result;
> +
> +	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%ld",
> +				grace_start);
> +	if (copied >= len) {
> +		/* just return nothing if output was truncated */
> +		kfree(result);
> +		return NULL;
> +	}
> +
> +	return result;
> +}
> +
>  static int
> -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
> +nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
>  {
> -	char *envp[2];
> +	char *envp[3];
>  	char *argv[4];
>  	int ret;
>  
> @@ -1140,10 +1192,12 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
>  
>  	dprintk("%s: cmd: %s\n", __func__, cmd);
>  	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
> -	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
> +	dprintk("%s: env0: %s\n", __func__, env0 ? env0 : "(null)");
> +	dprintk("%s: env1: %s\n", __func__, env1 ? env1 : "(null)");
>  
> -	envp[0] = legacy;
> -	envp[1] = NULL;
> +	envp[0] = env0;
> +	envp[1] = env1;
> +	envp[2] = NULL;
>  
>  	argv[0] = (char *)cltrack_prog;
>  	argv[1] = cmd;
> @@ -1187,28 +1241,40 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
>  }
>  
>  static int
> -nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
> +nfsd4_umh_cltrack_init(struct net *net)
>  {
> +	int ret;
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> +
>  	/* XXX: The usermode helper s not working in container yet. */
>  	if (net != &init_net) {
>  		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
>  			"tracking in a container!\n");
>  		return -EINVAL;
>  	}
> -	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
> +
> +	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
> +	kfree(grace_start);
> +	return ret;
>  }
>  
>  static void
>  nfsd4_umh_cltrack_create(struct nfs4_client *clp)
>  {
> -	char *hexid;
> +	char *hexid, *minorvers, *grace_start;
> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  
>  	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
>  	if (!hexid) {
>  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
>  		return;
>  	}
> -	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
> +	minorvers = nfsd4_cltrack_client_minorversion(clp);
> +	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> +	nfsd4_umh_cltrack_upcall("create", hexid, minorvers, grace_start);
> +	kfree(minorvers);
> +	kfree(grace_start);
>  	kfree(hexid);
>  }
>  
> @@ -1222,7 +1288,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
>  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
>  		return;
>  	}
> -	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
> +	nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL);
>  	kfree(hexid);
>  }
>  
> @@ -1238,7 +1304,7 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
>  		return -ENOMEM;
>  	}
>  	legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name);
> -	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
> +	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL);
>  	kfree(legacy);
>  	kfree(hexid);
>  	return ret;
> @@ -1252,7 +1318,7 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
>  
>  	sprintf(timestr, "%ld", nn->boot_time);
>  	legacy = nfsd4_cltrack_legacy_topdir();
> -	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
> +	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
>  	kfree(legacy);
>  }
>  
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 21becb29dae1..5b04353bf2ec 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6342,10 +6342,10 @@ nfs4_state_start_net(struct net *net)
>  	ret = nfs4_state_create_net(net);
>  	if (ret)
>  		return ret;
> -	nfsd4_client_tracking_init(net);
>  	nn->boot_time = get_seconds();
> -	locks_start_grace(net, &nn->nfsd4_manager);
>  	nn->grace_ended = false;
> +	locks_start_grace(net, &nn->nfsd4_manager);
> +	nfsd4_client_tracking_init(net);
>  	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
>  	       nn->nfsd4_grace, net);
>  	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
> -- 
> 1.9.3
> 

Re: [PATCH v2 3/5] nfsd: add a v4_end_grace file to /proc/fs/nfsd

[Jeff Layton]

On Thu, 4 Sep 2014 15:54:20 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Aug 19, 2014 at 02:38:27PM -0400, Jeff Layton wrote:
> > Allow a privileged userland process to end the v4 grace period early.
> > Any write to the file will cause the v4 grace period to be lifted.
> > The basic idea with this will be to allow the userland client tracking
> > program to lift the grace period once it knows that no more clients
> > will be reclaiming state.
> ...
> > +/**
> > + * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
> > + *
> > + * Input:
> > + *			buf:		ignored
> > + *			size:		zero
> > + * OR
> > + *
> > + * Input:
> > + * 			buf:		any value
> > + *			size:		non-zero length of C string in @buf
> > + * Output:
> > + *			passed-in buffer filled with "Y" or "N" with a newline
> > + *			and NULL-terminated C string. This indicates whether
> > + *			the grace period has ended in the current net
> > + *			namespace. Return code is the size in bytes of the
> > + *			string. Writing to the file will end the grace period
> > + *			for nfsd's v4 lock manager.
> > + */
> > +static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
> > +{
> > +	struct net *net = file->f_dentry->d_sb->s_fs_info;
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +
> > +	if (size > 0)
> > +		nfsd4_end_grace(nn);
> 
> Ditto for this one.
> 

Sure. I'll fix that up in the next iteration.

> Do we really need separate files for nlm and nfsd?
> 
> I think the separate nlm and nfsd grace periods may just be a historical
> mistake.
> 

statd and nfsdcltrack are separate programs, and both will need to
"sign off" before we can lift the grace period. With the way that grace
period management works today, I don't see a way to do this without
separate files. If you have an idea in mind for how to unify this
interface then I'm happy to entertain it however...

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 5/5] nfsd: pass extra info in env vars to upcalls to allow for early grace period end

[Jeff Layton]

On Thu, 4 Sep 2014 15:59:14 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Aug 19, 2014 at 02:38:29PM -0400, Jeff Layton wrote:
> > In order to support lifting the grace period early, we must tell
> > nfsdcltrack what sort of client the "create" upcall is for. We can't
> > reliably tell if a v4.0 client has completed reclaiming, so we can only
> > lift the grace period once all the v4.1+ clients have issued a
> > RECLAIM_COMPLETE and if there are no v4.0 clients.
> 
> We really only care about 4.0 vs 4.1 clients.  Maybe
> NFSDCLTRACK_CLIENT_WILL_RECLAIM_COMPLETE=Y|N would be better.
> 
> --b.
> 

We have v4.2 in the queue now, and v4.3 will eventually come later.
Maybe v4.4 or v4.5 will change how recovery works? I figured it was
better to pass down more specific info and let userland sort out
what to do with it.

That said, if you think that a simple y/n flag is better, then I'll go
with that.

> > 
> > Also, in order to lift the grace period, we have to tell userland when
> > the grace period started so that it can tell whether a RECLAIM_COMPLETE
> > has been issued for each client since then.
> > 
> > Since this is all optional info, we pass it along in environment
> > variables to the "init" and "create" upcalls. By doing this, we don't
> > need to revise the upcall format. The UMH upcall can simply make use of
> > this info if it happens to be present. If it's not then it can just
> > avoid lifting the grace period early.
> > 
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-------
> >  fs/nfsd/nfs4state.c   |  4 +--
> >  2 files changed, 80 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index a0d2ba956a3f..2b61bcd92b58 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -1062,6 +1062,8 @@ MODULE_PARM_DESC(cltrack_legacy_disable,
> >  
> >  #define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
> >  #define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
> > +#define CLIENT_MINORVERS_ENV_PREFIX "NFSDCLTRACK_CLIENT_MINORVERSION="
> > +#define GRACE_START_ENV_PREFIX "NFSDCLTRACK_GRACE_START="
> >  
> >  static char *
> >  nfsd4_cltrack_legacy_topdir(void)
> > @@ -1126,10 +1128,60 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
> >  	return result;
> >  }
> >  
> > +static char *
> > +nfsd4_cltrack_client_minorversion(struct nfs4_client *clp)
> > +{
> > +	int copied;
> > +	size_t len;
> > +	char *result;
> > +
> > +	/* prefix + max width of integer string + terminating NULL */
> > +	len = strlen(CLIENT_MINORVERS_ENV_PREFIX) + 10 + 1;
> > +
> > +	result = kmalloc(len, GFP_KERNEL);
> > +	if (!result)
> > +		return result;
> > +
> > +	copied = snprintf(result, len, CLIENT_MINORVERS_ENV_PREFIX "%u",
> > +				clp->cl_minorversion);
> > +	if (copied >= len) {
> > +		/* just return nothing if output was truncated */
> > +		kfree(result);
> > +		return NULL;
> > +	}
> > +
> > +	return result;
> > +}
> > +
> > +static char *
> > +nfsd4_cltrack_grace_start(time_t grace_start)
> > +{
> > +	int copied;
> > +	size_t len;
> > +	char *result;
> > +
> > +	/* prefix + max width of int64_t string + terminating NULL */
> > +	len = strlen(GRACE_START_ENV_PREFIX) + 22 + 1;
> > +
> > +	result = kmalloc(len, GFP_KERNEL);
> > +	if (!result)
> > +		return result;
> > +
> > +	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%ld",
> > +				grace_start);
> > +	if (copied >= len) {
> > +		/* just return nothing if output was truncated */
> > +		kfree(result);
> > +		return NULL;
> > +	}
> > +
> > +	return result;
> > +}
> > +
> >  static int
> > -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
> > +nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> >  {
> > -	char *envp[2];
> > +	char *envp[3];
> >  	char *argv[4];
> >  	int ret;
> >  
> > @@ -1140,10 +1192,12 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
> >  
> >  	dprintk("%s: cmd: %s\n", __func__, cmd);
> >  	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
> > -	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
> > +	dprintk("%s: env0: %s\n", __func__, env0 ? env0 : "(null)");
> > +	dprintk("%s: env1: %s\n", __func__, env1 ? env1 : "(null)");
> >  
> > -	envp[0] = legacy;
> > -	envp[1] = NULL;
> > +	envp[0] = env0;
> > +	envp[1] = env1;
> > +	envp[2] = NULL;
> >  
> >  	argv[0] = (char *)cltrack_prog;
> >  	argv[1] = cmd;
> > @@ -1187,28 +1241,40 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
> >  }
> >  
> >  static int
> > -nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
> > +nfsd4_umh_cltrack_init(struct net *net)
> >  {
> > +	int ret;
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> > +
> >  	/* XXX: The usermode helper s not working in container yet. */
> >  	if (net != &init_net) {
> >  		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
> >  			"tracking in a container!\n");
> >  		return -EINVAL;
> >  	}
> > -	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
> > +
> > +	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
> > +	kfree(grace_start);
> > +	return ret;
> >  }
> >  
> >  static void
> >  nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> >  {
> > -	char *hexid;
> > +	char *hexid, *minorvers, *grace_start;
> > +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> >  
> >  	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
> >  	if (!hexid) {
> >  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> >  		return;
> >  	}
> > -	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
> > +	minorvers = nfsd4_cltrack_client_minorversion(clp);
> > +	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> > +	nfsd4_umh_cltrack_upcall("create", hexid, minorvers, grace_start);
> > +	kfree(minorvers);
> > +	kfree(grace_start);
> >  	kfree(hexid);
> >  }
> >  
> > @@ -1222,7 +1288,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> >  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> >  		return;
> >  	}
> > -	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
> > +	nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL);
> >  	kfree(hexid);
> >  }
> >  
> > @@ -1238,7 +1304,7 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
> >  		return -ENOMEM;
> >  	}
> >  	legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name);
> > -	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
> > +	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL);
> >  	kfree(legacy);
> >  	kfree(hexid);
> >  	return ret;
> > @@ -1252,7 +1318,7 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
> >  
> >  	sprintf(timestr, "%ld", nn->boot_time);
> >  	legacy = nfsd4_cltrack_legacy_topdir();
> > -	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
> > +	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
> >  	kfree(legacy);
> >  }
> >  
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 21becb29dae1..5b04353bf2ec 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6342,10 +6342,10 @@ nfs4_state_start_net(struct net *net)
> >  	ret = nfs4_state_create_net(net);
> >  	if (ret)
> >  		return ret;
> > -	nfsd4_client_tracking_init(net);
> >  	nn->boot_time = get_seconds();
> > -	locks_start_grace(net, &nn->nfsd4_manager);
> >  	nn->grace_ended = false;
> > +	locks_start_grace(net, &nn->nfsd4_manager);
> > +	nfsd4_client_tracking_init(net);
> >  	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> >  	       nn->nfsd4_grace, net);
> >  	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 5/5] nfsd: pass extra info in env vars to upcalls to allow for early grace period end

[J. Bruce Fields]

On Fri, Sep 05, 2014 at 07:43:00AM -0400, Jeff Layton wrote:
> On Thu, 4 Sep 2014 15:59:14 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Tue, Aug 19, 2014 at 02:38:29PM -0400, Jeff Layton wrote:
> > > In order to support lifting the grace period early, we must tell
> > > nfsdcltrack what sort of client the "create" upcall is for. We can't
> > > reliably tell if a v4.0 client has completed reclaiming, so we can only
> > > lift the grace period once all the v4.1+ clients have issued a
> > > RECLAIM_COMPLETE and if there are no v4.0 clients.
> > 
> > We really only care about 4.0 vs 4.1 clients.  Maybe
> > NFSDCLTRACK_CLIENT_WILL_RECLAIM_COMPLETE=Y|N would be better.
> > 
> > --b.
> > 
> 
> We have v4.2 in the queue now, and v4.3 will eventually come later.
> Maybe v4.4 or v4.5 will change how recovery works? I figured it was
> better to pass down more specific info and let userland sort out
> what to do with it.
> 
> That said, if you think that a simple y/n flag is better, then I'll go
> with that.

Yes, I'd prefer the single flag, thanks.  That doesn't rule out adding
more information later if you turn out to be right.

--b.

> 
> > > 
> > > Also, in order to lift the grace period, we have to tell userland when
> > > the grace period started so that it can tell whether a RECLAIM_COMPLETE
> > > has been issued for each client since then.
> > > 
> > > Since this is all optional info, we pass it along in environment
> > > variables to the "init" and "create" upcalls. By doing this, we don't
> > > need to revise the upcall format. The UMH upcall can simply make use of
> > > this info if it happens to be present. If it's not then it can just
> > > avoid lifting the grace period early.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > > ---
> > >  fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-------
> > >  fs/nfsd/nfs4state.c   |  4 +--
> > >  2 files changed, 80 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > index a0d2ba956a3f..2b61bcd92b58 100644
> > > --- a/fs/nfsd/nfs4recover.c
> > > +++ b/fs/nfsd/nfs4recover.c
> > > @@ -1062,6 +1062,8 @@ MODULE_PARM_DESC(cltrack_legacy_disable,
> > >  
> > >  #define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
> > >  #define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
> > > +#define CLIENT_MINORVERS_ENV_PREFIX "NFSDCLTRACK_CLIENT_MINORVERSION="
> > > +#define GRACE_START_ENV_PREFIX "NFSDCLTRACK_GRACE_START="
> > >  
> > >  static char *
> > >  nfsd4_cltrack_legacy_topdir(void)
> > > @@ -1126,10 +1128,60 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
> > >  	return result;
> > >  }
> > >  
> > > +static char *
> > > +nfsd4_cltrack_client_minorversion(struct nfs4_client *clp)
> > > +{
> > > +	int copied;
> > > +	size_t len;
> > > +	char *result;
> > > +
> > > +	/* prefix + max width of integer string + terminating NULL */
> > > +	len = strlen(CLIENT_MINORVERS_ENV_PREFIX) + 10 + 1;
> > > +
> > > +	result = kmalloc(len, GFP_KERNEL);
> > > +	if (!result)
> > > +		return result;
> > > +
> > > +	copied = snprintf(result, len, CLIENT_MINORVERS_ENV_PREFIX "%u",
> > > +				clp->cl_minorversion);
> > > +	if (copied >= len) {
> > > +		/* just return nothing if output was truncated */
> > > +		kfree(result);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	return result;
> > > +}
> > > +
> > > +static char *
> > > +nfsd4_cltrack_grace_start(time_t grace_start)
> > > +{
> > > +	int copied;
> > > +	size_t len;
> > > +	char *result;
> > > +
> > > +	/* prefix + max width of int64_t string + terminating NULL */
> > > +	len = strlen(GRACE_START_ENV_PREFIX) + 22 + 1;
> > > +
> > > +	result = kmalloc(len, GFP_KERNEL);
> > > +	if (!result)
> > > +		return result;
> > > +
> > > +	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%ld",
> > > +				grace_start);
> > > +	if (copied >= len) {
> > > +		/* just return nothing if output was truncated */
> > > +		kfree(result);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	return result;
> > > +}
> > > +
> > >  static int
> > > -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
> > > +nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> > >  {
> > > -	char *envp[2];
> > > +	char *envp[3];
> > >  	char *argv[4];
> > >  	int ret;
> > >  
> > > @@ -1140,10 +1192,12 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
> > >  
> > >  	dprintk("%s: cmd: %s\n", __func__, cmd);
> > >  	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
> > > -	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
> > > +	dprintk("%s: env0: %s\n", __func__, env0 ? env0 : "(null)");
> > > +	dprintk("%s: env1: %s\n", __func__, env1 ? env1 : "(null)");
> > >  
> > > -	envp[0] = legacy;
> > > -	envp[1] = NULL;
> > > +	envp[0] = env0;
> > > +	envp[1] = env1;
> > > +	envp[2] = NULL;
> > >  
> > >  	argv[0] = (char *)cltrack_prog;
> > >  	argv[1] = cmd;
> > > @@ -1187,28 +1241,40 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
> > >  }
> > >  
> > >  static int
> > > -nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
> > > +nfsd4_umh_cltrack_init(struct net *net)
> > >  {
> > > +	int ret;
> > > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > +	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> > > +
> > >  	/* XXX: The usermode helper s not working in container yet. */
> > >  	if (net != &init_net) {
> > >  		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
> > >  			"tracking in a container!\n");
> > >  		return -EINVAL;
> > >  	}
> > > -	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
> > > +
> > > +	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
> > > +	kfree(grace_start);
> > > +	return ret;
> > >  }
> > >  
> > >  static void
> > >  nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> > >  {
> > > -	char *hexid;
> > > +	char *hexid, *minorvers, *grace_start;
> > > +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> > >  
> > >  	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
> > >  	if (!hexid) {
> > >  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> > >  		return;
> > >  	}
> > > -	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
> > > +	minorvers = nfsd4_cltrack_client_minorversion(clp);
> > > +	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> > > +	nfsd4_umh_cltrack_upcall("create", hexid, minorvers, grace_start);
> > > +	kfree(minorvers);
> > > +	kfree(grace_start);
> > >  	kfree(hexid);
> > >  }
> > >  
> > > @@ -1222,7 +1288,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> > >  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> > >  		return;
> > >  	}
> > > -	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
> > > +	nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL);
> > >  	kfree(hexid);
> > >  }
> > >  
> > > @@ -1238,7 +1304,7 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
> > >  		return -ENOMEM;
> > >  	}
> > >  	legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name);
> > > -	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
> > > +	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL);
> > >  	kfree(legacy);
> > >  	kfree(hexid);
> > >  	return ret;
> > > @@ -1252,7 +1318,7 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
> > >  
> > >  	sprintf(timestr, "%ld", nn->boot_time);
> > >  	legacy = nfsd4_cltrack_legacy_topdir();
> > > -	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
> > > +	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
> > >  	kfree(legacy);
> > >  }
> > >  
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 21becb29dae1..5b04353bf2ec 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -6342,10 +6342,10 @@ nfs4_state_start_net(struct net *net)
> > >  	ret = nfs4_state_create_net(net);
> > >  	if (ret)
> > >  		return ret;
> > > -	nfsd4_client_tracking_init(net);
> > >  	nn->boot_time = get_seconds();
> > > -	locks_start_grace(net, &nn->nfsd4_manager);
> > >  	nn->grace_ended = false;
> > > +	locks_start_grace(net, &nn->nfsd4_manager);
> > > +	nfsd4_client_tracking_init(net);
> > >  	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> > >  	       nn->nfsd4_grace, net);
> > >  	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
> > > -- 
> > > 1.9.3
> > > 
> 
> 
> -- 
> Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields wrote:
> On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > Currently, all of the grace period handling is part of lockd. Eventually
> > > though we'd like to be able to build v4-only servers, at which point
> > > we'll need to put all of this elsewhere.
> > > 
> > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > enable it automatically.
> > 
> > Thanks, applying this one for 3.18 indepedently of the others.
> 
> This code should also be fixed, though.
> 
> Currently nfsd is recording the grace period as done when its own timer
> runs out, but then it continuing to accept reclaims until lockd is also
> done.

I sat down to fix this today then decided there's not a real bug:

All the grace_done upcall really does is throw away any previous clients
that haven't reclaimed yet.

It's legal to do that as soon as the correct amount of time has passed.
It's actually OK to continue to allow the grace period to run past that
point, the only requirement is that all reclaims precede all new opens,
which the code still does enforce.

So I think it might just be worth a little extra explanation.

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a298c3d..ca6f6ee 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4122,8 +4122,23 @@ nfsd4_end_grace(struct nfsd_net *nn)
 
 	dprintk("NFSD: end of grace period\n");
 	nn->grace_ended = true;
+	/*
+	 * If the server goes down again right now, an NFSv4
+	 * client will still be allowed to reclaim after it comes back up,
+	 * even if it hasn't yet had a chance to reclaim state this time.
+	 */
 	nfsd4_record_grace_done(nn);
+	/*
+	 * At this point, NFSv4 clients can still reclaim.  But if the
+	 * server crashes, any that have not yet reclaimed will be out
+	 * of luck on the next boot.
+	 */
 	locks_end_grace(&nn->nfsd4_manager);
+	/*
+	 * At this point, and once lockd and/or any other containers
+	 * exit their grace period, further reclaims aren't permitted
+	 * and regular locking can resume.
+	 */
 }
 
 static time_t

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Mon, Sep 15, 2014 at 06:08:13PM -0400, J. Bruce Fields wrote:
> On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields wrote:
> > On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > > Currently, all of the grace period handling is part of lockd. Eventually
> > > > though we'd like to be able to build v4-only servers, at which point
> > > > we'll need to put all of this elsewhere.
> > > > 
> > > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > > enable it automatically.
> > > 
> > > Thanks, applying this one for 3.18 indepedently of the others.
> > 
> > This code should also be fixed, though.
> > 
> > Currently nfsd is recording the grace period as done when its own timer
> > runs out, but then it continuing to accept reclaims until lockd is also
> > done.
> 
> I sat down to fix this today then decided there's not a real bug:
> 
> All the grace_done upcall really does is throw away any previous clients
> that haven't reclaimed yet.
> 
> It's legal to do that as soon as the correct amount of time has passed.
> It's actually OK to continue to allow the grace period to run past that
> point, the only requirement is that all reclaims precede all new opens,
> which the code still does enforce.
> 
> So I think it might just be worth a little extra explanation.

I considered doing something like the following anyway (total
off-the-cuff draft, not tested), but I think it's overkill.

--b.

commit 675121da48fb4d7d85b58467c7f79dd3a6964879
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Sep 15 17:42:14 2014 -0400

    nfsd4: end grace only when last grace period ends
    
    In the case both lockd and nfsd4 are running, or when we have multiple
    containers exporting the same filesystem, there may be multiple grace
    periods.  In that case the grace period doesn't really end till the last
    one does and though it's harmless to do the grace_done upcall before
    then, it would probably be better not to.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
index ae6e58e..dc67e5d 100644
--- a/fs/nfs_common/grace.c
+++ b/fs/nfs_common/grace.c
@@ -12,6 +12,11 @@
 static int grace_net_id;
 static DEFINE_SPINLOCK(grace_lock);
 
+struct grace_net {
+	struct list_head *grace_list;
+	bool in_grace;
+};
+
 /**
  * locks_start_grace
  * @net: net namespace that this lock manager belongs to
@@ -27,10 +32,10 @@ static DEFINE_SPINLOCK(grace_lock);
 void
 locks_start_grace(struct net *net, struct lock_manager *lm)
 {
-	struct list_head *grace_list = net_generic(net, grace_net_id);
+	struct list_head *gn = net_generic(net, grace_net_id);
 
 	spin_lock(&grace_lock);
-	list_add(&lm->list, grace_list);
+	list_add(&lm->list, &gn->grace_list);
 	spin_unlock(&grace_lock);
 }
 EXPORT_SYMBOL_GPL(locks_start_grace);
@@ -49,9 +54,31 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
 void
 locks_end_grace(struct lock_manager *lm)
 {
+	struct list_head *gn = net_generic(net, grace_net_id);
+
+	if (list_empty(&lm->list))
+		return;
 	spin_lock(&grace_lock);
 	list_del_init(&lm->list);
 	spin_unlock(&grace_lock);
+	if (list_empty(&gn->grace_list))
+		return;
+	/*
+	 * If the server goes down again right now, an NFSv4
+	 * client will still be reclaim after it comes back up, even if
+	 * it hasn't yet had a chance to reclaim state this time.
+	 */
+	lm->finish_grace(lm);
+	/*
+	 * At this point, NFSv4 clients can still reclaim.  But if the
+	 * server crashes, any that have not yet reclaimed will be out
+	 * of luck on the next boot.
+	 */
+	ln->in_grace = false;
+	/*
+	 * At this point, no reclaims are permitted.  Regular locking
+	 * can resume.
+	 */
 }
 EXPORT_SYMBOL_GPL(locks_end_grace);
 
@@ -65,27 +92,28 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
 int
 locks_in_grace(struct net *net)
 {
-	struct list_head *grace_list = net_generic(net, grace_net_id);
+	struct list_head *gn = net_generic(net, grace_net_id);
 
-	return !list_empty(grace_list);
+	return gn->in_grace;
 }
 EXPORT_SYMBOL_GPL(locks_in_grace);
 
 static int __net_init
 grace_init_net(struct net *net)
 {
-	struct list_head *grace_list = net_generic(net, grace_net_id);
+	struct list_head *gn = net_generic(net, grace_net_id);
 
-	INIT_LIST_HEAD(grace_list);
+	INIT_LIST_HEAD(&gn->grace_list);
+	gn->in_grace = true;
 	return 0;
 }
 
 static void __net_exit
 grace_exit_net(struct net *net)
 {
-	struct list_head *grace_list = net_generic(net, grace_net_id);
+	struct list_head *gn = net_generic(net, grace_net_id);
 
-	BUG_ON(!list_empty(grace_list));
+	BUG_ON(!list_empty(&gn->grace_list));
 }
 
 static struct pernet_operations grace_net_ops = {
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fc88b57..968acd0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4122,7 +4122,6 @@ nfsd4_end_grace(struct nfsd_net *nn)
 
 	dprintk("NFSD: end of grace period\n");
 	nn->grace_ended = true;
-	nfsd4_record_grace_done(nn);
 	locks_end_grace(&nn->nfsd4_manager);
 	/*
 	 * Now that every NFSv4 client has had the chance to recover and
@@ -6341,6 +6340,13 @@ nfs4_state_destroy_net(struct net *net)
 	put_net(net);
 }
 
+void nfsd4_finish_grace(struct lock_manager *lm)
+{
+	struct nfsd_net *nn = lm->private;
+
+	nfsd4_record_grace_done(nn);
+}
+
 int
 nfs4_state_start_net(struct net *net)
 {
@@ -6352,6 +6358,8 @@ nfs4_state_start_net(struct net *net)
 		return ret;
 	nn->boot_time = get_seconds();
 	nn->grace_ended = false;
+	nn->nfsd4_manager.finish_grace = nfsd4_finish_grace;
+	nn->nfsd4_manager.private = nn;
 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9418772..0e33e7c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -876,6 +876,8 @@ struct lock_manager_operations {
 
 struct lock_manager {
 	struct list_head list;
+	void (*finish_grace)(struct lock_manager *lm);
+	void *private;
 };
 
 struct net;

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Mon, Sep 15, 2014 at 06:09:39PM -0400, J. Bruce Fields wrote:
> I considered doing something like the following anyway (total
> off-the-cuff draft, not tested), but I think it's overkill.

Oh, and the one other nit here is this.

--b.

commit 24e24d2e8125d917a49b3df6a5248d6ca5c9592b
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Sep 15 11:05:46 2014 -0400

    nfsd4: stop grace_time update at end of grace period
    
    The attempt to automatically set a new grace period time at the end of
    the grace period isn't really helpful.  We'll probably shut down and
    reboot before we actually make use of the new grace period time anyway.
    So may as well leave it up to the init system to get this right.
    
    This just confuses people when they see /proc/fs/nfsd/nfsv4gracetime
    change from what they set it to.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fc88b57..a298c3d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4124,12 +4124,6 @@ nfsd4_end_grace(struct nfsd_net *nn)
 	nn->grace_ended = true;
 	nfsd4_record_grace_done(nn);
 	locks_end_grace(&nn->nfsd4_manager);
-	/*
-	 * Now that every NFSv4 client has had the chance to recover and
-	 * to see the (possibly new, possibly shorter) lease time, we
-	 * can safely set the next grace time to the current lease time:
-	 */
-	nn->nfsd4_grace = nn->nfsd4_lease;
 }
 
 static time_t

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[Jeff Layton]

On Mon, 15 Sep 2014 18:08:13 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields wrote:
> > On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > > Currently, all of the grace period handling is part of lockd. Eventually
> > > > though we'd like to be able to build v4-only servers, at which point
> > > > we'll need to put all of this elsewhere.
> > > > 
> > > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > > enable it automatically.
> > > 
> > > Thanks, applying this one for 3.18 indepedently of the others.
> > 
> > This code should also be fixed, though.
> > 
> > Currently nfsd is recording the grace period as done when its own timer
> > runs out, but then it continuing to accept reclaims until lockd is also
> > done.
> 
> I sat down to fix this today then decided there's not a real bug:
> 
> All the grace_done upcall really does is throw away any previous clients
> that haven't reclaimed yet.
> 
> It's legal to do that as soon as the correct amount of time has passed.
> It's actually OK to continue to allow the grace period to run past that
> point, the only requirement is that all reclaims precede all new opens,
> which the code still does enforce.
> 
> So I think it might just be worth a little extra explanation.
> 
> --b.
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a298c3d..ca6f6ee 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4122,8 +4122,23 @@ nfsd4_end_grace(struct nfsd_net *nn)
>  
>  	dprintk("NFSD: end of grace period\n");
>  	nn->grace_ended = true;
> +	/*
> +	 * If the server goes down again right now, an NFSv4
> +	 * client will still be allowed to reclaim after it comes back up,
> +	 * even if it hasn't yet had a chance to reclaim state this time.
> +	 */

Might even be reasonable to lay out the v4.0/v4.1+ difference here? For
a v4.0 client any lock reclaim will do, but for v4.1+ the client must
have _completed_ reclaim at this point (a'la RECLAIM_COMPLETE)

...or at least that'll be the situation once the patches I sent go in...
 
>  	nfsd4_record_grace_done(nn);
> +	/*
> +	 * At this point, NFSv4 clients can still reclaim.  But if
> the
> +	 * server crashes, any that have not yet reclaimed will be
> out
> +	 * of luck on the next boot.
> +	 */
>  	locks_end_grace(&nn->nfsd4_manager);
> +	/*
> +	 * At this point, and once lockd and/or any other containers
> +	 * exit their grace period, further reclaims aren't permitted
> +	 * and regular locking can resume.
> +	 */
>  }
>  
>  static time_t
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

ACK, overall. The comments make sense to me and it does clarify things
a bit.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[Jeff Layton]

On Mon, 15 Sep 2014 18:09:39 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Sep 15, 2014 at 06:08:13PM -0400, J. Bruce Fields wrote:
> > On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields wrote:
> > > On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > > > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > > > Currently, all of the grace period handling is part of lockd. Eventually
> > > > > though we'd like to be able to build v4-only servers, at which point
> > > > > we'll need to put all of this elsewhere.
> > > > > 
> > > > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > > > enable it automatically.
> > > > 
> > > > Thanks, applying this one for 3.18 indepedently of the others.
> > > 
> > > This code should also be fixed, though.
> > > 
> > > Currently nfsd is recording the grace period as done when its own timer
> > > runs out, but then it continuing to accept reclaims until lockd is also
> > > done.
> > 
> > I sat down to fix this today then decided there's not a real bug:
> > 
> > All the grace_done upcall really does is throw away any previous clients
> > that haven't reclaimed yet.
> > 
> > It's legal to do that as soon as the correct amount of time has passed.
> > It's actually OK to continue to allow the grace period to run past that
> > point, the only requirement is that all reclaims precede all new opens,
> > which the code still does enforce.
> > 
> > So I think it might just be worth a little extra explanation.
> 
> I considered doing something like the following anyway (total
> off-the-cuff draft, not tested), but I think it's overkill.
> 
> --b.
> 

Yeah, I tend to agree. I don't see a real need for it.

In principle, I guess you could end up in a situation where the lockd
and nfsd grace periods different by a large amount. In that case, you
could end up in a situation where you started denying reclaims for v4.x
clients that haven't reclaimed anything yet, but then still didn't
allow them to establish new locks either.

So it might be reasonable to add something like this, but I don't see
it as anything most setups would ever hit -- particularly not in sane
configurations.


> commit 675121da48fb4d7d85b58467c7f79dd3a6964879
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Mon Sep 15 17:42:14 2014 -0400
> 
>     nfsd4: end grace only when last grace period ends
>     
>     In the case both lockd and nfsd4 are running, or when we have multiple
>     containers exporting the same filesystem, there may be multiple grace
>     periods.  In that case the grace period doesn't really end till the last
>     one does and though it's harmless to do the grace_done upcall before
>     then, it would probably be better not to.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> index ae6e58e..dc67e5d 100644
> --- a/fs/nfs_common/grace.c
> +++ b/fs/nfs_common/grace.c
> @@ -12,6 +12,11 @@
>  static int grace_net_id;
>  static DEFINE_SPINLOCK(grace_lock);
>  
> +struct grace_net {
> +	struct list_head *grace_list;
> +	bool in_grace;
> +};
> +
>  /**
>   * locks_start_grace
>   * @net: net namespace that this lock manager belongs to
> @@ -27,10 +32,10 @@ static DEFINE_SPINLOCK(grace_lock);
>  void
>  locks_start_grace(struct net *net, struct lock_manager *lm)
>  {
> -	struct list_head *grace_list = net_generic(net, grace_net_id);
> +	struct list_head *gn = net_generic(net, grace_net_id);
>  
>  	spin_lock(&grace_lock);
> -	list_add(&lm->list, grace_list);
> +	list_add(&lm->list, &gn->grace_list);
>  	spin_unlock(&grace_lock);
>  }
>  EXPORT_SYMBOL_GPL(locks_start_grace);
> @@ -49,9 +54,31 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
>  void
>  locks_end_grace(struct lock_manager *lm)
>  {
> +	struct list_head *gn = net_generic(net, grace_net_id);
> +
> +	if (list_empty(&lm->list))
> +		return;
>  	spin_lock(&grace_lock);
>  	list_del_init(&lm->list);
>  	spin_unlock(&grace_lock);
> +	if (list_empty(&gn->grace_list))
> +		return;
> +	/*
> +	 * If the server goes down again right now, an NFSv4
> +	 * client will still be reclaim after it comes back up, even if
> +	 * it hasn't yet had a chance to reclaim state this time.
> +	 */
> +	lm->finish_grace(lm);
> +	/*
> +	 * At this point, NFSv4 clients can still reclaim.  But if the
> +	 * server crashes, any that have not yet reclaimed will be out
> +	 * of luck on the next boot.
> +	 */
> +	ln->in_grace = false;

	gn->in_grace = false;

> +	/*
> +	 * At this point, no reclaims are permitted.  Regular locking
> +	 * can resume.
> +	 */
>  }
>  EXPORT_SYMBOL_GPL(locks_end_grace);
>  
> @@ -65,27 +92,28 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
>  int
>  locks_in_grace(struct net *net)
>  {
> -	struct list_head *grace_list = net_generic(net, grace_net_id);
> +	struct list_head *gn = net_generic(net, grace_net_id);
>  
> -	return !list_empty(grace_list);
> +	return gn->in_grace;
>  }
>  EXPORT_SYMBOL_GPL(locks_in_grace);
>  
>  static int __net_init
>  grace_init_net(struct net *net)
>  {
> -	struct list_head *grace_list = net_generic(net, grace_net_id);
> +	struct list_head *gn = net_generic(net, grace_net_id);
>  
> -	INIT_LIST_HEAD(grace_list);
> +	INIT_LIST_HEAD(&gn->grace_list);
> +	gn->in_grace = true;
>  	return 0;
>  }
>  
>  static void __net_exit
>  grace_exit_net(struct net *net)
>  {
> -	struct list_head *grace_list = net_generic(net, grace_net_id);
> +	struct list_head *gn = net_generic(net, grace_net_id);
>  
> -	BUG_ON(!list_empty(grace_list));
> +	BUG_ON(!list_empty(&gn->grace_list));
>  }
>  
>  static struct pernet_operations grace_net_ops = {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fc88b57..968acd0 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4122,7 +4122,6 @@ nfsd4_end_grace(struct nfsd_net *nn)
>  
>  	dprintk("NFSD: end of grace period\n");
>  	nn->grace_ended = true;
> -	nfsd4_record_grace_done(nn);
>  	locks_end_grace(&nn->nfsd4_manager);
>  	/*
>  	 * Now that every NFSv4 client has had the chance to recover and
> @@ -6341,6 +6340,13 @@ nfs4_state_destroy_net(struct net *net)
>  	put_net(net);
>  }
>  
> +void nfsd4_finish_grace(struct lock_manager *lm)
> +{
> +	struct nfsd_net *nn = lm->private;
> +
> +	nfsd4_record_grace_done(nn);
> +}
> +
>  int
>  nfs4_state_start_net(struct net *net)
>  {
> @@ -6352,6 +6358,8 @@ nfs4_state_start_net(struct net *net)
>  		return ret;
>  	nn->boot_time = get_seconds();
>  	nn->grace_ended = false;
> +	nn->nfsd4_manager.finish_grace = nfsd4_finish_grace;
> +	nn->nfsd4_manager.private = nn;
>  	locks_start_grace(net, &nn->nfsd4_manager);
>  	nfsd4_client_tracking_init(net);
>  	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9418772..0e33e7c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -876,6 +876,8 @@ struct lock_manager_operations {
>  
>  struct lock_manager {
>  	struct list_head list;
> +	void (*finish_grace)(struct lock_manager *lm);
> +	void *private;
>  };
>  
>  struct net;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 1/5] lockd: move lockd's grace period handling into its own module

[J. Bruce Fields]

On Mon, Sep 15, 2014 at 07:19:19PM -0400, Jeff Layton wrote:
> On Mon, 15 Sep 2014 18:09:39 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Sep 15, 2014 at 06:08:13PM -0400, J. Bruce Fields wrote:
> > > On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields wrote:
> > > > On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > > > > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > > > > Currently, all of the grace period handling is part of lockd. Eventually
> > > > > > though we'd like to be able to build v4-only servers, at which point
> > > > > > we'll need to put all of this elsewhere.
> > > > > > 
> > > > > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > > > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > > > > enable it automatically.
> > > > > 
> > > > > Thanks, applying this one for 3.18 indepedently of the others.
> > > > 
> > > > This code should also be fixed, though.
> > > > 
> > > > Currently nfsd is recording the grace period as done when its own timer
> > > > runs out, but then it continuing to accept reclaims until lockd is also
> > > > done.
> > > 
> > > I sat down to fix this today then decided there's not a real bug:
> > > 
> > > All the grace_done upcall really does is throw away any previous clients
> > > that haven't reclaimed yet.
> > > 
> > > It's legal to do that as soon as the correct amount of time has passed.
> > > It's actually OK to continue to allow the grace period to run past that
> > > point, the only requirement is that all reclaims precede all new opens,
> > > which the code still does enforce.
> > > 
> > > So I think it might just be worth a little extra explanation.
> > 
> > I considered doing something like the following anyway (total
> > off-the-cuff draft, not tested), but I think it's overkill.
> > 
> > --b.
> > 
> 
> Yeah, I tend to agree. I don't see a real need for it.
> 
> In principle, I guess you could end up in a situation where the lockd
> and nfsd grace periods different by a large amount. In that case, you
> could end up in a situation where you started denying reclaims for v4.x
> clients that haven't reclaimed anything yet, but then still didn't
> allow them to establish new locks either.

Oh, right, I forget that this also keeps old clients from doing further
reclaims during this boot.

I think that could actually be fixed purely in userspace--instead of
purging those old clients, just record the new end-of-grace timestamp
and keep them around a while longer.  You still know which ones to deny
on the next boot, but they're still around if you want to keep allowing
them to reclaim.

Anyway:

> So it might be reasonable to add something like this, but I don't see
> it as anything most setups would ever hit -- particularly not in sane
> configurations.

Yeah, no big deal.

--b.

> 
> 
> > commit 675121da48fb4d7d85b58467c7f79dd3a6964879
> > Author: J. Bruce Fields <bfields@redhat.com>
> > Date:   Mon Sep 15 17:42:14 2014 -0400
> > 
> >     nfsd4: end grace only when last grace period ends
> >     
> >     In the case both lockd and nfsd4 are running, or when we have multiple
> >     containers exporting the same filesystem, there may be multiple grace
> >     periods.  In that case the grace period doesn't really end till the last
> >     one does and though it's harmless to do the grace_done upcall before
> >     then, it would probably be better not to.
> >     
> >     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> > index ae6e58e..dc67e5d 100644
> > --- a/fs/nfs_common/grace.c
> > +++ b/fs/nfs_common/grace.c
> > @@ -12,6 +12,11 @@
> >  static int grace_net_id;
> >  static DEFINE_SPINLOCK(grace_lock);
> >  
> > +struct grace_net {
> > +	struct list_head *grace_list;
> > +	bool in_grace;
> > +};
> > +
> >  /**
> >   * locks_start_grace
> >   * @net: net namespace that this lock manager belongs to
> > @@ -27,10 +32,10 @@ static DEFINE_SPINLOCK(grace_lock);
> >  void
> >  locks_start_grace(struct net *net, struct lock_manager *lm)
> >  {
> > -	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +	struct list_head *gn = net_generic(net, grace_net_id);
> >  
> >  	spin_lock(&grace_lock);
> > -	list_add(&lm->list, grace_list);
> > +	list_add(&lm->list, &gn->grace_list);
> >  	spin_unlock(&grace_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(locks_start_grace);
> > @@ -49,9 +54,31 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> >  void
> >  locks_end_grace(struct lock_manager *lm)
> >  {
> > +	struct list_head *gn = net_generic(net, grace_net_id);
> > +
> > +	if (list_empty(&lm->list))
> > +		return;
> >  	spin_lock(&grace_lock);
> >  	list_del_init(&lm->list);
> >  	spin_unlock(&grace_lock);
> > +	if (list_empty(&gn->grace_list))
> > +		return;
> > +	/*
> > +	 * If the server goes down again right now, an NFSv4
> > +	 * client will still be reclaim after it comes back up, even if
> > +	 * it hasn't yet had a chance to reclaim state this time.
> > +	 */
> > +	lm->finish_grace(lm);
> > +	/*
> > +	 * At this point, NFSv4 clients can still reclaim.  But if the
> > +	 * server crashes, any that have not yet reclaimed will be out
> > +	 * of luck on the next boot.
> > +	 */
> > +	ln->in_grace = false;
> 
> 	gn->in_grace = false;
> 
> > +	/*
> > +	 * At this point, no reclaims are permitted.  Regular locking
> > +	 * can resume.
> > +	 */
> >  }
> >  EXPORT_SYMBOL_GPL(locks_end_grace);
> >  
> > @@ -65,27 +92,28 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> >  int
> >  locks_in_grace(struct net *net)
> >  {
> > -	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +	struct list_head *gn = net_generic(net, grace_net_id);
> >  
> > -	return !list_empty(grace_list);
> > +	return gn->in_grace;
> >  }
> >  EXPORT_SYMBOL_GPL(locks_in_grace);
> >  
> >  static int __net_init
> >  grace_init_net(struct net *net)
> >  {
> > -	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +	struct list_head *gn = net_generic(net, grace_net_id);
> >  
> > -	INIT_LIST_HEAD(grace_list);
> > +	INIT_LIST_HEAD(&gn->grace_list);
> > +	gn->in_grace = true;
> >  	return 0;
> >  }
> >  
> >  static void __net_exit
> >  grace_exit_net(struct net *net)
> >  {
> > -	struct list_head *grace_list = net_generic(net, grace_net_id);
> > +	struct list_head *gn = net_generic(net, grace_net_id);
> >  
> > -	BUG_ON(!list_empty(grace_list));
> > +	BUG_ON(!list_empty(&gn->grace_list));
> >  }
> >  
> >  static struct pernet_operations grace_net_ops = {
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fc88b57..968acd0 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4122,7 +4122,6 @@ nfsd4_end_grace(struct nfsd_net *nn)
> >  
> >  	dprintk("NFSD: end of grace period\n");
> >  	nn->grace_ended = true;
> > -	nfsd4_record_grace_done(nn);
> >  	locks_end_grace(&nn->nfsd4_manager);
> >  	/*
> >  	 * Now that every NFSv4 client has had the chance to recover and
> > @@ -6341,6 +6340,13 @@ nfs4_state_destroy_net(struct net *net)
> >  	put_net(net);
> >  }
> >  
> > +void nfsd4_finish_grace(struct lock_manager *lm)
> > +{
> > +	struct nfsd_net *nn = lm->private;
> > +
> > +	nfsd4_record_grace_done(nn);
> > +}
> > +
> >  int
> >  nfs4_state_start_net(struct net *net)
> >  {
> > @@ -6352,6 +6358,8 @@ nfs4_state_start_net(struct net *net)
> >  		return ret;
> >  	nn->boot_time = get_seconds();
> >  	nn->grace_ended = false;
> > +	nn->nfsd4_manager.finish_grace = nfsd4_finish_grace;
> > +	nn->nfsd4_manager.private = nn;
> >  	locks_start_grace(net, &nn->nfsd4_manager);
> >  	nfsd4_client_tracking_init(net);
> >  	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9418772..0e33e7c 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -876,6 +876,8 @@ struct lock_manager_operations {
> >  
> >  struct lock_manager {
> >  	struct list_head list;
> > +	void (*finish_grace)(struct lock_manager *lm);
> > +	void *private;
> >  };
> >  
> >  struct net;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
> Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

By the way, I've seen the following *before* your patches, but in case
you're still looking at reboot recovery problems:

I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
succeeds after a previous boot (with full grace period) during which the
client had failed to reclaim.

I managed to catch one trace, the relevant parts looked like:

	SETCLIENTID client1
	OPEN
	LOCK

	(server restart here)

	SETCLIENTID client2
	OPEN
	LOCK (lock that conflicts with client1's)

	(server restart here)

	SETCLIENTID client1
	OPEN CLAIM_PREVIOUS

And all those ops (including the last reclaim open) succeeded.

So I didn't have a chance to review it more carefully, but it certainly
looks like a server bug, not a test bug.  (Well, technically the server
behavior above is correct since it's not required to refuse anything
till we actually attempt to reclaim the original lock, but we know our
server's not that smart.)

But I haven't gotten any further than that....

--b.

On Tue, Aug 19, 2014 at 02:38:24PM -0400, Jeff Layton wrote:
> v2:
> - move grace period handling into its own module
> 
> One of the huge annoyances in dealing with knfsd is the 90s grace period
> that's imposed when the server reboots. This is not just an annoyance,
> but means a siginificant amount of "downtime" in many production
> environments.
> 
> This patchset aimed at reducing this pain. It adds a couple of /proc
> knobs that tell the lockd and nfsd lock managers to lift the grace
> period.
> 
> It also changes the UMH upcalls to pass a little bit of extra info in
> the form of environment variables so that the upcall program can
> determine whether there are still any clients that may be in the process
> of reclaiming.
> 
> There are also a couple of cleanup patches in here that are not strictly
> required. In particular, making a separate grace.ko module doesn't have
> to be done, but I think it's a good idea.
> 
> Jeff Layton (5):
>   lockd: move lockd's grace period handling into its own module
>   lockd: add a /proc/fs/lockd/nlm_end_grace file
>   nfsd: add a v4_end_grace file to /proc/fs/nfsd
>   nfsd: remove redundant boot_time parm from grace_done client tracking
>     op
>   nfsd: pass extra info in env vars to upcalls to allow for early grace
>     period end
> 
>  fs/Kconfig                       |   6 ++-
>  fs/lockd/Makefile                |   3 +-
>  fs/lockd/netns.h                 |   1 -
>  fs/lockd/procfs.c                |  76 +++++++++++++++++++++++++++
>  fs/lockd/procfs.h                |  28 ++++++++++
>  fs/lockd/svc.c                   |  10 +++-
>  fs/nfs_common/Makefile           |   3 +-
>  fs/{lockd => nfs_common}/grace.c |  68 +++++++++++++++++++++----
>  fs/nfsd/Kconfig                  |   1 +
>  fs/nfsd/nfs4recover.c            | 107 +++++++++++++++++++++++++++++++--------
>  fs/nfsd/nfs4state.c              |   8 +--
>  fs/nfsd/nfsctl.c                 |  35 +++++++++++++
>  fs/nfsd/state.h                  |   5 +-
>  include/linux/proc_fs.h          |   2 +
>  14 files changed, 312 insertions(+), 41 deletions(-)
>  create mode 100644 fs/lockd/procfs.c
>  create mode 100644 fs/lockd/procfs.h
>  rename fs/{lockd => nfs_common}/grace.c (50%)
> 
> -- 
> 1.9.3
> 

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Jeff Layton]

On Fri, 26 Sep 2014 14:39:49 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> By the way, I've seen the following *before* your patches, but in case
> you're still looking at reboot recovery problems:
> 
> I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
> succeeds after a previous boot (with full grace period) during which the
> client had failed to reclaim.
> 
> I managed to catch one trace, the relevant parts looked like:
> 
> 	SETCLIENTID client1
> 	OPEN
> 	LOCK
> 
> 	(server restart here)
> 
> 	SETCLIENTID client2
> 	OPEN
> 	LOCK (lock that conflicts with client1's)
> 
> 	(server restart here)
> 
> 	SETCLIENTID client1
> 	OPEN CLAIM_PREVIOUS
> 
> And all those ops (including the last reclaim open) succeeded.
> 
> So I didn't have a chance to review it more carefully, but it certainly
> looks like a server bug, not a test bug.  (Well, technically the server
> behavior above is correct since it's not required to refuse anything
> till we actually attempt to reclaim the original lock, but we know our
> server's not that smart.)
> 
> But I haven't gotten any further than that....
> 
> --b.
> 

Ewww...v4.0... ;)

Well, I guess that could happen if, after the first reboot, client1 also
did a SETCLIENTID *and* reclaimed something that didn't conflict with
the lock that client2 grabs...or, did an OPEN/OPEN_CONFIRM after the
grace period without reclaiming its lock previously.

If it didn't do one or the other, then its record should have been
cleaned out of the DB after the grace period ended between the reboots
and it wouldn't have been able to reclaim after the second reboot.

It's a bit of a pathological case, and I don't see a way to fix that in
the context of v4.0. The fact that there's no RECLAIM_COMPLETE is a
pretty nasty protocol bug, IMO. Yet another reason to start really
moving people toward v4.1+...

> On Tue, Aug 19, 2014 at 02:38:24PM -0400, Jeff Layton wrote:
> > v2:
> > - move grace period handling into its own module
> > 
> > One of the huge annoyances in dealing with knfsd is the 90s grace period
> > that's imposed when the server reboots. This is not just an annoyance,
> > but means a siginificant amount of "downtime" in many production
> > environments.
> > 
> > This patchset aimed at reducing this pain. It adds a couple of /proc
> > knobs that tell the lockd and nfsd lock managers to lift the grace
> > period.
> > 
> > It also changes the UMH upcalls to pass a little bit of extra info in
> > the form of environment variables so that the upcall program can
> > determine whether there are still any clients that may be in the process
> > of reclaiming.
> > 
> > There are also a couple of cleanup patches in here that are not strictly
> > required. In particular, making a separate grace.ko module doesn't have
> > to be done, but I think it's a good idea.
> > 
> > Jeff Layton (5):
> >   lockd: move lockd's grace period handling into its own module
> >   lockd: add a /proc/fs/lockd/nlm_end_grace file
> >   nfsd: add a v4_end_grace file to /proc/fs/nfsd
> >   nfsd: remove redundant boot_time parm from grace_done client tracking
> >     op
> >   nfsd: pass extra info in env vars to upcalls to allow for early grace
> >     period end
> > 
> >  fs/Kconfig                       |   6 ++-
> >  fs/lockd/Makefile                |   3 +-
> >  fs/lockd/netns.h                 |   1 -
> >  fs/lockd/procfs.c                |  76 +++++++++++++++++++++++++++
> >  fs/lockd/procfs.h                |  28 ++++++++++
> >  fs/lockd/svc.c                   |  10 +++-
> >  fs/nfs_common/Makefile           |   3 +-
> >  fs/{lockd => nfs_common}/grace.c |  68 +++++++++++++++++++++----
> >  fs/nfsd/Kconfig                  |   1 +
> >  fs/nfsd/nfs4recover.c            | 107 +++++++++++++++++++++++++++++++--------
> >  fs/nfsd/nfs4state.c              |   8 +--
> >  fs/nfsd/nfsctl.c                 |  35 +++++++++++++
> >  fs/nfsd/state.h                  |   5 +-
> >  include/linux/proc_fs.h          |   2 +
> >  14 files changed, 312 insertions(+), 41 deletions(-)
> >  create mode 100644 fs/lockd/procfs.c
> >  create mode 100644 fs/lockd/procfs.h
> >  rename fs/{lockd => nfs_common}/grace.c (50%)
> > 
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

On Fri, Sep 26, 2014 at 02:54:46PM -0400, Jeff Layton wrote:
> On Fri, 26 Sep 2014 14:39:49 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > By the way, I've seen the following *before* your patches, but in case
> > you're still looking at reboot recovery problems:
> > 
> > I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
> > succeeds after a previous boot (with full grace period) during which the
> > client had failed to reclaim.
> > 
> > I managed to catch one trace, the relevant parts looked like:
> > 
> > 	SETCLIENTID client1
> > 	OPEN
> > 	LOCK
> > 
> > 	(server restart here)
> > 
> > 	SETCLIENTID client2
> > 	OPEN
> > 	LOCK (lock that conflicts with client1's)
> > 
> > 	(server restart here)
> > 
> > 	SETCLIENTID client1
> > 	OPEN CLAIM_PREVIOUS
> > 
> > And all those ops (including the last reclaim open) succeeded.
> > 
> > So I didn't have a chance to review it more carefully, but it certainly
> > looks like a server bug, not a test bug.  (Well, technically the server
> > behavior above is correct since it's not required to refuse anything
> > till we actually attempt to reclaim the original lock, but we know our
> > server's not that smart.)
> > 
> > But I haven't gotten any further than that....
> > 
> > --b.
> > 
> 
> Ewww...v4.0... ;)
> 
> Well, I guess that could happen if, after the first reboot, client1 also
> did a SETCLIENTID *and* reclaimed something that didn't conflict with
> the lock that client2 grabs...or, did an OPEN/OPEN_CONFIRM after the
> grace period without reclaiming its lock previously.
> 
> If it didn't do one or the other, then its record should have been
> cleaned out of the DB after the grace period ended between the reboots
> and it wouldn't have been able to reclaim after the second reboot.

Yeah.  Is there an easy way to tell nfsdcltrack to log everything?  I'm
only seeing this occasionally.

> It's a bit of a pathological case, and I don't see a way to fix that in
> the context of v4.0. The fact that there's no RECLAIM_COMPLETE is a
> pretty nasty protocol bug, IMO. Yet another reason to start really
> moving people toward v4.1+...

I don't belive there's a protocol bug here.

A correct NFSv4.0 client wouldn't send the open reclaim in the case you
describe above.

As I understand it, the rule for the client is: you're allowed to
reclaim only the set locks that you held previously, where "the set of
locks you held previously" is "the set of locks held by the clientid
which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
example once client1 sends that unrelated OPEN reclaim it's giving up on
anything else it doesn't manage to reclaim this time around.

Any server that loses client state on reboot has no choice but to trust
clients to get this sort of thing right.

--b.

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Trond Myklebust]

On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
> As I understand it, the rule for the client is: you're allowed to
> reclaim only the set locks that you held previously, where "the set of
> locks you held previously" is "the set of locks held by the clientid
> which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
> example once client1 sends that unrelated OPEN reclaim it's giving up on
> anything else it doesn't manage to reclaim this time around.

The rule for the client is very simple: "You may attempt to reclaim
any locks that were held immediately prior to the reboot of the
server."
It doesn't matter how those locks were established (ordinary OPEN,
delegated open, reclaim open, LOCK, reclaim lock...).

However if the server reboots and the client did not manage to
re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
server's responsibility to block that client from reclaiming any
locks, since the client has no way to know how many times the server
has rebooted.
Ditto, of course, if the client tries to reclaim any locks outside the
grace period and the server isn't tracking whether or not those locks
have been handed out to another client.

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

On Fri, Sep 26, 2014 at 04:37:23PM -0400, Trond Myklebust wrote:
> On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > As I understand it, the rule for the client is: you're allowed to
> > reclaim only the set locks that you held previously, where "the set of
> > locks you held previously" is "the set of locks held by the clientid
> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
> > example once client1 sends that unrelated OPEN reclaim it's giving up on
> > anything else it doesn't manage to reclaim this time around.
> 
> The rule for the client is very simple: "You may attempt to reclaim
> any locks that were held immediately prior to the reboot of the
> server."
> It doesn't matter how those locks were established (ordinary OPEN,
> delegated open, reclaim open, LOCK, reclaim lock...).
> 
> However if the server reboots and the client did not manage to
> re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
> EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
> server's responsibility to block that client from reclaiming any
> locks, since the client has no way to know how many times the server
> has rebooted.
> Ditto, of course, if the client tries to reclaim any locks outside the
> grace period and the server isn't tracking whether or not those locks
> have been handed out to another client.

Agreed with everything except:

	(SETCLIENTID+SETCLIENTID_CONFIRM and/or
	EXCHANGE_ID+CREATE_SESSION)

If I remember correctly: RFC 5661 says the point where this happens is
actually RECLAIM_COMPLETE.  RFC 3530 was more vague but suggested first
OPEN reclaim or OPEN_CONFIRM, and 3530bis makes that explicit.

But the client can choose an earlier point without violating the
protocol--it means it will decline reclaiming some things it could have,
but that's safer than the reverse mistake.

--b.

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Trond Myklebust]

On Fri, Sep 26, 2014 at 4:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Sep 26, 2014 at 04:37:23PM -0400, Trond Myklebust wrote:
>> On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> >
>> > As I understand it, the rule for the client is: you're allowed to
>> > reclaim only the set locks that you held previously, where "the set of
>> > locks you held previously" is "the set of locks held by the clientid
>> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
>> > example once client1 sends that unrelated OPEN reclaim it's giving up on
>> > anything else it doesn't manage to reclaim this time around.
>>
>> The rule for the client is very simple: "You may attempt to reclaim
>> any locks that were held immediately prior to the reboot of the
>> server."
>> It doesn't matter how those locks were established (ordinary OPEN,
>> delegated open, reclaim open, LOCK, reclaim lock...).
>>
>> However if the server reboots and the client did not manage to
>> re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>> EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
>> server's responsibility to block that client from reclaiming any
>> locks, since the client has no way to know how many times the server
>> has rebooted.
>> Ditto, of course, if the client tries to reclaim any locks outside the
>> grace period and the server isn't tracking whether or not those locks
>> have been handed out to another client.
>
> Agreed with everything except:
>
>         (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>         EXCHANGE_ID+CREATE_SESSION)
>
> If I remember correctly: RFC 5661 says the point where this happens is
> actually RECLAIM_COMPLETE.  RFC 3530 was more vague but suggested first
> OPEN reclaim or OPEN_CONFIRM, and 3530bis makes that explicit.
>
> But the client can choose an earlier point without violating the
> protocol--it means it will decline reclaiming some things it could have,
> but that's safer than the reverse mistake.
>

Where is this documented? I'm not seeing it.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

On Fri, Sep 26, 2014 at 04:58:47PM -0400, Trond Myklebust wrote:
> On Fri, Sep 26, 2014 at 4:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Fri, Sep 26, 2014 at 04:37:23PM -0400, Trond Myklebust wrote:
> >> On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> >
> >> > As I understand it, the rule for the client is: you're allowed to
> >> > reclaim only the set locks that you held previously, where "the set of
> >> > locks you held previously" is "the set of locks held by the clientid
> >> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
> >> > example once client1 sends that unrelated OPEN reclaim it's giving up on
> >> > anything else it doesn't manage to reclaim this time around.
> >>
> >> The rule for the client is very simple: "You may attempt to reclaim
> >> any locks that were held immediately prior to the reboot of the
> >> server."
> >> It doesn't matter how those locks were established (ordinary OPEN,
> >> delegated open, reclaim open, LOCK, reclaim lock...).
> >>
> >> However if the server reboots and the client did not manage to
> >> re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
> >> EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
> >> server's responsibility to block that client from reclaiming any
> >> locks, since the client has no way to know how many times the server
> >> has rebooted.
> >> Ditto, of course, if the client tries to reclaim any locks outside the
> >> grace period and the server isn't tracking whether or not those locks
> >> have been handed out to another client.
> >
> > Agreed with everything except:
> >
> >         (SETCLIENTID+SETCLIENTID_CONFIRM and/or
> >         EXCHANGE_ID+CREATE_SESSION)
> >
> > If I remember correctly: RFC 5661 says the point where this happens is
> > actually RECLAIM_COMPLETE.  RFC 3530 was more vague but suggested first
> > OPEN reclaim or OPEN_CONFIRM, and 3530bis makes that explicit.
> >
> > But the client can choose an earlier point without violating the
> > protocol--it means it will decline reclaiming some things it could have,
> > but that's safer than the reverse mistake.
> >
> 
> Where is this documented? I'm not seeing it.

It's more vague than I remembered:

http://tools.ietf.org/html/rfc5661#section-8.4.3

	The server will set this for any client record in stable
	storage where the client has not done a suitable
	RECLAIM_COMPLETE (global or file system-specific depending on
	the target of the lock request) before it grants any new (i.e.,
	not reclaimed) lock to any client.

And the corresponding langue in 8.6.3 of rfc 3530 is:

	a timestamp that is updated the first time after a server boot
	or reboot the client acquires record locking, share reservation,
	or delegation state on the server.  The timestamp need not be
	updated on subsequent lock requests until the server reboots.

I thought there was something referring specifically to OPEN reclaim or
OPEN_CONFIRM as the point where "the client acquires record locking" but
can't find it on a quick skim.

I also say this is "vague" because, unfortunately, in both cases, this
language is part of a description of an example server implementation,
no actual protocol requirement is made explicit.

Which is weird given that noticing the partial-reclaim case was actually
Dave Noveck's original motivation for introducing RECLAIM_COMPLETE (then
RECOVERY_COMPLETE), with the grace-period shortening an extra benefit:

	http://osdir.com/ml/ietf.nfsv4/2006-01/msg00020.html

	Adding the RECOVERY_COMPLETE op allows this situation to be
	dealt with fairly simply. If a client has not recovered all of
	its locks and we have the possiblity of having given out a lock
	inconsistent with one of those (the normal realization of this
	would be that once we declare grace over with some client's
	reclaims not complete) we mark that client as essentially having
	had a lock effectively revoked and thus it would not allowed to
	reclaim locks after a subsequent reboot since it could no longer
	vouch for all the locks it thinks it had.

In the 3530 case we decided that the only safe point to choose was the
one described in the sample server implementation, so 3530bis says:

	A server may consider a client's lease "successfully
	established" once it has received an open operation from that
	client.

(And "open operation" probably is still too vague.)

Sorry for the length.

Anyway, if the client's currently doing this at SETCLIENTID_CONFIRM and
CREATE_SESSION then I think that's correct but more conservative than
necessary.  Which may be a good idea given that I think the chance of a
random server implementor making there way through all this is small.

--b.

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Trond Myklebust]

On Fri, Sep 26, 2014 at 5:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Sep 26, 2014 at 04:58:47PM -0400, Trond Myklebust wrote:
>> On Fri, Sep 26, 2014 at 4:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Fri, Sep 26, 2014 at 04:37:23PM -0400, Trond Myklebust wrote:
>> >> On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> >> >
>> >> > As I understand it, the rule for the client is: you're allowed to
>> >> > reclaim only the set locks that you held previously, where "the set of
>> >> > locks you held previously" is "the set of locks held by the clientid
>> >> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
>> >> > example once client1 sends that unrelated OPEN reclaim it's giving up on
>> >> > anything else it doesn't manage to reclaim this time around.
>> >>
>> >> The rule for the client is very simple: "You may attempt to reclaim
>> >> any locks that were held immediately prior to the reboot of the
>> >> server."
>> >> It doesn't matter how those locks were established (ordinary OPEN,
>> >> delegated open, reclaim open, LOCK, reclaim lock...).
>> >>
>> >> However if the server reboots and the client did not manage to
>> >> re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>> >> EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
>> >> server's responsibility to block that client from reclaiming any
>> >> locks, since the client has no way to know how many times the server
>> >> has rebooted.
>> >> Ditto, of course, if the client tries to reclaim any locks outside the
>> >> grace period and the server isn't tracking whether or not those locks
>> >> have been handed out to another client.
>> >
>> > Agreed with everything except:
>> >
>> >         (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>> >         EXCHANGE_ID+CREATE_SESSION)
>> >
>> > If I remember correctly: RFC 5661 says the point where this happens is
>> > actually RECLAIM_COMPLETE.  RFC 3530 was more vague but suggested first
>> > OPEN reclaim or OPEN_CONFIRM, and 3530bis makes that explicit.
>> >
>> > But the client can choose an earlier point without violating the
>> > protocol--it means it will decline reclaiming some things it could have,
>> > but that's safer than the reverse mistake.
>> >
>>
>> Where is this documented? I'm not seeing it.
>
> It's more vague than I remembered:
>
> http://tools.ietf.org/html/rfc5661#section-8.4.3
>
>         The server will set this for any client record in stable
>         storage where the client has not done a suitable
>         RECLAIM_COMPLETE (global or file system-specific depending on
>         the target of the lock request) before it grants any new (i.e.,
>         not reclaimed) lock to any client.

Yes, I read that. Then I read this:

   For the second edge condition, after the server restarts for a second
   time, the indication that the client had not completed its reclaims
   at the time at which the grace period ended means that the server
   must reject a reclaim from client A with the error NFS4ERR_NO_GRACE.

This text is just plain wrong, and we should fix it in an errata.
There is absolutely NO edge case condition for those locks that were
successfully reclaimed after the first reboot. There is absolutely no
reason why the client shouldn't be able to reclaim those locks after
reboot number 2.

All the absence of the RECLAIM_COMPLETE tells the server is that the
client MAY have held more locks; what is the server supposed to do
with that information? It's not the server that is responsible for
reclaiming locks.

> And the corresponding langue in 8.6.3 of rfc 3530 is:
>
>         a timestamp that is updated the first time after a server boot
>         or reboot the client acquires record locking, share reservation,
>         or delegation state on the server.  The timestamp need not be
>         updated on subsequent lock requests until the server reboots.
>
> I thought there was something referring specifically to OPEN reclaim or
> OPEN_CONFIRM as the point where "the client acquires record locking" but
> can't find it on a quick skim.
>
> I also say this is "vague" because, unfortunately, in both cases, this
> language is part of a description of an example server implementation,
> no actual protocol requirement is made explicit.
>
> Which is weird given that noticing the partial-reclaim case was actually
> Dave Noveck's original motivation for introducing RECLAIM_COMPLETE (then
> RECOVERY_COMPLETE), with the grace-period shortening an extra benefit:
>
>         http://osdir.com/ml/ietf.nfsv4/2006-01/msg00020.html
>
>         Adding the RECOVERY_COMPLETE op allows this situation to be
>         dealt with fairly simply. If a client has not recovered all of
>         its locks and we have the possiblity of having given out a lock
>         inconsistent with one of those (the normal realization of this
>         would be that once we declare grace over with some client's
>         reclaims not complete) we mark that client as essentially having
>         had a lock effectively revoked and thus it would not allowed to
>         reclaim locks after a subsequent reboot since it could no longer
>         vouch for all the locks it thinks it had.

Sigh. Another example of one of Dave's proposals going through without
adequate review because the reader died of exhaustion while wrestling
with the preamble text.
So, I agree that if the client were to try to reclaim a lock that it
didn't own (because it didn't manage to reclaim it) in the previous
boot instance would be a problem. However WHY would a sane client do
this?

> In the 3530 case we decided that the only safe point to choose was the
> one described in the sample server implementation, so 3530bis says:
>
>         A server may consider a client's lease "successfully
>         established" once it has received an open operation from that
>         client.
>
> (And "open operation" probably is still too vague.)
>
> Sorry for the length.
>
> Anyway, if the client's currently doing this at SETCLIENTID_CONFIRM and
> CREATE_SESSION then I think that's correct but more conservative than
> necessary.  Which may be a good idea given that I think the chance of a
> random server implementor making there way through all this is small.
>

The client is pruning all those locks that it did not manage to
reclaim before the grace period expired and/or the second reboot
occurred. That is the correct behaviour.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Trond Myklebust]

On Fri, Sep 26, 2014 at 6:17 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Fri, Sep 26, 2014 at 5:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> On Fri, Sep 26, 2014 at 04:58:47PM -0400, Trond Myklebust wrote:
>>> On Fri, Sep 26, 2014 at 4:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>> > On Fri, Sep 26, 2014 at 04:37:23PM -0400, Trond Myklebust wrote:
>>> >> On Fri, Sep 26, 2014 at 3:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>> >> >
>>> >> > As I understand it, the rule for the client is: you're allowed to
>>> >> > reclaim only the set locks that you held previously, where "the set of
>>> >> > locks you held previously" is "the set of locks held by the clientid
>>> >> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
>>> >> > example once client1 sends that unrelated OPEN reclaim it's giving up on
>>> >> > anything else it doesn't manage to reclaim this time around.
>>> >>
>>> >> The rule for the client is very simple: "You may attempt to reclaim
>>> >> any locks that were held immediately prior to the reboot of the
>>> >> server."
>>> >> It doesn't matter how those locks were established (ordinary OPEN,
>>> >> delegated open, reclaim open, LOCK, reclaim lock...).
>>> >>
>>> >> However if the server reboots and the client did not manage to
>>> >> re-establish a lease (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>>> >> EXCHANGE_ID+CREATE_SESSION) before the second reboot, then it is the
>>> >> server's responsibility to block that client from reclaiming any
>>> >> locks, since the client has no way to know how many times the server
>>> >> has rebooted.
>>> >> Ditto, of course, if the client tries to reclaim any locks outside the
>>> >> grace period and the server isn't tracking whether or not those locks
>>> >> have been handed out to another client.
>>> >
>>> > Agreed with everything except:
>>> >
>>> >         (SETCLIENTID+SETCLIENTID_CONFIRM and/or
>>> >         EXCHANGE_ID+CREATE_SESSION)
>>> >
>>> > If I remember correctly: RFC 5661 says the point where this happens is
>>> > actually RECLAIM_COMPLETE.  RFC 3530 was more vague but suggested first
>>> > OPEN reclaim or OPEN_CONFIRM, and 3530bis makes that explicit.
>>> >
>>> > But the client can choose an earlier point without violating the
>>> > protocol--it means it will decline reclaiming some things it could have,
>>> > but that's safer than the reverse mistake.
>>> >
>>>
>>> Where is this documented? I'm not seeing it.
>>
>> It's more vague than I remembered:
>>
>> http://tools.ietf.org/html/rfc5661#section-8.4.3
>>
>>         The server will set this for any client record in stable
>>         storage where the client has not done a suitable
>>         RECLAIM_COMPLETE (global or file system-specific depending on
>>         the target of the lock request) before it grants any new (i.e.,
>>         not reclaimed) lock to any client.
>
> Yes, I read that. Then I read this:
>
>    For the second edge condition, after the server restarts for a second
>    time, the indication that the client had not completed its reclaims
>    at the time at which the grace period ended means that the server
>    must reject a reclaim from client A with the error NFS4ERR_NO_GRACE.
>
> This text is just plain wrong, and we should fix it in an errata.
> There is absolutely NO edge case condition for those locks that were
> successfully reclaimed after the first reboot. There is absolutely no
> reason why the client shouldn't be able to reclaim those locks after
> reboot number 2.
>
> All the absence of the RECLAIM_COMPLETE tells the server is that the
> client MAY have held more locks; what is the server supposed to do
> with that information? It's not the server that is responsible for
> reclaiming locks.
>

Sigh, the information in RFC3530bis is still confused.

This makes sense:
   o  a timestamp that is updated the first time after a server boot or
      reboot the client acquires byte-range locking, share reservation,
      or delegation state on the server.  The timestamp need not be
      updated on subsequent lock requests until the server reboots.
....
This does not:
   For the second edge condition, after the server reboots for a second
   time, the record that the client had an unexpired record lock, share
   reservation, or delegation established before the server's previous
   incarnation means that the server must reject a reclaim from client A
   with the error NFS4ERR_NO_GRACE or NFS4ERR_RECLAIM_BAD.

Tom?

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Jeff Layton]

On Fri, 26 Sep 2014 15:46:17 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, Sep 26, 2014 at 02:54:46PM -0400, Jeff Layton wrote:
> > On Fri, 26 Sep 2014 14:39:49 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > By the way, I've seen the following *before* your patches, but in case
> > > you're still looking at reboot recovery problems:
> > > 
> > > I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
> > > succeeds after a previous boot (with full grace period) during which the
> > > client had failed to reclaim.
> > > 
> > > I managed to catch one trace, the relevant parts looked like:
> > > 
> > > 	SETCLIENTID client1
> > > 	OPEN
> > > 	LOCK
> > > 
> > > 	(server restart here)
> > > 
> > > 	SETCLIENTID client2
> > > 	OPEN
> > > 	LOCK (lock that conflicts with client1's)
> > > 
> > > 	(server restart here)
> > > 
> > > 	SETCLIENTID client1
> > > 	OPEN CLAIM_PREVIOUS
> > > 
> > > And all those ops (including the last reclaim open) succeeded.
> > > 
> > > So I didn't have a chance to review it more carefully, but it certainly
> > > looks like a server bug, not a test bug.  (Well, technically the server
> > > behavior above is correct since it's not required to refuse anything
> > > till we actually attempt to reclaim the original lock, but we know our
> > > server's not that smart.)
> > > 
> > > But I haven't gotten any further than that....
> > > 
> > > --b.
> > > 
> > 
> > Ewww...v4.0... ;)
> > 
> > Well, I guess that could happen if, after the first reboot, client1 also
> > did a SETCLIENTID *and* reclaimed something that didn't conflict with
> > the lock that client2 grabs...or, did an OPEN/OPEN_CONFIRM after the
> > grace period without reclaiming its lock previously.
> > 
> > If it didn't do one or the other, then its record should have been
> > cleaned out of the DB after the grace period ended between the reboots
> > and it wouldn't have been able to reclaim after the second reboot.
> 
> Yeah.  Is there an easy way to tell nfsdcltrack to log everything?  I'm
> only seeing this occasionally.
> 

If you can make sure that it's run with the '-d' flag then that'll make
it do debug level logging. Unfortunately, the kernel doesn't have a
handy switch to enable that so you'll need to patch the kernel (or
maybe wrap nfsdcltrack) to make that happen.

Maybe we should add a module parm to nfsd.ko that makes it run
nfsdcltrack with -d?

> > It's a bit of a pathological case, and I don't see a way to fix that in
> > the context of v4.0. The fact that there's no RECLAIM_COMPLETE is a
> > pretty nasty protocol bug, IMO. Yet another reason to start really
> > moving people toward v4.1+...
> 
> I don't belive there's a protocol bug here.
> 
> A correct NFSv4.0 client wouldn't send the open reclaim in the case you
> describe above.
> 

It would in the partial reclaim case.

Suppose we start reclaiming things but don't get everything before
there's a network partition. The server ends the grace period and then
hands out the conflicting lock to client2. It then reboots again and the
network partition heals. At that point, client1 could try to reclaim
everything it had before (including the lock that conflicts with the
one that client2 had).

I'd still argue that this is a protocol bug. Without RECLAIM_COMPLETE,
the server simply has no way to know whether the reclaims done by
client1 are complete or not.

> As I understand it, the rule for the client is: you're allowed to
> reclaim only the set locks that you held previously, where "the set of
> locks you held previously" is "the set of locks held by the clientid
> which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
> example once client1 sends that unrelated OPEN reclaim it's giving up
> on anything else it doesn't manage to reclaim this time around.
> 
> Any server that loses client state on reboot has no choice but to
> trust clients to get this sort of thing right.
> 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

On Sat, Sep 27, 2014 at 09:04:58AM -0400, Jeff Layton wrote:
> On Fri, 26 Sep 2014 15:46:17 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Fri, Sep 26, 2014 at 02:54:46PM -0400, Jeff Layton wrote:
> > > On Fri, 26 Sep 2014 14:39:49 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > By the way, I've seen the following *before* your patches, but in case
> > > > you're still looking at reboot recovery problems:
> > > > 
> > > > I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
> > > > succeeds after a previous boot (with full grace period) during which the
> > > > client had failed to reclaim.
> > > > 
> > > > I managed to catch one trace, the relevant parts looked like:
> > > > 
> > > > 	SETCLIENTID client1
> > > > 	OPEN
> > > > 	LOCK
> > > > 
> > > > 	(server restart here)
> > > > 
> > > > 	SETCLIENTID client2
> > > > 	OPEN
> > > > 	LOCK (lock that conflicts with client1's)
> > > > 
> > > > 	(server restart here)
> > > > 
> > > > 	SETCLIENTID client1
> > > > 	OPEN CLAIM_PREVIOUS
> > > > 
> > > > And all those ops (including the last reclaim open) succeeded.
> > > > 
> > > > So I didn't have a chance to review it more carefully, but it certainly
> > > > looks like a server bug, not a test bug.  (Well, technically the server
> > > > behavior above is correct since it's not required to refuse anything
> > > > till we actually attempt to reclaim the original lock, but we know our
> > > > server's not that smart.)
> > > > 
> > > > But I haven't gotten any further than that....
> > > > 
> > > > --b.
> > > > 
> > > 
> > > Ewww...v4.0... ;)
> > > 
> > > Well, I guess that could happen if, after the first reboot, client1 also
> > > did a SETCLIENTID *and* reclaimed something that didn't conflict with
> > > the lock that client2 grabs...or, did an OPEN/OPEN_CONFIRM after the
> > > grace period without reclaiming its lock previously.
> > > 
> > > If it didn't do one or the other, then its record should have been
> > > cleaned out of the DB after the grace period ended between the reboots
> > > and it wouldn't have been able to reclaim after the second reboot.
> > 
> > Yeah.  Is there an easy way to tell nfsdcltrack to log everything?  I'm
> > only seeing this occasionally.
> > 
> 
> If you can make sure that it's run with the '-d' flag then that'll make
> it do debug level logging. Unfortunately, the kernel doesn't have a
> handy switch to enable that so you'll need to patch the kernel (or
> maybe wrap nfsdcltrack) to make that happen.
> 
> Maybe we should add a module parm to nfsd.ko that makes it run
> nfsdcltrack with -d?

Would userland configuration (e.g. and optional /etc/ file) be more
flexible?

> 
> > > It's a bit of a pathological case, and I don't see a way to fix that in
> > > the context of v4.0. The fact that there's no RECLAIM_COMPLETE is a
> > > pretty nasty protocol bug, IMO. Yet another reason to start really
> > > moving people toward v4.1+...
> > 
> > I don't belive there's a protocol bug here.
> > 
> > A correct NFSv4.0 client wouldn't send the open reclaim in the case you
> > describe above.
> > 
> 
> It would in the partial reclaim case.

My (and I think Trond's) understanding is that client1 would not send
the reclaim in that case.  In the case of a partial reclaim, a correct
4.0 client will stop trying to reclaim the state that it failed to
reclaim previously.  My understanding is that is how the linux client in
fact behaves.

So the bug would be in the client, not the protocol.  That said,
admittedly:

	- the "protocol" here seems to be largely in our heads; the spec
	  does a poor job of explaining all this.
	- we (not suprisingly) disagree on some of the details.

Nevertheless, I believe that despite our disagreement there's no actual
bug at least between the existing Linux client and server.  And that
3530bis and 5661 were at least *supposed* to deal with this case....

Oh, look, Trond and you have written a pile more of text on this on the
ietf list.  Should I even try to catch up, or would be better off just
spending the rest of my day back in bed?

--b.

> 
> Suppose we start reclaiming things but don't get everything before
> there's a network partition. The server ends the grace period and then
> hands out the conflicting lock to client2. It then reboots again and the
> network partition heals. At that point, client1 could try to reclaim
> everything it had before (including the lock that conflicts with the
> one that client2 had).
> 
> I'd still argue that this is a protocol bug. Without RECLAIM_COMPLETE,
> the server simply has no way to know whether the reclaims done by
> client1 are complete or not.
> 
> > As I understand it, the rule for the client is: you're allowed to
> > reclaim only the set locks that you held previously, where "the set of
> > locks you held previously" is "the set of locks held by the clientid
> > which last managed to send a reclaim OPEN or OPEN_CONFIRM".  So for
> > example once client1 sends that unrelated OPEN reclaim it's giving up
> > on anything else it doesn't manage to reclaim this time around.
> > 
> > Any server that loses client state on reboot has no choice but to
> > trust clients to get this sort of thing right.
> > 
> 
> 
> -- 
> Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Trond Myklebust]

On Mon, Sep 29, 2014 at 12:44 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> Oh, look, Trond and you have written a pile more of text on this on the
> ietf list.  Should I even try to catch up, or would be better off just
> spending the rest of my day back in bed?
>

Sleep, Bruce...

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[Jeff Layton]

On Mon, 29 Sep 2014 12:44:26 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Sat, Sep 27, 2014 at 09:04:58AM -0400, Jeff Layton wrote:
> > On Fri, 26 Sep 2014 15:46:17 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Fri, Sep 26, 2014 at 02:54:46PM -0400, Jeff Layton wrote:
> > > > On Fri, 26 Sep 2014 14:39:49 -0400
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > By the way, I've seen the following *before* your patches, but in case
> > > > > you're still looking at reboot recovery problems:
> > > > > 
> > > > > I'm getting sporadic failures in the REBT6 pynfs test--a reclaim open
> > > > > succeeds after a previous boot (with full grace period) during which the
> > > > > client had failed to reclaim.
> > > > > 
> > > > > I managed to catch one trace, the relevant parts looked like:
> > > > > 
> > > > > 	SETCLIENTID client1
> > > > > 	OPEN
> > > > > 	LOCK
> > > > > 
> > > > > 	(server restart here)
> > > > > 
> > > > > 	SETCLIENTID client2
> > > > > 	OPEN
> > > > > 	LOCK (lock that conflicts with client1's)
> > > > > 
> > > > > 	(server restart here)
> > > > > 
> > > > > 	SETCLIENTID client1
> > > > > 	OPEN CLAIM_PREVIOUS
> > > > > 
> > > > > And all those ops (including the last reclaim open) succeeded.
> > > > > 
> > > > > So I didn't have a chance to review it more carefully, but it certainly
> > > > > looks like a server bug, not a test bug.  (Well, technically the server
> > > > > behavior above is correct since it's not required to refuse anything
> > > > > till we actually attempt to reclaim the original lock, but we know our
> > > > > server's not that smart.)
> > > > > 
> > > > > But I haven't gotten any further than that....
> > > > > 
> > > > > --b.
> > > > > 
> > > > 
> > > > Ewww...v4.0... ;)
> > > > 
> > > > Well, I guess that could happen if, after the first reboot, client1 also
> > > > did a SETCLIENTID *and* reclaimed something that didn't conflict with
> > > > the lock that client2 grabs...or, did an OPEN/OPEN_CONFIRM after the
> > > > grace period without reclaiming its lock previously.
> > > > 
> > > > If it didn't do one or the other, then its record should have been
> > > > cleaned out of the DB after the grace period ended between the reboots
> > > > and it wouldn't have been able to reclaim after the second reboot.
> > > 
> > > Yeah.  Is there an easy way to tell nfsdcltrack to log everything?  I'm
> > > only seeing this occasionally.
> > > 
> > 
> > If you can make sure that it's run with the '-d' flag then that'll make
> > it do debug level logging. Unfortunately, the kernel doesn't have a
> > handy switch to enable that so you'll need to patch the kernel (or
> > maybe wrap nfsdcltrack) to make that happen.
> > 
> > Maybe we should add a module parm to nfsd.ko that makes it run
> > nfsdcltrack with -d?
> 
> Would userland configuration (e.g. and optional /etc/ file) be more
> flexible?
> 

Yeah, that might not hurt.

We do want this thing to run pretty quickly, but if the config file
were only processed if it were present (and it usually wasn't) then
that might not be too bad.

> > 
> > > > It's a bit of a pathological case, and I don't see a way to fix that in
> > > > the context of v4.0. The fact that there's no RECLAIM_COMPLETE is a
> > > > pretty nasty protocol bug, IMO. Yet another reason to start really
> > > > moving people toward v4.1+...
> > > 
> > > I don't belive there's a protocol bug here.
> > > 
> > > A correct NFSv4.0 client wouldn't send the open reclaim in the case you
> > > describe above.
> > > 
> > 
> > It would in the partial reclaim case.
> 
> My (and I think Trond's) understanding is that client1 would not send
> the reclaim in that case.  In the case of a partial reclaim, a correct
> 4.0 client will stop trying to reclaim the state that it failed to
> reclaim previously.  My understanding is that is how the linux client in
> fact behaves.
> 
> So the bug would be in the client, not the protocol.  That said,
> admittedly:
> 
> 	- the "protocol" here seems to be largely in our heads; the spec
> 	  does a poor job of explaining all this.
> 	- we (not suprisingly) disagree on some of the details.
> 
> Nevertheless, I believe that despite our disagreement there's no actual
> bug at least between the existing Linux client and server.  And that
> 3530bis and 5661 were at least *supposed* to deal with this case....
> 
> Oh, look, Trond and you have written a pile more of text on this on the
> ietf list.  Should I even try to catch up, or would be better off just
> spending the rest of my day back in bed?
> 
> --b.
> 

If there is any bug, I think it's only going to crop up under extremely
rare circumstances -- network partitions cropping up during reclaims,
multiple reboots, etc...

We probably ought to try and clarify this stuff for the spec (and our
own implementations) but I don't think there's any cause for alarm...

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v2 0/5] nfsd: support for lifting grace period early

[J. Bruce Fields]

On Mon, Sep 29, 2014 at 01:11:30PM -0400, Jeff Layton wrote:
> If there is any bug, I think it's only going to crop up under extremely
> rare circumstances -- network partitions cropping up during reclaims,
> multiple reboots, etc...
> 
> We probably ought to try and clarify this stuff for the spec (and our
> own implementations) but I don't think there's any cause for alarm...

Yes in this case I agree.

In general though reboot-recovery interoperability is hard to test and
bad documentation increases the chance we'll get it wrong.

(And these "edge cases" could also be more common that we expect: e.g.
if power failures come in clusters, and bring your switches down at
about the same time, could multiple-restarts-with-network-partitions be
the normal case?)

--b.