2006-10-17 17:25:29

by Frank Filz

[permalink] [raw]
Subject: [patch 0/4] correct kernel locking for RPC rpc_release callbacks

The following set of patches corrects use of the big kernel lock in
protecting RPC calls and associated call backs.

A problem was noted where nfs_async_unlink_release() was called without
the BKL being held as expected. Chuck Lever and Trond Myklebust noted
that the rpc_release() callback is a recent addition and other callbacks
might also expect the BKL to be held.

With the addition of lock_kernel()/unlock_kernel() around all calls to
the rpc_release() callbacks, there is no need for NFS code to hold the
BKL when making calls to rpc_execute(), rpc_call_sync(), and
rpc_call_async().

Frank Filz



-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2006-10-18 20:22:05

by Chuck Lever

[permalink] [raw]
Subject: Re: [patch 0/4] correct kernel locking for RPC rpc_release callbacks

On 10/17/06, Frank Filz <[email protected]> wrote:
> The following set of patches corrects use of the big kernel lock in
> protecting RPC calls and associated call backs.
>
> A problem was noted where nfs_async_unlink_release() was called without
> the BKL being held as expected. Chuck Lever and Trond Myklebust noted
> that the rpc_release() callback is a recent addition and other callbacks
> might also expect the BKL to be held.
>
> With the addition of lock_kernel()/unlock_kernel() around all calls to
> the rpc_release() callbacks, there is no need for NFS code to hold the
> BKL when making calls to rpc_execute(), rpc_call_sync(), and
> rpc_call_async().

I spent a little time looking at these, and they look reasonable to
me. I didn't see any paths that were now exposed by removing the BKL,
but I'm not the most rigorous reviewer.

I am a little concerned that the RPC scheduler is taking and dropping
such a heavy-weight serialization primitive multiple times per RPC.
Perhaps the next step should be removing the requirement for the
lock/unlock calls around the tk_action call to reduce this overhead.

--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2006-10-18 22:41:26

by Trond Myklebust

[permalink] [raw]
Subject: Re: [patch 0/4] correct kernel locking for RPC rpc_release callbacks

On Wed, 2006-10-18 at 16:22 -0400, Chuck Lever wrote:
> I am a little concerned that the RPC scheduler is taking and dropping
> such a heavy-weight serialization primitive multiple times per RPC.
> Perhaps the next step should be removing the requirement for the
> lock/unlock calls around the tk_action call to reduce this overhead.

I've already applied and tested the following patch. It should push the
BKL out of the actual RPC code, but keeps it for the auth_gss and
NFS/NLM encode/decode routines. The next step should be to review the
latter...

Cheers,
Trond

-------------------------------------------------------
From: Trond Myklebust <[email protected]>
Date: Wed, 18 Oct 2006 16:01:06 -0400
Subject: SUNRPC: Remove BKL around the RPC socket operations etc.

All internal RPC client operations should no longer depend on the BKL,
however lockd and NFS callbacks may still require it.

Signed-off-by: Trond Myklebust <[email protected]>
---

net/sunrpc/clnt.c | 8 +++++++-
net/sunrpc/sched.c | 8 ++++----
2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index a17cf0e..90a0fb1 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -27,6 +27,7 @@ #include <linux/module.h>
#include <linux/types.h>
#include <linux/mm.h>
#include <linux/slab.h>
+#include <linux/smp_lock.h>
#include <linux/utsname.h>
#include <linux/workqueue.h>

@@ -811,8 +812,10 @@ call_encode(struct rpc_task *task)
if (encode == NULL)
return;

+ lock_kernel();
task->tk_status = rpcauth_wrap_req(task, encode, req, p,
task->tk_msg.rpc_argp);
+ unlock_kernel();
if (task->tk_status == -ENOMEM) {
/* XXX: Is this sane? */
rpc_delay(task, 3*HZ);
@@ -1143,9 +1146,12 @@ call_decode(struct rpc_task *task)

task->tk_action = rpc_exit_task;

- if (decode)
+ if (decode) {
+ lock_kernel();
task->tk_status = rpcauth_unwrap_resp(task, decode, req, p,
task->tk_msg.rpc_resp);
+ unlock_kernel();
+ }
dprintk("RPC: %4d call_decode result %d\n", task->tk_pid,
task->tk_status);
return;
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 287fe9a..7cb888a 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -561,7 +561,9 @@ void rpc_delay(struct rpc_task *task, un
*/
static void rpc_prepare_task(struct rpc_task *task)
{
+ lock_kernel();
task->tk_ops->rpc_call_prepare(task, task->tk_calldata);
+ unlock_kernel();
}

/*
@@ -571,7 +573,9 @@ void rpc_exit_task(struct rpc_task *task
{
task->tk_action = NULL;
if (task->tk_ops->rpc_call_done != NULL) {
+ lock_kernel();
task->tk_ops->rpc_call_done(task, task->tk_calldata);
+ unlock_kernel();
if (task->tk_action != NULL) {
WARN_ON(RPC_ASSASSINATED(task));
/* Always release the RPC slot and buffer memory */
@@ -624,9 +628,7 @@ static int __rpc_execute(struct rpc_task
*/
save_callback=task->tk_callback;
task->tk_callback=NULL;
- lock_kernel();
save_callback(task);
- unlock_kernel();
}

/*
@@ -637,9 +639,7 @@ static int __rpc_execute(struct rpc_task
if (!RPC_IS_QUEUED(task)) {
if (task->tk_action == NULL)
break;
- lock_kernel();
task->tk_action(task);
- unlock_kernel();
}

/*



-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs