[PATCH v3 0/5] nfsd: overhaul the client name tracking code

[PATCH v3 0/5] nfsd: overhaul the client name tracking code

[Jeff Layton]

This is the third iteration of this patchset. The main changes since
the last set are fairly minor:

- the new cl_flags field has been merged with the cl_cb_flags field.
  There are only 2 bits being used in cl_cb_flags currently, so it
  made sense not to bloat out the struct with a new field.

- some minor changes to the upcall struct for better alignment (as
  suggested by Benny)

One of the things that Bruce has long had on his wishlist is to replace
the client name tracking code that the kernel uses:

    http://wiki.linux-nfs.org/wiki/index.php/Nfsd4_server_recovery

The existing code manipulates the filesystem directly to track this
info. Not only is that something that makes the VFS maintainers look
askance at knfsd, but it also is unsuitable in a clustered
configuration.

Typically we think of the grace period as a property of the server, but
with a clustered filesystem, we need to consider it as a property of the
cluster as a whole (or of the clustered filesystem). On a cold startup
of the cluster, once any node grants a non-reclaim lock, then no more
reclaim can be allowed on any node. Grace periods must be coordinated
amongst all cluster nodes.

In order to achieve that goal, we need to first allow the client name
"reclaim" to be cluster aware as well. This patchset is a move toward
that goal and covers the initial kernel part of such a change. A
patchset to add a daemon to handle the upcalls will follow.

The goal with this patchset is to replace the existing functionality,
without disturbing the existing code too much. For now, the plan is
to leave the old tracking code in place until it becomes a burden to
maintain. At that point, we'll need to come up with a plan to
transition everyone off of the legacy state tracker.

I think this set is now ready for merge and I'd like to see it go
in for 3.3 if possible. The userspace piece will follow this in
a separate patchset.

Jeff Layton (5):
  nfsd: add nfsd4_client_tracking_ops struct and a way to set it
  sunrpc: create nfsd dir in rpc_pipefs
  nfsd: convert nfs4_client->cl_cb_flags to a generic flags field
  nfsd: add a header describing upcall to nfsdcld
  nfsd: add the infrastructure to handle the cld upcall

 fs/nfsd/nfs4callback.c   |   12 +-
 fs/nfsd/nfs4recover.c    |  439 ++++++++++++++++++++++++++++++++++++++++++++-
 fs/nfsd/nfs4state.c      |   46 ++---
 fs/nfsd/state.h          |   21 ++-
 include/linux/nfsd/cld.h |   56 ++++++
 net/sunrpc/rpc_pipe.c    |    5 +
 6 files changed, 526 insertions(+), 53 deletions(-)
 create mode 100644 include/linux/nfsd/cld.h

[PATCH v3 1/5] nfsd: add nfsd4_client_tracking_ops struct and a way to set it

[Jeff Layton]

Abstract out the mechanism that we use to track clients into a set of
client name tracking functions.

This gives us a mechanism to plug in a new set of client tracking
functions without disturbing the callers. It also gives us a way to
decide on what tracking scheme to use at runtime.

For now, this just looks like pointless abstraction, but later we'll
add a new alternate scheme for tracking clients on stable storage.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c |  123 +++++++++++++++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4state.c   |   46 +++++++------------
 fs/nfsd/state.h       |   13 +++--
 3 files changed, 138 insertions(+), 44 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index ed083b9..62fd534 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -43,9 +43,20 @@
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
+/* Declarations */
+struct nfsd4_client_tracking_ops {
+	int (*init)(void);
+	void (*exit)(void);
+	int (*create)(struct nfs4_client *);
+	int (*remove)(struct nfs4_client *);
+	int (*check)(struct nfs4_client *);
+	int (*grace_done)(time_t);
+};
+
 /* Globals */
 static struct file *rec_file;
 static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
+static const struct nfsd4_client_tracking_ops *client_tracking_ops;
 
 static int
 nfs4_save_creds(const struct cred **original_creds)
@@ -259,14 +270,14 @@ out_unlock:
 	return status;
 }
 
-void
+static int
 nfsd4_remove_clid_dir(struct nfs4_client *clp)
 {
 	const struct cred *original_cred;
 	int status;
 
 	if (!rec_file || !clp->cl_firststate)
-		return;
+		return 0;
 
 	status = mnt_want_write(rec_file->f_path.mnt);
 	if (status)
@@ -286,7 +297,7 @@ out:
 	if (status)
 		printk("NFSD: Failed to remove expired client state directory"
 				" %.*s\n", HEXDIR_LEN, clp->cl_recdir);
-	return;
+	return 0;
 }
 
 static int
@@ -305,12 +316,12 @@ purge_old(struct dentry *parent, struct dentry *child)
 	return 0;
 }
 
-void
-nfsd4_recdir_purge_old(void) {
+static int
+nfsd4_recdir_purge_old(time_t boot_time __attribute__ ((unused))) {
 	int status;
 
 	if (!rec_file)
-		return;
+		return 0;
 	status = mnt_want_write(rec_file->f_path.mnt);
 	if (status)
 		goto out;
@@ -322,6 +333,7 @@ out:
 	if (status)
 		printk("nfsd4: failed to purge old clients from recovery"
 			" directory %s\n", rec_file->f_path.dentry->d_name.name);
+	return status;
 }
 
 static int
@@ -337,7 +349,7 @@ load_recdir(struct dentry *parent, struct dentry *child)
 	return 0;
 }
 
-int
+static int
 nfsd4_recdir_load(void) {
 	int status;
 
@@ -355,8 +367,8 @@ nfsd4_recdir_load(void) {
  * Hold reference to the recovery directory.
  */
 
-void
-nfsd4_init_recdir()
+static int
+nfsd4_init_recdir(void)
 {
 	const struct cred *original_cred;
 	int status;
@@ -371,17 +383,34 @@ nfsd4_init_recdir()
 		printk("NFSD: Unable to change credentials to find recovery"
 		       " directory: error %d\n",
 		       status);
-		return;
+		return status;
 	}
 
 	rec_file = filp_open(user_recovery_dirname, O_RDONLY | O_DIRECTORY, 0);
 	if (IS_ERR(rec_file)) {
 		printk("NFSD: unable to find recovery directory %s\n",
 				user_recovery_dirname);
+		status = PTR_ERR(rec_file);
 		rec_file = NULL;
 	}
 
 	nfs4_reset_creds(original_cred);
+	return status;
+}
+
+static int
+nfsd4_load_reboot_recovery_data(void)
+{
+	int status;
+
+	nfs4_lock_state();
+	status = nfsd4_init_recdir();
+	if (!status)
+		status = nfsd4_recdir_load();
+	nfs4_unlock_state();
+	if (status)
+		printk("NFSD: Failure reading reboot recovery data\n");
+	return status;
 }
 
 void
@@ -419,3 +448,77 @@ nfs4_recoverydir(void)
 {
 	return user_recovery_dirname;
 }
+
+static int
+nfsd4_check_legacy_client(struct nfs4_client *clp)
+{
+	if (nfsd4_find_reclaim_client(clp) != NULL)
+		return 0;
+	else
+		return -ENOENT;
+}
+
+static const struct nfsd4_client_tracking_ops nfsd4_legacy_tracking_ops = {
+	.init		= nfsd4_load_reboot_recovery_data,
+	.exit		= nfsd4_shutdown_recdir,
+	.create		= nfsd4_create_clid_dir,
+	.remove		= nfsd4_remove_clid_dir,
+	.check		= nfsd4_check_legacy_client,
+	.grace_done	= nfsd4_recdir_purge_old,
+};
+
+int
+nfsd4_client_tracking_init(void)
+{
+	int status;
+
+	client_tracking_ops = &nfsd4_legacy_tracking_ops;
+	status = client_tracking_ops->init();
+	if (status) {
+		printk(KERN_WARNING "NFSD: Unable to initialize client "
+				    "recovery tracking! (%d)\n", status);
+		client_tracking_ops = NULL;
+	}
+	return status;
+}
+
+void
+nfsd4_client_tracking_exit(void)
+{
+	if (!client_tracking_ops)
+		return;
+	client_tracking_ops->exit();
+	client_tracking_ops = NULL;
+}
+
+int
+nfsd4_client_record_create(struct nfs4_client *clp)
+{
+	if (!client_tracking_ops)
+		return -EOPNOTSUPP;
+	return client_tracking_ops->create(clp);
+}
+
+int
+nfsd4_client_record_remove(struct nfs4_client *clp)
+{
+	if (!client_tracking_ops)
+		return -EOPNOTSUPP;
+	return client_tracking_ops->remove(clp);
+}
+
+int
+nfsd4_client_record_check(struct nfs4_client *clp)
+{
+	if (!client_tracking_ops)
+		return -EOPNOTSUPP;
+	return client_tracking_ops->check(clp);
+}
+
+int
+nfsd4_grace_done(time_t boot_time)
+{
+	if (!client_tracking_ops)
+		return -EOPNOTSUPP;
+	return client_tracking_ops->grace_done(boot_time);
+}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 47e94e3..e32ef02 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2038,7 +2038,7 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp, struct nfsd4_compound_state *csta
 		goto out;
 
 	status = nfs_ok;
-	nfsd4_create_clid_dir(cstate->session->se_client);
+	nfsd4_client_record_create(cstate->session->se_client);
 out:
 	nfs4_unlock_state();
 	return status;
@@ -2233,7 +2233,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			conf = find_confirmed_client_by_str(unconf->cl_recdir,
 							    hash);
 			if (conf) {
-				nfsd4_remove_clid_dir(conf);
+				nfsd4_client_record_remove(conf);
 				expire_client(conf);
 			}
 			move_to_confirmed(unconf);
@@ -3057,7 +3057,7 @@ static void
 nfsd4_end_grace(void)
 {
 	dprintk("NFSD: end of grace period\n");
-	nfsd4_recdir_purge_old();
+	nfsd4_grace_done(boot_time);
 	locks_end_grace(&nfsd4_manager);
 	/*
 	 * Now that every NFSv4 client has had the chance to recover and
@@ -3106,7 +3106,7 @@ nfs4_laundromat(void)
 		clp = list_entry(pos, struct nfs4_client, cl_lru);
 		dprintk("NFSD: purging unused client (clientid %08x)\n",
 			clp->cl_clientid.cl_id);
-		nfsd4_remove_clid_dir(clp);
+		nfsd4_client_record_remove(clp);
 		expire_client(clp);
 	}
 	spin_lock(&recall_lock);
@@ -3531,7 +3531,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
 		__func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
 
-	nfsd4_create_clid_dir(oo->oo_owner.so_client);
+	nfsd4_client_record_create(oo->oo_owner.so_client);
 	status = nfs_ok;
 out:
 	if (!cstate->replay_owner)
@@ -4358,19 +4358,13 @@ nfs4_release_reclaim(void)
 
 /*
  * called from OPEN, CLAIM_PREVIOUS with a new clientid. */
-static struct nfs4_client_reclaim *
-nfs4_find_reclaim_client(clientid_t *clid)
+struct nfs4_client_reclaim *
+nfsd4_find_reclaim_client(struct nfs4_client *clp)
 {
 	unsigned int strhashval;
-	struct nfs4_client *clp;
 	struct nfs4_client_reclaim *crp = NULL;
 
 
-	/* find clientid in conf_id_hashtbl */
-	clp = find_confirmed_client(clid);
-	if (clp == NULL)
-		return NULL;
-
 	dprintk("NFSD: nfs4_find_reclaim_client for %.*s with recdir %s\n",
 		            clp->cl_name.len, clp->cl_name.data,
 			    clp->cl_recdir);
@@ -4391,7 +4385,14 @@ nfs4_find_reclaim_client(clientid_t *clid)
 __be32
 nfs4_check_open_reclaim(clientid_t *clid)
 {
-	return nfs4_find_reclaim_client(clid) ? nfs_ok : nfserr_reclaim_bad;
+	struct nfs4_client *clp;
+
+	/* find clientid in conf_id_hashtbl */
+	clp = find_confirmed_client(clid);
+	if (clp == NULL)
+		return nfserr_reclaim_bad;
+
+	return nfsd4_client_record_check(clp) ? nfserr_reclaim_bad : nfs_ok;
 }
 
 /* initialization to perform at module load time: */
@@ -4430,19 +4431,6 @@ nfs4_state_init(void)
 	return 0;
 }
 
-static void
-nfsd4_load_reboot_recovery_data(void)
-{
-	int status;
-
-	nfs4_lock_state();
-	nfsd4_init_recdir();
-	status = nfsd4_recdir_load();
-	nfs4_unlock_state();
-	if (status)
-		printk("NFSD: Failure reading reboot recovery data\n");
-}
-
 /*
  * Since the lifetime of a delegation isn't limited to that of an open, a
  * client may quite reasonably hang on to a delegation as long as it has
@@ -4495,7 +4483,7 @@ out_free_laundry:
 int
 nfs4_state_start(void)
 {
-	nfsd4_load_reboot_recovery_data();
+	nfsd4_client_tracking_init();
 	return __nfs4_state_start();
 }
 
@@ -4530,7 +4518,7 @@ __nfs4_state_shutdown(void)
 		unhash_delegation(dp);
 	}
 
-	nfsd4_shutdown_recdir();
+	nfsd4_client_tracking_exit();
 }
 
 void
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index a3cf384..b07f5ea 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -462,6 +462,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
 extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
+extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct nfs4_client *crp);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
 extern void nfs4_free_openowner(struct nfs4_openowner *);
 extern void nfs4_free_lockowner(struct nfs4_lockowner *);
@@ -476,16 +477,18 @@ extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
-extern void nfsd4_init_recdir(void);
-extern int nfsd4_recdir_load(void);
 extern void nfsd4_shutdown_recdir(void);
 extern int nfs4_client_to_reclaim(const char *name);
 extern int nfs4_has_reclaimed_state(const char *name, bool use_exchange_id);
-extern void nfsd4_recdir_purge_old(void);
-extern int nfsd4_create_clid_dir(struct nfs4_client *clp);
-extern void nfsd4_remove_clid_dir(struct nfs4_client *clp);
 extern void release_session_client(struct nfsd4_session *);
 extern __be32 nfs4_validate_stateid(struct nfs4_client *, stateid_t *);
 extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
 
+/* nfs4recover operations */
+extern int nfsd4_client_tracking_init(void);
+extern void nfsd4_client_tracking_exit(void);
+extern int nfsd4_client_record_create(struct nfs4_client *clp);
+extern int nfsd4_client_record_remove(struct nfs4_client *clp);
+extern int nfsd4_client_record_check(struct nfs4_client *clp);
+extern int nfsd4_grace_done(time_t boot_time);
 #endif   /* NFSD4_STATE_H */
-- 
1.7.1

[PATCH v3 2/5] sunrpc: create nfsd dir in rpc_pipefs

[Jeff Layton]

Add a new top-level dir in rpc_pipefs to hold the pipe for the clientid
upcall.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/rpc_pipe.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index bfddd68..7cb8ab7 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -987,6 +987,7 @@ enum {
 	RPCAUTH_statd,
 	RPCAUTH_nfsd4_cb,
 	RPCAUTH_cache,
+	RPCAUTH_nfsd,
 	RPCAUTH_RootEOF
 };
 
@@ -1019,6 +1020,10 @@ static const struct rpc_filelist files[] = {
 		.name = "cache",
 		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
 	},
+	[RPCAUTH_nfsd] = {
+		.name = "nfsd",
+		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
+	},
 };
 
 static int
-- 
1.7.1

[PATCH v3 3/5] nfsd: convert nfs4_client->cl_cb_flags to a generic flags field

[Jeff Layton]

We'll need a way to flag the nfs4_client as already being recorded on
stable storage so that we don't continually upcall.

The cl_cb_flags field is only using 2 bits right now, so repurpose that
to a generic flags field. Rename NFSD4_CLIENT_KILL to
NFSD4_CLIENT_CB_KILL to make it evident that it's part of the callback
flags. Add a mask that we can use for existing checks that look to see
whether any flags are set, so that the new STABLE flag doesn't
interfere.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4callback.c |   12 ++++++------
 fs/nfsd/state.h        |    8 +++++---
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7748d6a..dc5da6b 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -756,7 +756,7 @@ void nfsd4_probe_callback(struct nfs4_client *clp)
 {
 	/* XXX: atomicity?  Also, should we be using cl_cb_flags? */
 	clp->cl_cb_state = NFSD4_CB_UNKNOWN;
-	set_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_cb_flags);
+	set_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_flags);
 	do_probe_callback(clp);
 }
 
@@ -915,7 +915,7 @@ void nfsd4_destroy_callback_queue(void)
 /* must be called under the state lock */
 void nfsd4_shutdown_callback(struct nfs4_client *clp)
 {
-	set_bit(NFSD4_CLIENT_KILL, &clp->cl_cb_flags);
+	set_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags);
 	/*
 	 * Note this won't actually result in a null callback;
 	 * instead, nfsd4_do_callback_rpc() will detect the killed
@@ -966,15 +966,15 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 		clp->cl_cb_conn.cb_xprt = NULL;
 	}
-	if (test_bit(NFSD4_CLIENT_KILL, &clp->cl_cb_flags))
+	if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
 		return;
 	spin_lock(&clp->cl_lock);
 	/*
 	 * Only serialized callback code is allowed to clear these
 	 * flags; main nfsd code can only set them:
 	 */
-	BUG_ON(!clp->cl_cb_flags);
-	clear_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_cb_flags);
+	BUG_ON(!(clp->cl_flags & NFSD4_CLIENT_CB_FLAG_MASK));
+	clear_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_flags);
 	memcpy(&conn, &cb->cb_clp->cl_cb_conn, sizeof(struct nfs4_cb_conn));
 	c = __nfsd4_find_backchannel(clp);
 	if (c) {
@@ -1000,7 +1000,7 @@ void nfsd4_do_callback_rpc(struct work_struct *w)
 	struct nfs4_client *clp = cb->cb_clp;
 	struct rpc_clnt *clnt;
 
-	if (clp->cl_cb_flags)
+	if (clp->cl_flags & NFSD4_CLIENT_CB_FLAG_MASK)
 		nfsd4_process_cb_update(cb);
 
 	clnt = clp->cl_cb_client;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index b07f5ea..1a21547 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -250,9 +250,11 @@ struct nfs4_client {
 
 	/* for v4.0 and v4.1 callbacks: */
 	struct nfs4_cb_conn	cl_cb_conn;
-#define NFSD4_CLIENT_CB_UPDATE	1
-#define NFSD4_CLIENT_KILL	2
-	unsigned long		cl_cb_flags;
+#define NFSD4_CLIENT_STABLE		(0)	/* client on stable storage */
+#define NFSD4_CLIENT_CB_UPDATE		(1)
+#define NFSD4_CLIENT_CB_KILL		(2)
+#define NFSD4_CLIENT_CB_FLAG_MASK	(0x6)
+	unsigned long		cl_flags;
 	struct rpc_clnt		*cl_cb_client;
 	u32			cl_cb_ident;
 #define NFSD4_CB_UP		0
-- 
1.7.1

[PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

The daemon takes a versioned binary struct. Hopefully this should allow
us to revise the struct later if it becomes necessary.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/nfsd/cld.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/nfsd/cld.h

diff --git a/include/linux/nfsd/cld.h b/include/linux/nfsd/cld.h
new file mode 100644
index 0000000..d97d678
--- /dev/null
+++ b/include/linux/nfsd/cld.h
@@ -0,0 +1,56 @@
+/*
+ * fs/nfsd/cld.h - upcall description for nfsdcld communication
+ *
+ * Copyright (c) 2011 Red Hat, Inc.
+ * Author(s): Jeff Layton <jlayton@redhat.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _NFSD_CLD_H
+#define _NFSD_CLD_H
+
+/* latest upcall version available */
+#define CLD_UPCALL_VERSION 1
+
+/* defined by RFC3530 */
+#define NFS4_OPAQUE_LIMIT 1024
+
+enum cld_command {
+	Cld_Create,		/* create a record for this cm_id */
+	Cld_Expire,		/* expire record for this cm_id */
+	Cld_Allow,		/* is this cm_id allowed? */
+	Cld_GraceDone,		/* grace period is complete */
+};
+
+/* representation of long-form NFSv4 client ID */
+struct cld_name {
+	uint16_t	cn_len;				/* length of cm_id */
+	unsigned char	cn_id[NFS4_OPAQUE_LIMIT];	/* client-provided */
+} __attribute__((packed));
+
+/* message struct for communication with userspace */
+struct cld_msg {
+	uint8_t		cm_vers;		/* upcall version */
+	uint8_t		cm_cmd;			/* upcall command */
+	int16_t		cm_status;		/* return code */
+	uint32_t	cm_xid;			/* transaction id */
+	union {
+		int64_t		cm_gracetime;	/* grace period start time */
+		struct cld_name	cm_name;
+	} __attribute__((packed)) cm_u;
+} __attribute__((packed));
+
+#endif /* !_NFSD_CLD_H */
-- 
1.7.1

[PATCH v3 5/5] nfsd: add the infrastructure to handle the cld upcall

[Jeff Layton]

...and add a mechanism for switching between the "legacy" tracker and
the new one. The decision is made by looking to see whether the
v4recoverydir exists. If it does, then the legacy client tracker is
used.

If it's not, then the kernel will create a "cld" pipe in rpc_pipefs.
That pipe is used to talk to a daemon for handling the upcall.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c |  318 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 317 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 62fd534..333d65a 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1,5 +1,6 @@
 /*
 *  Copyright (c) 2004 The Regents of the University of Michigan.
+*  Copyright (c) 2011 Jeff Layton <jlayton@redhat.com>
 *  All rights reserved.
 *
 *  Andy Adamson <andros@citi.umich.edu>
@@ -36,6 +37,10 @@
 #include <linux/namei.h>
 #include <linux/crypto.h>
 #include <linux/sched.h>
+#include <linux/fs.h>
+#include <linux/sunrpc/rpc_pipe_fs.h>
+#include <linux/sunrpc/clnt.h>
+#include <linux/nfsd/cld.h>
 
 #include "nfsd.h"
 #include "state.h"
@@ -467,12 +472,323 @@ static const struct nfsd4_client_tracking_ops nfsd4_legacy_tracking_ops = {
 	.grace_done	= nfsd4_recdir_purge_old,
 };
 
+/* Globals */
+#define NFSD_PIPE_DIR		"/nfsd"
+
+static struct dentry *cld_pipe;
+
+/* list of cld_msg's that are currently in use */
+static DEFINE_SPINLOCK(cld_lock);
+static LIST_HEAD(cld_list);
+static unsigned int cld_xid;
+
+struct cld_upcall {
+	struct list_head	cu_list;
+	struct task_struct *	cu_task;
+	struct cld_msg	cu_msg;
+};
+
+static int
+nfsd4_cld_upcall(struct cld_msg *cmsg)
+{
+	int ret;
+	struct rpc_pipe_msg msg;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.data = cmsg;
+	msg.len = sizeof(*cmsg);
+
+	ret = rpc_queue_upcall(cld_pipe->d_inode, &msg);
+	if (ret < 0)
+		goto out;
+
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	schedule();
+	__set_current_state(TASK_RUNNING);
+
+out:
+	return ret;
+}
+
+ssize_t cld_pipe_downcall(struct file *filp, const char __user *src,
+			     size_t mlen)
+{
+	struct cld_upcall *tmp, *cup;
+	struct cld_msg *cmsg = (struct cld_msg *)src;
+	uint32_t xid;
+
+	if (mlen != sizeof(*cmsg)) {
+		dprintk("%s: got %lu bytes, expected %lu\n", __func__, mlen,
+			sizeof(*cmsg));
+		return -EINVAL;
+	}
+
+	/* copy just the xid so we can try to find that */
+	if (copy_from_user(&xid, &cmsg->cm_xid, sizeof(xid)) != 0) {
+		dprintk("%s: error when copying xid from userspace", __func__);
+		return -EFAULT;
+	}
+
+	/* walk the list and find corresponding xid */
+	cup = NULL;
+	spin_lock(&cld_lock);
+	list_for_each_entry(tmp, &cld_list, cu_list) {
+		if (get_unaligned(&tmp->cu_msg.cm_xid) == xid) {
+			cup = tmp;
+			list_del_init(&cup->cu_list);
+			break;
+		}
+	}
+	spin_unlock(&cld_lock);
+
+	/* couldn't find upcall? */
+	if (!cup) {
+		dprintk("%s: couldn't find upcall -- xid=%u\n", __func__,
+			cup->cu_msg.cm_xid);
+		return -EINVAL;
+	}
+
+	if (copy_from_user(&cup->cu_msg, src, mlen) != 0)
+		return -EFAULT;
+
+	wake_up_process(cup->cu_task);
+	return mlen;
+}
+
+void
+cld_pipe_destroy_msg(struct rpc_pipe_msg *msg)
+{
+	struct cld_msg *cmsg = msg->data;
+	struct cld_upcall *cup = container_of(cmsg, struct cld_upcall,
+						 cu_msg);
+
+	if (msg->errno >= 0)
+		return;
+	wake_up_process(cup->cu_task);
+}
+
+static const struct rpc_pipe_ops cld_upcall_ops = {
+	.upcall		= rpc_pipe_generic_upcall,
+	.downcall	= cld_pipe_downcall,
+	.destroy_msg	= cld_pipe_destroy_msg,
+};
+
+int
+nfsd4_init_cld_pipe(void)
+{
+	int ret;
+	struct path path;
+	struct vfsmount *mnt;
+
+	if (cld_pipe)
+		return 0;
+
+	mnt = rpc_get_mount();
+	if (IS_ERR(mnt))
+		return PTR_ERR(mnt);
+
+	ret = vfs_path_lookup(mnt->mnt_root, mnt, NFSD_PIPE_DIR, 0, &path);
+	if (ret)
+		goto err;
+
+	cld_pipe = rpc_mkpipe(path.dentry, "cld", NULL,
+				 &cld_upcall_ops, RPC_PIPE_WAIT_FOR_OPEN);
+	path_put(&path);
+	if (!IS_ERR(cld_pipe))
+		return 0;
+
+	ret = PTR_ERR(cld_pipe);
+err:
+	rpc_put_mount();
+	return ret;
+}
+
+void
+nfsd4_remove_cld_pipe(void)
+{
+	int ret;
+
+	ret = rpc_unlink(cld_pipe);
+	if (ret)
+		printk(KERN_ERR "NFSD: error removing cld pipe: %d\n", ret);
+	cld_pipe = NULL;
+	rpc_put_mount();
+}
+
+static struct cld_upcall *
+alloc_cld_upcall(void)
+{
+	struct cld_upcall *new, *tmp;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return new;
+
+	/* FIXME: hard cap on number in flight? */
+restart_search:
+	spin_lock(&cld_lock);
+	list_for_each_entry(tmp, &cld_list, cu_list) {
+		if (tmp->cu_msg.cm_xid == cld_xid) {
+			cld_xid++;
+			spin_unlock(&cld_lock);
+			goto restart_search;
+		}
+	}
+	new->cu_task = current;
+	new->cu_msg.cm_vers = CLD_UPCALL_VERSION;
+	put_unaligned(cld_xid++, &new->cu_msg.cm_xid);
+	list_add(&new->cu_list, &cld_list);
+	spin_unlock(&cld_lock);
+
+	dprintk("%s: allocated xid %u\n", __func__, new->cu_msg.cm_xid);
+
+	return new;
+}
+
+static void
+free_cld_upcall(struct cld_upcall *victim)
+{
+	spin_lock(&cld_lock);
+	list_del(&victim->cu_list);
+	spin_unlock(&cld_lock);
+	kfree(victim);
+}
+
+/* Ask daemon to create a new record */
+static int
+nfsd4_cld_create(struct nfs4_client *clp)
+{
+	int ret;
+	struct cld_upcall *cup;
+
+	/* Don't upcall if it's already stored */
+	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
+		return -EEXIST;
+
+	cup = alloc_cld_upcall();
+	if (!cup)
+		return -ENOMEM;
+
+	cup->cu_msg.cm_cmd = Cld_Create;
+	cup->cu_msg.cm_u.cm_name.cn_len = clp->cl_name.len;
+	memcpy(cup->cu_msg.cm_u.cm_name.cn_id, clp->cl_name.data,
+			clp->cl_name.len);
+
+	ret = nfsd4_cld_upcall(&cup->cu_msg);
+	if (!ret) {
+		ret = cup->cu_msg.cm_status;
+		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	}
+
+	free_cld_upcall(cup);
+	return ret;
+}
+
+/* Ask daemon to remove a record */
+static int
+nfsd4_cld_remove(struct nfs4_client *clp)
+{
+	int ret;
+	struct cld_upcall *cup;
+
+	/* Don't upcall if it's already stored */
+	if (!test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
+		return -ENOENT;
+
+	cup = alloc_cld_upcall();
+	if (!cup)
+		return -ENOMEM;
+
+	cup->cu_msg.cm_cmd = Cld_Expire;
+	cup->cu_msg.cm_u.cm_name.cn_len = clp->cl_name.len;
+	memcpy(cup->cu_msg.cm_u.cm_name.cn_id, clp->cl_name.data,
+			clp->cl_name.len);
+
+	ret = nfsd4_cld_upcall(&cup->cu_msg);
+	if (!ret) {
+		ret = cup->cu_msg.cm_status;
+		clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	}
+
+	free_cld_upcall(cup);
+	return ret;
+}
+
+/* Check for presence of a record, and update its timestamp */
+static int
+nfsd4_cld_check(struct nfs4_client *clp)
+{
+	int ret;
+	struct cld_upcall *cup;
+
+	/* Don't upcall if one was already stored during this grace pd */
+	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
+		return 0;
+
+	cup = alloc_cld_upcall();
+	if (!cup)
+		return -ENOMEM;
+
+	cup->cu_msg.cm_cmd = Cld_Allow;
+	cup->cu_msg.cm_u.cm_name.cn_len = clp->cl_name.len;
+	memcpy(cup->cu_msg.cm_u.cm_name.cn_id, clp->cl_name.data,
+			clp->cl_name.len);
+
+	ret = nfsd4_cld_upcall(&cup->cu_msg);
+	if (!ret) {
+		ret = cup->cu_msg.cm_status;
+		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	}
+
+	free_cld_upcall(cup);
+	return ret;
+}
+
+static int
+nfsd4_cld_grace_done(time_t boot_time)
+{
+	int ret;
+	struct cld_upcall *cup;
+
+	cup = alloc_cld_upcall();
+	if (!cup)
+		return -ENOMEM;
+
+	cup->cu_msg.cm_cmd = Cld_GraceDone;
+	cup->cu_msg.cm_u.cm_gracetime = (int64_t)boot_time;
+	ret = nfsd4_cld_upcall(&cup->cu_msg);
+	if (!ret)
+		ret = cup->cu_msg.cm_status;
+
+	free_cld_upcall(cup);
+	return ret;
+}
+
+static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops = {
+	.init		= nfsd4_init_cld_pipe,
+	.exit		= nfsd4_remove_cld_pipe,
+	.create		= nfsd4_cld_create,
+	.remove		= nfsd4_cld_remove,
+	.check		= nfsd4_cld_check,
+	.grace_done	= nfsd4_cld_grace_done,
+};
+
 int
 nfsd4_client_tracking_init(void)
 {
 	int status;
+	struct path path;
 
-	client_tracking_ops = &nfsd4_legacy_tracking_ops;
+	if (!client_tracking_ops) {
+		client_tracking_ops = &nfsd4_cld_tracking_ops;
+		status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path);
+		if (!status) {
+			if (S_ISDIR(path.dentry->d_inode->i_mode))
+				client_tracking_ops =
+						&nfsd4_legacy_tracking_ops;
+			path_put(&path);
+		}
+	}
 	status = client_tracking_ops->init();
 	if (status) {
 		printk(KERN_WARNING "NFSD: Unable to initialize client "
-- 
1.7.1

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Chuck Lever]

On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:

> The daemon takes a versioned binary struct. Hopefully this should allow
> us to revise the struct later if it becomes necessary.

This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?

> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> include/linux/nfsd/cld.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 56 insertions(+), 0 deletions(-)
> create mode 100644 include/linux/nfsd/cld.h
> 
> diff --git a/include/linux/nfsd/cld.h b/include/linux/nfsd/cld.h
> new file mode 100644
> index 0000000..d97d678
> --- /dev/null
> +++ b/include/linux/nfsd/cld.h
> @@ -0,0 +1,56 @@
> +/*
> + * fs/nfsd/cld.h - upcall description for nfsdcld communication
> + *
> + * Copyright (c) 2011 Red Hat, Inc.
> + * Author(s): Jeff Layton <jlayton@redhat.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef _NFSD_CLD_H
> +#define _NFSD_CLD_H
> +
> +/* latest upcall version available */
> +#define CLD_UPCALL_VERSION 1
> +
> +/* defined by RFC3530 */
> +#define NFS4_OPAQUE_LIMIT 1024
> +
> +enum cld_command {
> +	Cld_Create,		/* create a record for this cm_id */
> +	Cld_Expire,		/* expire record for this cm_id */
> +	Cld_Allow,		/* is this cm_id allowed? */
> +	Cld_GraceDone,		/* grace period is complete */
> +};
> +
> +/* representation of long-form NFSv4 client ID */
> +struct cld_name {
> +	uint16_t	cn_len;				/* length of cm_id */
> +	unsigned char	cn_id[NFS4_OPAQUE_LIMIT];	/* client-provided */
> +} __attribute__((packed));
> +
> +/* message struct for communication with userspace */
> +struct cld_msg {
> +	uint8_t		cm_vers;		/* upcall version */
> +	uint8_t		cm_cmd;			/* upcall command */
> +	int16_t		cm_status;		/* return code */
> +	uint32_t	cm_xid;			/* transaction id */
> +	union {
> +		int64_t		cm_gracetime;	/* grace period start time */
> +		struct cld_name	cm_name;
> +	} __attribute__((packed)) cm_u;
> +} __attribute__((packed));
> +
> +#endif /* !_NFSD_CLD_H */
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 15:45:01 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> 
> > The daemon takes a versioned binary struct. Hopefully this should allow
> > us to revise the struct later if it becomes necessary.
> 
> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> 

That's the main reason. We could, of course encode that string in hex or
something, and decode it on the other end. No one has presented a
strong argument for doing it that way as of yet though.

If anyone feels strongly about that, then it would be helpful to have
them pipe up now and state why they do...

> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > include/linux/nfsd/cld.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 56 insertions(+), 0 deletions(-)
> > create mode 100644 include/linux/nfsd/cld.h
> > 
> > diff --git a/include/linux/nfsd/cld.h b/include/linux/nfsd/cld.h
> > new file mode 100644
> > index 0000000..d97d678
> > --- /dev/null
> > +++ b/include/linux/nfsd/cld.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * fs/nfsd/cld.h - upcall description for nfsdcld communication
> > + *
> > + * Copyright (c) 2011 Red Hat, Inc.
> > + * Author(s): Jeff Layton <jlayton@redhat.com>
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with this program; if not, write to the Free Software
> > + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> > +
> > +#ifndef _NFSD_CLD_H
> > +#define _NFSD_CLD_H
> > +
> > +/* latest upcall version available */
> > +#define CLD_UPCALL_VERSION 1
> > +
> > +/* defined by RFC3530 */
> > +#define NFS4_OPAQUE_LIMIT 1024
> > +
> > +enum cld_command {
> > +	Cld_Create,		/* create a record for this cm_id */
> > +	Cld_Expire,		/* expire record for this cm_id */
> > +	Cld_Allow,		/* is this cm_id allowed? */
> > +	Cld_GraceDone,		/* grace period is complete */
> > +};
> > +
> > +/* representation of long-form NFSv4 client ID */
> > +struct cld_name {
> > +	uint16_t	cn_len;				/* length of cm_id */
> > +	unsigned char	cn_id[NFS4_OPAQUE_LIMIT];	/* client-provided */
> > +} __attribute__((packed));
> > +
> > +/* message struct for communication with userspace */
> > +struct cld_msg {
> > +	uint8_t		cm_vers;		/* upcall version */
> > +	uint8_t		cm_cmd;			/* upcall command */
> > +	int16_t		cm_status;		/* return code */
> > +	uint32_t	cm_xid;			/* transaction id */
> > +	union {
> > +		int64_t		cm_gracetime;	/* grace period start time */
> > +		struct cld_name	cm_name;
> > +	} __attribute__((packed)) cm_u;
> > +} __attribute__((packed));
> > +
> > +#endif /* !_NFSD_CLD_H */
> > -- 
> > 1.7.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Chuck Lever]

On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:

> On Wed, 21 Dec 2011 15:45:01 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
>> 
>>> The daemon takes a versioned binary struct. Hopefully this should allow
>>> us to revise the struct later if it becomes necessary.
>> 
>> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
>> 
> 
> That's the main reason. We could, of course encode that string in hex or
> something, and decode it on the other end. No one has presented a
> strong argument for doing it that way as of yet though.
> 
> If anyone feels strongly about that, then it would be helpful to have
> them pipe up now and state why they do...

<pipe>Because binary data structures are difficult to work with over time, which is why other NFSD user space interfaces are text-based.</pipe>

;-)

> 
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> include/linux/nfsd/cld.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 files changed, 56 insertions(+), 0 deletions(-)
>>> create mode 100644 include/linux/nfsd/cld.h
>>> 
>>> diff --git a/include/linux/nfsd/cld.h b/include/linux/nfsd/cld.h
>>> new file mode 100644
>>> index 0000000..d97d678
>>> --- /dev/null
>>> +++ b/include/linux/nfsd/cld.h
>>> @@ -0,0 +1,56 @@
>>> +/*
>>> + * fs/nfsd/cld.h - upcall description for nfsdcld communication
>>> + *
>>> + * Copyright (c) 2011 Red Hat, Inc.
>>> + * Author(s): Jeff Layton <jlayton@redhat.com>
>>> + *
>>> + *  This program is free software; you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation; either version 2 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  This program is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + *  You should have received a copy of the GNU General Public License
>>> + *  along with this program; if not, write to the Free Software
>>> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>>> + */
>>> +
>>> +#ifndef _NFSD_CLD_H
>>> +#define _NFSD_CLD_H
>>> +
>>> +/* latest upcall version available */
>>> +#define CLD_UPCALL_VERSION 1
>>> +
>>> +/* defined by RFC3530 */
>>> +#define NFS4_OPAQUE_LIMIT 1024
>>> +
>>> +enum cld_command {
>>> +	Cld_Create,		/* create a record for this cm_id */
>>> +	Cld_Expire,		/* expire record for this cm_id */
>>> +	Cld_Allow,		/* is this cm_id allowed? */
>>> +	Cld_GraceDone,		/* grace period is complete */
>>> +};
>>> +
>>> +/* representation of long-form NFSv4 client ID */
>>> +struct cld_name {
>>> +	uint16_t	cn_len;				/* length of cm_id */
>>> +	unsigned char	cn_id[NFS4_OPAQUE_LIMIT];	/* client-provided */
>>> +} __attribute__((packed));
>>> +
>>> +/* message struct for communication with userspace */
>>> +struct cld_msg {
>>> +	uint8_t		cm_vers;		/* upcall version */
>>> +	uint8_t		cm_cmd;			/* upcall command */
>>> +	int16_t		cm_status;		/* return code */
>>> +	uint32_t	cm_xid;			/* transaction id */
>>> +	union {
>>> +		int64_t		cm_gracetime;	/* grace period start time */
>>> +		struct cld_name	cm_name;
>>> +	} __attribute__((packed)) cm_u;
>>> +} __attribute__((packed));
>>> +
>>> +#endif /* !_NFSD_CLD_H */
>>> -- 
>>> 1.7.1
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 16:37:30 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> 
> > On Wed, 21 Dec 2011 15:45:01 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> >> 
> >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> >> 
> >>> The daemon takes a versioned binary struct. Hopefully this should allow
> >>> us to revise the struct later if it becomes necessary.
> >> 
> >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> >> 
> > 
> > That's the main reason. We could, of course encode that string in hex or
> > something, and decode it on the other end. No one has presented a
> > strong argument for doing it that way as of yet though.
> > 
> > If anyone feels strongly about that, then it would be helpful to have
> > them pipe up now and state why they do...
> 
> <pipe>Because binary data structures are difficult to work with over time, which is why other NFSD user space interfaces are text-based.</pipe>
> 
> ;-)
> 

*sigh* that was the sort of comments I was hoping to get out of the RFC
postings. But ok...

I'll see about respinning the whole thing with either a text-based or
XDR-based upcall/downcall format. That'll take a while, but I'll see if
I can get it in shape in time for 3.3.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 16:48:10 -0500
Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 21 Dec 2011 16:37:30 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > 
> > On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> > 
> > > On Wed, 21 Dec 2011 15:45:01 -0500
> > > Chuck Lever <chuck.lever@oracle.com> wrote:
> > > 
> > >> 
> > >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> > >> 
> > >>> The daemon takes a versioned binary struct. Hopefully this should allow
> > >>> us to revise the struct later if it becomes necessary.
> > >> 
> > >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> > >> 
> > > 
> > > That's the main reason. We could, of course encode that string in hex or
> > > something, and decode it on the other end. No one has presented a
> > > strong argument for doing it that way as of yet though.
> > > 
> > > If anyone feels strongly about that, then it would be helpful to have
> > > them pipe up now and state why they do...
> > 
> > <pipe>Because binary data structures are difficult to work with over time, which is why other NFSD user space interfaces are text-based.</pipe>
> > 
> > ;-)
> > 
> 
> *sigh* that was the sort of comments I was hoping to get out of the RFC
> postings. But ok...
> 
> I'll see about respinning the whole thing with either a text-based or
> XDR-based upcall/downcall format. That'll take a while, but I'll see if
> I can get it in shape in time for 3.3.
> 

...and while we're discussing it. Does anyone have thoughts on which
would be better? I'd probably prefer a text-based format. That's more
flexible but is also going to be more verbose. I think we're still well
under a page with these requests even in text however so I don't really
see that as a problem.

-- 
Jeff Layton <jlayton@redhat.com>

RE: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Peter Staubach]

By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.

I would suggest that XDR encoding seems quite natural to use when passing binary structs around.

	Thanx...

		ps


-----Original Message-----
From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
Sent: Wednesday, December 21, 2011 4:59 PM
To: Jeff Layton
Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

On Wed, 21 Dec 2011 16:48:10 -0500
Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 21 Dec 2011 16:37:30 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > 
> > On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> > 
> > > On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
> > > <chuck.lever@oracle.com> wrote:
> > > 
> > >> 
> > >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> > >> 
> > >>> The daemon takes a versioned binary struct. Hopefully this 
> > >>> should allow us to revise the struct later if it becomes necessary.
> > >> 
> > >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> > >> 
> > > 
> > > That's the main reason. We could, of course encode that string in 
> > > hex or something, and decode it on the other end. No one has 
> > > presented a strong argument for doing it that way as of yet though.
> > > 
> > > If anyone feels strongly about that, then it would be helpful to 
> > > have them pipe up now and state why they do...
> > 
> > <pipe>Because binary data structures are difficult to work with over 
> > time, which is why other NFSD user space interfaces are 
> > text-based.</pipe>
> > 
> > ;-)
> > 
> 
> *sigh* that was the sort of comments I was hoping to get out of the 
> RFC postings. But ok...
> 
> I'll see about respinning the whole thing with either a text-based or 
> XDR-based upcall/downcall format. That'll take a while, but I'll see 
> if I can get it in shape in time for 3.3.
> 

...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.

--
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Chuck Lever]

On Dec 21, 2011, at 5:04 PM, Peter Staubach wrote:

> By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.

No, I mean something akin to what is done for the other NFSD upcall and downcall arguments and results.  A single set of decimal digits for simple arguments, and "keyword=value keyword=value,value" and so on for complex arguments.  I recommend text because that is similar to what is used for every other modern user space API that NFSD has.

> I would suggest that XDR encoding seems quite natural to use when passing binary structs around.

To be clear, there is just one binary data item here, and that's nfs_client_id4.  The other data items are integers, which have well-defined text representations.

A new and separate API without any legacy baggage might be XDR, but text seems more appropriate here given the existence of other text APIs.  I'm not religious about this, just highlighting a few design choices.

> 	Thanx...
> 
> 		ps
> 
> 
> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
> Sent: Wednesday, December 21, 2011 4:59 PM
> To: Jeff Layton
> Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld
> 
> On Wed, 21 Dec 2011 16:48:10 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> 
>> On Wed, 21 Dec 2011 16:37:30 -0500
>> Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>>> 
>>> On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
>>> 
>>>> On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
>>>> <chuck.lever@oracle.com> wrote:
>>>> 
>>>>> 
>>>>> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
>>>>> 
>>>>>> The daemon takes a versioned binary struct. Hopefully this 
>>>>>> should allow us to revise the struct later if it becomes necessary.
>>>>> 
>>>>> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
>>>>> 
>>>> 
>>>> That's the main reason. We could, of course encode that string in 
>>>> hex or something, and decode it on the other end. No one has 
>>>> presented a strong argument for doing it that way as of yet though.
>>>> 
>>>> If anyone feels strongly about that, then it would be helpful to 
>>>> have them pipe up now and state why they do...
>>> 
>>> <pipe>Because binary data structures are difficult to work with over 
>>> time, which is why other NFSD user space interfaces are 
>>> text-based.</pipe>
>>> 
>>> ;-)
>>> 
>> 
>> *sigh* that was the sort of comments I was hoping to get out of the 
>> RFC postings. But ok...
>> 
>> I'll see about respinning the whole thing with either a text-based or 
>> XDR-based upcall/downcall format. That'll take a while, but I'll see 
>> if I can get it in shape in time for 3.3.
>> 
> 
> ...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.
> 
> --
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 17:04:02 -0500
Peter Staubach <pstaubach@exagrid.com> wrote:

> By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.
> 
> I would suggest that XDR encoding seems quite natural to use when passing binary structs around.
> 

No, I was thinking along the lines of encoding the struct fields as
key/value pairs in a string:

    "ver=1,cmd=create,clname=0x1a012bd..."

...binary stuff like the nfs_client_id4 would be hex encoded. Integers
would be converted to text representations, etc.

I think if we're going to go to the trouble of encoding this stuff at
all, it makes sense to do it in as flexible a manner as possible.

Now that I think about it, using XDR has little benefit over the current
packed binary struct. If we ever need to rev the format, it'll be just
as much of a pain. With key/value pairs, we should be able to ensure
that a mismatch between kernel and userspace versions is more easily
dealt with.

> 
> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
> Sent: Wednesday, December 21, 2011 4:59 PM
> To: Jeff Layton
> Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld
> 
> On Wed, 21 Dec 2011 16:48:10 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Wed, 21 Dec 2011 16:37:30 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> > > 
> > > On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> > > 
> > > > On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
> > > > <chuck.lever@oracle.com> wrote:
> > > > 
> > > >> 
> > > >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> > > >> 
> > > >>> The daemon takes a versioned binary struct. Hopefully this 
> > > >>> should allow us to revise the struct later if it becomes necessary.
> > > >> 
> > > >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> > > >> 
> > > > 
> > > > That's the main reason. We could, of course encode that string in 
> > > > hex or something, and decode it on the other end. No one has 
> > > > presented a strong argument for doing it that way as of yet though.
> > > > 
> > > > If anyone feels strongly about that, then it would be helpful to 
> > > > have them pipe up now and state why they do...
> > > 
> > > <pipe>Because binary data structures are difficult to work with over 
> > > time, which is why other NFSD user space interfaces are 
> > > text-based.</pipe>
> > > 
> > > ;-)
> > > 
> > 
> > *sigh* that was the sort of comments I was hoping to get out of the 
> > RFC postings. But ok...
> > 
> > I'll see about respinning the whole thing with either a text-based or 
> > XDR-based upcall/downcall format. That'll take a while, but I'll see 
> > if I can get it in shape in time for 3.3.
> > 
> 
> ...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.
> 

-- 
Jeff Layton <jlayton@redhat.com>

RE: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Peter Staubach]

Hi.

I can't really specify things either, but XDR parsing of something which is fixed in size and format is much less overhead than parsing of strings.  Even adding for the possibility of versioning, XDR is going to come out vastly easier to implement.

Text string parsing is very nice when variable strings are passed, in varying orders, with some appearing and others not.  In this case, all fields must be present.  There isn't any benefit to passing them in random orders, so it appears that the primary benefit of the text string parsing is minimized.

	Thanx...

		ps


-----Original Message-----
From: Chuck Lever [mailto:chuck.lever@oracle.com] 
Sent: Wednesday, December 21, 2011 5:22 PM
To: Peter Staubach
Cc: Jeff Layton; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld


On Dec 21, 2011, at 5:04 PM, Peter Staubach wrote:

> By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.

No, I mean something akin to what is done for the other NFSD upcall and downcall arguments and results.  A single set of decimal digits for simple arguments, and "keyword=value keyword=value,value" and so on for complex arguments.  I recommend text because that is similar to what is used for every other modern user space API that NFSD has.

> I would suggest that XDR encoding seems quite natural to use when passing binary structs around.

To be clear, there is just one binary data item here, and that's nfs_client_id4.  The other data items are integers, which have well-defined text representations.

A new and separate API without any legacy baggage might be XDR, but text seems more appropriate here given the existence of other text APIs.  I'm not religious about this, just highlighting a few design choices.

> 	Thanx...
> 
> 		ps
> 
> 
> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org 
> [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
> Sent: Wednesday, December 21, 2011 4:59 PM
> To: Jeff Layton
> Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; 
> linux-nfs@vger.kernel.org
> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to 
> nfsdcld
> 
> On Wed, 21 Dec 2011 16:48:10 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> 
>> On Wed, 21 Dec 2011 16:37:30 -0500
>> Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>>> 
>>> On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
>>> 
>>>> On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
>>>> <chuck.lever@oracle.com> wrote:
>>>> 
>>>>> 
>>>>> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
>>>>> 
>>>>>> The daemon takes a versioned binary struct. Hopefully this should 
>>>>>> allow us to revise the struct later if it becomes necessary.
>>>>> 
>>>>> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
>>>>> 
>>>> 
>>>> That's the main reason. We could, of course encode that string in 
>>>> hex or something, and decode it on the other end. No one has 
>>>> presented a strong argument for doing it that way as of yet though.
>>>> 
>>>> If anyone feels strongly about that, then it would be helpful to 
>>>> have them pipe up now and state why they do...
>>> 
>>> <pipe>Because binary data structures are difficult to work with over 
>>> time, which is why other NFSD user space interfaces are 
>>> text-based.</pipe>
>>> 
>>> ;-)
>>> 
>> 
>> *sigh* that was the sort of comments I was hoping to get out of the 
>> RFC postings. But ok...
>> 
>> I'll see about respinning the whole thing with either a text-based or 
>> XDR-based upcall/downcall format. That'll take a while, but I'll see 
>> if I can get it in shape in time for 3.3.
>> 
> 
> ...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.
> 
> --
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" 
> in the body of a message to majordomo@vger.kernel.org More majordomo 
> info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

RE: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Peter Staubach]

I will need to disagree with the assertion regarding XDR and the packed struct.  XDR can handle a versioned struct very neatly.  It can do so just as easily as text string parsing and with greatly reduced overhead.

		Ps


-----Original Message-----
From: Jeff Layton [mailto:jlayton@redhat.com] 
Sent: Wednesday, December 21, 2011 5:28 PM
To: Peter Staubach
Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

On Wed, 21 Dec 2011 17:04:02 -0500
Peter Staubach <pstaubach@exagrid.com> wrote:

> By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.
> 
> I would suggest that XDR encoding seems quite natural to use when passing binary structs around.
> 

No, I was thinking along the lines of encoding the struct fields as key/value pairs in a string:

    "ver=1,cmd=create,clname=0x1a012bd..."

...binary stuff like the nfs_client_id4 would be hex encoded. Integers would be converted to text representations, etc.

I think if we're going to go to the trouble of encoding this stuff at all, it makes sense to do it in as flexible a manner as possible.

Now that I think about it, using XDR has little benefit over the current packed binary struct. If we ever need to rev the format, it'll be just as much of a pain. With key/value pairs, we should be able to ensure that a mismatch between kernel and userspace versions is more easily dealt with.

> 
> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org 
> [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
> Sent: Wednesday, December 21, 2011 4:59 PM
> To: Jeff Layton
> Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; 
> linux-nfs@vger.kernel.org
> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to 
> nfsdcld
> 
> On Wed, 21 Dec 2011 16:48:10 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Wed, 21 Dec 2011 16:37:30 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> > > 
> > > On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> > > 
> > > > On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
> > > > <chuck.lever@oracle.com> wrote:
> > > > 
> > > >> 
> > > >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> > > >> 
> > > >>> The daemon takes a versioned binary struct. Hopefully this 
> > > >>> should allow us to revise the struct later if it becomes necessary.
> > > >> 
> > > >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> > > >> 
> > > > 
> > > > That's the main reason. We could, of course encode that string 
> > > > in hex or something, and decode it on the other end. No one has 
> > > > presented a strong argument for doing it that way as of yet though.
> > > > 
> > > > If anyone feels strongly about that, then it would be helpful to 
> > > > have them pipe up now and state why they do...
> > > 
> > > <pipe>Because binary data structures are difficult to work with 
> > > over time, which is why other NFSD user space interfaces are 
> > > text-based.</pipe>
> > > 
> > > ;-)
> > > 
> > 
> > *sigh* that was the sort of comments I was hoping to get out of the 
> > RFC postings. But ok...
> > 
> > I'll see about respinning the whole thing with either a text-based 
> > or XDR-based upcall/downcall format. That'll take a while, but I'll 
> > see if I can get it in shape in time for 3.3.
> > 
> 
> ...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.
> 

--
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Chuck Lever]

On Dec 21, 2011, at 5:31 PM, Peter Staubach wrote:

> Hi.
> 
> I can't really specify things either, but XDR parsing of something which is fixed in size and format is much less overhead than parsing of strings.  Even adding for the possibility of versioning, XDR is going to come out vastly easier to implement.

In the kernel, not so much.  We always have to code either text-based or XDR-based encoding or decoding by hand there.  The only benefit is in user space, if you can integrate machine-built rpcgen stubs with your code.  The text-based helpers in nfs-utils are not hard to use, though.

> Text string parsing is very nice when variable strings are passed, in varying orders, with some appearing and others not.  In this case, all fields must be present.  There isn't any benefit to passing them in random orders, so it appears that the primary benefit of the text string parsing is minimized.

There is some argument variability.  There is a union in Jeff's data structure.

But again, I'm not religious, I actually like XDR for many things.  Here I think it's simply useful to be consistent with what is already in place.  Text-based APIs also have an observability advantage; in fact, you can write scripts to text-based APIs a lot easier than you can to XDR-based ones.

If we go with XDR for this API, the natural question an impartial reviewer might ask is "why does this NFSD API use XDR but others of equal or greater complexity do not?"  I might be more inclined to choose XDR for this application if there was some commitment to converting some of the other NFSD user space APIs.

> 	Thanx...
> 
> 		ps
> 
> 
> -----Original Message-----
> From: Chuck Lever [mailto:chuck.lever@oracle.com] 
> Sent: Wednesday, December 21, 2011 5:22 PM
> To: Peter Staubach
> Cc: Jeff Layton; bfields@fieldses.org; steved@redhat.com; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld
> 
> 
> On Dec 21, 2011, at 5:04 PM, Peter Staubach wrote:
> 
>> By text based, you don't really mean encoding the binary structure as a string of ascii hex bytes, right?  Bleah.
> 
> No, I mean something akin to what is done for the other NFSD upcall and downcall arguments and results.  A single set of decimal digits for simple arguments, and "keyword=value keyword=value,value" and so on for complex arguments.  I recommend text because that is similar to what is used for every other modern user space API that NFSD has.
> 
>> I would suggest that XDR encoding seems quite natural to use when passing binary structs around.
> 
> To be clear, there is just one binary data item here, and that's nfs_client_id4.  The other data items are integers, which have well-defined text representations.
> 
> A new and separate API without any legacy baggage might be XDR, but text seems more appropriate here given the existence of other text APIs.  I'm not religious about this, just highlighting a few design choices.
> 
>> 	Thanx...
>> 
>> 		ps
>> 
>> 
>> -----Original Message-----
>> From: linux-nfs-owner@vger.kernel.org 
>> [mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
>> Sent: Wednesday, December 21, 2011 4:59 PM
>> To: Jeff Layton
>> Cc: Chuck Lever; bfields@fieldses.org; steved@redhat.com; 
>> linux-nfs@vger.kernel.org
>> Subject: Re: [PATCH v3 4/5] nfsd: add a header describing upcall to 
>> nfsdcld
>> 
>> On Wed, 21 Dec 2011 16:48:10 -0500
>> Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> On Wed, 21 Dec 2011 16:37:30 -0500
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>> 
>>>> 
>>>> On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
>>>> 
>>>>> On Wed, 21 Dec 2011 15:45:01 -0500 Chuck Lever 
>>>>> <chuck.lever@oracle.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
>>>>>> 
>>>>>>> The daemon takes a versioned binary struct. Hopefully this should 
>>>>>>> allow us to revise the struct later if it becomes necessary.
>>>>>> 
>>>>>> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
>>>>>> 
>>>>> 
>>>>> That's the main reason. We could, of course encode that string in 
>>>>> hex or something, and decode it on the other end. No one has 
>>>>> presented a strong argument for doing it that way as of yet though.
>>>>> 
>>>>> If anyone feels strongly about that, then it would be helpful to 
>>>>> have them pipe up now and state why they do...
>>>> 
>>>> <pipe>Because binary data structures are difficult to work with over 
>>>> time, which is why other NFSD user space interfaces are 
>>>> text-based.</pipe>
>>>> 
>>>> ;-)
>>>> 
>>> 
>>> *sigh* that was the sort of comments I was hoping to get out of the 
>>> RFC postings. But ok...
>>> 
>>> I'll see about respinning the whole thing with either a text-based or 
>>> XDR-based upcall/downcall format. That'll take a while, but I'll see 
>>> if I can get it in shape in time for 3.3.
>>> 
>> 
>> ...and while we're discussing it. Does anyone have thoughts on which would be better? I'd probably prefer a text-based format. That's more flexible but is also going to be more verbose. I think we're still well under a page with these requests even in text however so I don't really see that as a problem.
>> 
>> --
>> Jeff Layton <jlayton@redhat.com>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" 
>> in the body of a message to majordomo@vger.kernel.org More majordomo 
>> info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 17:33:23 -0500
Peter Staubach <pstaubach@exagrid.com> wrote:

> I will need to disagree with the assertion regarding XDR and the packed struct.  XDR can handle a versioned struct very neatly.  It can do so just as easily as text string parsing and with greatly reduced overhead.
> 

Ok, I'll bite -- what does XDR give us in this situation over a packed
struct?

Clearly there are benefits when the producer and consumer may have
considerable differences, as is the case between networked hosts. For
instance, different endianness or word size, etc.

Here though, the producer and consumer are the same host, so we can be
reasonably sure that the endianness is the same. I made sure to define
the struct with explicit sizes for the fields, so word size isn't a
factor. There's also a version field at the head so we could
potentially rev the upcall struct version later if needed. An XDR
format would need the same thing...

AFAICS, Using XDR is just going to add extra overhead here, but not
eliminate any of the drawbacks. If we have to rev the format later,
we'll be in the same situation.

A text based upcall has similar issues too, but it does at least make
debugging simpler. As Chuck points out too, there is some variability
in arguments as well. I think too that we're going to end up adding
some other upcalls here, and those will also need a different set
of arguments from what's there today.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jeff Layton]

On Wed, 21 Dec 2011 16:37:30 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Dec 21, 2011, at 4:33 PM, Jeff Layton wrote:
> 
> > On Wed, 21 Dec 2011 15:45:01 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> >> 
> >> On Dec 21, 2011, at 3:34 PM, Jeff Layton wrote:
> >> 
> >>> The daemon takes a versioned binary struct. Hopefully this should allow
> >>> us to revise the struct later if it becomes necessary.
> >> 
> >> This breaks the pattern of using text-based up- and downcalls in NFSD.  I assume this is binary because nfs_client_id4 is a string of opaque bytes?
> >> 
> > 
> > That's the main reason. We could, of course encode that string in hex or
> > something, and decode it on the other end. No one has presented a
> > strong argument for doing it that way as of yet though.
> > 
> > If anyone feels strongly about that, then it would be helpful to have
> > them pipe up now and state why they do...
> 
> <pipe>Because binary data structures are difficult to work with over time, which is why other NFSD user space interfaces are text-based.</pipe>
> 
> ;-)
> 
> 

Now that I've started looking at actually implementing this, I'm
leaning toward keeping this as a packed binary struct. The upcalls to
which you refer mostly use the cache_* interfaces in the kernel, and
the actual upcall "pipes" are in /proc. That code is already set up to
use primarily text based interfaces so it makes sense there.

I looked at using that here and determined that the cache_* interface
was not well suited to this task. Those are geared toward interfaces
(like exportfs or mountd) where the information is primarily stored and
manipulated in userspace and the kernel upcalls to fetch that info and
populate its cache.

In this case, the info is mostly coming from kernel space originally
and I believe that rpc_pipefs was better suited for this task. The
upcalls that use rpc_pipefs almost universally use a binary struct to
pass data between userspace and the kernel.

Certainly this could use text or XDR, but I don't see that as a clear
benefit here. It's going to add a lot of extra overhead and complexity
to both the kernel and userspace code and we'll still have similar
concerns with extending the interface later.

The union that the code currently uses for passing call-specific args
is 1k currently. If we're concerned about that expandablity then we can
simply pad that out to a larger size.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jim Rees]

Jeff Layton wrote:

  Now that I've started looking at actually implementing this, I'm
  leaning toward keeping this as a packed binary struct. The upcalls to
  which you refer mostly use the cache_* interfaces in the kernel, and
  the actual upcall "pipes" are in /proc. That code is already set up to
  use primarily text based interfaces so it makes sense there.
  
  I looked at using that here and determined that the cache_* interface
  was not well suited to this task. Those are geared toward interfaces
  (like exportfs or mountd) where the information is primarily stored and
  manipulated in userspace and the kernel upcalls to fetch that info and
  populate its cache.
  
  In this case, the info is mostly coming from kernel space originally
  and I believe that rpc_pipefs was better suited for this task. The
  upcalls that use rpc_pipefs almost universally use a binary struct to
  pass data between userspace and the kernel.

Having recently added a new rpc_pipe upcall for idmapd, my only suggestion
is that you think about compatibility issues if the message format changes.
Ideally you want any combination of new/old * kernel/user to work.  But I
suspect you've already thought about this.

I see no harm in using binary for an interface that's only intended for a
particular user daemon (assuming you solve the compatibility problem).  Text
is useful for things in /proc that someone might want to manipulate using
their own tool.  For something like idmapd I don't think a text interface is
necessary.  No one is going to write their own idmapd, and if they do, the
binary upcall interface will be the least of their worries.  The same may be
true of nfsdcld.

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[Jim Rees]

Jim Rees wrote:

  Having recently added a new rpc_pipe upcall for idmapd,...

That should read "blkmapd" of course.

Re: [PATCH v3 4/5] nfsd: add a header describing upcall to nfsdcld

[J. Bruce Fields]

On Thu, Dec 22, 2011 at 10:20:51AM -0500, Jim Rees wrote:
> Jeff Layton wrote:
> 
>   Now that I've started looking at actually implementing this, I'm
>   leaning toward keeping this as a packed binary struct. The upcalls to
>   which you refer mostly use the cache_* interfaces in the kernel, and
>   the actual upcall "pipes" are in /proc. That code is already set up to
>   use primarily text based interfaces so it makes sense there.
>   
>   I looked at using that here and determined that the cache_* interface
>   was not well suited to this task. Those are geared toward interfaces
>   (like exportfs or mountd) where the information is primarily stored and
>   manipulated in userspace and the kernel upcalls to fetch that info and
>   populate its cache.
>   
>   In this case, the info is mostly coming from kernel space originally
>   and I believe that rpc_pipefs was better suited for this task. The
>   upcalls that use rpc_pipefs almost universally use a binary struct to
>   pass data between userspace and the kernel.
> 
> Having recently added a new rpc_pipe upcall for idmapd, my only suggestion
> is that you think about compatibility issues if the message format changes.
> Ideally you want any combination of new/old * kernel/user to work.  But I
> suspect you've already thought about this.

Yes, I think the 8-bit version and command numbers give us what we'd
need.  We should make sure the daemon returns a well-defined error in
the case it doesn't know about a new command or version number.  And
then any backwards compatibility problem can be solved with sufficient
application of switch statements.  Hopefully unnecessary, but should be
safe.

So the current format encoding looks OK to me.  Unless someone has a
real slam-dunk of an argument I'd rather stick with it for now.

--b.

> I see no harm in using binary for an interface that's only intended for a
> particular user daemon (assuming you solve the compatibility problem).  Text
> is useful for things in /proc that someone might want to manipulate using
> their own tool.  For something like idmapd I don't think a text interface is
> necessary.  No one is going to write their own idmapd, and if they do, the
> binary upcall interface will be the least of their worries.  The same may be
> true of nfsdcld.