[PATCH 1/6] SUNRPC: Fix a memory leak in the backchannel code

[PATCH 1/6] SUNRPC: Fix a memory leak in the backchannel code

[Trond Myklebust]

req->rq_private_buf isn't initialised when xprt_setup_backchannel calls
xprt_free_allocation.

Fixes: fb7a0b9addbdb ("nfs41: New backchannel helper routines")
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 net/sunrpc/backchannel_rqst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 9dd0ea8db463..28504dfd3dad 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -60,7 +60,7 @@ static void xprt_free_allocation(struct rpc_rqst *req)
 
 	dprintk("RPC:        free allocations for req= %p\n", req);
 	WARN_ON_ONCE(test_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state));
-	xbufp = &req->rq_private_buf;
+	xbufp = &req->rq_rcv_buf;
 	free_page((unsigned long)xbufp->head[0].iov_base);
 	xbufp = &req->rq_snd_buf;
 	free_page((unsigned long)xbufp->head[0].iov_base);
-- 
2.4.2

[PATCH 2/6] SUNRPC: Backchannel handle socket nospace

[Trond Myklebust]

If the socket was busy due to a socket nospace error, then we should
retry the send.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 net/sunrpc/clnt.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e6ce1517367f..fa70f663eb92 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1951,24 +1951,22 @@ call_bc_transmit(struct rpc_task *task)
 {
 	struct rpc_rqst *req = task->tk_rqstp;
 
-	if (!xprt_prepare_transmit(task)) {
-		/*
-		 * Could not reserve the transport. Try again after the
-		 * transport is released.
-		 */
-		task->tk_status = 0;
-		task->tk_action = call_bc_transmit;
-		return;
-	}
+	if (!xprt_prepare_transmit(task))
+		goto out_retry;
 
-	task->tk_action = rpc_exit_task;
 	if (task->tk_status < 0) {
 		printk(KERN_NOTICE "RPC: Could not send backchannel reply "
 			"error: %d\n", task->tk_status);
-		return;
+		goto out_done;
 	}
+	if (req->rq_connect_cookie != req->rq_xprt->connect_cookie)
+		req->rq_bytes_sent = 0;
 
 	xprt_transmit(task);
+
+	if (task->tk_status == -EAGAIN)
+		goto out_nospace;
+
 	xprt_end_transmit(task);
 	dprint_status(task);
 	switch (task->tk_status) {
@@ -2002,6 +2000,13 @@ call_bc_transmit(struct rpc_task *task)
 		break;
 	}
 	rpc_wake_up_queued_task(&req->rq_xprt->pending, task);
+out_done:
+	task->tk_action = rpc_exit_task;
+	return;
+out_nospace:
+	req->rq_connect_cookie = req->rq_xprt->connect_cookie;
+out_retry:
+	task->tk_status = 0;
 }
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 
-- 
2.4.2

[PATCH 3/6] SUNRPC: Clean up bc_send()

[Trond Myklebust]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up: Merge bc_send() into bc_svc_process().

Note: even thought this touches svc.c, it is a client-side change.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 include/linux/sunrpc/bc_xprt.h |  1 -
 net/sunrpc/Makefile            |  2 +-
 net/sunrpc/bc_svc.c            | 63 ------------------------------------------
 net/sunrpc/svc.c               | 33 ++++++++++++++++------
 4 files changed, 26 insertions(+), 73 deletions(-)
 delete mode 100644 net/sunrpc/bc_svc.c

diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
index 2ca67b55e0fe..8df43c9f11dc 100644
--- a/include/linux/sunrpc/bc_xprt.h
+++ b/include/linux/sunrpc/bc_xprt.h
@@ -37,7 +37,6 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
 void xprt_free_bc_request(struct rpc_rqst *req);
 int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
 void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
-int bc_send(struct rpc_rqst *req);
 
 /*
  * Determine if a shared backchannel is in use
diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
index 15e6f6c23c5d..1b8e68d0e690 100644
--- a/net/sunrpc/Makefile
+++ b/net/sunrpc/Makefile
@@ -15,6 +15,6 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
 	    sunrpc_syms.o cache.o rpc_pipe.o \
 	    svc_xprt.o
 sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
-sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o
+sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o
 sunrpc-$(CONFIG_PROC_FS) += stats.o
 sunrpc-$(CONFIG_SYSCTL) += sysctl.o
diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
deleted file mode 100644
index 15c7a8a1c24f..000000000000
--- a/net/sunrpc/bc_svc.c
+++ /dev/null
@@ -1,63 +0,0 @@
-/******************************************************************************
-
-(c) 2007 Network Appliance, Inc.  All Rights Reserved.
-(c) 2009 NetApp.  All Rights Reserved.
-
-NetApp provides this source code under the GPL v2 License.
-The GPL v2 license is available at
-http://opensource.org/licenses/gpl-license.php.
-
-THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
-CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
-EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
-PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
-PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
-LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
-NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
-SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-******************************************************************************/
-
-/*
- * The NFSv4.1 callback service helper routines.
- * They implement the transport level processing required to send the
- * reply over an existing open connection previously established by the client.
- */
-
-#include <linux/module.h>
-
-#include <linux/sunrpc/xprt.h>
-#include <linux/sunrpc/sched.h>
-#include <linux/sunrpc/bc_xprt.h>
-
-#define RPCDBG_FACILITY	RPCDBG_SVCDSP
-
-/* Empty callback ops */
-static const struct rpc_call_ops nfs41_callback_ops = {
-};
-
-
-/*
- * Send the callback reply
- */
-int bc_send(struct rpc_rqst *req)
-{
-	struct rpc_task *task;
-	int ret;
-
-	dprintk("RPC:       bc_send req= %p\n", req);
-	task = rpc_run_bc_task(req, &nfs41_callback_ops);
-	if (IS_ERR(task))
-		ret = PTR_ERR(task);
-	else {
-		WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
-		ret = task->tk_status;
-		rpc_put_task(task);
-	}
-	dprintk("RPC:       bc_send ret= %d\n", ret);
-	return ret;
-}
-
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 78974e4d9ad2..e144902d382e 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1350,6 +1350,11 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 {
 	struct kvec	*argv = &rqstp->rq_arg.head[0];
 	struct kvec	*resv = &rqstp->rq_res.head[0];
+	static const struct rpc_call_ops reply_ops = { };
+	struct rpc_task *task;
+	int error;
+
+	dprintk("svc: %s(%p)\n", __func__, req);
 
 	/* Build the svc_rqst used by the common processing routine */
 	rqstp->rq_xprt = serv->sv_bc_xprt;
@@ -1372,21 +1377,33 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 
 	/*
 	 * Skip the next two words because they've already been
-	 * processed in the trasport
+	 * processed in the transport
 	 */
 	svc_getu32(argv);	/* XID */
 	svc_getnl(argv);	/* CALLDIR */
 
-	/* Returns 1 for send, 0 for drop */
-	if (svc_process_common(rqstp, argv, resv)) {
-		memcpy(&req->rq_snd_buf, &rqstp->rq_res,
-						sizeof(req->rq_snd_buf));
-		return bc_send(req);
-	} else {
-		/* drop request */
+	/* Parse and execute the bc call */
+	if (!svc_process_common(rqstp, argv, resv)) {
+		/* Processing error: drop the request */
 		xprt_free_bc_request(req);
 		return 0;
 	}
+
+	/* Finally, send the reply synchronously */
+	memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
+	task = rpc_run_bc_task(req, &reply_ops);
+	if (IS_ERR(task)) {
+		error = PTR_ERR(task);
+		goto out;
+	}
+
+	WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
+	error = task->tk_status;
+	rpc_put_task(task);
+
+out:
+	dprintk("svc: %s(), error=%d\n", __func__, error);
+	return error;
 }
 EXPORT_SYMBOL_GPL(bc_svc_process);
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
-- 
2.4.2

[PATCH 4/6] SUNRPC: Remove unused argument 'tk_ops' in rpc_run_bc_task

[Trond Myklebust]

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 include/linux/sunrpc/sched.h | 3 +--
 net/sunrpc/clnt.c            | 5 ++---
 net/sunrpc/svc.c             | 3 +--
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 5f1e6bd4c316..2aa68976a482 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -205,8 +205,7 @@ struct rpc_wait_queue {
  */
 struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
 struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
-struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
-				const struct rpc_call_ops *ops);
+struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req);
 void		rpc_put_task(struct rpc_task *);
 void		rpc_put_task_async(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index fa70f663eb92..46c13bca8555 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1033,13 +1033,12 @@ EXPORT_SYMBOL_GPL(rpc_call_async);
  * @req: RPC request
  * @tk_ops: RPC call ops
  */
-struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
-				const struct rpc_call_ops *tk_ops)
+struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
 {
 	struct rpc_task *task;
 	struct xdr_buf *xbufp = &req->rq_snd_buf;
 	struct rpc_task_setup task_setup_data = {
-		.callback_ops = tk_ops,
+		.callback_ops = &rpc_default_ops,
 	};
 
 	dprintk("RPC: rpc_run_bc_task req= %p\n", req);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index e144902d382e..51c8cad5e765 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1350,7 +1350,6 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 {
 	struct kvec	*argv = &rqstp->rq_arg.head[0];
 	struct kvec	*resv = &rqstp->rq_res.head[0];
-	static const struct rpc_call_ops reply_ops = { };
 	struct rpc_task *task;
 	int error;
 
@@ -1391,7 +1390,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 
 	/* Finally, send the reply synchronously */
 	memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
-	task = rpc_run_bc_task(req, &reply_ops);
+	task = rpc_run_bc_task(req);
 	if (IS_ERR(task)) {
 		error = PTR_ERR(task);
 		goto out;
-- 
2.4.2

[PATCH 5/6] SUNRPC: Clean up allocation and freeing of back channel requests

[Trond Myklebust]

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 net/sunrpc/backchannel_rqst.c | 95 +++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 40 deletions(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 28504dfd3dad..1baa41099469 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -67,6 +67,55 @@ static void xprt_free_allocation(struct rpc_rqst *req)
 	kfree(req);
 }
 
+static int xprt_alloc_xdr_buf(struct xdr_buf *buf, gfp_t gfp_flags)
+{
+	struct page *page;
+	/* Preallocate one XDR receive buffer */
+	page = alloc_page(gfp_flags);
+	if (page == NULL)
+		return -ENOMEM;
+	buf->head[0].iov_base = page_address(page);
+	buf->head[0].iov_len = PAGE_SIZE;
+	buf->tail[0].iov_base = NULL;
+	buf->tail[0].iov_len = 0;
+	buf->page_len = 0;
+	buf->len = 0;
+	buf->buflen = PAGE_SIZE;
+	return 0;
+}
+
+static
+struct rpc_rqst *xprt_alloc_bc_req(struct rpc_xprt *xprt, gfp_t gfp_flags)
+{
+	struct rpc_rqst *req;
+
+	/* Pre-allocate one backchannel rpc_rqst */
+	req = kzalloc(sizeof(*req), gfp_flags);
+	if (req == NULL)
+		return NULL;
+
+	req->rq_xprt = xprt;
+	INIT_LIST_HEAD(&req->rq_list);
+	INIT_LIST_HEAD(&req->rq_bc_list);
+
+	/* Preallocate one XDR receive buffer */
+	if (xprt_alloc_xdr_buf(&req->rq_rcv_buf, gfp_flags) < 0) {
+		printk(KERN_ERR "Failed to create bc receive xbuf\n");
+		goto out_free;
+	}
+	req->rq_rcv_buf.len = PAGE_SIZE;
+
+	/* Preallocate one XDR send buffer */
+	if (xprt_alloc_xdr_buf(&req->rq_snd_buf, gfp_flags) < 0) {
+		printk(KERN_ERR "Failed to create bc snd xbuf\n");
+		goto out_free;
+	}
+	return req;
+out_free:
+	xprt_free_allocation(req);
+	return NULL;
+}
+
 /*
  * Preallocate up to min_reqs structures and related buffers for use
  * by the backchannel.  This function can be called multiple times
@@ -87,9 +136,7 @@ static void xprt_free_allocation(struct rpc_rqst *req)
  */
 int xprt_setup_backchannel(struct rpc_xprt *xprt, unsigned int min_reqs)
 {
-	struct page *page_rcv = NULL, *page_snd = NULL;
-	struct xdr_buf *xbufp = NULL;
-	struct rpc_rqst *req, *tmp;
+	struct rpc_rqst *req;
 	struct list_head tmp_list;
 	int i;
 
@@ -106,7 +153,7 @@ int xprt_setup_backchannel(struct rpc_xprt *xprt, unsigned int min_reqs)
 	INIT_LIST_HEAD(&tmp_list);
 	for (i = 0; i < min_reqs; i++) {
 		/* Pre-allocate one backchannel rpc_rqst */
-		req = kzalloc(sizeof(struct rpc_rqst), GFP_KERNEL);
+		req = xprt_alloc_bc_req(xprt, GFP_KERNEL);
 		if (req == NULL) {
 			printk(KERN_ERR "Failed to create bc rpc_rqst\n");
 			goto out_free;
@@ -115,41 +162,6 @@ int xprt_setup_backchannel(struct rpc_xprt *xprt, unsigned int min_reqs)
 		/* Add the allocated buffer to the tmp list */
 		dprintk("RPC:       adding req= %p\n", req);
 		list_add(&req->rq_bc_pa_list, &tmp_list);
-
-		req->rq_xprt = xprt;
-		INIT_LIST_HEAD(&req->rq_list);
-		INIT_LIST_HEAD(&req->rq_bc_list);
-
-		/* Preallocate one XDR receive buffer */
-		page_rcv = alloc_page(GFP_KERNEL);
-		if (page_rcv == NULL) {
-			printk(KERN_ERR "Failed to create bc receive xbuf\n");
-			goto out_free;
-		}
-		xbufp = &req->rq_rcv_buf;
-		xbufp->head[0].iov_base = page_address(page_rcv);
-		xbufp->head[0].iov_len = PAGE_SIZE;
-		xbufp->tail[0].iov_base = NULL;
-		xbufp->tail[0].iov_len = 0;
-		xbufp->page_len = 0;
-		xbufp->len = PAGE_SIZE;
-		xbufp->buflen = PAGE_SIZE;
-
-		/* Preallocate one XDR send buffer */
-		page_snd = alloc_page(GFP_KERNEL);
-		if (page_snd == NULL) {
-			printk(KERN_ERR "Failed to create bc snd xbuf\n");
-			goto out_free;
-		}
-
-		xbufp = &req->rq_snd_buf;
-		xbufp->head[0].iov_base = page_address(page_snd);
-		xbufp->head[0].iov_len = 0;
-		xbufp->tail[0].iov_base = NULL;
-		xbufp->tail[0].iov_len = 0;
-		xbufp->page_len = 0;
-		xbufp->len = 0;
-		xbufp->buflen = PAGE_SIZE;
 	}
 
 	/*
@@ -167,7 +179,10 @@ out_free:
 	/*
 	 * Memory allocation failed, free the temporary list
 	 */
-	list_for_each_entry_safe(req, tmp, &tmp_list, rq_bc_pa_list) {
+	while (!list_empty(&tmp_list)) {
+		req = list_first_entry(&tmp_list,
+				struct rpc_rqst,
+				rq_bc_pa_list);
 		list_del(&req->rq_bc_pa_list);
 		xprt_free_allocation(req);
 	}
-- 
2.4.2

[PATCH 6/6] SUNRPC: Fix a backchannel race

[Trond Myklebust]

We need to allow the server to send a new request immediately after we've
replied to the previous one. Right now, there is a window between the
send and the release of the old request in rpc_put_task(), where the
server could send us a new backchannel RPC call, and we have no
request to service it.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 include/linux/sunrpc/xprt.h   |  3 ++-
 net/sunrpc/backchannel_rqst.c | 37 ++++++++++++++++++++++++-------------
 net/sunrpc/svc.c              |  6 +++++-
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 8b93ef53df3c..bc9c39d6d30d 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -212,7 +212,8 @@ struct rpc_xprt {
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
 	struct svc_serv		*bc_serv;       /* The RPC service which will */
 						/* process the callback */
-	unsigned int		bc_alloc_count;	/* Total number of preallocs */
+	int			bc_alloc_count;	/* Total number of preallocs */
+	atomic_t		bc_free_slots;
 	spinlock_t		bc_pa_lock;	/* Protects the preallocated
 						 * items */
 	struct list_head	bc_pa_list;	/* List of preallocated
diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 1baa41099469..9825ff0f91d6 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -37,16 +37,18 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 static inline int xprt_need_to_requeue(struct rpc_xprt *xprt)
 {
-	return xprt->bc_alloc_count > 0;
+	return xprt->bc_alloc_count < atomic_read(&xprt->bc_free_slots);
 }
 
 static inline void xprt_inc_alloc_count(struct rpc_xprt *xprt, unsigned int n)
 {
+	atomic_add(n, &xprt->bc_free_slots);
 	xprt->bc_alloc_count += n;
 }
 
 static inline int xprt_dec_alloc_count(struct rpc_xprt *xprt, unsigned int n)
 {
+	atomic_sub(n, &xprt->bc_free_slots);
 	return xprt->bc_alloc_count -= n;
 }
 
@@ -232,9 +234,15 @@ static struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt, __be32 xid)
 	struct rpc_rqst *req = NULL;
 
 	dprintk("RPC:       allocate a backchannel request\n");
-	if (list_empty(&xprt->bc_pa_list))
+	if (atomic_read(&xprt->bc_free_slots) <= 0)
 		goto not_found;
-
+	if (list_empty(&xprt->bc_pa_list)) {
+		req = xprt_alloc_bc_req(xprt, GFP_ATOMIC);
+		if (!req)
+			goto not_found;
+		/* Note: this 'free' request adds it to xprt->bc_pa_list */
+		xprt_free_bc_request(req);
+	}
 	req = list_first_entry(&xprt->bc_pa_list, struct rpc_rqst,
 				rq_bc_pa_list);
 	req->rq_reply_bytes_recvd = 0;
@@ -260,11 +268,21 @@ void xprt_free_bc_request(struct rpc_rqst *req)
 
 	req->rq_connect_cookie = xprt->connect_cookie - 1;
 	smp_mb__before_atomic();
-	WARN_ON_ONCE(!test_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state));
 	clear_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
 	smp_mb__after_atomic();
 
-	if (!xprt_need_to_requeue(xprt)) {
+	/*
+	 * Return it to the list of preallocations so that it
+	 * may be reused by a new callback request.
+	 */
+	spin_lock_bh(&xprt->bc_pa_lock);
+	if (xprt_need_to_requeue(xprt)) {
+		list_add_tail(&req->rq_bc_pa_list, &xprt->bc_pa_list);
+		xprt->bc_alloc_count++;
+		req = NULL;
+	}
+	spin_unlock_bh(&xprt->bc_pa_lock);
+	if (req != NULL) {
 		/*
 		 * The last remaining session was destroyed while this
 		 * entry was in use.  Free the entry and don't attempt
@@ -275,14 +293,6 @@ void xprt_free_bc_request(struct rpc_rqst *req)
 		xprt_free_allocation(req);
 		return;
 	}
-
-	/*
-	 * Return it to the list of preallocations so that it
-	 * may be reused by a new callback request.
-	 */
-	spin_lock_bh(&xprt->bc_pa_lock);
-	list_add_tail(&req->rq_bc_pa_list, &xprt->bc_pa_list);
-	spin_unlock_bh(&xprt->bc_pa_lock);
 }
 
 /*
@@ -326,6 +336,7 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
 
 	spin_lock(&xprt->bc_pa_lock);
 	list_del(&req->rq_bc_pa_list);
+	xprt->bc_alloc_count--;
 	spin_unlock(&xprt->bc_pa_lock);
 
 	req->rq_private_buf.len = copied;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 51c8cad5e765..b47f02f60b9c 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1351,6 +1351,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 	struct kvec	*argv = &rqstp->rq_arg.head[0];
 	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct rpc_task *task;
+	int proc_error;
 	int error;
 
 	dprintk("svc: %s(%p)\n", __func__, req);
@@ -1382,7 +1383,10 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 	svc_getnl(argv);	/* CALLDIR */
 
 	/* Parse and execute the bc call */
-	if (!svc_process_common(rqstp, argv, resv)) {
+	proc_error = svc_process_common(rqstp, argv, resv);
+
+	atomic_inc(&req->rq_xprt->bc_free_slots);
+	if (!proc_error) {
 		/* Processing error: drop the request */
 		xprt_free_bc_request(req);
 		return 0;
-- 
2.4.2

Re: [PATCH 4/6] SUNRPC: Remove unused argument 'tk_ops' in rpc_run_bc_task

[Chuck Lever]

On Jun 4, 2015, at 5:26 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

Yeah, I was wondering about that. Seems reasonable.

> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> include/linux/sunrpc/sched.h | 3 +--
> net/sunrpc/clnt.c            | 5 ++---
> net/sunrpc/svc.c             | 3 +--
> 3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index 5f1e6bd4c316..2aa68976a482 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -205,8 +205,7 @@ struct rpc_wait_queue {
>  */
> struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
> struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
> -struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
> -				const struct rpc_call_ops *ops);
> +struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req);
> void		rpc_put_task(struct rpc_task *);
> void		rpc_put_task_async(struct rpc_task *);
> void		rpc_exit_task(struct rpc_task *);
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index fa70f663eb92..46c13bca8555 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1033,13 +1033,12 @@ EXPORT_SYMBOL_GPL(rpc_call_async);
>  * @req: RPC request
>  * @tk_ops: RPC call ops
>  */
> -struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
> -				const struct rpc_call_ops *tk_ops)
> +struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req)
> {
> 	struct rpc_task *task;
> 	struct xdr_buf *xbufp = &req->rq_snd_buf;
> 	struct rpc_task_setup task_setup_data = {
> -		.callback_ops = tk_ops,
> +		.callback_ops = &rpc_default_ops,
> 	};
> 
> 	dprintk("RPC: rpc_run_bc_task req= %p\n", req);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index e144902d382e..51c8cad5e765 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1350,7 +1350,6 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> {
> 	struct kvec	*argv = &rqstp->rq_arg.head[0];
> 	struct kvec	*resv = &rqstp->rq_res.head[0];
> -	static const struct rpc_call_ops reply_ops = { };
> 	struct rpc_task *task;
> 	int error;
> 
> @@ -1391,7 +1390,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> 
> 	/* Finally, send the reply synchronously */
> 	memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
> -	task = rpc_run_bc_task(req, &reply_ops);
> +	task = rpc_run_bc_task(req);
> 	if (IS_ERR(task)) {
> 		error = PTR_ERR(task);
> 		goto out;
> -- 
> 2.4.2
> 
> --
> 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 3/6] SUNRPC: Clean up bc_send()

[Andy Adamson]

On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
>
> Clean up: Merge bc_send() into bc_svc_process().
>
> Note: even thought this touches svc.c, it is a client-side change.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  include/linux/sunrpc/bc_xprt.h |  1 -
>  net/sunrpc/Makefile            |  2 +-
>  net/sunrpc/bc_svc.c            | 63 ------------------------------------------
>  net/sunrpc/svc.c               | 33 ++++++++++++++++------
>  4 files changed, 26 insertions(+), 73 deletions(-)
>  delete mode 100644 net/sunrpc/bc_svc.c
>
> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
> index 2ca67b55e0fe..8df43c9f11dc 100644
> --- a/include/linux/sunrpc/bc_xprt.h
> +++ b/include/linux/sunrpc/bc_xprt.h
> @@ -37,7 +37,6 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
>  void xprt_free_bc_request(struct rpc_rqst *req);
>  int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
>  void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
> -int bc_send(struct rpc_rqst *req);
>
>  /*
>   * Determine if a shared backchannel is in use
> diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
> index 15e6f6c23c5d..1b8e68d0e690 100644
> --- a/net/sunrpc/Makefile
> +++ b/net/sunrpc/Makefile
> @@ -15,6 +15,6 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
>             sunrpc_syms.o cache.o rpc_pipe.o \
>             svc_xprt.o
>  sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
> -sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o
> +sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o
>  sunrpc-$(CONFIG_PROC_FS) += stats.o
>  sunrpc-$(CONFIG_SYSCTL) += sysctl.o
> diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
> deleted file mode 100644
> index 15c7a8a1c24f..000000000000
> --- a/net/sunrpc/bc_svc.c
> +++ /dev/null
> @@ -1,63 +0,0 @@
> -/******************************************************************************
> -
> -(c) 2007 Network Appliance, Inc.  All Rights Reserved.
> -(c) 2009 NetApp.  All Rights Reserved.
> -
> -NetApp provides this source code under the GPL v2 License.
> -The GPL v2 license is available at
> -http://opensource.org/licenses/gpl-license.php.
> -
> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> -"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> -LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> -A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> -CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> -EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> -PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> -PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> -LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> -NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> -
> -******************************************************************************/

Why is the above being removed?

-->Andy

> -
> -/*
> - * The NFSv4.1 callback service helper routines.
> - * They implement the transport level processing required to send the
> - * reply over an existing open connection previously established by the client.
> - */
> -
> -#include <linux/module.h>
> -
> -#include <linux/sunrpc/xprt.h>
> -#include <linux/sunrpc/sched.h>
> -#include <linux/sunrpc/bc_xprt.h>
> -
> -#define RPCDBG_FACILITY        RPCDBG_SVCDSP
> -
> -/* Empty callback ops */
> -static const struct rpc_call_ops nfs41_callback_ops = {
> -};
> -
> -
> -/*
> - * Send the callback reply
> - */
> -int bc_send(struct rpc_rqst *req)
> -{
> -       struct rpc_task *task;
> -       int ret;
> -
> -       dprintk("RPC:       bc_send req= %p\n", req);
> -       task = rpc_run_bc_task(req, &nfs41_callback_ops);
> -       if (IS_ERR(task))
> -               ret = PTR_ERR(task);
> -       else {
> -               WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
> -               ret = task->tk_status;
> -               rpc_put_task(task);
> -       }
> -       dprintk("RPC:       bc_send ret= %d\n", ret);
> -       return ret;
> -}
> -
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 78974e4d9ad2..e144902d382e 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1350,6 +1350,11 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
>  {
>         struct kvec     *argv = &rqstp->rq_arg.head[0];
>         struct kvec     *resv = &rqstp->rq_res.head[0];
> +       static const struct rpc_call_ops reply_ops = { };
> +       struct rpc_task *task;
> +       int error;
> +
> +       dprintk("svc: %s(%p)\n", __func__, req);
>
>         /* Build the svc_rqst used by the common processing routine */
>         rqstp->rq_xprt = serv->sv_bc_xprt;
> @@ -1372,21 +1377,33 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
>
>         /*
>          * Skip the next two words because they've already been
> -        * processed in the trasport
> +        * processed in the transport
>          */
>         svc_getu32(argv);       /* XID */
>         svc_getnl(argv);        /* CALLDIR */
>
> -       /* Returns 1 for send, 0 for drop */
> -       if (svc_process_common(rqstp, argv, resv)) {
> -               memcpy(&req->rq_snd_buf, &rqstp->rq_res,
> -                                               sizeof(req->rq_snd_buf));
> -               return bc_send(req);
> -       } else {
> -               /* drop request */
> +       /* Parse and execute the bc call */
> +       if (!svc_process_common(rqstp, argv, resv)) {
> +               /* Processing error: drop the request */
>                 xprt_free_bc_request(req);
>                 return 0;
>         }
> +
> +       /* Finally, send the reply synchronously */
> +       memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
> +       task = rpc_run_bc_task(req, &reply_ops);
> +       if (IS_ERR(task)) {
> +               error = PTR_ERR(task);
> +               goto out;
> +       }
> +
> +       WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
> +       error = task->tk_status;
> +       rpc_put_task(task);
> +
> +out:
> +       dprintk("svc: %s(), error=%d\n", __func__, error);
> +       return error;
>  }
>  EXPORT_SYMBOL_GPL(bc_svc_process);
>  #endif /* CONFIG_SUNRPC_BACKCHANNEL */
> --
> 2.4.2
>
> --
> 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 3/6] SUNRPC: Clean up bc_send()

[Chuck Lever]

On Jun 5, 2015, at 11:50 AM, Andy Adamson <androsadamson@gmail.com> wrote:

> On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> Clean up: Merge bc_send() into bc_svc_process().
>> 
>> Note: even thought this touches svc.c, it is a client-side change.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>> include/linux/sunrpc/bc_xprt.h |  1 -
>> net/sunrpc/Makefile            |  2 +-
>> net/sunrpc/bc_svc.c            | 63 ------------------------------------------
>> net/sunrpc/svc.c               | 33 ++++++++++++++++------
>> 4 files changed, 26 insertions(+), 73 deletions(-)
>> delete mode 100644 net/sunrpc/bc_svc.c
>> 
>> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
>> index 2ca67b55e0fe..8df43c9f11dc 100644
>> --- a/include/linux/sunrpc/bc_xprt.h
>> +++ b/include/linux/sunrpc/bc_xprt.h
>> @@ -37,7 +37,6 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
>> void xprt_free_bc_request(struct rpc_rqst *req);
>> int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
>> void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
>> -int bc_send(struct rpc_rqst *req);
>> 
>> /*
>>  * Determine if a shared backchannel is in use
>> diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
>> index 15e6f6c23c5d..1b8e68d0e690 100644
>> --- a/net/sunrpc/Makefile
>> +++ b/net/sunrpc/Makefile
>> @@ -15,6 +15,6 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
>>            sunrpc_syms.o cache.o rpc_pipe.o \
>>            svc_xprt.o
>> sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
>> -sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o
>> +sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o
>> sunrpc-$(CONFIG_PROC_FS) += stats.o
>> sunrpc-$(CONFIG_SYSCTL) += sysctl.o
>> diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
>> deleted file mode 100644
>> index 15c7a8a1c24f..000000000000
>> --- a/net/sunrpc/bc_svc.c
>> +++ /dev/null
>> @@ -1,63 +0,0 @@
>> -/******************************************************************************
>> -
>> -(c) 2007 Network Appliance, Inc.  All Rights Reserved.
>> -(c) 2009 NetApp.  All Rights Reserved.
>> -
>> -NetApp provides this source code under the GPL v2 License.
>> -The GPL v2 license is available at
>> -http://opensource.org/licenses/gpl-license.php.
>> -
>> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> -"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> -LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> -A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
>> -CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>> -EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>> -PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>> -PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> -LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
>> -NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> -
>> -******************************************************************************/
> 
> Why is the above being removed?

Hi Andy-

Interesting question, and IANAL.

Where should the boilerplate go if the below function is merged into
existing generic server code? I suppose we could fold the NetApp
copyright into the comment at the top of net/sunrpc/svc.c , but the
merged code is paraphrased rather than copied directly.

Suggestions/opinions are welcome.


> -->Andy
> 
>> -
>> -/*
>> - * The NFSv4.1 callback service helper routines.
>> - * They implement the transport level processing required to send the
>> - * reply over an existing open connection previously established by the client.
>> - */
>> -
>> -#include <linux/module.h>
>> -
>> -#include <linux/sunrpc/xprt.h>
>> -#include <linux/sunrpc/sched.h>
>> -#include <linux/sunrpc/bc_xprt.h>
>> -
>> -#define RPCDBG_FACILITY        RPCDBG_SVCDSP
>> -
>> -/* Empty callback ops */
>> -static const struct rpc_call_ops nfs41_callback_ops = {
>> -};
>> -
>> -
>> -/*
>> - * Send the callback reply
>> - */
>> -int bc_send(struct rpc_rqst *req)
>> -{
>> -       struct rpc_task *task;
>> -       int ret;
>> -
>> -       dprintk("RPC:       bc_send req= %p\n", req);
>> -       task = rpc_run_bc_task(req, &nfs41_callback_ops);
>> -       if (IS_ERR(task))
>> -               ret = PTR_ERR(task);
>> -       else {
>> -               WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
>> -               ret = task->tk_status;
>> -               rpc_put_task(task);
>> -       }
>> -       dprintk("RPC:       bc_send ret= %d\n", ret);
>> -       return ret;
>> -}
>> -
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 78974e4d9ad2..e144902d382e 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1350,6 +1350,11 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
>> {
>>        struct kvec     *argv = &rqstp->rq_arg.head[0];
>>        struct kvec     *resv = &rqstp->rq_res.head[0];
>> +       static const struct rpc_call_ops reply_ops = { };
>> +       struct rpc_task *task;
>> +       int error;
>> +
>> +       dprintk("svc: %s(%p)\n", __func__, req);
>> 
>>        /* Build the svc_rqst used by the common processing routine */
>>        rqstp->rq_xprt = serv->sv_bc_xprt;
>> @@ -1372,21 +1377,33 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
>> 
>>        /*
>>         * Skip the next two words because they've already been
>> -        * processed in the trasport
>> +        * processed in the transport
>>         */
>>        svc_getu32(argv);       /* XID */
>>        svc_getnl(argv);        /* CALLDIR */
>> 
>> -       /* Returns 1 for send, 0 for drop */
>> -       if (svc_process_common(rqstp, argv, resv)) {
>> -               memcpy(&req->rq_snd_buf, &rqstp->rq_res,
>> -                                               sizeof(req->rq_snd_buf));
>> -               return bc_send(req);
>> -       } else {
>> -               /* drop request */
>> +       /* Parse and execute the bc call */
>> +       if (!svc_process_common(rqstp, argv, resv)) {
>> +               /* Processing error: drop the request */
>>                xprt_free_bc_request(req);
>>                return 0;
>>        }
>> +
>> +       /* Finally, send the reply synchronously */
>> +       memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
>> +       task = rpc_run_bc_task(req, &reply_ops);
>> +       if (IS_ERR(task)) {
>> +               error = PTR_ERR(task);
>> +               goto out;
>> +       }
>> +
>> +       WARN_ON_ONCE(atomic_read(&task->tk_count) != 1);
>> +       error = task->tk_status;
>> +       rpc_put_task(task);
>> +
>> +out:
>> +       dprintk("svc: %s(), error=%d\n", __func__, error);
>> +       return error;
>> }
>> EXPORT_SYMBOL_GPL(bc_svc_process);
>> #endif /* CONFIG_SUNRPC_BACKCHANNEL */
>> --
>> 2.4.2
>> 
>> --
>> 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
> --
> 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 3/6] SUNRPC: Clean up bc_send()

[J. Bruce Fields]

On Fri, Jun 05, 2015 at 12:02:11PM -0400, Chuck Lever wrote:
> 
> On Jun 5, 2015, at 11:50 AM, Andy Adamson <androsadamson@gmail.com> wrote:
> 
> > On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
> > <trond.myklebust@primarydata.com> wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >> 
> >> Clean up: Merge bc_send() into bc_svc_process().
> >> 
> >> Note: even thought this touches svc.c, it is a client-side change.
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> >> ---
> >> include/linux/sunrpc/bc_xprt.h |  1 -
> >> net/sunrpc/Makefile            |  2 +-
> >> net/sunrpc/bc_svc.c            | 63 ------------------------------------------
> >> net/sunrpc/svc.c               | 33 ++++++++++++++++------
> >> 4 files changed, 26 insertions(+), 73 deletions(-)
> >> delete mode 100644 net/sunrpc/bc_svc.c
> >> 
> >> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
> >> index 2ca67b55e0fe..8df43c9f11dc 100644
> >> --- a/include/linux/sunrpc/bc_xprt.h
> >> +++ b/include/linux/sunrpc/bc_xprt.h
> >> @@ -37,7 +37,6 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
> >> void xprt_free_bc_request(struct rpc_rqst *req);
> >> int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
> >> void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
> >> -int bc_send(struct rpc_rqst *req);
> >> 
> >> /*
> >>  * Determine if a shared backchannel is in use
> >> diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
> >> index 15e6f6c23c5d..1b8e68d0e690 100644
> >> --- a/net/sunrpc/Makefile
> >> +++ b/net/sunrpc/Makefile
> >> @@ -15,6 +15,6 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
> >>            sunrpc_syms.o cache.o rpc_pipe.o \
> >>            svc_xprt.o
> >> sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
> >> -sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o
> >> +sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o
> >> sunrpc-$(CONFIG_PROC_FS) += stats.o
> >> sunrpc-$(CONFIG_SYSCTL) += sysctl.o
> >> diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
> >> deleted file mode 100644
> >> index 15c7a8a1c24f..000000000000
> >> --- a/net/sunrpc/bc_svc.c
> >> +++ /dev/null
> >> @@ -1,63 +0,0 @@
> >> -/******************************************************************************
> >> -
> >> -(c) 2007 Network Appliance, Inc.  All Rights Reserved.
> >> -(c) 2009 NetApp.  All Rights Reserved.
> >> -
> >> -NetApp provides this source code under the GPL v2 License.
> >> -The GPL v2 license is available at
> >> -http://opensource.org/licenses/gpl-license.php.
> >> -
> >> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> >> -"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> >> -LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> >> -A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> >> -CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> >> -EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> >> -PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> >> -PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> >> -LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> >> -NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> >> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >> -
> >> -******************************************************************************/
> > 
> > Why is the above being removed?
> 
> Hi Andy-
> 
> Interesting question, and IANAL.
> 
> Where should the boilerplate go if the below function is merged into
> existing generic server code? I suppose we could fold the NetApp
> copyright into the comment at the top of net/sunrpc/svc.c , but the
> merged code is paraphrased rather than copied directly.
> 
> Suggestions/opinions are welcome.

I don't know, the above looks like pretty generic boilerplate, and I'd
rather not accumulate a that kind of thing every time we move a
significant piece of code around.  Could we compromise by adding a line
or two to net/sunrpc/svc.c following the existing convention there?
("backchannel implementation originally (c) 2007 Network Appliance and
(c) 2009 Netapp", or similar?)  Not a lawyer either, just looking for a
way to keep it both honest and concise....

--b.

Re: [PATCH 3/6] SUNRPC: Clean up bc_send()

[Chuck Lever]

On Jun 8, 2015, at 4:47 PM, bfields@fieldses.org wrote:

> On Fri, Jun 05, 2015 at 12:02:11PM -0400, Chuck Lever wrote:
>> 
>> On Jun 5, 2015, at 11:50 AM, Andy Adamson <androsadamson@gmail.com> wrote:
>> 
>>> On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
>>> <trond.myklebust@primarydata.com> wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>> 
>>>> Clean up: Merge bc_send() into bc_svc_process().
>>>> 
>>>> Note: even thought this touches svc.c, it is a client-side change.
>>>> 
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>> ---
>>>> include/linux/sunrpc/bc_xprt.h |  1 -
>>>> net/sunrpc/Makefile            |  2 +-
>>>> net/sunrpc/bc_svc.c            | 63 ------------------------------------------
>>>> net/sunrpc/svc.c               | 33 ++++++++++++++++------
>>>> 4 files changed, 26 insertions(+), 73 deletions(-)
>>>> delete mode 100644 net/sunrpc/bc_svc.c
>>>> 
>>>> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
>>>> index 2ca67b55e0fe..8df43c9f11dc 100644
>>>> --- a/include/linux/sunrpc/bc_xprt.h
>>>> +++ b/include/linux/sunrpc/bc_xprt.h
>>>> @@ -37,7 +37,6 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied);
>>>> void xprt_free_bc_request(struct rpc_rqst *req);
>>>> int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
>>>> void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
>>>> -int bc_send(struct rpc_rqst *req);
>>>> 
>>>> /*
>>>> * Determine if a shared backchannel is in use
>>>> diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
>>>> index 15e6f6c23c5d..1b8e68d0e690 100644
>>>> --- a/net/sunrpc/Makefile
>>>> +++ b/net/sunrpc/Makefile
>>>> @@ -15,6 +15,6 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
>>>>           sunrpc_syms.o cache.o rpc_pipe.o \
>>>>           svc_xprt.o
>>>> sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
>>>> -sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o
>>>> +sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o
>>>> sunrpc-$(CONFIG_PROC_FS) += stats.o
>>>> sunrpc-$(CONFIG_SYSCTL) += sysctl.o
>>>> diff --git a/net/sunrpc/bc_svc.c b/net/sunrpc/bc_svc.c
>>>> deleted file mode 100644
>>>> index 15c7a8a1c24f..000000000000
>>>> --- a/net/sunrpc/bc_svc.c
>>>> +++ /dev/null
>>>> @@ -1,63 +0,0 @@
>>>> -/******************************************************************************
>>>> -
>>>> -(c) 2007 Network Appliance, Inc.  All Rights Reserved.
>>>> -(c) 2009 NetApp.  All Rights Reserved.
>>>> -
>>>> -NetApp provides this source code under the GPL v2 License.
>>>> -The GPL v2 license is available at
>>>> -http://opensource.org/licenses/gpl-license.php.
>>>> -
>>>> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>>>> -"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>>>> -LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>>>> -A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
>>>> -CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>>>> -EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>>>> -PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>>>> -PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>>>> -LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
>>>> -NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>>>> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>>> -
>>>> -******************************************************************************/
>>> 
>>> Why is the above being removed?
>> 
>> Hi Andy-
>> 
>> Interesting question, and IANAL.
>> 
>> Where should the boilerplate go if the below function is merged into
>> existing generic server code? I suppose we could fold the NetApp
>> copyright into the comment at the top of net/sunrpc/svc.c , but the
>> merged code is paraphrased rather than copied directly.
>> 
>> Suggestions/opinions are welcome.
> 
> I don't know, the above looks like pretty generic boilerplate, and I'd
> rather not accumulate a that kind of thing every time we move a
> significant piece of code around.  Could we compromise by adding a line
> or two to net/sunrpc/svc.c following the existing convention there?
> ("backchannel implementation originally (c) 2007 Network Appliance and
> (c) 2009 Netapp", or similar?)  Not a lawyer either, just looking for a
> way to keep it both honest and concise….

Yes, that’s exactly what I was thinking. Sorry I wasn’t clear.

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

Re: [PATCH 3/6] SUNRPC: Clean up bc_send()

[Trond Myklebust]

On Mon, Jun 8, 2015 at 4:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> I don't know, the above looks like pretty generic boilerplate, and I'd
> rather not accumulate a that kind of thing every time we move a
> significant piece of code around.  Could we compromise by adding a line
> or two to net/sunrpc/svc.c following the existing convention there?
> ("backchannel implementation originally (c) 2007 Network Appliance and
> (c) 2009 Netapp", or similar?)  Not a lawyer either, just looking for a
> way to keep it both honest and concise....
>

For significant contributions, we do sometimes allow contributors to
add a copyright notice. The other case where it is usually done is
when creating a new file, on the theory that the GPL does require that
code be copyrighted in order to be enforceable.

I'd suggest just a simple:

"Portions of this code are (c) 2007 Network Appliance, (c) 2009 NetApp".

Trond

Re: [PATCH 6/6] SUNRPC: Fix a backchannel race

[Chuck Lever]

Hi Trond-


On Jun 4, 2015, at 5:26 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> We need to allow the server to send a new request immediately after we've
> replied to the previous one. Right now, there is a window between the
> send and the release of the old request in rpc_put_task(), where the
> server could send us a new backchannel RPC call, and we have no
> request to service it.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> include/linux/sunrpc/xprt.h   |  3 ++-
> net/sunrpc/backchannel_rqst.c | 37 ++++++++++++++++++++++++-------------
> net/sunrpc/svc.c              |  6 +++++-
> 3 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index 8b93ef53df3c..bc9c39d6d30d 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -212,7 +212,8 @@ struct rpc_xprt {
> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> 	struct svc_serv		*bc_serv;       /* The RPC service which will */
> 						/* process the callback */
> -	unsigned int		bc_alloc_count;	/* Total number of preallocs */
> +	int			bc_alloc_count;	/* Total number of preallocs */
> +	atomic_t		bc_free_slots;
> 	spinlock_t		bc_pa_lock;	/* Protects the preallocated
> 						 * items */
> 	struct list_head	bc_pa_list;	/* List of preallocated
> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
> index 1baa41099469..9825ff0f91d6 100644
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -37,16 +37,18 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> static inline int xprt_need_to_requeue(struct rpc_xprt *xprt)
> {
> -	return xprt->bc_alloc_count > 0;
> +	return xprt->bc_alloc_count < atomic_read(&xprt->bc_free_slots);
> }
> 
> static inline void xprt_inc_alloc_count(struct rpc_xprt *xprt, unsigned int n)
> {
> +	atomic_add(n, &xprt->bc_free_slots);
> 	xprt->bc_alloc_count += n;
> }
> 
> static inline int xprt_dec_alloc_count(struct rpc_xprt *xprt, unsigned int n)
> {
> +	atomic_sub(n, &xprt->bc_free_slots);
> 	return xprt->bc_alloc_count -= n;
> }
> 
> @@ -232,9 +234,15 @@ static struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt, __be32 xid)
> 	struct rpc_rqst *req = NULL;
> 
> 	dprintk("RPC:       allocate a backchannel request\n");
> -	if (list_empty(&xprt->bc_pa_list))
> +	if (atomic_read(&xprt->bc_free_slots) <= 0)
> 		goto not_found;
> -
> +	if (list_empty(&xprt->bc_pa_list)) {
> +		req = xprt_alloc_bc_req(xprt, GFP_ATOMIC);
> +		if (!req)
> +			goto not_found;
> +		/* Note: this 'free' request adds it to xprt->bc_pa_list */
> +		xprt_free_bc_request(req);

xprt_alloc_bc_request() already holds xprt->bc_pa_lock.
xprt_free_bc_request() will take it again to add this fresh
request to xprt->bc_pa_list.

One of our testers hit this.

Can you simply open-code the list_add_tail() here?


> +	}
> 	req = list_first_entry(&xprt->bc_pa_list, struct rpc_rqst,
> 				rq_bc_pa_list);
> 	req->rq_reply_bytes_recvd = 0;
> @@ -260,11 +268,21 @@ void xprt_free_bc_request(struct rpc_rqst *req)
> 
> 	req->rq_connect_cookie = xprt->connect_cookie - 1;
> 	smp_mb__before_atomic();
> -	WARN_ON_ONCE(!test_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state));
> 	clear_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
> 	smp_mb__after_atomic();
> 
> -	if (!xprt_need_to_requeue(xprt)) {
> +	/*
> +	 * Return it to the list of preallocations so that it
> +	 * may be reused by a new callback request.
> +	 */
> +	spin_lock_bh(&xprt->bc_pa_lock);
> +	if (xprt_need_to_requeue(xprt)) {
> +		list_add_tail(&req->rq_bc_pa_list, &xprt->bc_pa_list);
> +		xprt->bc_alloc_count++;
> +		req = NULL;
> +	}
> +	spin_unlock_bh(&xprt->bc_pa_lock);
> +	if (req != NULL) {
> 		/*
> 		 * The last remaining session was destroyed while this
> 		 * entry was in use.  Free the entry and don't attempt
> @@ -275,14 +293,6 @@ void xprt_free_bc_request(struct rpc_rqst *req)
> 		xprt_free_allocation(req);
> 		return;
> 	}
> -
> -	/*
> -	 * Return it to the list of preallocations so that it
> -	 * may be reused by a new callback request.
> -	 */
> -	spin_lock_bh(&xprt->bc_pa_lock);
> -	list_add_tail(&req->rq_bc_pa_list, &xprt->bc_pa_list);
> -	spin_unlock_bh(&xprt->bc_pa_lock);
> }
> 
> /*
> @@ -326,6 +336,7 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
> 
> 	spin_lock(&xprt->bc_pa_lock);
> 	list_del(&req->rq_bc_pa_list);
> +	xprt->bc_alloc_count--;
> 	spin_unlock(&xprt->bc_pa_lock);
> 
> 	req->rq_private_buf.len = copied;
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 51c8cad5e765..b47f02f60b9c 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1351,6 +1351,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> 	struct kvec	*argv = &rqstp->rq_arg.head[0];
> 	struct kvec	*resv = &rqstp->rq_res.head[0];
> 	struct rpc_task *task;
> +	int proc_error;
> 	int error;
> 
> 	dprintk("svc: %s(%p)\n", __func__, req);
> @@ -1382,7 +1383,10 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> 	svc_getnl(argv);	/* CALLDIR */
> 
> 	/* Parse and execute the bc call */
> -	if (!svc_process_common(rqstp, argv, resv)) {
> +	proc_error = svc_process_common(rqstp, argv, resv);
> +
> +	atomic_inc(&req->rq_xprt->bc_free_slots);
> +	if (!proc_error) {
> 		/* Processing error: drop the request */
> 		xprt_free_bc_request(req);
> 		return 0;
> -- 
> 2.4.2
> 
> --
> 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