req->rq_private_buf isn't initialised when xprt_setup_backchannel calls
xprt_free_allocation.
Fixes: fb7a0b9addbdb ("nfs41: New backchannel helper routines")
Cc: [email protected]
Signed-off-by: Trond Myklebust <[email protected]>
---
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
If the socket was busy due to a socket nospace error, then we should
retry the send.
Signed-off-by: Trond Myklebust <[email protected]>
---
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
From: Chuck Lever <[email protected]>
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 <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
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
Signed-off-by: Trond Myklebust <[email protected]>
---
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
Signed-off-by: Trond Myklebust <[email protected]>
---
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
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 <[email protected]>
---
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
On Jun 4, 2015, at 5:26 PM, Trond Myklebust <[email protected]> wrote:
Yeah, I was wondering about that. Seems reasonable.
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
<[email protected]> wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 5, 2015, at 11:50 AM, Andy Adamson <[email protected]> wrote:
> On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
> <[email protected]> wrote:
>> From: Chuck Lever <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> 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 [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, Jun 05, 2015 at 12:02:11PM -0400, Chuck Lever wrote:
>
> On Jun 5, 2015, at 11:50 AM, Andy Adamson <[email protected]> wrote:
>
> > On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
> > <[email protected]> wrote:
> >> From: Chuck Lever <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Trond Myklebust <[email protected]>
> >> ---
> >> 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.
On Jun 8, 2015, at 4:47 PM, [email protected] wrote:
> On Fri, Jun 05, 2015 at 12:02:11PM -0400, Chuck Lever wrote:
>>
>> On Jun 5, 2015, at 11:50 AM, Andy Adamson <[email protected]> wrote:
>>
>>> On Thu, Jun 4, 2015 at 5:26 PM, Trond Myklebust
>>> <[email protected]> wrote:
>>>> From: Chuck Lever <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Trond Myklebust <[email protected]>
>>>> ---
>>>> 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
On Mon, Jun 8, 2015 at 4:47 PM, J. Bruce Fields <[email protected]> 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
Hi Trond-
On Jun 4, 2015, at 5:26 PM, Trond Myklebust <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever