NLM failover unlock commands

We've implemented two new NFSD procfs files:

o /proc/fs/nfsd/unlock_ip
o /proc/fs/nfsd/unlock_filesystem

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now)
as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

fs/nfsd/nfsctl.c +16 -0

+int
+nlmsvc_fo_cmd(int cmd, void *datap, int grace_time)
+{
+	nlm_fo_cmd fo_cmd;
+	int rc=-EINVAL;
+
+	fo_printk("lockd: nlmsvc_fo_cmd enter, cmd=%d, datap=0x%p, gp=%d\n",
+		cmd, datap, grace_time);
+
+	fo_cmd.cmd   = cmd;
+	fo_cmd.stat  = 0;
+	fo_cmd.gp    = 0;
+	fo_cmd.datap = datap;
+
+	/* "if" place holder for NFSD_FO_RESUME */
+	{

Quoted 5 lines · wcheng at redhat.com

We've implemented two new NFSD procfs files:

o /proc/fs/nfsd/unlock_ip
o /proc/fs/nfsd/unlock_filesystem

Quoted 9 lines

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now)
as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

I'm happy with this interface and the code looks credible, so

Acked-by NeilBrown <[email protected]>

however......

This small patch has not been changed since our last discussion:
http://www.opensubscriber.com/message/nfs at lists.sourceforge.net/6348912.html

To recap the issue, a client could ask for a posix lock that invokes:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

server calls nlm4svc_proc_lock() ->
* server lookup file (f_count++)
* server lock the file
* server calls nlm_release_host
* server calls nlm_release_file (f_count--)
* server return to client with status 0

As part of the lookup file, the lock stays on vfs inode->i_flock list with zero f_count. Any call into nlm_traverse_files() will BUG() in locks_remove_flock() (fs/locks.c:2034) during fclose(), if that file happens to be of no interest to that particular search. Since after nlm_inspect_file(), the logic unconditionally checks for possible removing of the file. As the file is not blocked, nothing to do with shares, and f_count is zero, it will get removed from hash and fclose() invoked with the posix lock hanging on i_flock list.

-- Wendy

This fixes the incorrect fclose call inside nlm_traverse_files() where a posix lock could still be held by NFS client. Problem was found in a kernel panic inside locks_remove_flock() (fs/locks.c:2034) as part of the fclose call due to NFS-NLM locks still hanging on inode->i_flock list.

Signed-off-by S. Wendy Cheng <[email protected]>

 svcsubs.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

fs/lockd/svcsubs.c +1 -2

@@ -332,8 +332,7 @@ nlm_traverse_files(struct nlm_host *host
 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
-			 && !file->f_shares && !file->f_count) {
+			if (!nlm_file_inuse(file)) {
 				hlist_del(&file->f_list);
 				nlmsvc_ops->fclose(file->f_file);
 				kfree(file);

If I'm reading this correctly, this bug is introduced by your previous patch.

The important difference between the old code and the new code here is that the old code tests "file->f_locks" while the new code iterates through i_flock to see if there are any lockd locks.

f_locks is set to a count of lockd locks in nlm_traverse_locks which *was* always called by nlm_inspect_file which is called immediately before the code you are changing.
But since your patch, nlm_inspect_file does not always call nlm_traverse_locks, so there is a chance that f_locks is wrong.

With this patch, f_locks it not used at all any more.

Introducing a bug in one patch and fixing in the next is bad style.

Some options:

Have an initial patch which removes all references to f_locks and includes the change in this patch. With that in place your main patch won't introduce a bug. If you do this, you should attempt to understand and justify the performance impact (does nlm_traverse_files become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the call to nlm_inspect_file.

something else???

So NAK for this one in it's current form... unless I've misunderstood something.

Quoted 8 lines · Neil Brown

Some options:

Have an initial patch which removes all references to f_locks and includes the change in this patch. With that in place your main patch won't introduce a bug. If you do this, you should attempt to understand and justify the performance impact (does nlm_traverse_files become quadratic in number of locks. Is that acceptable?).

Quoted 6 lines

Change the first patch to explicitly update f_count if you bypass the call to nlm_inspect_file.

something else???

Let's see what hch says in another email... will come back to this soon.

Quoted 17 lines · Wendy Cheng

Quoted 14 lines · Neil Brown

Some options:

Have an initial patch which removes all references to f_locks and includes the change in this patch. With that in place your main patch won't introduce a bug. If you do this, you should attempt to understand and justify the performance impact (does nlm_traverse_files become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the call to nlm_inspect_file.

something else???

Quoted 2 lines

Let's see what hch says in another email... will come back to this soon.

Will do a quick and dirty performance measure tomorrow when I get to the office. Then we'll go from there.

The point here is "with this patch, f_locks it not used at all any more." Note that we have a nice inline function "nlm_file_inuse", why should we use f_locks (that I assume people agree that it is awkward) ? Could we simply drop f_locks all together in this section of code?

+extern __u32 in_aton(const char *str);

(quoted diff) +8 -0

+int
+nfsd_fo_cmd(int cmd, char *datap, int grace_period)
+{
+	struct nameidata nd;
+	void *objp = (void *)datap;
+	int rc=0;
+
+	if (cmd == NFSD_FO_PATH) { 

{

(quoted diff) +4 -0

+	/* Cluster failover has timing constraints. There is a slight
+	 * performance hit if nlm_fo_unlock_match() is implemented as 
+	 * a match fn (since it will be invoked for each block, share,
+	 * and lock later when the lists are traversed). Instead, we 

Ok, I played around with this and cleaned up the ip/path codepathes to be entirely setup which helped the code quite a bit. Also a few other

cleanups and two bugfixes (postive error code returned and missing path_release) fell out of it. I still don't like what's going on in fs/lockd/svcsubs.c, it would be much better if the cluster unlock code simply didn't use nlm_traverse_files but did it's own loop over the nlm hosts. That should also absolete the second patch.

Index: linux-2.6/fs/nfsd/nfsctl.c
===================================================================

fs/nfsd/nfsctl.c +60 -0

@@ -22,6 +22,7 @@
 #include <linux/seq_file.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
+#include <linux/inet.h>
 #include <linux/string.h>
 #include <linux/smp_lock.h>
 #include <linux/ctype.h>

@@ -35,6 +36,7 @@
 #include <linux/nfsd/cache.h>
 #include <linux/nfsd/xdr.h>
 #include <linux/nfsd/syscall.h>
+#include <linux/lockd/lockd.h>
 
 #include <asm/uaccess.h>
 

@@ -52,6 +54,8 @@ enum {
 	NFSD_Getfs,
 	NFSD_List,
 	NFSD_Fh,
+	NFSD_FO_UnlockIP,
+	NFSD_FO_UnlockFS,
 	NFSD_Threads,
 	NFSD_Pool_Threads,
 	NFSD_Versions,

@@ -88,6 +92,9 @@ static ssize_t write_leasetime(struct fi
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
 #endif
 
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size);
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size);
+
 static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_Svc] = write_svc,
 	[NFSD_Add] = write_add,

@@ -97,6 +104,8 @@ static ssize_t (*write_op[])(struct file
 	[NFSD_Getfd] = write_getfd,
 	[NFSD_Getfs] = write_getfs,
 	[NFSD_Fh] = write_filehandle,
+	[NFSD_FO_UnlockIP] = failover_unlock_ip,
+	[NFSD_FO_UnlockFS] = failover_unlock_fs,
 	[NFSD_Threads] = write_threads,
 	[NFSD_Pool_Threads] = write_pool_threads,
 	[NFSD_Versions] = write_versions,

@@ -288,6 +297,55 @@ static ssize_t write_getfd(struct file *
 	return err;
 }
 
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;
+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;
+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;
+
+	server_ip = in_aton(fo_path);
+	return nlmsvc_failover_ip(server_ip[0]);
+}
+
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
+{
+	struct nameidata nd;
+	char *fo_path;
+	char *mesg;
+	int error;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;
+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;
+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;
+
+	error = path_lookup(fo_path, 0, &nd);
+	if (error)
+		return error;
+
+	error = nlmsvc_failover_path(&nd);
+
+	path_release(&nd);
+	return error;
+}
+
 static ssize_t write_filehandle(struct file *file, char *buf, size_t size)
 {
 	/* request is:

@@ -646,6 +704,8 @@ static int nfsd_fill_super(struct super_
 		[NFSD_Getfd] = {".getfd", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Getfs] = {".getfs", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_List] = {"exports", &exports_operations, S_IRUGO},
+		[NFSD_FO_UnlockIP] = {"unlock_ip", &transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_FO_UnlockFS] = {"unlock_filesystem", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Fh] = {"filehandle", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},

Index: linux-2.6/fs/lockd/svcsubs.c
===================================================================

fs/lockd/svcsubs.c +76 -1

@@ -18,6 +18,8 @@
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
 #include <linux/lockd/sm_inter.h>
+#include <linux/module.h>
+#include <linux/mount.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCSUBS
 

@@ -87,7 +89,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	unsigned int	hash;
 	__be32		nfserr;
 
-	nlm_debug_print_fh("nlm_file_lookup", f);
+	nlm_debug_print_fh("nlm_lookup_file", f);
 
 	hash = file_hash(f);
 

@@ -123,6 +125,9 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 
 	hlist_add_head(&file->f_list, &nlm_files[hash]);
 
+	/* fill in f_iaddr for nlm lock failover */
+	file->f_iaddr = rqstp->rq_daddr;
+
 found:
 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
 	*result = file;

@@ -194,12 +199,63 @@ again:
 	return 0;
 }
 
+static int
+nlmsvc_fo_unlock_match_path(void *datap, struct nlm_file *file)
+{
+	struct nameidata *nd = datap;
+	return nd->mnt == file->f_file->f_path.mnt;
+}
+
+static int
+nlmsvc_fo_unlock_match_ip(void *datap, struct nlm_file *file)
+{
+	struct in_addr *in = datap;
+
+	return file->f_iaddr.addr.s_addr == in->s_addr;
+}
+
+/*
+ * To fit the logic into current lockd code structure, we add a
+ * little wrapper function here. The real matching task should be
+ * carried out by nlm_fo_check_fsid().
+ */
+
+static int nlmsvc_fo_match_path(struct nlm_host *dummy1,
+		struct nlm_host *dummy2)
+{
+        return 1;
+}
+
+static int nlmsvc_fo_match_ip(struct nlm_host *dummy1, struct nlm_host *dummy2)
+{
+        return 1;
+}
+
 /*
  * Inspect a single file
  */
 static inline int
 nlm_inspect_file(struct nlm_host *host, struct nlm_file *file, nlm_host_match_fn_t match)
 {
+	/*
+	 * Cluster failover has timing constraints. There is a slight
+	 * performance hit if nlm_fo_unlock_match() is implemented as
+	 * a match fn (since it will be invoked for each block, share,
+	 * and lock later when the lists are traversed). Instead, we
+	 * add path-matching logic into the following unlikely clause.
+	 * If matches, the dummy nlmsvc_fo_match will always return
+	 * true.
+	 */
+	dprintk("nlm_inspect_files: file=%p\n", file);
+	if (unlikely(match == nlmsvc_fo_match_path)) {
+		if (!nlmsvc_fo_unlock_match_path(host, file))
+			return 0;
+	}
+	if (unlikely(match == nlmsvc_fo_match_ip)) {
+		if (!nlmsvc_fo_unlock_match_ip(host, file))
+			return 0;
+	}
+
 	nlmsvc_traverse_blocks(host, file, match);
 	nlmsvc_traverse_shares(host, file, match);
 	return nlm_traverse_locks(host, file, match);

@@ -370,3 +426,22 @@ nlmsvc_invalidate_all(void)
 	 */
 	nlm_traverse_files(NULL, nlmsvc_is_client);
 }
+
+/*
+ * Release locks associated with an export fsid upon failover
+ *      invoked via nfsd nfsctl call (write_fo_unlock).
+ */
+int
+nlmsvc_failover_path(struct nameidata *nd)
+{
+	return nlm_traverse_files((struct nlm_host*)nd, nlmsvc_fo_match_path);
+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_path);
+
+int
+nlmsvc_failover_ip(__be32 server_addr)
+{
+	return nlm_traverse_files((struct nlm_host *)&server_addr,
+				nlmsvc_fo_match_ip);
+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);

Index: linux-2.6/include/linux/lockd/lockd.h
===================================================================

include/linux/lockd/lockd.h +7 -0

@@ -113,6 +113,7 @@ struct nlm_file {
 	unsigned int		f_locks;	/* guesstimate # of locks */
 	unsigned int		f_count;	/* reference count */
 	struct mutex		f_mutex;	/* avoid concurrent access */
+	union svc_addr_u	f_iaddr;	/* server ip for failover */
 };
 
 /*

@@ -214,6 +215,12 @@ void		  nlmsvc_mark_resources(void);
 void		  nlmsvc_free_host_resources(struct nlm_host *);
 void		  nlmsvc_invalidate_all(void);
 
+/*
+ * Cluster failover support
+ */
+int           nlmsvc_failover_path(struct nameidata *nd);
+int           nlmsvc_failover_ip(__be32 server_addr);
+
 static __inline__ struct inode *
 nlmsvc_file_inode(struct nlm_file *file)
 {

Quoted 3 lines · Christoph Hellwig

Ok, I played around with this and cleaned up the ip/path codepathes to be entirely setup which helped the code quite a bit. Also a few other

Thanks for doing this :) . In the middle of running it with our cluster test - if passed, will repost it. Get your "signed-off" line ready ?

Quoted 6 lines · Wendy Cheng

Quoted 3 lines · Christoph Hellwig

Ok, I played around with this and cleaned up the ip/path codepathes to be entirely setup which helped the code quite a bit. Also a few other

Thanks for doing this :) . In the middle of running it with our cluster test - if passed, will repost it. Get your "signed-off" line ready ?

Not quite yet. I'm not happy with what's going on in svcsubs.c in the current form.

I've added another (untested) idea patch below which adds a second match function to nlm_traverse_files to remove the current hardcoded hack. If that works out we'll just need to incorporate Neil's feedback to the second patch somehow.

This is a combined patch that has:

* changes made by Christoph Hellwig
* code segment that handles f_locks so we would not walk inode->i_flock list twice.

If agreed, please re-add your "ack-by" and "signed-off" lines respectively. Thanks ...

-- Wendy

Two new NFSD procfs files are added:
/proc/fs/nfsd/unlock_ip
/proc/fs/nfsd/unlock_filesystem

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now) as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

Signed-off-by S. Wendy Cheng <[email protected]>

Signed-off-by Lon Hohberger <[email protected]>

This is a combined patch that has:

* changes made by Christoph Hellwig
* code segment that handles f_locks so we would not walk inode->i_flock list twice.

If agreed, please re-add your "ack-by" and "signed-off" lines respectively. Thanks ...

-- Wendy

Two new NFSD procfs files are added:
/proc/fs/nfsd/unlock_ip
/proc/fs/nfsd/unlock_filesystem

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now) as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

Signed-off-by S. Wendy Cheng <[email protected]>

Signed-off-by Lon Hohberger <[email protected]>

 fs/lockd/svcsubs.c          |   68 ++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfsctl.c            |   62 ++++++++++++++++++++++++++++++++++++++++
 include/linux/lockd/lockd.h |    7 ++++
 3 files changed, 129 insertions(+), 8 deletions(-)

fs/nfsd/nfsctl.c +9 -0

@@ -22,6 +22,7 @@
 #include <linux/seq_file.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
+#include <linux/inet.h>
 #include <linux/string.h>
 #include <linux/smp_lock.h>
 #include <linux/ctype.h>

@@ -35,6 +36,7 @@
 #include <linux/nfsd/cache.h>
 #include <linux/nfsd/xdr.h>
 #include <linux/nfsd/syscall.h>
+#include <linux/lockd/lockd.h>
 
 #include <asm/uaccess.h>
 

@@ -52,6 +54,8 @@ enum {
 	NFSD_Getfs,
 	NFSD_List,
 	NFSD_Fh,
+	NFSD_FO_UnlockIP,
+	NFSD_FO_UnlockFS,
 	NFSD_Threads,
 	NFSD_Pool_Threads,
 	NFSD_Versions,

@@ -88,6 +92,9 @@ static ssize_t write_leasetime(struct fi
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
 #endif
 
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size);
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size);
+
 static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_Svc] = write_svc,
 	[NFSD_Add] = write_add,

@@ -97,6 +104,8 @@ static ssize_t (*write_op[])(struct file
 	[NFSD_Getfd] = write_getfd,
 	[NFSD_Getfs] = write_getfs,
 	[NFSD_Fh] = write_filehandle,
+	[NFSD_FO_UnlockIP] = failover_unlock_ip,
+	[NFSD_FO_UnlockFS] = failover_unlock_fs,
 	[NFSD_Threads] = write_threads,
 	[NFSD_Pool_Threads] = write_pool_threads,
 	[NFSD_Versions] = write_versions,

@@ -288,6 +297,55 @@ static ssize_t write_getfd(struct file *
 	return err;
 }

fs/nfsd/nfsctl.c +10 -0

+	return nlmsvc_failover_ip(server_ip);
+}
+
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
+{
+	struct nameidata nd;
+	char *fo_path;
+	char *mesg;
+	int error;
+

fs/nfsd/nfsctl.c +15 -0

+
+	error = path_lookup(fo_path, 0, &nd);
+	if (error)
+		return error;
+
+	error = nlmsvc_failover_path(&nd);
+
+	path_release(&nd);
+	return error;
+}
+
 static ssize_t write_filehandle(struct file *file, char *buf, size_t size)
 {
 	/* request is:

@@ -646,6 +704,10 @@ static int nfsd_fill_super(struct super_
 		[NFSD_Getfd] = {".getfd", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Getfs] = {".getfs", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_List] = {"exports", &exports_operations, S_IRUGO},
+		[NFSD_FO_UnlockIP] = {"unlock_ip",
+					&transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
+					&transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Fh] = {"filehandle", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},

fs/lockd/svcsubs.c +16 -4

@@ -18,6 +18,8 @@
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
 #include <linux/lockd/sm_inter.h>
+#include <linux/module.h>
+#include <linux/mount.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCSUBS
 

@@ -87,7 +89,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	unsigned int	hash;
 	__be32		nfserr;
 
-	nlm_debug_print_fh("nlm_file_lookup", f);
+	nlm_debug_print_fh("nlm_lookup_file", f);
 
 	hash = file_hash(f);
 

@@ -123,6 +125,9 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 
 	hlist_add_head(&file->f_list, &nlm_files[hash]);
 
+	/* fill in f_iaddr for nlm lock failover */
+	file->f_iaddr = rqstp->rq_daddr;
+
 found:
 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
 	*result = file;

@@ -194,6 +199,12 @@ again:
 	return 0;
 }
 
+static int
+nlmsvc_always_match(struct nlm_host *dummy1, struct nlm_host *dummy2)
+{
+	return 1;
+}
+
 /*
  * Inspect a single file
  */

@@ -230,27 +241,37 @@ nlm_file_inuse(struct nlm_file *file)
  * Loop over all files in the file table.
  */
 static int
-nlm_traverse_files(struct nlm_host *host, nlm_host_match_fn_t match)
+nlm_traverse_files(void *data, nlm_host_match_fn_t match,
+		int (*failover)(void *data, struct nlm_file *file))
 {
 	struct hlist_node *pos, *next;
 	struct nlm_file	*file;
-	int i, ret = 0;
+	int i, ret = 0, inspect_file;
 
 	mutex_lock(&nlm_file_mutex);
 	for (i = 0; i < FILE_NRHASH; i++) {
 		hlist_for_each_entry_safe(file, pos, next, &nlm_files[i], f_list) {
 			file->f_count++;
 			mutex_unlock(&nlm_file_mutex);
+			inspect_file = 1;
 
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, match))

fs/lockd/svcsubs.c +35 -4

 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
+			if (!file->f_locks && list_empty(&file->f_blocks)
 			 && !file->f_shares && !file->f_count) {
 				hlist_del(&file->f_list);
 				nlmsvc_ops->fclose(file->f_file);

@@ -337,7 +358,7 @@ void
 nlmsvc_mark_resources(void)
 {
 	dprintk("lockd: nlmsvc_mark_resources\n");
-	nlm_traverse_files(NULL, nlmsvc_mark_host);
+	nlm_traverse_files(NULL, nlmsvc_mark_host, NULL);
 }
 
 /*

@@ -348,7 +369,7 @@ nlmsvc_free_host_resources(struct nlm_ho
 {
 	dprintk("lockd: nlmsvc_free_host_resources\n");
 
-	if (nlm_traverse_files(host, nlmsvc_same_host)) {
+	if (nlm_traverse_files(host, nlmsvc_same_host, NULL)) {
 		printk(KERN_WARNING
 			"lockd: couldn't remove all locks held by %s\n",
 			host->h_name);

@@ -368,5 +389,36 @@ nlmsvc_invalidate_all(void)
 	 * turn, which is about as inefficient as it gets.
 	 * Now we just do it once in nlm_traverse_files.
 	 */
-	nlm_traverse_files(NULL, nlmsvc_is_client);
+	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
+}
+
+static int
+nlmsvc_failover_file_ok_path(void *datap, struct nlm_file *file)
+{
+	struct nameidata *nd = datap;
+	return nd->mnt == file->f_file->f_path.mnt;
+}
+
+int
+nlmsvc_failover_path(struct nameidata *nd)
+{
+	return nlm_traverse_files(nd, nlmsvc_always_match,
+			nlmsvc_failover_file_ok_path);
+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_path);
+
+static int
+nlmsvc_failover_file_ok_ip(void *datap, struct nlm_file *file)
+{
+	__be32 *server_addr = datap;
+
+	return file->f_iaddr.addr.s_addr == *server_addr;
+}
+
+int
+nlmsvc_failover_ip(__be32 server_addr)
+{
+	return nlm_traverse_files(&server_addr, nlmsvc_always_match,
+			nlmsvc_failover_file_ok_ip);
 }
+EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);

include/linux/lockd/lockd.h +7 -0

@@ -113,6 +113,7 @@ struct nlm_file {
 	unsigned int		f_locks;	/* guesstimate # of locks */
 	unsigned int		f_count;	/* reference count */
 	struct mutex		f_mutex;	/* avoid concurrent access */
+	union svc_addr_u	f_iaddr;	/* server ip for failover */
 };
 
 /*

@@ -214,6 +215,12 @@ void		  nlmsvc_mark_resources(void);
 void		  nlmsvc_free_host_resources(struct nlm_host *);
 void		  nlmsvc_invalidate_all(void);
 
+/*
+ * Cluster failover support
+ */
+int           nlmsvc_failover_path(struct nameidata *nd);
+int           nlmsvc_failover_ip(__be32 server_addr);
+
 static __inline__ struct inode *
 nlmsvc_file_inode(struct nlm_file *file)
 {

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 308

Quoted 10 lines

fs/nfsd/nfsctl.c +9 -0

+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;

Not only is size never negative, it's actually an unsigned type here, so this is a no-op.

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 311

Quoted 4 lines

fs/nfsd/nfsctl.c +3 -0

+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;

The other write methods in this file actually just do

	if (buf[size-1] != '\n')
		return -EINVAL;

I don't know why. But absent some reason, I'd rather these two new files behaved the same as existing ones.

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 315

Quoted 4 lines

fs/nfsd/nfsctl.c +4 -0

+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;

"mesg" is unneeded here, right? You can just do:

	fo_path = buf;
	if (qword_get(&buf, buf, size) < 0)

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	server_ip = in_aton(fo_path);

It'd be nice if we could sanity-check this. (Is there code already in the kernel someplace to do this?)

Quoted 14 lines · On Monday January 14, [email protected] wrote:

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 308

Quoted 11 lines

+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)

fs/nfsd/nfsctl.c +8 -0

+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;

Not only is size never negative, it's actually an unsigned type here, so this is a no-op.

No, It it equivalent to

          if (size == 0)

which alternative is clearer and more maintainable is debatable.

Quoted 8 lines

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 311

Quoted 3 lines

fs/nfsd/nfsctl.c +3 -0

+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;

The other write methods in this file actually just do

	if (buf[size-1] != '\n')
		return -EINVAL;

and those which don't check for size == 0 are underflowing an array. That should probably be fixed.

Quoted 11 lines

I don't know why. But absent some reason, I'd rather these two new files behaved the same as existing ones.

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 315

Quoted 4 lines

fs/nfsd/nfsctl.c +4 -0

+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;

"mesg" is unneeded here, right? You can just do:

Quoted 8 lines

	fo_path = buf;
	if (qword_get(&buf, buf, size) < 0)

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	server_ip = in_aton(fo_path);

It'd be nice if we could sanity-check this. (Is there code already in the kernel someplace to do this?)

In ip_map_parse we do:

	if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
		return -EINVAL;
	...
	addr.s_addr =
		htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);

I suspect that would fit in an inline function somewhere quite nicely. but where?

Quoted 13 lines · bfields at fieldses.org

Review

Quoted 10 lines

(quoted diff) +9 -0

+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;

Not only is size never negative, it's actually an unsigned type here, so this is a no-op.

No, It it equivalent to

          if (size == 0)

which alternative is clearer and more maintainable is debatable.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;

The other write methods in this file actually just do

Quoted 2 lines

	if (buf[size-1] != '\n')
		return -EINVAL;

and those which don't check for size == 0 are underflowing an array. That should probably be fixed.

?

--b.

commit 6685389d610950126f700d25f3d010c7049441c3

Author: J. Bruce Fields <[email protected]>
Date:   Tue Jan 22 17:40:42 2008 -0500

nfsd: more careful input validation in nfsctl write methods

Neil Brown points out that we're checking buf[size-1] in a couple places without first checking whether size is zero.

Actually, given the implementation of simple_transaction_get(), buf[-1] is zero, so in both of these cases the subsequent check of the value of buf[size-1] will catch this case.

But it seems fragile to depend on that, so add explicit checks for this case.

Signed-off-by J. Bruce Fields <[email protected]>

Cc Neil Brown <[email protected]>

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 308

Quoted 11 lines

fs/nfsd/nfsctl.c +9 -0

+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;

Not only is size never negative, it's actually an unsigned type here, so this is a no-op.

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 311

Quoted 5 lines

fs/nfsd/nfsctl.c +3 -0

+
+	if (buf[size-1] == '\n')
+		buf[size-1] = 0;

The other write methods in this file actually just do

	if (buf[size-1] != '\n')
		return -EINVAL;

I don't know why. But absent some reason, I'd rather these two new files behaved the same as existing ones.

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 315

Quoted 6 lines

fs/nfsd/nfsctl.c +4 -0

+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;

"mesg" is unneeded here, right? You can just do:

	fo_path = buf;
	if (qword_get(&buf, buf, size) < 0)

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+	server_ip = in_aton(fo_path);

It'd be nice if we could sanity-check this. (Is there code already in the kernel someplace to do this?)

And, this isn't your problem for now, but eventually I guess this will have to accept an ivp6 address as well?

Quoted 13 lines

fs/nfsd/nfsctl.c +12 -0

+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
+{
+	struct nameidata nd;
+	char *fo_path;
+	char *mesg;
+	int error;
+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;
+
+	if (buf[size-1] == '\n')

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 315

Quoted 6 lines

fs/nfsd/nfsctl.c +5 -0

+		buf[size-1] = 0;
+
+	fo_path = mesg = buf;
+	if (qword_get(&mesg, fo_path, size) < 0)
+		return -EINVAL;

Same comments as above.

Quoted 32 lines

/*
* Inspect a single file
*/

(quoted diff) +10 -3

@@ -230,27 +241,37 @@ nlm_file_inuse(struct nlm_file *file)
  * Loop over all files in the file table.
  */
 static int
-nlm_traverse_files(struct nlm_host *host, nlm_host_match_fn_t match)
+nlm_traverse_files(void *data, nlm_host_match_fn_t match,
+		int (*failover)(void *data, struct nlm_file *file))
 {
 	struct hlist_node *pos, *next;
 	struct nlm_file	*file;
-	int i, ret = 0;
+	int i, ret = 0, inspect_file;
 
 	mutex_lock(&nlm_file_mutex);
 	for (i = 0; i < FILE_NRHASH; i++) {
 		hlist_for_each_entry_safe(file, pos, next, &nlm_files[i], f_list) {
 			file->f_count++;
 			mutex_unlock(&nlm_file_mutex);
+			inspect_file = 1;
 
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, match))
+
+			if (unlikely(failover)) {
+				if (!failover(data, file)) {
+					inspect_file = 0;
+					file->f_locks = nlm_file_inuse(file);
+				}

Review nlm_file_inuse(struct nlm_file *file) · line 270

Quoted 5 lines

(quoted diff) +3 -0

+			}
+
+			if (inspect_file && nlm_inspect_file(data, file, match))
 				ret = 1;
 

This seems quite complicated! I don't have an alternative suggestion, though.

Quoted 6 lines · J. Bruce Fields

Quoted 6 lines

+static ssize_t failover_unlock_ip(struct file *file, char *buf,
size_t size)

fs/nfsd/nfsctl.c +4 -0

+{
+	__be32 server_ip;
+	char *fo_path;
+	char *mesg;

Quoted 9 lines

fs/nfsd/nfsctl.c static ssize_t write_getfd(struct file * · line 308

Quoted 5 lines

fs/nfsd/nfsctl.c +4 -0

+
+	/* sanity check */
+	if (size <= 0)
+		return -EINVAL;

Not only is size never negative, it's actually an unsigned type here, so this is a no-op.

Based on comment from Neil, let's keep this ?

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+	server_ip = in_aton(fo_path);

It'd be nice if we could sanity-check this. (Is there code already in the kernel someplace to do this?)

The argument here is that if this is a bogus address, the search (of nlm files list) will fail (not found) later anyway.

Failover is normally
time critical. Quicker we finish, less issues will be seen. The sanity check here can be skipped (imo).

Quoted 3 lines

And, this isn't your problem for now, but eventually I guess this will have to accept an ivp6 address as well?

yeah .. Thinking about this right now.. May be borrowing the code from ip_map_parse() as Neil pointed out in another mail ?

This is a combined patch that has:

* changes made by Christoph Hellwig
* code segment that handles f_locks so we would not walk inode->i_flock list twice.

If agreed, please re-add your "ack-by" and "signed-off" lines respectively. Thanks ...

(quoted diff) +2 -2

-	int i, ret = 0;
+	int i, ret = 0, inspect_file;
 
 	mutex_lock(&nlm_file_mutex);
 	for (i = 0; i < FILE_NRHASH; i++) {
 		hlist_for_each_entry_safe(file, pos, next, &nlm_files[i], f_list) {
 			file->f_count++;
 			mutex_unlock(&nlm_file_mutex);
+			inspect_file = 1;
 
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, match))

Quoted 7 lines · wcheng at redhat.com

This is a combined patch that has:

* changes made by Christoph Hellwig
* code segment that handles f_locks so we would not walk inode->i_flock list twice.

if (unlikely(failover) &&

    !failover(data, file))
	file->f_locks = nlm_file_inuse(file);
else if (nlm_inspect_file(data, file, match))
	ret = 1;

Though the logic still isn't very clear... maybe:

if (likely(failover == NULL) ||

    failover(data, file))
	ret |= nlm_inspect_file(data, file, match);
else
    file->f_locks = nlm_file_inuse(file);

Actually I would like to make nlm_inspect_file return 'void'. The returned value of '1' is ultimately either ignored or it triggers a BUG(). And the case where it triggers a BUG is the "host != NULL" case. (I think - if someone could check, that would be good). So putting BUG_ON(host) in nlm_traverse_locks (along with a nice big comment) would mean we can discard the return value from nlm_traverse_locks and nlm_inspect_file and nlm_traverse_files.

Quoted 5 lines

Review

Quoted 6 lines

(quoted diff) +1 -1

 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
+			if (!file->f_locks && list_empty(&file->f_blocks)

Is this change actually achieving something? or is it just noise?

NeilBrown

I don't feel comfortable to change the existing code structure, especially a BUG() statement. It would be better to separate lock failover function away from lockd code clean-up. This is to make it easier for problem isolations (just in case).

On the other hand, if we view "ret" as a file count that tells us how many files fail to get unlocked, it would be great for debugging purpose. So the changes I made (currently in the middle of cluster testing) end up like this:

if (likely(is_failover_file == NULL) ||
is_failover_file(data, file)) {
/*
* Note that nlm_inspect_file updates f_locks
* and ret is the number of files that can't
* be unlocked.
*/
ret += nlm_inspect_file(data, file, match);
} else
file->f_locks = nlm_file_inuse(file);

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +1 -1

 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
+			if (!file->f_locks && list_empty(&file->f_blocks)

Is this change actually achieving something? or is it just noise?

Not really - but I thought checking for f_locks would be faster (tiny bit of optimization :))

Quoted 5 lines · wcheng at redhat.com

I don't feel comfortable to change the existing code structure, especially a BUG() statement. It would be better to separate lock failover function away from lockd code clean-up. This is to make it easier for problem isolations (just in case).

Fair enough.

Quoted 5 lines

On the other hand, if we view "ret" as a file count that tells us how many files fail to get unlocked, it would be great for debugging purpose. So the changes I made (currently in the middle of cluster testing) end up like this:

Quoted 11 lines

if (likely(is_failover_file == NULL) ||
is_failover_file(data, file)) {
/*
* Note that nlm_inspect_file updates f_locks
* and ret is the number of files that can't
* be unlocked.
*/
ret += nlm_inspect_file(data, file, match);
} else
file->f_locks = nlm_file_inuse(file);

Looks good.

Quoted 10 lines

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +1 -1

 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
+			if (!file->f_locks && list_empty(&file->f_blocks)

Is this change actually achieving something? or is it just noise?

Quoted 3 lines

Not really - but I thought checking for f_locks would be faster (tiny bit of optimization :))

You don't want to put the fastest operation first. You want the one that is most likely to fail (as it is an '&&' where failure aborts the sequence).
So you would need some argument (preferably with measurements) to say which of the conditions will fail most often, before rearranging as an optimisation.

Revised version of the patch:

* based on comment from Neil Brown, use sscanf() to parse IP address (a cool idea imo).
* the "ret" inside nlm_traverse_files() is now the file count that can't be unlocked.
* other minor changes from latest round of review.

Thanks to all for the review !

-- Wendy

Two new NFSD procfs files are added:
/proc/fs/nfsd/unlock_ip
/proc/fs/nfsd/unlock_filesystem

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now) as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

Signed-off-by S. Wendy Cheng <[email protected]>

Signed-off-by Lon Hohberger <[email protected]>

Signed-off-by Christoph Hellwig <[email protected]>

 fs/lockd/svcsubs.c          |   66 +++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfsctl.c            |   65 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/lockd/lockd.h |    7 ++++
 3 files changed, 131 insertions(+), 7 deletions(-)

fs/nfsd/nfsctl.c +65 -0

@@ -22,6 +22,7 @@
 #include <linux/seq_file.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
+#include <linux/inet.h>
 #include <linux/string.h>
 #include <linux/smp_lock.h>
 #include <linux/ctype.h>

@@ -35,6 +36,7 @@
 #include <linux/nfsd/cache.h>
 #include <linux/nfsd/xdr.h>
 #include <linux/nfsd/syscall.h>
+#include <linux/lockd/lockd.h>
 
 #include <asm/uaccess.h>
 

@@ -52,6 +54,8 @@ enum {
 	NFSD_Getfs,
 	NFSD_List,
 	NFSD_Fh,
+	NFSD_FO_UnlockIP,
+	NFSD_FO_UnlockFS,
 	NFSD_Threads,
 	NFSD_Pool_Threads,
 	NFSD_Versions,

@@ -88,6 +92,9 @@ static ssize_t write_leasetime(struct fi
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
 #endif
 
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size);
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size);
+
 static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_Svc] = write_svc,
 	[NFSD_Add] = write_add,

@@ -97,6 +104,8 @@ static ssize_t (*write_op[])(struct file
 	[NFSD_Getfd] = write_getfd,
 	[NFSD_Getfs] = write_getfs,
 	[NFSD_Fh] = write_filehandle,
+	[NFSD_FO_UnlockIP] = failover_unlock_ip,
+	[NFSD_FO_UnlockFS] = failover_unlock_fs,
 	[NFSD_Threads] = write_threads,
 	[NFSD_Pool_Threads] = write_pool_threads,
 	[NFSD_Versions] = write_versions,

@@ -288,6 +297,58 @@ static ssize_t write_getfd(struct file *
 	return err;
 }
 
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	char *fo_path, c;
+	int b1, b2, b3, b4;
+
+	/* sanity check */
+	if (size == 0)
+		return -EINVAL;
+
+	if (buf[size-1] != '\n')
+		return -EINVAL;
+
+	fo_path = buf;
+	if (qword_get(&buf, fo_path, size) < 0)
+		return -EINVAL;
+
+	/* get ipv4 address */
+	if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
+		return -EINVAL;
+	server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
+
+	return nlmsvc_failover_ip(server_ip);
+}
+
+static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
+{
+	struct nameidata nd;
+	char *fo_path;
+	int error;
+
+	/* sanity check */
+	if (size == 0)
+		return -EINVAL;
+
+	if (buf[size-1] != '\n')
+		return -EINVAL;
+
+	fo_path = buf;
+	if (qword_get(&buf, fo_path, size) < 0)
+		return -EINVAL;
+
+	error = path_lookup(fo_path, 0, &nd);
+	if (error)
+		return error;
+
+	error = nlmsvc_failover_path(&nd);
+
+	path_release(&nd);
+	return error;
+}
+
 static ssize_t write_filehandle(struct file *file, char *buf, size_t size)
 {
 	/* request is:

@@ -646,6 +707,10 @@ static int nfsd_fill_super(struct super_
 		[NFSD_Getfd] = {".getfd", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Getfs] = {".getfs", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_List] = {"exports", &exports_operations, S_IRUGO},
+		[NFSD_FO_UnlockIP] = {"unlock_ip",
+					&transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
+					&transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Fh] = {"filehandle", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},

fs/lockd/svcsubs.c +59 -7

@@ -18,6 +18,8 @@
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
 #include <linux/lockd/sm_inter.h>
+#include <linux/module.h>
+#include <linux/mount.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCSUBS
 

@@ -87,7 +89,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	unsigned int	hash;
 	__be32		nfserr;
 
-	nlm_debug_print_fh("nlm_file_lookup", f);
+	nlm_debug_print_fh("nlm_lookup_file", f);
 
 	hash = file_hash(f);
 

@@ -123,6 +125,9 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 
 	hlist_add_head(&file->f_list, &nlm_files[hash]);
 
+	/* fill in f_iaddr for nlm lock failover */
+	file->f_iaddr = rqstp->rq_daddr;
+
 found:
 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
 	*result = file;

@@ -194,6 +199,12 @@ again:
 	return 0;
 }
 
+static int
+nlmsvc_always_match(struct nlm_host *dummy1, struct nlm_host *dummy2)
+{
+	return 1;
+}
+
 /*
  * Inspect a single file
  */

@@ -230,7 +241,8 @@ nlm_file_inuse(struct nlm_file *file)
  * Loop over all files in the file table.
  */
 static int
-nlm_traverse_files(struct nlm_host *host, nlm_host_match_fn_t match)
+nlm_traverse_files(void *data, nlm_host_match_fn_t match,
+		int (*is_failover_file)(void *data, struct nlm_file *file))
 {
 	struct hlist_node *pos, *next;
 	struct nlm_file	*file;

@@ -244,8 +256,17 @@ nlm_traverse_files(struct nlm_host *host
 
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, match))
-				ret = 1;
+
+			if (likely(is_failover_file == NULL) ||
+				is_failover_file(data, file)) {
+				/*
+				 *  Note that nlm_inspect_file updates f_locks
+				 *  and ret is the number of files that can't
+				 *  be unlocked.
+				 */
+				ret += nlm_inspect_file(data, file, match);
+			} else
+				file->f_locks = nlm_file_inuse(file);
 
 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;

@@ -337,7 +358,7 @@ void
 nlmsvc_mark_resources(void)
 {
 	dprintk("lockd: nlmsvc_mark_resources\n");
-	nlm_traverse_files(NULL, nlmsvc_mark_host);
+	nlm_traverse_files(NULL, nlmsvc_mark_host, NULL);
 }
 
 /*

@@ -348,7 +369,7 @@ nlmsvc_free_host_resources(struct nlm_ho
 {
 	dprintk("lockd: nlmsvc_free_host_resources\n");
 
-	if (nlm_traverse_files(host, nlmsvc_same_host)) {
+	if (nlm_traverse_files(host, nlmsvc_same_host, NULL)) {
 		printk(KERN_WARNING
 			"lockd: couldn't remove all locks held by %s\n",
 			host->h_name);

@@ -368,5 +389,36 @@ nlmsvc_invalidate_all(void)
 	 * turn, which is about as inefficient as it gets.
 	 * Now we just do it once in nlm_traverse_files.
 	 */
-	nlm_traverse_files(NULL, nlmsvc_is_client);
+	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
+}
+
+static int
+nlmsvc_failover_file_ok_path(void *datap, struct nlm_file *file)
+{
+	struct nameidata *nd = datap;
+	return nd->mnt == file->f_file->f_path.mnt;
+}
+
+int
+nlmsvc_failover_path(struct nameidata *nd)
+{
+	return nlm_traverse_files(nd, nlmsvc_always_match,
+			nlmsvc_failover_file_ok_path);
+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_path);
+
+static int
+nlmsvc_failover_file_ok_ip(void *datap, struct nlm_file *file)
+{
+	__be32 *server_addr = datap;
+
+	return file->f_iaddr.addr.s_addr == *server_addr;
+}
+
+int
+nlmsvc_failover_ip(__be32 server_addr)
+{
+	return nlm_traverse_files(&server_addr, nlmsvc_always_match,
+			nlmsvc_failover_file_ok_ip);
 }
+EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);

include/linux/lockd/lockd.h +7 -0

@@ -113,6 +113,7 @@ struct nlm_file {
 	unsigned int		f_locks;	/* guesstimate # of locks */
 	unsigned int		f_count;	/* reference count */
 	struct mutex		f_mutex;	/* avoid concurrent access */
+	union svc_addr_u	f_iaddr;	/* server ip for failover */
 };
 
 /*

@@ -214,6 +215,12 @@ void		  nlmsvc_mark_resources(void);
 void		  nlmsvc_free_host_resources(struct nlm_host *);
 void		  nlmsvc_invalidate_all(void);
 
+/*
+ * Cluster failover support
+ */
+int           nlmsvc_failover_path(struct nameidata *nd);
+int           nlmsvc_failover_ip(__be32 server_addr);
+
 static __inline__ struct inode *
 nlmsvc_file_inode(struct nlm_file *file)
 {

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

But presumably in some scenarios clients can get confused, and we need to ensure that stale locks are not left behind?

We've discussed this before, but we should get the answer into comments in the code (or on the patches).

Add a more detailed description into the top of the patch itself. I'm working on the resume patch now - it will include an overall write-up in the Documentation directory.

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

The expected sequence of events can be:
1. Tear down the IP address

2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required, write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Quoted 6 lines

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

The expected sequence of events can be:
1. Tear down the IP address

You might consider using iptables at this point for dropping outgoing packets with that source IP address to catch any packet still in flight. It fixed ESTALE problems for me IIRC (NFSv3, UDP).

Quoted 8 lines

2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required, write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Quoted 3 lines · J. Bruce Fields

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

Failover normally is done via virtual IP address - so per-ip base method should be the core routine. However, for non-cluster filesystem such as ext3/4, changing server also implies umount. If there are clients not following rule and obtaining locks via different ip interfaces, umount would fail that ends up aborting the failover process. That's the place we need the per-filesystem method.

ServerA:
1. Tear down the IP address
2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required,
write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Sometime ago we were looking at "export name" as the core method (so per-filesystem method is a subset of that). Unfortunately, the prototype efforts showed the code would be too intrusive (if filesystem sub-tree is exported).

Quoted 6 lines

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

This is true for non-cluster filesystem. But a cluster filesystem can be exported from multiple servers.

Quoted 3 lines

But presumably in some scenarios clients can get confused, and we need to ensure that stale locks are not left behind?

Yes.

Quoted 4 lines

We've discussed this before, but we should get the answer into comments in the code (or on the patches).

ok, working on it. or should we add something into linux/Documentation to describe the overall logic ?

Yeah, sounds good. Maybe under Documentation/filesystems? And it might also be helpful to leave a reference to it in the code, e.g., in nfsctl.c:

	/*
	 * The following are used for failover; see
	 * Documentation/filesystems/nfsd-failover.txt for details.
	 */

--b.

Quoted 9 lines · J. Bruce Fields

Yeah, sounds good. Maybe under Documentation/filesystems? And it might also be helpful to leave a reference to it in the code, e.g., in nfsctl.c:

	/*
	 * The following are used for failover; see
	 * Documentation/filesystems/nfsd-failover.txt for details.
	 */

Quoted 2 lines

--b.

ok, looks good ... but let's "fix" this on the resume patch ? or you want it on the unlock patch *now* ?

Quoted 3 lines · J. Bruce Fields

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

Failover normally is done via virtual IP address - so per-ip base method should be the core routine. However, for non-cluster filesystem such as ext3/4, changing server also implies umount. If there are clients not following rule and obtaining locks via different ip interfaces, umount would fail that ends up aborting the failover process. That's the place we need the per-filesystem method.

ServerA:
1. Tear down the IP address
2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required,
write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Sometime ago we were looking at "export name" as the core method (so per-filesystem method is a subset of that). Unfortunately, the prototype efforts showed the code would be too intrusive (if filesystem sub-tree is exported).

Quoted 6 lines

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

This is true for non-cluster filesystem. But a cluster filesystem can be exported from multiple servers.

Quoted 33 lines · Wendy Cheng

Quoted 4 lines · J. Bruce Fields

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

Failover normally is done via virtual IP address - so per-ip base method should be the core routine. However, for non-cluster filesystem such as ext3/4, changing server also implies umount. If there are clients not following rule and obtaining locks via different ip interfaces, umount would fail that ends up aborting the failover process. That's the place we need the per-filesystem method.

ServerA:
1. Tear down the IP address
2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required,
write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Sometime ago we were looking at "export name" as the core method (so per-filesystem method is a subset of that). Unfortunately, the prototype efforts showed the code would be too intrusive (if filesystem sub-tree is exported).

Quoted 7 lines

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

Quoted 3 lines

This is true for non-cluster filesystem. But a cluster filesystem can be exported from multiple servers.

But that last sentence:

it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

is still true in the cluster filesystem case, right?

--b.

Quoted 44 lines · J. Bruce Fields

Quoted 26 lines · Wendy Cheng

Quoted 3 lines · J. Bruce Fields

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

Failover normally is done via virtual IP address - so per-ip base method should be the core routine. However, for non-cluster filesystem such as ext3/4, changing server also implies umount. If there are clients not following rule and obtaining locks via different ip interfaces, umount would fail that ends up aborting the failover process. That's the place we need the per-filesystem method.

ServerA:
1. Tear down the IP address
2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required,
write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Sometime ago we were looking at "export name" as the core method (so per-filesystem method is a subset of that). Unfortunately, the prototype efforts showed the code would be too intrusive (if filesystem sub-tree is exported).

Quoted 9 lines

Quoted 6 lines

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

This is true for non-cluster filesystem. But a cluster filesystem can be exported from multiple servers.

But that last sentence:

it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

Quoted 4 lines

is still true in the cluster filesystem case, right?

--b.

Yes .... Wendy

Quoted 8 lines · Wendy Cheng

Quoted 8 lines

it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

is still true in the cluster filesystem case, right?

--b.

Quoted 1 line

Yes .... Wendy

In one situations (buggy client? Weird network failure?) could that fail to be the case?

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

I assume the intent here with this implementation is that the node taking over will start lock recovery for the ip address? With that perspective, I guess it would be important that each file system only be accessed with a single ip address otherwise lock recovery will not work correctly since another node/ip could accept locks for that filesystem, possibly "stealing" a lock that is in recovery. As I recall, our implementation put the entire filesystem cluster-wide into recovery during fail-over.

Quoted 51 lines · Wendy Cheng

Quoted 49 lines · J. Bruce Fields

Quoted 36 lines · Wendy Cheng

Quoted 4 lines · J. Bruce Fields

Remind me: why do we need both per-ip and per-filesystem methods? In practice, I assume that we'll always do *both*?

Failover normally is done via virtual IP address - so per-ip base method should be the core routine. However, for non-cluster filesystem such as ext3/4, changing server also implies umount. If there are clients not following rule and obtaining locks via different ip interfaces, umount would fail that ends up aborting the failover process. That's the place we need the per-filesystem method.

ServerA:
1. Tear down the IP address
2. Unexport the path
3. Write IP to /proc/fs/nfsd/unlock_ip to unlock files
4. If unmount required,
write path name to /proc/fs/nfsd/unlock_filesystem, then unmount.
5. Signal peer to begin take-over.

Sometime ago we were looking at "export name" as the core method (so per-filesystem method is a subset of that). Unfortunately, the prototype efforts showed the code would be too intrusive (if filesystem sub-tree is exported).

Quoted 7 lines

We're migrating clients by moving a server ip address from one node to another. And I assume we're permitting at most one node to export each filesystem at a time. So it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

Quoted 4 lines

This is true for non-cluster filesystem. But a cluster filesystem can be exported from multiple servers.

But that last sentence:

it *should* be the case that the set of locks held on the filesystem(s) that are moving are the same as the set of locks held by the virtual ip that is moving.

Quoted 7 lines

Quoted 5 lines

is still true in the cluster filesystem case, right?

--b.

Yes .... Wendy

In one situations (buggy client? Weird network failure?) could that fail to be the case?

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

--b.

Quoted 4 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

Quoted 2 lines

--b.

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Remember,
locking is about latency. Adding more checking will hurt latency.

To summarize a phone conversation from today:

Quoted 5 lines · Wendy Cheng

Quoted 4 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

Quoted 8 lines

Quoted 2 lines

--b.

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.

Quoted 6 lines

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.

Maybe a silly question but what about using "exportfs -r" for this?

To summarize a phone conversation from today:

Quoted 5 lines · Wendy Cheng

Quoted 4 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

Quoted 8 lines

Quoted 2 lines

--b.

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.
- In the case of a filesystem that may be mounted from multiple nodes at once, we need to make sure we're not leaving a window allowing other applications to claim locks that nfs clients haven't recovered yet.
- Ideally we'd like this to be possible without making the filesystem block all lock requests during a 90-second grace period; instead it should only have to block those requests that conflict with to-be-recovered locks.
- All this should work for nfsv4, where we want to eventually also allow migration of individual clients, and client-initiated failover.

I absolutely don't want to delay solving this particular problem until all the above is figured out, but I would like to be reasonably confident that the new user-interface can be extended naturally to handle the above cases; or at least that it won't unnecessarily complicate their implementation.

I'll try to sketch an implementation of most of the above in the next week.

Quoted 37 lines · J. Bruce Fields

To summarize a phone conversation from today:

Quoted 6 lines · Wendy Cheng

Quoted 4 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

Quoted 7 lines

Quoted 3 lines

--b.

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.
- In the case of a filesystem that may be mounted from multiple nodes at once, we need to make sure we're not leaving a window allowing other applications to claim locks that nfs clients haven't recovered yet.
- Ideally we'd like this to be possible without making the filesystem block all lock requests during a 90-second grace period; instead it should only have to block those requests that conflict with to-be-recovered locks.
- All this should work for nfsv4, where we want to eventually also allow migration of individual clients, and client-initiated failover.

Quoted 9 lines

I absolutely don't want to delay solving this particular problem until all the above is figured out, but I would like to be reasonably confident that the new user-interface can be extended naturally to handle the above cases; or at least that it won't unnecessarily complicate their implementation.

I'll try to sketch an implementation of most of the above in the next week.

Bah. Apologies, this is taking me longer than it should to figure out--I've only barely started writing patches.

The basic idea, though:

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

So it makes sense to me to restrict locking from the beginning to prevent that from happening. Therefore I'd like to add a call at the beginning like:

	echo "192.168.1.1 /exports/example" > /proc/fs/nfsd/start_grace

before any exports are set up, which both starts a grace period, and tells nfs to allow locks on the filesystem /exports/example only if they're addressed to the server ip 192.168.1.1. Then on shutdown,

	echo "192.168.1.1" >/proc/fs/nfsd/unlock_ip

should be sufficient to guarantee that nfsd/lockd no longer holds locks on /exports/example.

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

The other advantage to having the server-ip from the start is that at the time we make lock requests to the cluster filesystem, we can tell it that the locks associated with 192.168.1.1 are special: they may migrate as a group to another node, and on node failure they should (if possible) be held to give a chance for another node to take them over.

Internally I'd like to have an object like

	struct lock_manager {
		char *lm_name;
		...
	}

for each server ip address. A pointer to this structure would be passed with each lock request, allowing the filesystem to associate locks to lock_manager's. The name would be a string derived from the server ip address that the cluster can compare to match reclaim requests with the locks that they're reclaiming from another node.

(And in the NFSv4 case we would eventually also allow lock_managers with single nfsv4 client (as opposed to server-ip) granularity.)

Does that seem sane?

But it's taking me longer than I'd like to get patches that implement this. Hopefully by next week I can get working code together for people to look at....

Quoted 101 lines · J. Bruce Fields

Quoted 41 lines · J. Bruce Fields

To summarize a phone conversation from today:

Quoted 8 lines · Wendy Cheng

Quoted 6 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

--b.

Quoted 9 lines

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.
- In the case of a filesystem that may be mounted from multiple nodes at once, we need to make sure we're not leaving a window allowing other applications to claim locks that nfs clients haven't recovered yet.
- Ideally we'd like this to be possible without making the filesystem block all lock requests during a 90-second grace period; instead it should only have to block those requests that conflict with to-be-recovered locks.
- All this should work for nfsv4, where we want to eventually also allow migration of individual clients, and client-initiated failover.

Quoted 9 lines

I absolutely don't want to delay solving this particular problem until all the above is figured out, but I would like to be reasonably confident that the new user-interface can be extended naturally to handle the above cases; or at least that it won't unnecessarily complicate their implementation.

I'll try to sketch an implementation of most of the above in the next week.

Bah. Apologies, this is taking me longer than it should to figure out--I've only barely started writing patches.

The basic idea, though:

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

So it makes sense to me to restrict locking from the beginning to prevent that from happening. Therefore I'd like to add a call at the beginning like:

	echo "192.168.1.1 /exports/example" > /proc/fs/nfsd/start_grace

before any exports are set up, which both starts a grace period, and tells nfs to allow locks on the filesystem /exports/example only if they're addressed to the server ip 192.168.1.1. Then on shutdown,

	echo "192.168.1.1" >/proc/fs/nfsd/unlock_ip

should be sufficient to guarantee that nfsd/lockd no longer holds locks on /exports/example.

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

The other advantage to having the server-ip from the start is that at the time we make lock requests to the cluster filesystem, we can tell it that the locks associated with 192.168.1.1 are special: they may migrate as a group to another node, and on node failure they should (if possible) be held to give a chance for another node to take them over.

Internally I'd like to have an object like

	struct lock_manager {
		char *lm_name;
		...
	}

for each server ip address. A pointer to this structure would be passed with each lock request, allowing the filesystem to associate locks to lock_manager's. The name would be a string derived from the server ip address that the cluster can compare to match reclaim requests with the locks that they're reclaiming from another node.

Quoted 8 lines

(And in the NFSv4 case we would eventually also allow lock_managers with single nfsv4 client (as opposed to server-ip) granularity.)

Does that seem sane?

But it's taking me longer than I'd like to get patches that implement this. Hopefully by next week I can get working code together for people to look at....

This seems somewhat less than scalable and not particularly generally useful except in this specific sort of situation to me. The ratios between servers and clients tend to be one to many, where many can be not uncommonly 4 digits. What happens if the cluster is 1000+ nodes? This strikes me as tough on a systems administrator.

It seems to me that it would be nice to be able to solve this problem someplace else like a cluster lock manager and not clutter up the NFS lock manager.

Perhaps the cluster folks could explain why this problem can't be solved there? Is this another case of attempting to create a cluster concept without actually doing all of the work to make the cluster appear to be a single system?

Or am I misunderstanding the situation here?

Quoted 40 lines · J. Bruce Fields

To summarize a phone conversation from today:

Quoted 9 lines · Wendy Cheng

Quoted 7 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a certain filesystem that wasn't sent to a certain server IP.)

--b.

Quoted 7 lines

It is doable... could be added into the "resume" patch that is currently being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade-off(s). I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.
- In the case of a filesystem that may be mounted from multiple nodes at once, we need to make sure we're not leaving a window allowing other applications to claim locks that nfs clients haven't recovered yet.
- Ideally we'd like this to be possible without making the filesystem block all lock requests during a 90-second grace period; instead it should only have to block those requests that conflict with to-be-recovered locks.
- All this should work for nfsv4, where we want to eventually also allow migration of individual clients, and client-initiated failover.

Quoted 9 lines

I absolutely don't want to delay solving this particular problem until all the above is figured out, but I would like to be reasonably confident that the new user-interface can be extended naturally to handle the above cases; or at least that it won't unnecessarily complicate their implementation.

I'll try to sketch an implementation of most of the above in the next week.

Bah. Apologies, this is taking me longer than it should to figure out--I've only barely started writing patches.

The basic idea, though:

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

So it makes sense to me to restrict locking from the beginning to prevent that from happening. Therefore I'd like to add a call at the beginning like:

	echo "192.168.1.1 /exports/example" > /proc/fs/nfsd/start_grace

before any exports are set up, which both starts a grace period, and tells nfs to allow locks on the filesystem /exports/example only if they're addressed to the server ip 192.168.1.1. Then on shutdown,

	echo "192.168.1.1" >/proc/fs/nfsd/unlock_ip

should be sufficient to guarantee that nfsd/lockd no longer holds locks on /exports/example.

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

The other advantage to having the server-ip from the start is that at the time we make lock requests to the cluster filesystem, we can tell it that the locks associated with 192.168.1.1 are special: they may migrate as a group to another node, and on node failure they should (if possible) be held to give a chance for another node to take them over.

Internally I'd like to have an object like

	struct lock_manager {
		char *lm_name;
		...
	}

for each server ip address. A pointer to this structure would be passed with each lock request, allowing the filesystem to associate locks to lock_manager's. The name would be a string derived from the server ip address that the cluster can compare to match reclaim requests with the locks that they're reclaiming from another node.

(And in the NFSv4 case we would eventually also allow lock_managers with single nfsv4 client (as opposed to server-ip) granularity.)

Does that seem sane?

But it's taking me longer than I'd like to get patches that implement this. Hopefully by next week I can get working code together for people to look at....

--b.

Quoted 6 lines · J. Bruce Fields

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

That is not a correct assumption. The two commands (unlock_ip and unlock_pathname) are not necessarily called together. It is ok for local filesystem (ext3) but not for cluster filesystem where the very same filesystem (or subtree) can be exported from multiple servers using different subtrees.

Also as we discussed before, it is
"unlock_filesystem", *not* "unlock_pathname" (this implies sub-tree exports) due to implementation difficulties (see the "Implementation Notes" from http://people.redhat.com/wcheng/Patches/NFS/NLM/004.txt).

Quoted 6 lines

So it makes sense to me to restrict locking from the beginning to prevent that from happening. Therefore I'd like to add a call at the beginning like:

	echo "192.168.1.1 /exports/example" > /proc/fs/nfsd/start_grace

My second patch set is about to be sent out (doing text description at this moment .. sorry for the delay).

Quoted 25 lines

before any exports are set up, which both starts a grace period, and tells nfs to allow locks on the filesystem /exports/example only if they're addressed to the server ip 192.168.1.1. Then on shutdown,

	echo "192.168.1.1" >/proc/fs/nfsd/unlock_ip

should be sufficient to guarantee that nfsd/lockd no longer holds locks on /exports/example.

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

The other advantage to having the server-ip from the start is that at the time we make lock requests to the cluster filesystem, we can tell it that the locks associated with 192.168.1.1 are special: they may migrate as a group to another node, and on node failure they should (if possible) be held to give a chance for another node to take them over.

Internally I'd like to have an object like

	struct lock_manager {
		char *lm_name;
		...
	}

Quoted 6 lines

for each server ip address. A pointer to this structure would be passed with each lock request, allowing the filesystem to associate locks to lock_manager's. The name would be a string derived from the server ip address that the cluster can compare to match reclaim requests with the locks that they're reclaiming from another node.

I still don't have a warm feeling about adding this (at this moment) - somehow feel we over-engineer the lock failover issues.

Remember lock failover is just a small portion of the general NFS server failover (for both HA and load balancing purpose) issues. Maybe we should have something simple and usable for 2.6 kernel before adding this type of complication ?

Quoted 9 lines · Wendy Cheng

Quoted 7 lines · J. Bruce Fields

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

Quoted 6 lines

That is not a correct assumption. The two commands (unlock_ip and unlock_pathname) are not necessarily called together. It is ok for local filesystem (ext3) but not for cluster filesystem where the very same filesystem (or subtree) can be exported from multiple servers using different subtrees.

Ouch. Are people really doing that, and why? What happens if the subtrees share files (because of hard links) that are locked from both nodes?

Quoted 6 lines

Also as we discussed before, it is
"unlock_filesystem", *not* "unlock_pathname" (this implies sub-tree exports) due to implementation difficulties (see the "Implementation Notes" from http://people.redhat.com/wcheng/Patches/NFS/NLM/004.txt).

Unless I misread the latest patch, it's actually matching on the vfsmount, right?

I guess that means we *could* handle the above situation by doing a

	mount --bind /path/to/export/point /path/to/export/point

on each export, at which point there will be a separate vfsmount for each export point?

But I don't think that's what we really want. The goal is to ensure that the nfs server holds no locks on a disk filesystem so we can unmount it completely from this machine and mount it elsewhere. So we should really be removing all locks for the superblock, not just for a particular mount of that superblock. Otherwise we'll have odd problems if someone happens to do the unlock_filesystem downcall from a different namespace or something.

Quoted 17 lines · J. Bruce Fields

Quoted 14 lines · Wendy Cheng

Quoted 6 lines · J. Bruce Fields

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

That is not a correct assumption. The two commands (unlock_ip and unlock_pathname) are not necessarily called together. It is ok for local filesystem (ext3) but not for cluster filesystem where the very same filesystem (or subtree) can be exported from multiple servers using different subtrees.

Quoted 4 lines

Ouch. Are people really doing that, and why? What happens if the subtrees share files (because of hard links) that are locked from both nodes?

It is *more* common than you would expect - say server1 exports "/mnt/gfs/maildir/namea-j" and server2 exports "/mnt/gfs/maildir/namek-z".

Quoted 27 lines · Wendy Cheng

Quoted 25 lines · J. Bruce Fields

Quoted 10 lines · Wendy Cheng

Quoted 7 lines · J. Bruce Fields

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need* both is that it's possible that a misconfigured client could grab locks for a (server ip, export) combination that it isn't supposed to.

Quoted 7 lines

That is not a correct assumption. The two commands (unlock_ip and unlock_pathname) are not necessarily called together. It is ok for local filesystem (ext3) but not for cluster filesystem where the very same filesystem (or subtree) can be exported from multiple servers using different subtrees.

Ouch. Are people really doing that, and why? What happens if the subtrees share files (because of hard links) that are locked from both nodes?

Quoted 4 lines

It is *more* common than you would expect - say server1 exports "/mnt/gfs/maildir/namea-j" and server2 exports "/mnt/gfs/maildir/namek-z".

I believe it, but how hard would it be for them to just set those up as separate partitions?

Quoted 47 lines · J. Bruce Fields

To summarize a phone conversation from today:

Quoted 8 lines · Wendy Cheng

Quoted 7 lines · J. Bruce Fields

Would there be any advantage to enforcing that requirement in the server? (For example, teaching nlm to reject any locking request for a
certain filesystem that wasn't sent to a certain server IP.)

--b.

Quoted 8 lines

It is doable... could be added into the "resume" patch that is currently
being tested (since the logic is so similar to the per-ip base grace period) that should be out for review no later than next Monday.

However, as any new code added into the system, there are trade- off(s).
I'm not sure we want to keep enhancing this too much though.

Sure. And I don't want to make this terribly complicated. The patch looks good, and solves a clear problem. That said, there are a few related problems we'd like to solve:

- We want to be able to move an export to a node with an already active nfs server. Currently that requires restarting all of nfsd on the target node. This is what I understand your next patch fixes.
- In the case of a filesystem that may be mounted from multiple nodes at once, we need to make sure we're not leaving a window allowing other applications to claim locks that nfs clients haven't recovered yet.
- Ideally we'd like this to be possible without making the filesystem block all lock requests during a 90-second grace period; instead it should only have to block those requests that conflict with to-be-recovered locks.
- All this should work for nfsv4, where we want to eventually also allow migration of individual clients, and client-initiated failover.

I absolutely don't want to delay solving this particular problem until
all the above is figured out, but I would like to be reasonably confident that the new user-interface can be extended naturally to handle the above cases; or at least that it won't unnecessarily complicate their implementation.

Quoted 2 lines

I'll try to sketch an implementation of most of the above in the next week.

Bah. Apologies, this is taking me longer than it should to figure out--I've only barely started writing patches.

The basic idea, though:

In practice, it seems that both the unlock_ip and unlock_pathname methods that revoke locks are going to be called together. The two separate calls therefore seem a little redundant. The reason we *need*
both is that it's possible that a misconfigured client could grab locks
for a (server ip, export) combination that it isn't supposed to.

So it makes sense to me to restrict locking from the beginning to prevent that from happening. Therefore I'd like to add a call at the beginning like:

	echo "192.168.1.1 /exports/example" > /proc/fs/nfsd/start_grace

before any exports are set up, which both starts a grace period, and tells nfs to allow locks on the filesystem /exports/example only if they're addressed to the server ip 192.168.1.1. Then on shutdown,

	echo "192.168.1.1" >/proc/fs/nfsd/unlock_ip

should be sufficient to guarantee that nfsd/lockd no longer holds locks
on /exports/example.

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

The other advantage to having the server-ip from the start is that at the time we make lock requests to the cluster filesystem, we can tell it
that the locks associated with 192.168.1.1 are special: they may migrate
as a group to another node, and on node failure they should (if possible) be held to give a chance for another node to take them over.

Internally I'd like to have an object like

	struct lock_manager {
		char *lm_name;
		...
	}

for each server ip address. A pointer to this structure would be passed
with each lock request, allowing the filesystem to associate locks to lock_manager's. The name would be a string derived from the server ip address that the cluster can compare to match reclaim requests with the
locks that they're reclaiming from another node.

(And in the NFSv4 case we would eventually also allow lock_managers with
single nfsv4 client (as opposed to server-ip) granularity.)

Does that seem sane?

In the case of a cluster filesystem, what I hope we end up with is an api with calls to the filesystem like:

	lock_manager_start(lock_manager, super_block);
	lock_manager_end_grace(lock_manager, super_block);
	lock_manager_shutdown(lock_manager, super_block);

Quoted 3 lines

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

Quoted 3 lines · Felix Blyakher

Quoted 3 lines

(I think Wendy's pretty close to that api already after adding the second method to start grace?)

For reclaiming grace period issues, maybe we should move to
https://www.redhat.com/archives/cluster-devel/2008-January/
msg00340.html thread ?

I view this (unlock) patch set is mostly done and really don't want to add more complications to it.

Revised version of the patch:

* based on comment from Neil Brown, use sscanf() to parse IP address (a cool idea imo).
* the "ret" inside nlm_traverse_files() is now the file count that can't be unlocked.
* other minor changes from latest round of review.

Thanks to all for the review !

-- Wendy

Two new NFSD procfs files are added:
/proc/fs/nfsd/unlock_ip
/proc/fs/nfsd/unlock_filesystem

They are intended to allow admin or user mode script to release NLM locks based on either a path name or a server in-bound ip address (ipv4 for now) as;

shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem

Signed-off-by S. Wendy Cheng <[email protected]>

Signed-off-by Lon Hohberger <[email protected]>

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 4 lines

(quoted diff) +4 -0

+/* cluster failover support */
+
+typedef struct {
+	int     cmd;

Review

Quoted 6 lines

(quoted diff) +4 -0

+	int     stat;
+	int     gp;
+	void    *datap;
+} nlm_fo_cmd;

please don't introduce typedefs for struct types.

Quoted 4 lines · Christoph Hellwig

Quoted 4 lines

(quoted diff) +4 -0

+/* cluster failover support */
+
+typedef struct {
+	int     cmd;

Quoted 10 lines

Review

Quoted 6 lines

(quoted diff) +4 -0

+	int     stat;
+	int     gp;
+	void    *datap;
+} nlm_fo_cmd;

please don't introduce typedefs for struct types.

I don't do much community version of linux code so its coding standard is new to me. Any reason for this (not doing typedefs) ?

Quoted 7 lines

Quoted 7 lines

server calls nlm4svc_proc_lock() ->
* server lookup file (f_count++)
* server lock the file
* server calls nlm_release_host
* server calls nlm_release_file (f_count--)
* server return to client with status 0