[PATCH 0/14] Updates to nfs41 client backchannel for 2.6.31

[PATCH 0/14] Updates to nfs41 client backchannel for 2.6.31

[Ricardo Labiaga]

Trond,

Here's a set of patches that address the comments received so far on the
"nfs41 client backchannel for 2.6.31" series.  We'll squash these into
Benny's tree, but I wanted to first submit them as stand alone patches for
your review.

Benny,

Each patch indicates the patch it should in turn be squased with.
Note that patch 10 simply backs out the changes of the original patch. 
We'll also need to manually move the removal of the 'xprt_free_bc_request'
export symbol into the correct patch through git-rebase.  We can do that
next week during bakeathon.


[PATCH 01/14] SQUASHME: Type check arguments of nfs_callback_up
[PATCH 02/14] SQUASHME: Update copyright notice and explain page allocation
[PATCH 03/14] SQUASHME: Update Copyright notice and fix formatting
[PATCH 04/14] SQUASHME: rpc_count_iostats incorrectly exits early
[PATCH 05/14] SQUASHME: Convert rpc_reply_expected() to inline function
[PATCH 06/14] SQUASHME: Remove unnecessary BUG_ON()
[PATCH 07/14] SQUASHME: Rename variable
[PATCH 08/14] SQUASHME: Removal of ugly #ifdefs
[PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into the XDR buffer
[PATCH 10/14] SQUASHME: nfs41: sunrpc: Don't skip past the RPC call direction
[PATCH 11/14] SQUASHME: Moves embedded #ifdefs into #ifdef function blocks
[PATCH 12/14] SQUASHME: Removes bc_svc_process() declaration
[PATCH 13/14] SQUASHME: Move bc_svc_process() declaration to correct patch
[PATCH 14/14] SQUASHME: Update copyright

Thanks,

- ricardo

[PATCH 01/14] SQUASHME: Type check arguments of nfs_callback_up

[Ricardo Labiaga]

[squash with: nfs41: minorversion support for nfs4_{init, destroy}_callback

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 fs/nfs/callback.c |    5 +----
 fs/nfs/callback.h |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 972e38b..4395c96 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -214,7 +214,7 @@ out:
 /*
  * Bring up the callback thread if it is not already up.
  */
-int nfs_callback_up(u32 minorversion, void *args)
+int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 {
 	struct svc_serv *serv = NULL;
 	struct svc_rqst *rqstp;
@@ -222,9 +222,6 @@ int nfs_callback_up(u32 minorversion, void *args)
 	struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
 	char svc_name[12];
 	int ret = 0;
-#if defined(CONFIG_NFS_V4_1)
-	struct rpc_xprt *xprt = (struct rpc_xprt *)args;
-#endif /* CONFIG_NFS_V4_1 */
 
 	mutex_lock(&nfs_callback_mutex);
 	if (cb_info->users++ || cb_info->task != NULL) {
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index 4bd0daf..07baa82 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -112,7 +112,7 @@ extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getat
 extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
 
 #ifdef CONFIG_NFS_V4
-extern int nfs_callback_up(u32 minorversion, void *args);
+extern int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt);
 extern void nfs_callback_down(int minorversion);
 #endif /* CONFIG_NFS_V4 */
 
-- 
1.5.4.3

[PATCH 02/14] SQUASHME: Update copyright notice and explain page allocation

[Ricardo Labiaga]

[squash with: nfs41: New backchannel helper routines]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/backchannel_rqst.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 8f177a5..5a7d342 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -1,8 +1,9 @@
 /******************************************************************************
 
 (c) 2007 Network Appliance, Inc.  All Rights Reserved.
+(c) 2009 NetApp.  All Rights Reserved.
 
-Network Appliance provides this source code under the GPL v2 License.
+NetApp provides this source code under the GPL v2 License.
 The GPL v2 license is available at
 http://opensource.org/licenses/gpl-license.php.
 
@@ -75,6 +76,14 @@ static void xprt_free_allocation(struct rpc_rqst *req)
  * incoming callback request.  It's up to the higher levels in the
  * stack to enforce that the maximum number of session slots is not
  * being exceeded.
+ *
+ * Some callback arguments can be large.  For example, a pNFS server
+ * using multiple deviceids.  The list can be unbound, but the client
+ * has the ability to tell the server the maximum size of the callback
+ * requests.  Each deviceID is 16 bytes, so allocate one page
+ * for the arguments to have enough room to receive a number of these
+ * deviceIDs.  The NFS client indicates to the pNFS server that its
+ * callback requests can be up to 4096 bytes in size.
  */
 int xprt_setup_backchannel(struct rpc_xprt *xprt, unsigned int min_reqs)
 {
-- 
1.5.4.3

[PATCH 03/14] SQUASHME: Update Copyright notice and fix formatting

[Ricardo Labiaga]

[squash with: nfs41: New include/linux/sunrpc/bc_xprt.h]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 include/linux/sunrpc/bc_xprt.h |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
index 3016c00..6508f0d 100644
--- a/include/linux/sunrpc/bc_xprt.h
+++ b/include/linux/sunrpc/bc_xprt.h
@@ -1,8 +1,8 @@
 /******************************************************************************
 
-(c) 2008 Network Appliance, Inc.  All Rights Reserved.
+(c) 2008 NetApp.  All Rights Reserved.
 
-Network Appliance provides this source code under the GPL v2 License.
+NetApp provides this source code under the GPL v2 License.
 The GPL v2 license is available at
 http://opensource.org/licenses/gpl-license.php.
 
@@ -41,7 +41,9 @@ int bc_send(struct rpc_rqst *req);
 #else /* CONFIG_NFS_V4_1 */
 static inline int xprt_setup_backchannel(struct rpc_xprt *xprt,
 					 unsigned int min_reqs)
-{ return 0; }
+{
+	return 0;
+}
 #endif /* CONFIG_NFS_V4_1 */
 #endif /* _LINUX_SUNRPC_BC_XPRT_H */
 
-- 
1.5.4.3

[PATCH 04/14] SQUASHME: rpc_count_iostats incorrectly exits early

[Ricardo Labiaga]

It's incorrectly taking an early exit if the tk_client has metrics.

[squash with: nfs41: Add backchannel processing support to RPC state machine]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/stats.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index c1517e2..1b4e679 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -145,7 +145,7 @@ void rpc_count_iostats(struct rpc_task *task)
 	struct rpc_iostats *op_metrics;
 	long rtt, execute, queue;
 
-	if (!task->tk_client || task->tk_client->cl_metrics || !req)
+	if (!task->tk_client || !task->tk_client->cl_metrics || !req)
 		return;
 
 	stats = task->tk_client->cl_metrics;
-- 
1.5.4.3

[PATCH 05/14] SQUASHME: Convert rpc_reply_expected() to inline function

[Ricardo Labiaga]

[squash with: nfs41: Add backchannel processing support to RPC state machine]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/sunrpc.h |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h
index f753d51..045f175 100644
--- a/net/sunrpc/sunrpc.h
+++ b/net/sunrpc/sunrpc.h
@@ -21,15 +21,17 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 ******************************************************************************/
 
 /*
- * Functions and macros used internally by RPC
+ * Functions used internally by RPC
  */
 
 #ifndef _NET_SUNRPC_SUNRPC_H
 #define _NET_SUNRPC_SUNRPC_H
 
-#define rpc_reply_expected(task) \
-	(((task)->tk_msg.rpc_proc != NULL) && \
-	((task)->tk_msg.rpc_proc->p_decode != NULL))
+static inline int rpc_reply_expected(struct rpc_task *task)
+{
+	return (task->tk_msg.rpc_proc != NULL) &&
+		(task->tk_msg.rpc_proc->p_decode != NULL);
+}
 
 int svc_send_common(struct socket *sock, struct xdr_buf *xdr,
 		struct page *headpage, unsigned long headoffset,
-- 
1.5.4.3

[PATCH 06/14] SQUASHME: Remove unnecessary BUG_ON()

[Ricardo Labiaga]

It was useful during early development, no longer necessary.

[squash with: nfs41: Add backchannel processing support to RPC state machine]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/xprt.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 2b189af..a8c6e8c 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1004,7 +1004,6 @@ void xprt_release(struct rpc_task *task)
 	struct rpc_rqst	*req;
 	int prealloc;
 
-	BUG_ON(atomic_read(&task->tk_count) < 0);
 	if (!(req = task->tk_rqstp))
 		return;
 	prealloc = bc_prealloc(req);	/* Preallocated backchannel request? */
-- 
1.5.4.3

[PATCH 07/14] SQUASHME: Rename variable

[Ricardo Labiaga]

[squash with: nfs41: Add backchannel processing support to RPC state machine]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/xprt.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index a8c6e8c..23c623b 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1002,11 +1002,14 @@ void xprt_release(struct rpc_task *task)
 {
 	struct rpc_xprt	*xprt;
 	struct rpc_rqst	*req;
-	int prealloc;
+	int is_bc_request;
 
 	if (!(req = task->tk_rqstp))
 		return;
-	prealloc = bc_prealloc(req);	/* Preallocated backchannel request? */
+
+	/* Preallocated backchannel request? */
+	is_bc_request = bc_prealloc(req);
+
 	xprt = req->rq_xprt;
 	rpc_count_iostats(task);
 	spin_lock_bh(&xprt->transport_lock);
@@ -1030,7 +1033,7 @@ void xprt_release(struct rpc_task *task)
 	 * Early exit if this is a backchannel preallocated request.
 	 * There is no need to have it added to the RPC slot list.
 	 */
-	if (prealloc)
+	if (is_bc_request)
 		return;
 
 	memset(req, 0, sizeof(*req));	/* mark unused */
-- 
1.5.4.3

[PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Ricardo Labiaga]

[squash with: nfs41: Implement NFSv4.1 callback service process]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 fs/nfs/callback.c |   50 +++++++++++++++++++++++++++++++++++---------------
 1 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 4395c96..116424e 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -209,6 +209,39 @@ out:
 	dprintk("--> %s return %p\n", __func__, rqstp);
 	return rqstp;
 }
+
+static inline void nfs_callback_svc_setup(u32 minorversion,
+		struct svc_serv *serv, struct rpc_xprt *xprt,
+		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
+{
+	if (minorversion) {
+		*rqstpp = nfs41_callback_up(serv, xprt);
+		*callback_svc = nfs41_callback_svc;
+	} else {
+		*rqstpp = nfs4_callback_up(serv);
+		*callback_svc = nfs4_callback_svc;
+	}
+}
+
+static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
+		struct nfs_callback_data *cb_info)
+{
+	if (minorversion)
+		xprt->bc_serv = cb_info->serv;
+}
+#else
+static inline void nfs_callback_svc_setup(u32 minorversion,
+		struct svc_serv *serv, struct rpc_xprt *xprt,
+		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
+{
+	*rqstpp = nfs4_callback_up(serv);
+	*callback_svc = nfs4_callback_svc;
+}
+
+static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
+		struct nfs_callback_data *cb_info)
+{
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 /*
@@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 
 	mutex_lock(&nfs_callback_mutex);
 	if (cb_info->users++ || cb_info->task != NULL) {
-#if defined(CONFIG_NFS_V4_1)
-		if (minorversion)
-			xprt->bc_serv = cb_info->serv;
-#endif /* CONFIG_NFS_V4_1 */
+		nfs_callback_bc_serv(minorversion, xprt, cb_info);
 		goto out;
 	}
 	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
@@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 
 	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
 	 * need to monitor and control them both */
-	if (!minorversion) {
-		rqstp = nfs4_callback_up(serv);
-		callback_svc = nfs4_callback_svc;
-	} else {
-#if defined(CONFIG_NFS_V4_1)
-		rqstp = nfs41_callback_up(serv, xprt);
-		callback_svc = nfs41_callback_svc;
-#else  /* CONFIG_NFS_V4_1 */
-		BUG();
-#endif /* CONFIG_NFS_V4_1 */
-	}
+	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp, &callback_svc);
 
 	if (IS_ERR(rqstp)) {
 		ret = PTR_ERR(rqstp);
-- 
1.5.4.3

[PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into the XDR buffer

[Ricardo Labiaga]

[squash with: nfs41: Skip past the RPC call direction]

An earlier nfs41/sunrpc patch incorrectly assumed that all transports will
read the RPC direction and route callbacks or replies to the processing
routines.  This is only currently implemented for TCP, so rpc_verify_header()
needs to continue verifying that it is processing an RPC_REPLY.

Reading and storing the RPC direction is a three step process.

1. xs_tcp_read_calldir() reads the RPC direction, but it will not store it
in the XDR buffer since the 'struct rpc_rqst' is not yet available.

2. The 'struct rpc_rqst' is obtained during the TCP_RCV_COPY_DATA state.
This state need not necessarily be preceeded by the TCP_RCV_READ_CALLDIR.
For example, we may be reading a continuation packet to a large reply.
Therefore, we can't simply obtain the 'struct rpc_rqst' during the
TCP_RCV_READ_CALLDIR state and assume it's available during TCP_RCV_COPY_DATA.

This patch adds a new TCP_RCV_READ_CALLDIR flag to indicate the need to
read the RPC direction.  It then uses TCP_RCV_COPY_CALLDIR to indicate the
RPC direction needs to be saved after the 'struct rpc_rqst' has been allocated.

3. The 'struct rpc_rqst' is obtained by the xs_tcp_read_data() helper
functions.  xs_tcp_read_common() then saves the RPC direction in the XDR
buffer if TCP_RCV_COPY_CALLDIR is set.  This will happen when we're reading
the data immediately after the direction was read.  xs_tcp_read_common()
then clears this flag.

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/xprtsock.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 8ec2600..fe57ebd 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -275,12 +275,13 @@ struct sock_xprt {
 #define TCP_RCV_COPY_FRAGHDR	(1UL << 1)
 #define TCP_RCV_COPY_XID	(1UL << 2)
 #define TCP_RCV_COPY_DATA	(1UL << 3)
-#define TCP_RCV_COPY_CALLDIR	(1UL << 4)
+#define TCP_RCV_READ_CALLDIR	(1UL << 4)
+#define TCP_RCV_COPY_CALLDIR	(1UL << 5)
 
 /*
  * TCP RPC flags
  */
-#define TCP_RPC_REPLY		(1UL << 5)
+#define TCP_RPC_REPLY		(1UL << 6)
 
 static inline struct sockaddr *xs_addr(struct rpc_xprt *xprt)
 {
@@ -1002,7 +1003,7 @@ static inline void xs_tcp_read_xid(struct sock_xprt *transport, struct xdr_skb_r
 	if (used != len)
 		return;
 	transport->tcp_flags &= ~TCP_RCV_COPY_XID;
-	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
+	transport->tcp_flags |= TCP_RCV_READ_CALLDIR;
 	transport->tcp_copied = 4;
 	dprintk("RPC:       reading %s XID %08x\n",
 			(transport->tcp_flags & TCP_RPC_REPLY) ? "reply for"
@@ -1031,9 +1032,13 @@ static inline void xs_tcp_read_calldir(struct sock_xprt *transport,
 	transport->tcp_offset += used;
 	if (used != len)
 		return;
-	transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;
+	transport->tcp_flags &= ~TCP_RCV_READ_CALLDIR;
+	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
 	transport->tcp_flags |= TCP_RCV_COPY_DATA;
-	transport->tcp_copied += 4;
+	/*
+	 * We don't yet have the XDR buffer, so we will write the calldir
+	 * out after we get the buffer from the 'struct rpc_rqst'
+	 */
 	if (ntohl(calldir) == RPC_REPLY)
 		transport->tcp_flags |= TCP_RPC_REPLY;
 	else
@@ -1055,6 +1060,20 @@ static inline void xs_tcp_read_common(struct rpc_xprt *xprt,
 	ssize_t r;
 
 	rcvbuf = &req->rq_private_buf;
+
+	if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
+		/*
+		 * Save the RPC direction in the XDR buffer
+		 */
+		__be32	calldir = transport->tcp_flags & TCP_RPC_REPLY ?
+					htonl(RPC_REPLY) : 0;
+
+		memcpy(rcvbuf->head[0].iov_base + transport->tcp_copied,
+			&calldir, sizeof(calldir));
+		transport->tcp_copied += sizeof(calldir);
+		transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;
+	}
+
 	len = desc->count;
 	if (len > transport->tcp_reclen - transport->tcp_offset) {
 		struct xdr_skb_reader my_desc;
@@ -1258,7 +1277,7 @@ static int xs_tcp_data_recv(read_descriptor_t *rd_desc, struct sk_buff *skb, uns
 			continue;
 		}
 		/* Read in the call/reply flag */
-		if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
+		if (transport->tcp_flags & TCP_RCV_READ_CALLDIR) {
 			xs_tcp_read_calldir(transport, &desc);
 			continue;
 		}
-- 
1.5.4.3

[PATCH 10/14] SQUASHME: nfs41: sunrpc: Don't skip past the RPC call direction

[Ricardo Labiaga]

[squash with: nfs41: Skippast the RPC call direction]

xs_tcp_read_data() has been modified to include the RPC call direction in the
XDR buffer.  We need to read the direction during the header verification.

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/clnt.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e7fffd2..d5a85a9 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1507,12 +1507,12 @@ rpc_verify_header(struct rpc_task *task)
 	if ((len -= 3) < 0)
 		goto out_overflow;
 
-	/*
-	 * Skip the XID and call direction.
-	 * The underlying transport has read the XID and RPC call direction
-	 * to determine this is an RPC reply.
-	 */
-	p += 2;
+	p += 1; /* skip XID */
+	if ((n = ntohl(*p++)) != RPC_REPLY) {
+		dprintk("RPC: %5u %s: not an RPC reply: %x\n",
+			task->tk_pid, __func__, n);
+		goto out_garbage;
+	}
 
 	if ((n = ntohl(*p++)) != RPC_MSG_ACCEPTED) {
 		if (--len < 0)
-- 
1.5.4.3

[PATCH 11/14] SQUASHME: Moves embedded #ifdefs into #ifdef function blocks

[Ricardo Labiaga]

[squash with: nfs41: New xs_tcp_read_data()]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/xprtsock.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index fe57ebd..c6f24c0 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1207,6 +1207,23 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
 
 	return 0;
 }
+
+static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
+					struct xdr_skb_reader *desc)
+{
+	struct sock_xprt *transport =
+				container_of(xprt, struct sock_xprt, xprt);
+
+	return (transport->tcp_flags & TCP_RPC_REPLY) ?
+		xs_tcp_read_reply(xprt, desc) :
+		xs_tcp_read_callback(xprt, desc);
+}
+#else
+static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
+					struct xdr_skb_reader *desc)
+{
+	return xs_tcp_read_reply(xprt, desc);
+}
 #endif /* CONFIG_NFS_V4_1 */
 
 /*
@@ -1218,17 +1235,8 @@ static void xs_tcp_read_data(struct rpc_xprt *xprt,
 {
 	struct sock_xprt *transport =
 				container_of(xprt, struct sock_xprt, xprt);
-	int status;
-
-#if defined(CONFIG_NFS_V4_1)
-	status = (transport->tcp_flags & TCP_RPC_REPLY) ?
-		xs_tcp_read_reply(xprt, desc) :
-		xs_tcp_read_callback(xprt, desc);
-#else
-	status = xs_tcp_read_reply(xprt, desc);
-#endif /* CONFIG_NFS_V4_1 */
 
-	if (status == 0)
+	if (_xs_tcp_read_data(xprt, desc) == 0)
 		xs_tcp_check_fraghdr(transport);
 	else {
 		/*
-- 
1.5.4.3

[PATCH 12/14] SQUASHME: Removes bc_svc_process() declaration

[Ricardo Labiaga]

[squash with: nfs41: Implement Nfsv4.1 callback service process]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 include/linux/sunrpc/svc.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 52e8cb0..b8234f2 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -418,8 +418,6 @@ int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 int		   svc_pool_stats_open(struct svc_serv *serv, struct file *file);
 void		   svc_destroy(struct svc_serv *);
 int		   svc_process(struct svc_rqst *);
-int		   bc_svc_process(struct svc_serv *, struct rpc_rqst *,
-			struct svc_rqst *);
 int		   svc_register(const struct svc_serv *, const int,
 				const unsigned short, const unsigned short);
 
-- 
1.5.4.3

[PATCH 13/14] SQUASHME: Move bc_svc_process() declaration to correct patch

[Ricardo Labiaga]

[squash with: nfs41: Backchannel bc_svc_process()]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 include/linux/sunrpc/svc.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index b8234f2..52e8cb0 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -418,6 +418,8 @@ int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 int		   svc_pool_stats_open(struct svc_serv *serv, struct file *file);
 void		   svc_destroy(struct svc_serv *);
 int		   svc_process(struct svc_rqst *);
+int		   bc_svc_process(struct svc_serv *, struct rpc_rqst *,
+			struct svc_rqst *);
 int		   svc_register(const struct svc_serv *, const int,
 				const unsigned short, const unsigned short);
 
-- 
1.5.4.3

[PATCH 14/14] SQUASHME: Update copyright

[Ricardo Labiaga]

[squash with: nfs41: Backhannel callback service helper routines]

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
---
 net/sunrpc/bc_svc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
index b13f51d..13f214f 100644
--- a/net/sunrpc/bc_svc.c
+++ b/net/sunrpc/bc_svc.c
@@ -1,8 +1,9 @@
 /******************************************************************************
 
 (c) 2007 Network Appliance, Inc.  All Rights Reserved.
+(c) 2009 NetApp.  All Rights Reserved.
 
-Network Appliance provides this source code under the GPL v2 License.
+NetApp provides this source code under the GPL v2 License.
 The GPL v2 license is available at
 http://opensource.org/licenses/gpl-license.php.
 
-- 
1.5.4.3

Re: [PATCH 11/14] SQUASHME: Moves embedded #ifdefs into #ifdef function blocks

[Benny Halevy]

On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> [squash with: nfs41: New xs_tcp_read_data()]
> 
> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> ---
>  net/sunrpc/xprtsock.c |   28 ++++++++++++++++++----------
>  1 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index fe57ebd..c6f24c0 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1207,6 +1207,23 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>  
>  	return 0;
>  }
> +
> +static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
> +					struct xdr_skb_reader *desc)
> +{
> +	struct sock_xprt *transport =
> +				container_of(xprt, struct sock_xprt, xprt);
> +
> +	return (transport->tcp_flags & TCP_RPC_REPLY) ?
> +		xs_tcp_read_reply(xprt, desc) :
> +		xs_tcp_read_callback(xprt, desc);
> +}
> +#else
> +static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
> +					struct xdr_skb_reader *desc)
> +{
> +	return xs_tcp_read_reply(xprt, desc);
> +}
>  #endif /* CONFIG_NFS_V4_1 */

Again, I'd be inclined to avoid duplicating code.
If we want still to use an #ifdef (and in this case
we might as well just always check the tcp_flags)
the I'd suggest to code this as follows:

static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
				    struct xdr_skb_reader *desc)
{
#ifdef /* CONFIG_NFS_V4_1 */
	struct sock_xprt *transport =
				container_of(xprt, struct sock_xprt, xprt);

	if (!(transport->tcp_flags & TCP_RPC_REPLY))
		return xs_tcp_read_callback(xprt, desc);
#ifdef /* CONFIG_NFS_V4_1 */

	return xs_tcp_read_reply(xprt, desc);
}

Benny

>  
>  /*
> @@ -1218,17 +1235,8 @@ static void xs_tcp_read_data(struct rpc_xprt *xprt,
>  {
>  	struct sock_xprt *transport =
>  				container_of(xprt, struct sock_xprt, xprt);
> -	int status;
> -
> -#if defined(CONFIG_NFS_V4_1)
> -	status = (transport->tcp_flags & TCP_RPC_REPLY) ?
> -		xs_tcp_read_reply(xprt, desc) :
> -		xs_tcp_read_callback(xprt, desc);
> -#else
> -	status = xs_tcp_read_reply(xprt, desc);
> -#endif /* CONFIG_NFS_V4_1 */
>  
> -	if (status == 0)
> +	if (_xs_tcp_read_data(xprt, desc) == 0)
>  		xs_tcp_check_fraghdr(transport);
>  	else {
>  		/*

Re: [PATCH 11/14] SQUASHME: Moves embedded #ifdefs into #ifdef function blocks

[Trond Myklebust]

On Sun, 2009-06-14 at 10:39 -0400, Benny Halevy wrote:
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> > [squash with: nfs41: New xs_tcp_read_data()]
> > 
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  net/sunrpc/xprtsock.c |   28 ++++++++++++++++++----------
> >  1 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index fe57ebd..c6f24c0 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1207,6 +1207,23 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
> >  
> >  	return 0;
> >  }
> > +
> > +static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
> > +					struct xdr_skb_reader *desc)
> > +{
> > +	struct sock_xprt *transport =
> > +				container_of(xprt, struct sock_xprt, xprt);
> > +
> > +	return (transport->tcp_flags & TCP_RPC_REPLY) ?
> > +		xs_tcp_read_reply(xprt, desc) :
> > +		xs_tcp_read_callback(xprt, desc);
> > +}
> > +#else
> > +static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
> > +					struct xdr_skb_reader *desc)
> > +{
> > +	return xs_tcp_read_reply(xprt, desc);
> > +}
> >  #endif /* CONFIG_NFS_V4_1 */
> 
> Again, I'd be inclined to avoid duplicating code.
> If we want still to use an #ifdef (and in this case
> we might as well just always check the tcp_flags)
> the I'd suggest to code this as follows:
> 
> static inline int _xs_tcp_read_data(struct rpc_xprt *xprt,
> 				    struct xdr_skb_reader *desc)
> {
> #ifdef /* CONFIG_NFS_V4_1 */
> 	struct sock_xprt *transport =
> 				container_of(xprt, struct sock_xprt, xprt);
> 
> 	if (!(transport->tcp_flags & TCP_RPC_REPLY))
> 		return xs_tcp_read_callback(xprt, desc);
> #ifdef /* CONFIG_NFS_V4_1 */
> 
> 	return xs_tcp_read_reply(xprt, desc);
> }
> 
> Benny
> 
> >  
> >  /*
> > @@ -1218,17 +1235,8 @@ static void xs_tcp_read_data(struct rpc_xprt *xprt,
> >  {
> >  	struct sock_xprt *transport =
> >  				container_of(xprt, struct sock_xprt, xprt);
> > -	int status;
> > -
> > -#if defined(CONFIG_NFS_V4_1)
> > -	status = (transport->tcp_flags & TCP_RPC_REPLY) ?
> > -		xs_tcp_read_reply(xprt, desc) :
> > -		xs_tcp_read_callback(xprt, desc);
> > -#else
> > -	status = xs_tcp_read_reply(xprt, desc);
> > -#endif /* CONFIG_NFS_V4_1 */
> >  
> > -	if (status == 0)
> > +	if (_xs_tcp_read_data(xprt, desc) == 0)
> >  		xs_tcp_check_fraghdr(transport);
> >  	else {
> >  		/*
> 

NO! Just put the bits that are v4.1 specific in a function (make said
function empty in the !v4.1 case, and call it from the common code.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 10/14] SQUASHME: nfs41: sunrpc: Don't skip past the RPC call direction

[Benny Halevy]

On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> [squash with: nfs41: Skippast the RPC call direction]
> 
> xs_tcp_read_data() has been modified to include the RPC call direction in the
> XDR buffer.  We need to read the direction during the header verification.
> 
> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> ---
>  net/sunrpc/clnt.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index e7fffd2..d5a85a9 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1507,12 +1507,12 @@ rpc_verify_header(struct rpc_task *task)
>  	if ((len -= 3) < 0)
>  		goto out_overflow;
>  
> -	/*
> -	 * Skip the XID and call direction.
> -	 * The underlying transport has read the XID and RPC call direction
> -	 * to determine this is an RPC reply.
> -	 */
> -	p += 2;
> +	p += 1; /* skip XID */
> +	if ((n = ntohl(*p++)) != RPC_REPLY) {
> +		dprintk("RPC: %5u %s: not an RPC reply: %x\n",
> +			task->tk_pid, __func__, n);
> +		goto out_garbage;
> +	}
>  
>  	if ((n = ntohl(*p++)) != RPC_MSG_ACCEPTED) {
>  		if (--len < 0)

BTW, for bisectability reasons it looks like this patch needs to
be part of the previous patch:
"[PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into the XDR buffer"
Otherwise it introduces a bug that this patch fixes.
(just a nit, not that it matters much if both are to be squashed
into the same patch eventually)

Benny

RE: [PATCH 10/14] SQUASHME: nfs41: sunrpc: Don't skip past the RPC call direction

[Labiaga, Ricardo]

> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy@panasas.com]
> Sent: Sunday, June 14, 2009 7:35 AM
> To: Labiaga, Ricardo
> Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH 10/14] SQUASHME: nfs41: sunrpc: Don't skip past
the
> RPC call direction
> 
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga
<Ricardo.Labiaga@netapp.com>
> wrote:
> > [squash with: nfs41: Skippast the RPC call direction]
> >
> > xs_tcp_read_data() has been modified to include the RPC call
direction
> in the
> > XDR buffer.  We need to read the direction during the header
> verification.
> >
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  net/sunrpc/clnt.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index e7fffd2..d5a85a9 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -1507,12 +1507,12 @@ rpc_verify_header(struct rpc_task *task)
> >  	if ((len -= 3) < 0)
> >  		goto out_overflow;
> >
> > -	/*
> > -	 * Skip the XID and call direction.
> > -	 * The underlying transport has read the XID and RPC call
direction
> > -	 * to determine this is an RPC reply.
> > -	 */
> > -	p += 2;
> > +	p += 1; /* skip XID */
> > +	if ((n = ntohl(*p++)) != RPC_REPLY) {
> > +		dprintk("RPC: %5u %s: not an RPC reply: %x\n",
> > +			task->tk_pid, __func__, n);
> > +		goto out_garbage;
> > +	}
> >
> >  	if ((n = ntohl(*p++)) != RPC_MSG_ACCEPTED) {
> >  		if (--len < 0)
> 
> BTW, for bisectability reasons it looks like this patch needs to
> be part of the previous patch:
> "[PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into
the
> XDR buffer"
> Otherwise it introduces a bug that this patch fixes.
> (just a nit, not that it matters much if both are to be squashed
> into the same patch eventually)
> 

This patch obliterates the patch that introduced this functionality
completely.  It removes the changes made by the original patch " nfs41:
Skip past the RPC call direction" line by line.  The reason it's a
separate patch is because it makes the original patch disappear - we'll
never submit it.  I was intending to simply remove the initial patch
(and this one) using git-rebase this week at bakeathon.

- ricardo

> Benny

Re: [PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into the XDR buffer

[Benny Halevy]

On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> [squash with: nfs41: Skip past the RPC call direction]
> 
> An earlier nfs41/sunrpc patch incorrectly assumed that all transports will
> read the RPC direction and route callbacks or replies to the processing
> routines.  This is only currently implemented for TCP, so rpc_verify_header()
> needs to continue verifying that it is processing an RPC_REPLY.
> 
> Reading and storing the RPC direction is a three step process.
> 
> 1. xs_tcp_read_calldir() reads the RPC direction, but it will not store it
> in the XDR buffer since the 'struct rpc_rqst' is not yet available.
> 
> 2. The 'struct rpc_rqst' is obtained during the TCP_RCV_COPY_DATA state.
> This state need not necessarily be preceeded by the TCP_RCV_READ_CALLDIR.
> For example, we may be reading a continuation packet to a large reply.
> Therefore, we can't simply obtain the 'struct rpc_rqst' during the
> TCP_RCV_READ_CALLDIR state and assume it's available during TCP_RCV_COPY_DATA.
> 
> This patch adds a new TCP_RCV_READ_CALLDIR flag to indicate the need to
> read the RPC direction.  It then uses TCP_RCV_COPY_CALLDIR to indicate the
> RPC direction needs to be saved after the 'struct rpc_rqst' has been allocated.
> 
> 3. The 'struct rpc_rqst' is obtained by the xs_tcp_read_data() helper
> functions.  xs_tcp_read_common() then saves the RPC direction in the XDR
> buffer if TCP_RCV_COPY_CALLDIR is set.  This will happen when we're reading
> the data immediately after the direction was read.  xs_tcp_read_common()
> then clears this flag.
> 
> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> ---
>  net/sunrpc/xprtsock.c |   31 +++++++++++++++++++++++++------
>  1 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 8ec2600..fe57ebd 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -275,12 +275,13 @@ struct sock_xprt {
>  #define TCP_RCV_COPY_FRAGHDR	(1UL << 1)
>  #define TCP_RCV_COPY_XID	(1UL << 2)
>  #define TCP_RCV_COPY_DATA	(1UL << 3)
> -#define TCP_RCV_COPY_CALLDIR	(1UL << 4)
> +#define TCP_RCV_READ_CALLDIR	(1UL << 4)
> +#define TCP_RCV_COPY_CALLDIR	(1UL << 5)
>  
>  /*
>   * TCP RPC flags
>   */
> -#define TCP_RPC_REPLY		(1UL << 5)
> +#define TCP_RPC_REPLY		(1UL << 6)
>  
>  static inline struct sockaddr *xs_addr(struct rpc_xprt *xprt)
>  {
> @@ -1002,7 +1003,7 @@ static inline void xs_tcp_read_xid(struct sock_xprt *transport, struct xdr_skb_r
>  	if (used != len)
>  		return;
>  	transport->tcp_flags &= ~TCP_RCV_COPY_XID;
> -	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
> +	transport->tcp_flags |= TCP_RCV_READ_CALLDIR;
>  	transport->tcp_copied = 4;
>  	dprintk("RPC:       reading %s XID %08x\n",
>  			(transport->tcp_flags & TCP_RPC_REPLY) ? "reply for"
> @@ -1031,9 +1032,13 @@ static inline void xs_tcp_read_calldir(struct sock_xprt *transport,
>  	transport->tcp_offset += used;
>  	if (used != len)
>  		return;
> -	transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;
> +	transport->tcp_flags &= ~TCP_RCV_READ_CALLDIR;
> +	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
>  	transport->tcp_flags |= TCP_RCV_COPY_DATA;
> -	transport->tcp_copied += 4;
> +	/*
> +	 * We don't yet have the XDR buffer, so we will write the calldir
> +	 * out after we get the buffer from the 'struct rpc_rqst'
> +	 */
>  	if (ntohl(calldir) == RPC_REPLY)
>  		transport->tcp_flags |= TCP_RPC_REPLY;
>  	else
> @@ -1055,6 +1060,20 @@ static inline void xs_tcp_read_common(struct rpc_xprt *xprt,
>  	ssize_t r;
>  
>  	rcvbuf = &req->rq_private_buf;
> +
> +	if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
> +		/*
> +		 * Save the RPC direction in the XDR buffer
> +		 */
> +		__be32	calldir = transport->tcp_flags & TCP_RPC_REPLY ?
> +					htonl(RPC_REPLY) : 0;
> +
> +		memcpy(rcvbuf->head[0].iov_base + transport->tcp_copied,
> +			&calldir, sizeof(calldir));
> +		transport->tcp_copied += sizeof(calldir);
> +		transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;

Can this be done in xs_tcp_read_calldir()?

Benny

> +	}
> +
>  	len = desc->count;
>  	if (len > transport->tcp_reclen - transport->tcp_offset) {
>  		struct xdr_skb_reader my_desc;
> @@ -1258,7 +1277,7 @@ static int xs_tcp_data_recv(read_descriptor_t *rd_desc, struct sk_buff *skb, uns
>  			continue;
>  		}
>  		/* Read in the call/reply flag */
> -		if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
> +		if (transport->tcp_flags & TCP_RCV_READ_CALLDIR) {
>  			xs_tcp_read_calldir(transport, &desc);
>  			continue;
>  		}

RE: [PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction back into the XDR buffer

[Labiaga, Ricardo]

> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy@panasas.com]
> Sent: Friday, June 12, 2009 7:22 AM
> To: Labiaga, Ricardo
> Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH 09/14] SQUASHME: nfs41: sunrpc: Add RPC direction
back
> into the XDR buffer
> 
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga
<Ricardo.Labiaga@netapp.com>
> wrote:
> > [squash with: nfs41: Skip past the RPC call direction]
> >
> > An earlier nfs41/sunrpc patch incorrectly assumed that all
transports
> will
> > read the RPC direction and route callbacks or replies to the
processing
> > routines.  This is only currently implemented for TCP, so
> rpc_verify_header()
> > needs to continue verifying that it is processing an RPC_REPLY.
> >
> > Reading and storing the RPC direction is a three step process.
> >
> > 1. xs_tcp_read_calldir() reads the RPC direction, but it will not
store
> it
> > in the XDR buffer since the 'struct rpc_rqst' is not yet available.
> >
> > 2. The 'struct rpc_rqst' is obtained during the TCP_RCV_COPY_DATA
state.
> > This state need not necessarily be preceeded by the
> TCP_RCV_READ_CALLDIR.
> > For example, we may be reading a continuation packet to a large
reply.
> > Therefore, we can't simply obtain the 'struct rpc_rqst' during the
> > TCP_RCV_READ_CALLDIR state and assume it's available during
> TCP_RCV_COPY_DATA.
> >
> > This patch adds a new TCP_RCV_READ_CALLDIR flag to indicate the need
to
> > read the RPC direction.  It then uses TCP_RCV_COPY_CALLDIR to
indicate
> the
> > RPC direction needs to be saved after the 'struct rpc_rqst' has been
> allocated.
> >
> > 3. The 'struct rpc_rqst' is obtained by the xs_tcp_read_data()
helper
> > functions.  xs_tcp_read_common() then saves the RPC direction in the
XDR
> > buffer if TCP_RCV_COPY_CALLDIR is set.  This will happen when we're
> reading
> > the data immediately after the direction was read.
xs_tcp_read_common()
> > then clears this flag.
> >
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  net/sunrpc/xprtsock.c |   31 +++++++++++++++++++++++++------
> >  1 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 8ec2600..fe57ebd 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -275,12 +275,13 @@ struct sock_xprt {
> >  #define TCP_RCV_COPY_FRAGHDR	(1UL << 1)
> >  #define TCP_RCV_COPY_XID	(1UL << 2)
> >  #define TCP_RCV_COPY_DATA	(1UL << 3)
> > -#define TCP_RCV_COPY_CALLDIR	(1UL << 4)
> > +#define TCP_RCV_READ_CALLDIR	(1UL << 4)
> > +#define TCP_RCV_COPY_CALLDIR	(1UL << 5)
> >
> >  /*
> >   * TCP RPC flags
> >   */
> > -#define TCP_RPC_REPLY		(1UL << 5)
> > +#define TCP_RPC_REPLY		(1UL << 6)
> >
> >  static inline struct sockaddr *xs_addr(struct rpc_xprt *xprt)
> >  {
> > @@ -1002,7 +1003,7 @@ static inline void xs_tcp_read_xid(struct
> sock_xprt *transport, struct xdr_skb_r
> >  	if (used != len)
> >  		return;
> >  	transport->tcp_flags &= ~TCP_RCV_COPY_XID;
> > -	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
> > +	transport->tcp_flags |= TCP_RCV_READ_CALLDIR;
> >  	transport->tcp_copied = 4;
> >  	dprintk("RPC:       reading %s XID %08x\n",
> >  			(transport->tcp_flags & TCP_RPC_REPLY) ? "reply
for"
> > @@ -1031,9 +1032,13 @@ static inline void xs_tcp_read_calldir(struct
> sock_xprt *transport,
> >  	transport->tcp_offset += used;
> >  	if (used != len)
> >  		return;
> > -	transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;
> > +	transport->tcp_flags &= ~TCP_RCV_READ_CALLDIR;
> > +	transport->tcp_flags |= TCP_RCV_COPY_CALLDIR;
> >  	transport->tcp_flags |= TCP_RCV_COPY_DATA;
> > -	transport->tcp_copied += 4;
> > +	/*
> > +	 * We don't yet have the XDR buffer, so we will write the
calldir
> > +	 * out after we get the buffer from the 'struct rpc_rqst'
> > +	 */
> >  	if (ntohl(calldir) == RPC_REPLY)
> >  		transport->tcp_flags |= TCP_RPC_REPLY;
> >  	else
> > @@ -1055,6 +1060,20 @@ static inline void xs_tcp_read_common(struct
> rpc_xprt *xprt,
> >  	ssize_t r;
> >
> >  	rcvbuf = &req->rq_private_buf;
> > +
> > +	if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
> > +		/*
> > +		 * Save the RPC direction in the XDR buffer
> > +		 */
> > +		__be32	calldir = transport->tcp_flags & TCP_RPC_REPLY ?
> > +					htonl(RPC_REPLY) : 0;
> > +
> > +		memcpy(rcvbuf->head[0].iov_base + transport->tcp_copied,
> > +			&calldir, sizeof(calldir));
> > +		transport->tcp_copied += sizeof(calldir);
> > +		transport->tcp_flags &= ~TCP_RCV_COPY_CALLDIR;
> 
> Can this be done in xs_tcp_read_calldir()?
> 

No, because we don't yet have the XDR buffer.  The XDR buffer is in the
'struct rpc_rqst' which we can only obtain after we read the RPC
direction.  This because we need to see if we get it from the list of
pending replies, or from the list of preallocated callback buffers.  We
can not search for this in xs_tcp_read_calldir() because we also need to
obtain it for RPC continuation fragments that will not have the RPC call
direction bit set.

I played with another version of the patch that created a new state for
allocation of the rpc_rqst buffer, but ran into problems.  I'll work
some more on that version of the patch in the next few weeks, but I'd
like to get the submitted version in right now.

- ricardo


> Benny
> 
> > +	}
> > +
> >  	len = desc->count;
> >  	if (len > transport->tcp_reclen - transport->tcp_offset) {
> >  		struct xdr_skb_reader my_desc;
> > @@ -1258,7 +1277,7 @@ static int xs_tcp_data_recv(read_descriptor_t
> *rd_desc, struct sk_buff *skb, uns
> >  			continue;
> >  		}
> >  		/* Read in the call/reply flag */
> > -		if (transport->tcp_flags & TCP_RCV_COPY_CALLDIR) {
> > +		if (transport->tcp_flags & TCP_RCV_READ_CALLDIR) {
> >  			xs_tcp_read_calldir(transport, &desc);
> >  			continue;
> >  		}

Re: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Benny Halevy]

On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> [squash with: nfs41: Implement NFSv4.1 callback service process]
> 
> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> ---
>  fs/nfs/callback.c |   50 +++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 4395c96..116424e 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -209,6 +209,39 @@ out:
>  	dprintk("--> %s return %p\n", __func__, rqstp);
>  	return rqstp;
>  }
> +
> +static inline void nfs_callback_svc_setup(u32 minorversion,
> +		struct svc_serv *serv, struct rpc_xprt *xprt,
> +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> +{
> +	if (minorversion) {
> +		*rqstpp = nfs41_callback_up(serv, xprt);
> +		*callback_svc = nfs41_callback_svc;
> +	} else {
> +		*rqstpp = nfs4_callback_up(serv);
> +		*callback_svc = nfs4_callback_svc;
> +	}
> +}
> +
> +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> +		struct nfs_callback_data *cb_info)
> +{
> +	if (minorversion)
> +		xprt->bc_serv = cb_info->serv;
> +}
> +#else
> +static inline void nfs_callback_svc_setup(u32 minorversion,
> +		struct svc_serv *serv, struct rpc_xprt *xprt,
> +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> +{
> +	*rqstpp = nfs4_callback_up(serv);
> +	*callback_svc = nfs4_callback_svc;
> +}
> +
> +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> +		struct nfs_callback_data *cb_info)
> +{
> +}
>  #endif /* CONFIG_NFS_V4_1 */

Although this removes the ungly #ifdefs from inside nfs_callback_up
it just introduces them elsewhere and what's worse, it adds code
duplication which is even more unreadable and harder to maintain.

How about something along these line?

static inline void nfs_callback_svc_setup(u32 minorversion,
		struct svc_serv *serv, struct rpc_xprt *xprt,
		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
{
#ifdef CONFIG_NFS_V4_1
	if (minorversion) {
		*rqstpp = nfs41_callback_up(serv, xprt);
		*callback_svc = nfs41_callback_svc;
		return;
	}
#endif /* CONFIG_NFS_V4_1 */
	*rqstpp = nfs4_callback_up(serv);
	*callback_svc = nfs4_callback_svc;
}

static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
		struct nfs_callback_data *cb_info)
{
#ifdef CONFIG_NFS_V4_1
	if (minorversion)
		xprt->bc_serv = cb_info->serv;
#endif /* CONFIG_NFS_V4_1 */
}

Benny

>  
>  /*
> @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
>  
>  	mutex_lock(&nfs_callback_mutex);
>  	if (cb_info->users++ || cb_info->task != NULL) {
> -#if defined(CONFIG_NFS_V4_1)
> -		if (minorversion)
> -			xprt->bc_serv = cb_info->serv;
> -#endif /* CONFIG_NFS_V4_1 */
> +		nfs_callback_bc_serv(minorversion, xprt, cb_info);
>  		goto out;
>  	}
>  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
> @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
>  
>  	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
>  	 * need to monitor and control them both */
> -	if (!minorversion) {
> -		rqstp = nfs4_callback_up(serv);
> -		callback_svc = nfs4_callback_svc;
> -	} else {
> -#if defined(CONFIG_NFS_V4_1)
> -		rqstp = nfs41_callback_up(serv, xprt);
> -		callback_svc = nfs41_callback_svc;
> -#else  /* CONFIG_NFS_V4_1 */
> -		BUG();
> -#endif /* CONFIG_NFS_V4_1 */
> -	}
> +	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp, &callback_svc);
>  
>  	if (IS_ERR(rqstp)) {
>  		ret = PTR_ERR(rqstp);

Re: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Trond Myklebust]

On Sun, 2009-06-14 at 10:30 -0400, Benny Halevy wrote:
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
> > [squash with: nfs41: Implement NFSv4.1 callback service process]
> > 
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  fs/nfs/callback.c |   50 +++++++++++++++++++++++++++++++++++---------------
> >  1 files changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index 4395c96..116424e 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -209,6 +209,39 @@ out:
> >  	dprintk("--> %s return %p\n", __func__, rqstp);
> >  	return rqstp;
> >  }
> > +
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> > +{
> > +	if (minorversion) {
> > +		*rqstpp = nfs41_callback_up(serv, xprt);
> > +		*callback_svc = nfs41_callback_svc;
> > +	} else {
> > +		*rqstpp = nfs4_callback_up(serv);
> > +		*callback_svc = nfs4_callback_svc;
> > +	}
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +	if (minorversion)
> > +		xprt->bc_serv = cb_info->serv;
> > +}
> > +#else
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> > +{
> > +	*rqstpp = nfs4_callback_up(serv);
> > +	*callback_svc = nfs4_callback_svc;
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +}
> >  #endif /* CONFIG_NFS_V4_1 */
> 
> Although this removes the ungly #ifdefs from inside nfs_callback_up
> it just introduces them elsewhere and what's worse, it adds code
> duplication which is even more unreadable and harder to maintain.
> 
> How about something along these line?
> 
> static inline void nfs_callback_svc_setup(u32 minorversion,
> 		struct svc_serv *serv, struct rpc_xprt *xprt,
> 		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion) {
> 		*rqstpp = nfs41_callback_up(serv, xprt);
> 		*callback_svc = nfs41_callback_svc;
> 		return;
> 	}
> #endif /* CONFIG_NFS_V4_1 */\

NACK.... That's _exactly_ the kind of unreadable nonsense I _DON'T_ want
to see....

> 	*rqstpp = nfs4_callback_up(serv);
> 	*callback_svc = nfs4_callback_svc;
> }
> 
> static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> 		struct nfs_callback_data *cb_info)
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion)
> 		xprt->bc_serv = cb_info->serv;
> #endif /* CONFIG_NFS_V4_1 */
> }
> 
> Benny
> 
> >  
> >  /*
> > @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
> >  
> >  	mutex_lock(&nfs_callback_mutex);
> >  	if (cb_info->users++ || cb_info->task != NULL) {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		if (minorversion)
> > -			xprt->bc_serv = cb_info->serv;
> > -#endif /* CONFIG_NFS_V4_1 */
> > +		nfs_callback_bc_serv(minorversion, xprt, cb_info);
> >  		goto out;
> >  	}
> >  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
> > @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
> >  
> >  	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
> >  	 * need to monitor and control them both */
> > -	if (!minorversion) {
> > -		rqstp = nfs4_callback_up(serv);
> > -		callback_svc = nfs4_callback_svc;
> > -	} else {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		rqstp = nfs41_callback_up(serv, xprt);
> > -		callback_svc = nfs41_callback_svc;
> > -#else  /* CONFIG_NFS_V4_1 */
> > -		BUG();
> > -#endif /* CONFIG_NFS_V4_1 */
> > -	}
> > +	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp, &callback_svc);
> >  
> >  	if (IS_ERR(rqstp)) {
> >  		ret = PTR_ERR(rqstp);
> 
> --
> 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

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [pnfs] [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Boaz Harrosh]

On 06/14/2009 07:53 PM, Trond Myklebust wrote:
> On Sun, 2009-06-14 at 10:30 -0400, Benny Halevy wrote:
>> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
>>> [squash with: nfs41: Implement NFSv4.1 callback service process]
>>>
>>> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
>>> ---
>>>  fs/nfs/callback.c |   50 +++++++++++++++++++++++++++++++++++---------------
>>>  1 files changed, 35 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>>> index 4395c96..116424e 100644
>>> --- a/fs/nfs/callback.c
>>> +++ b/fs/nfs/callback.c
>>> @@ -209,6 +209,39 @@ out:
>>>  	dprintk("--> %s return %p\n", __func__, rqstp);
>>>  	return rqstp;
>>>  }
>>> +
>>> +static inline void nfs_callback_svc_setup(u32 minorversion,
>>> +		struct svc_serv *serv, struct rpc_xprt *xprt,
>>> +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
>>> +{
>>> +	if (minorversion) {
>>> +		*rqstpp = nfs41_callback_up(serv, xprt);
>>> +		*callback_svc = nfs41_callback_svc;
>>> +	} else {
>>> +		*rqstpp = nfs4_callback_up(serv);
>>> +		*callback_svc = nfs4_callback_svc;
>>> +	}
>>> +}
>>> +
>>> +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
>>> +		struct nfs_callback_data *cb_info)
>>> +{
>>> +	if (minorversion)
>>> +		xprt->bc_serv = cb_info->serv;
>>> +}
>>> +#else
>>> +static inline void nfs_callback_svc_setup(u32 minorversion,
>>> +		struct svc_serv *serv, struct rpc_xprt *xprt,
>>> +		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
>>> +{
>>> +	*rqstpp = nfs4_callback_up(serv);
>>> +	*callback_svc = nfs4_callback_svc;
>>> +}
>>> +
>>> +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
>>> +		struct nfs_callback_data *cb_info)
>>> +{
>>> +}
>>>  #endif /* CONFIG_NFS_V4_1 */
>> Although this removes the ungly #ifdefs from inside nfs_callback_up
>> it just introduces them elsewhere and what's worse, it adds code
>> duplication which is even more unreadable and harder to maintain.
>>
>> How about something along these line?
>>
>> static inline void nfs_callback_svc_setup(u32 minorversion,
>> 		struct svc_serv *serv, struct rpc_xprt *xprt,
>> 		struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
>> {
>> #ifdef CONFIG_NFS_V4_1
>> 	if (minorversion) {
>> 		*rqstpp = nfs41_callback_up(serv, xprt);
>> 		*callback_svc = nfs41_callback_svc;
>> 		return;
>> 	}
>> #endif /* CONFIG_NFS_V4_1 */\
> 
> NACK.... That's _exactly_ the kind of unreadable nonsense I _DON'T_ want
> to see....
> 
if you do somewhere global
#ifdef CONFIG_NFS_V4_1
	...
#else
	const int minorversion = 0;
#endif

Then the above if (minorversion) will be optimized out if not CONFIG_NFS_V4_1,
by the compiler. (And as a bonus it still gets parsed even if not defined)

>> 	*rqstpp = nfs4_callback_up(serv);
>> 	*callback_svc = nfs4_callback_svc;
>> }
>>
>> static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
>> 		struct nfs_callback_data *cb_info)
>> {
>> #ifdef CONFIG_NFS_V4_1
>> 	if (minorversion)
>> 		xprt->bc_serv = cb_info->serv;
>> #endif /* CONFIG_NFS_V4_1 */
>> }
>>
>> Benny
>>
>>>  
>>>  /*
>>> @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
>>>  
>>>  	mutex_lock(&nfs_callback_mutex);
>>>  	if (cb_info->users++ || cb_info->task != NULL) {
>>> -#if defined(CONFIG_NFS_V4_1)
>>> -		if (minorversion)
>>> -			xprt->bc_serv = cb_info->serv;
>>> -#endif /* CONFIG_NFS_V4_1 */
>>> +		nfs_callback_bc_serv(minorversion, xprt, cb_info);
>>>  		goto out;
>>>  	}
>>>  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
>>> @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
>>>  
>>>  	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
>>>  	 * need to monitor and control them both */
>>> -	if (!minorversion) {
>>> -		rqstp = nfs4_callback_up(serv);
>>> -		callback_svc = nfs4_callback_svc;
>>> -	} else {
>>> -#if defined(CONFIG_NFS_V4_1)
>>> -		rqstp = nfs41_callback_up(serv, xprt);
>>> -		callback_svc = nfs41_callback_svc;
>>> -#else  /* CONFIG_NFS_V4_1 */
>>> -		BUG();
>>> -#endif /* CONFIG_NFS_V4_1 */
>>> -	}
>>> +	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp, &callback_svc);
>>>  
>>>  	if (IS_ERR(rqstp)) {
>>>  		ret = PTR_ERR(rqstp);
>> --
>> 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
> 

Boaz

RE: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Labiaga, Ricardo]

> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy@panasas.com]
> Sent: Sunday, June 14, 2009 7:30 AM
> To: Labiaga, Ricardo
> Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs
> 
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga
<Ricardo.Labiaga@netapp.com>
> wrote:
> > [squash with: nfs41: Implement NFSv4.1 callback service process]
> >
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  fs/nfs/callback.c |   50
+++++++++++++++++++++++++++++++++++-----------
> ----
> >  1 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index 4395c96..116424e 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -209,6 +209,39 @@ out:
> >  	dprintk("--> %s return %p\n", __func__, rqstp);
> >  	return rqstp;
> >  }
> > +
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> > +{
> > +	if (minorversion) {
> > +		*rqstpp = nfs41_callback_up(serv, xprt);
> > +		*callback_svc = nfs41_callback_svc;
> > +	} else {
> > +		*rqstpp = nfs4_callback_up(serv);
> > +		*callback_svc = nfs4_callback_svc;
> > +	}
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct
> rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +	if (minorversion)
> > +		xprt->bc_serv = cb_info->serv;
> > +}
> > +#else
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> > +{
> > +	*rqstpp = nfs4_callback_up(serv);
> > +	*callback_svc = nfs4_callback_svc;
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct
> rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +}
> >  #endif /* CONFIG_NFS_V4_1 */
> 
> Although this removes the ungly #ifdefs from inside nfs_callback_up
> it just introduces them elsewhere and what's worse, it adds code
> duplication which is even more unreadable and harder to maintain.
> 

There are two lines in common between the #ifdef and #else case.

+	*rqstpp = nfs4_callback_up(serv);
+	*callback_svc = nfs4_callback_svc;

Calling it code duplication is a bit of a stretch :-)

- ricardo

> How about something along these line?
> 
> static inline void nfs_callback_svc_setup(u32 minorversion,
> 		struct svc_serv *serv, struct rpc_xprt *xprt,
> 		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion) {
> 		*rqstpp = nfs41_callback_up(serv, xprt);
> 		*callback_svc = nfs41_callback_svc;
> 		return;
> 	}
> #endif /* CONFIG_NFS_V4_1 */
> 	*rqstpp = nfs4_callback_up(serv);
> 	*callback_svc = nfs4_callback_svc;
> }
> 
> static inline void nfs_callback_bc_serv(u32 minorversion, struct
rpc_xprt
> *xprt,
> 		struct nfs_callback_data *cb_info)
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion)
> 		xprt->bc_serv = cb_info->serv;
> #endif /* CONFIG_NFS_V4_1 */
> }
> 
> Benny
> 
> >
> >  /*
> > @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct
> rpc_xprt *xprt)
> >
> >  	mutex_lock(&nfs_callback_mutex);
> >  	if (cb_info->users++ || cb_info->task != NULL) {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		if (minorversion)
> > -			xprt->bc_serv = cb_info->serv;
> > -#endif /* CONFIG_NFS_V4_1 */
> > +		nfs_callback_bc_serv(minorversion, xprt, cb_info);
> >  		goto out;
> >  	}
> >  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
> NULL);
> > @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct
> rpc_xprt *xprt)
> >
> >  	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
> >  	 * need to monitor and control them both */
> > -	if (!minorversion) {
> > -		rqstp = nfs4_callback_up(serv);
> > -		callback_svc = nfs4_callback_svc;
> > -	} else {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		rqstp = nfs41_callback_up(serv, xprt);
> > -		callback_svc = nfs41_callback_svc;
> > -#else  /* CONFIG_NFS_V4_1 */
> > -		BUG();
> > -#endif /* CONFIG_NFS_V4_1 */
> > -	}
> > +	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp,
> &callback_svc);
> >
> >  	if (IS_ERR(rqstp)) {
> >  		ret = PTR_ERR(rqstp);
> 
> --
> 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 08/14] SQUASHME: Removal of ugly #ifdefs

[Halevy, Benny]

> There are two lines in common between the #ifdef and #else case.
> 
> +	*rqstpp = nfs4_callback_up(serv);
> +	*callback_svc = nfs4_callback_svc;

In the function body...
There's also the function's header.

> 
> Calling it code duplication is a bit of a stretch :-)

I agree it isn't much but still any change you will make
in the future will have to bapplied on both copies and that's bad (IMO).

Benny

> 
> - ricardo

-----Original Message-----
From: Labiaga, Ricardo [mailto:Ricardo.Labiaga@netapp.com]
Sent: Mon 2009-06-15 18:31
To: Halevy, Benny
Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org
Subject: RE: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs
 
> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy@panasas.com]
> Sent: Sunday, June 14, 2009 7:30 AM
> To: Labiaga, Ricardo
> Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs
> 
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga
<Ricardo.Labiaga@netapp.com>
> wrote:
> > [squash with: nfs41: Implement NFSv4.1 callback service process]
> >
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> > ---
> >  fs/nfs/callback.c |   50
+++++++++++++++++++++++++++++++++++-----------
> ----
> >  1 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index 4395c96..116424e 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -209,6 +209,39 @@ out:
> >  	dprintk("--> %s return %p\n", __func__, rqstp);
> >  	return rqstp;
> >  }
> > +
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> > +{
> > +	if (minorversion) {
> > +		*rqstpp = nfs41_callback_up(serv, xprt);
> > +		*callback_svc = nfs41_callback_svc;
> > +	} else {
> > +		*rqstpp = nfs4_callback_up(serv);
> > +		*callback_svc = nfs4_callback_svc;
> > +	}
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct
> rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +	if (minorversion)
> > +		xprt->bc_serv = cb_info->serv;
> > +}
> > +#else
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > +		struct svc_serv *serv, struct rpc_xprt *xprt,
> > +		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> > +{
> > +	*rqstpp = nfs4_callback_up(serv);
> > +	*callback_svc = nfs4_callback_svc;
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct
> rpc_xprt *xprt,
> > +		struct nfs_callback_data *cb_info)
> > +{
> > +}
> >  #endif /* CONFIG_NFS_V4_1 */
> 
> Although this removes the ungly #ifdefs from inside nfs_callback_up
> it just introduces them elsewhere and what's worse, it adds code
> duplication which is even more unreadable and harder to maintain.
> 

There are two lines in common between the #ifdef and #else case.

+	*rqstpp = nfs4_callback_up(serv);
+	*callback_svc = nfs4_callback_svc;

Calling it code duplication is a bit of a stretch :-)

- ricardo

> How about something along these line?
> 
> static inline void nfs_callback_svc_setup(u32 minorversion,
> 		struct svc_serv *serv, struct rpc_xprt *xprt,
> 		struct svc_rqst **rqstpp, int (**callback_svc)(void
*vrqstp))
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion) {
> 		*rqstpp = nfs41_callback_up(serv, xprt);
> 		*callback_svc = nfs41_callback_svc;
> 		return;
> 	}
> #endif /* CONFIG_NFS_V4_1 */
> 	*rqstpp = nfs4_callback_up(serv);
> 	*callback_svc = nfs4_callback_svc;
> }
> 
> static inline void nfs_callback_bc_serv(u32 minorversion, struct
rpc_xprt
> *xprt,
> 		struct nfs_callback_data *cb_info)
> {
> #ifdef CONFIG_NFS_V4_1
> 	if (minorversion)
> 		xprt->bc_serv = cb_info->serv;
> #endif /* CONFIG_NFS_V4_1 */
> }
> 
> Benny
> 
> >
> >  /*
> > @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct
> rpc_xprt *xprt)
> >
> >  	mutex_lock(&nfs_callback_mutex);
> >  	if (cb_info->users++ || cb_info->task != NULL) {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		if (minorversion)
> > -			xprt->bc_serv = cb_info->serv;
> > -#endif /* CONFIG_NFS_V4_1 */
> > +		nfs_callback_bc_serv(minorversion, xprt, cb_info);
> >  		goto out;
> >  	}
> >  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
> NULL);
> > @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct
> rpc_xprt *xprt)
> >
> >  	/* FIXME: either 4.0 or 4.1 callback service can be up at a time
> >  	 * need to monitor and control them both */
> > -	if (!minorversion) {
> > -		rqstp = nfs4_callback_up(serv);
> > -		callback_svc = nfs4_callback_svc;
> > -	} else {
> > -#if defined(CONFIG_NFS_V4_1)
> > -		rqstp = nfs41_callback_up(serv, xprt);
> > -		callback_svc = nfs41_callback_svc;
> > -#else  /* CONFIG_NFS_V4_1 */
> > -		BUG();
> > -#endif /* CONFIG_NFS_V4_1 */
> > -	}
> > +	nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp,
> &callback_svc);
> >
> >  	if (IS_ERR(rqstp)) {
> >  		ret = PTR_ERR(rqstp);
> 
> --
> 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