NLM add resume procfs file

Add a new nfsd procfs file "resume_ip" to enable per-ip base lockd grace period:

Example Usage:
root-shell> echo 10.1.1.64 > /proc/fs/nfsd/resume_ip

Only support IPV4 address for this patch submission.

-- Wendy

Add a new nfsd procfs file "resume_ip" to enable per-ip base lockd grace period:

Example Usage:
root-shell> echo 10.1.1.64 > /proc/fs/nfsd/resume_ip

Only support IPV4 address for this patch submission.

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

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

 fs/lockd/svcsubs.c          |    9 +++++++++
 fs/nfsd/nfsctl.c            |   43 ++++++++++++++++++++++++++++++++++++++-----
 include/linux/lockd/lockd.h |    1 +
 3 files changed, 48 insertions(+), 5 deletions(-)

fs/nfsd/nfsctl.c +17 -5

@@ -56,6 +56,7 @@ enum {
 	NFSD_Fh,
 	NFSD_FO_UnlockIP,
 	NFSD_FO_UnlockFS,
+	NFSD_FO_ResumeIP,
 	NFSD_Threads,
 	NFSD_Pool_Threads,
 	NFSD_Versions,

@@ -94,6 +95,7 @@ static ssize_t write_recoverydir(struct 
 
 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 failover_resume_ip(struct file *file, char *buf, size_t size);
 
 static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_Svc] = write_svc,

@@ -106,6 +108,7 @@ static ssize_t (*write_op[])(struct file
 	[NFSD_Fh] = write_filehandle,
 	[NFSD_FO_UnlockIP] = failover_unlock_ip,
 	[NFSD_FO_UnlockFS] = failover_unlock_fs,
+	[NFSD_FO_ResumeIP] = failover_resume_ip,
 	[NFSD_Threads] = write_threads,
 	[NFSD_Pool_Threads] = write_pool_threads,
 	[NFSD_Versions] = write_versions,

@@ -297,11 +300,13 @@ static ssize_t write_getfd(struct file *
 	return err;
 }
 
-static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
+static int failover_parse_ip(struct file *file,
+			     char *buf,
+			     size_t size,
+			     __be32 *server_ip)
 {
-	__be32 server_ip;
 	char *fo_path, c;
-	int b1, b2, b3, b4;
+	int b1, b2, b3, b4, len;
 
 	/* sanity check */
 	if (size == 0)

@@ -315,13 +320,39 @@ static ssize_t failover_unlock_ip(struct
 		return -EINVAL;
 
 	/* get ipv4 address */
-	if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
+	len = sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c);
+	if (len != 4)
 		return -EINVAL;
-	server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
+
+	*server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
+
+	return len;
+}
+
+static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)

fs/nfsd/nfsctl.c +4 -0

+}
+
 static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
 {
 	struct nameidata nd;

@@ -711,6 +742,8 @@ static int nfsd_fill_super(struct super_
 					&transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
 					&transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_FO_ResumeIP] = {"resume_ip",
+					&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 +3 -0

+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_setgrace);
+

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

@@ -220,6 +220,7 @@ void		  nlmsvc_invalidate_all(void);
  */
 int           nlmsvc_failover_path(struct nameidata *nd);
 int           nlmsvc_failover_ip(__be32 server_addr);
+int           nlmsvc_failover_setgrace(void *server_ip, int ip_size);
 
 static __inline__ struct inode *
 nlmsvc_file_inode(struct nlm_file *file)

Review

Quoted 10 lines · Wendy Cheng

(quoted diff) +10 -0

+static ssize_t failover_resume_ip(struct file *file, char *buf, size_t size)
+{
+	__be32 server_ip;
+	int len;
+
+	len = failover_parse_ip(file, buf, size, &server_ip);
+	if (len < 0)
+		return len;
+
+	return nlmsvc_failover_setgrace(&server_ip, len);

Does this really need to take a void *? And the &server_ip makes it look like server_ip may be modified, which is slightly confusing.

Maybe the next patches will remind me why it's being done this way....

Quoted 18 lines

(quoted diff) +4 -0

+}
+
 static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
 {
 	struct nameidata nd;

@@ -711,6 +742,8 @@ static int nfsd_fill_super(struct super_
 					&transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
 					&transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_FO_ResumeIP] = {"resume_ip",
+					&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 nlmsvc_failover_ip(__be32 server_addr) · line 430

Quoted 9 lines

fs/lockd/svcsubs.c +6 -0

@@ -422,3 +422,12 @@ nlmsvc_failover_ip(__be32 server_addr)
 			nlmsvc_failover_file_ok_ip);
 }
 EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);
+
+int
+nlmsvc_failover_setgrace(void *server_ip, int ip_size)
+{
+	/* implemented by resume_002.patch */
+	return ENOSYS;

OK, terrifically trivial nit, but: that should be -ENOSYS?

--b.

Quoted 13 lines

fs/lockd/svcsubs.c +3 -0

+}
+EXPORT_SYMBOL_GPL(nlmsvc_failover_setgrace);
+

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

@@ -220,6 +220,7 @@ void		  nlmsvc_invalidate_all(void);
  */
 int           nlmsvc_failover_path(struct nameidata *nd);
 int           nlmsvc_failover_ip(__be32 server_addr);
+int           nlmsvc_failover_setgrace(void *server_ip, int ip_size);
 
 static __inline__ struct inode *
 nlmsvc_file_inode(struct nlm_file *file)

Quoted 6 lines

(quoted diff) +5 -0

+	int rc;
+
+	rc = failover_parse_ip(file, buf, size, &server_ip);
+	if (rc < 0)
+		return rc;

Quoted 3 lines

return nlmsvc_failover_ip(server_ip);
}

Looks great, but it would fit more logically with the previous patch. (If you know you're going to end up using this code in two places, may as well write it that way from the start.)

Quoted 4 lines · J. Bruce Fields

Quoted 4 lines

(quoted diff) +4 -0

+	int rc;
+
+	rc = failover_parse_ip(file, buf, size, &server_ip);
+	if (rc < 0)

Quoted 9 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		return rc;
  	return nlmsvc_failover_ip(server_ip);
 }

Looks great, but it would fit more logically with the previous patch. (If you know you're going to end up using this code in two places, may as well write it that way from the start.)

The original unlock patch did have a shared routine for this purpose. After review, its code structure got changed a little bit. Since the revised version has non-trivial amount of testing efforts behind it, I think it is better to do the change here, instead of the well-tested unlock patch.

On the other hand, I cut the resume patch into three pieces mostly for review purpose. Do you think it would be easier (for your git tree works) that I combine these three small patches into a big resume patch after all the review comments are incorporated into the code ?