Re: [PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
On Tue, Aug 28, 2007 at 09:26:36AM +1000, Neil Brown wrote: > On Monday August 27, [EMAIL PROTECTED] wrote: > > > > +/* Reference counting, callback cleanup, etc., all look racy as heck. > > + * And why is cb_set an atomic? */ > > Agreed so do we really want this code in mainline? is the old > code so bad that this is better? The code in question is just moved, not rewritten, so those are preexisting problems that I was just adding a whiny comment about in hopes it would remind me some more patches were probably needed. (Thanks for the reminder) > - cb_set should not be atomic. > - This looks like a job for async-rpc rather than a kernel thread The problem isn't the wait for the rpc itself, it's the creation of the new cred that's needed in the gss case. That could also be made asynchronous, but as there's several other delicate changes in the pipeline (keyring stuff, etc.), I'd rather not touch it for now. > - If you do use a thread, you at least want __module_get before >starting the thread, and module_put_and_exit to terminate the >thread. > - Can you just use 'cb_client' rather than cb_set? If you move >rpc_create into the thread, you don't need to set cb_client until >the callback is successful. Then add a 'cb_active' flag bit >so that you don't have two callbacks at the same time, and it >should be less racy.. Yep, probably so. --b. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
On Monday August 27, [EMAIL PROTECTED] wrote: > > +/* Reference counting, callback cleanup, etc., all look racy as heck. > + * And why is cb_set an atomic? */ Agreed so do we really want this code in mainline? is the old code so bad that this is better? - cb_set should not be atomic. - This looks like a job for async-rpc rather than a kernel thread - If you do use a thread, you at least want __module_get before starting the thread, and module_put_and_exit to terminate the thread. - Can you just use 'cb_client' rather than cb_set? If you move rpc_create into the thread, you don't need to set cb_client until the callback is successful. Then add a 'cb_active' flag bit so that you don't have two callbacks at the same time, and it should be less racy.. The other 14 patches all look ok. Thanks, NeilBrown - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
From: J. Bruce Fields <[EMAIL PROTECTED]>
We want to allow gss on the callback channel, so people using krb5 can
still get the benefits of delegations.
But looking up the rpc credential can take some time in that case. And
we shouldn't delay the response to setclientid_confirm while we wait.
It may be inefficient, but for now the simplest solution is just to
spawn a new thread as necessary for the purpose.
(Thanks to Adrian Bunk for catching a missing static here.)
Signed-off-by: "J. Bruce Fields" <[EMAIL PROTECTED]>
Cc: Adrian Bunk <[EMAIL PROTECTED]>
---
fs/nfsd/nfs4callback.c | 71 +++-
1 files changed, 34 insertions(+), 37 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 31d6633..c17a520 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -365,6 +366,35 @@ nfsd4_lookupcred(struct nfs4_client *clp, int taskflags)
return ret;
}
+/* Reference counting, callback cleanup, etc., all look racy as heck.
+ * And why is cb_set an atomic? */
+
+static int do_probe_callback(void *data)
+{
+ struct nfs4_client *clp = data;
+ struct nfs4_callback *cb = >cl_callback;
+ struct rpc_message msg = {
+ .rpc_proc = _cb_procedures[NFSPROC4_CLNT_CB_NULL],
+ .rpc_argp = clp,
+ };
+ int status;
+
+ msg.rpc_cred = nfsd4_lookupcred(clp, 0);
+ if (IS_ERR(msg.rpc_cred))
+ goto out;
+ status = rpc_call_sync(cb->cb_client, , RPC_TASK_SOFT);
+ put_rpccred(msg.rpc_cred);
+
+ if (status) {
+ rpc_shutdown_client(cb->cb_client);
+ cb->cb_client = NULL;
+ } else
+ atomic_set(>cb_set, 1);
+out:
+ put_nfs4_client(clp);
+ return 0;
+}
+
/*
* Set up the callback client and put a NFSPROC4_CB_NULL on the wire...
*/
@@ -390,11 +420,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
.authflavor = RPC_AUTH_UNIX,/* XXX: need
AUTH_GSS... */
.flags = (RPC_CLNT_CREATE_NOPING),
};
- struct rpc_message msg = {
- .rpc_proc = _cb_procedures[NFSPROC4_CLNT_CB_NULL],
- .rpc_argp = clp,
- };
- int status;
+ struct task_struct *t;
if (atomic_read(>cb_set))
return;
@@ -426,16 +452,11 @@ nfsd4_probe_callback(struct nfs4_client *clp)
/* the task holds a reference to the nfs4_client struct */
atomic_inc(>cl_count);
- msg.rpc_cred = nfsd4_lookupcred(clp,0);
- if (IS_ERR(msg.rpc_cred))
- goto out_release_clp;
- status = rpc_call_async(cb->cb_client, , RPC_TASK_ASYNC,
_cb_null_ops, NULL);
- put_rpccred(msg.rpc_cred);
+ t = kthread_run(do_probe_callback, clp, "nfs4_cb_probe");
- if (status != 0) {
- dprintk("NFSD: asynchronous NFSPROC4_CB_NULL failed!\n");
+ if (IS_ERR(t))
goto out_release_clp;
- }
+
return;
out_release_clp:
@@ -447,30 +468,6 @@ out_err:
(int)clp->cl_name.len, clp->cl_name.data);
}
-static void
-nfs4_cb_null(struct rpc_task *task, void *dummy)
-{
- struct nfs4_client *clp = (struct nfs4_client *)task->tk_msg.rpc_argp;
- struct nfs4_callback *cb = >cl_callback;
- __be32 addr = htonl(cb->cb_addr);
-
- dprintk("NFSD: nfs4_cb_null task->tk_status %d\n", task->tk_status);
-
- if (task->tk_status < 0) {
- dprintk("NFSD: callback establishment to client %.*s failed\n",
- (int)clp->cl_name.len, clp->cl_name.data);
- goto out;
- }
- atomic_set(>cb_set, 1);
- dprintk("NFSD: callback set to client %u.%u.%u.%u\n", NIPQUAD(addr));
-out:
- put_nfs4_client(clp);
-}
-
-static const struct rpc_call_ops nfs4_cb_null_ops = {
- .rpc_call_done = nfs4_cb_null,
-};
-
/*
* called with dp->dl_count inc'ed.
* nfs4_lock_state() may or may not have been called.
--
1.5.3.rc5.19.g0734d
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
From: J. Bruce Fields [EMAIL PROTECTED]
We want to allow gss on the callback channel, so people using krb5 can
still get the benefits of delegations.
But looking up the rpc credential can take some time in that case. And
we shouldn't delay the response to setclientid_confirm while we wait.
It may be inefficient, but for now the simplest solution is just to
spawn a new thread as necessary for the purpose.
(Thanks to Adrian Bunk for catching a missing static here.)
Signed-off-by: J. Bruce Fields [EMAIL PROTECTED]
Cc: Adrian Bunk [EMAIL PROTECTED]
---
fs/nfsd/nfs4callback.c | 71 +++-
1 files changed, 34 insertions(+), 37 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 31d6633..c17a520 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -39,6 +39,7 @@
#include linux/errno.h
#include linux/delay.h
#include linux/sched.h
+#include linux/kthread.h
#include linux/sunrpc/xdr.h
#include linux/sunrpc/svc.h
#include linux/sunrpc/clnt.h
@@ -365,6 +366,35 @@ nfsd4_lookupcred(struct nfs4_client *clp, int taskflags)
return ret;
}
+/* Reference counting, callback cleanup, etc., all look racy as heck.
+ * And why is cb_set an atomic? */
+
+static int do_probe_callback(void *data)
+{
+ struct nfs4_client *clp = data;
+ struct nfs4_callback *cb = clp-cl_callback;
+ struct rpc_message msg = {
+ .rpc_proc = nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
+ .rpc_argp = clp,
+ };
+ int status;
+
+ msg.rpc_cred = nfsd4_lookupcred(clp, 0);
+ if (IS_ERR(msg.rpc_cred))
+ goto out;
+ status = rpc_call_sync(cb-cb_client, msg, RPC_TASK_SOFT);
+ put_rpccred(msg.rpc_cred);
+
+ if (status) {
+ rpc_shutdown_client(cb-cb_client);
+ cb-cb_client = NULL;
+ } else
+ atomic_set(cb-cb_set, 1);
+out:
+ put_nfs4_client(clp);
+ return 0;
+}
+
/*
* Set up the callback client and put a NFSPROC4_CB_NULL on the wire...
*/
@@ -390,11 +420,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
.authflavor = RPC_AUTH_UNIX,/* XXX: need
AUTH_GSS... */
.flags = (RPC_CLNT_CREATE_NOPING),
};
- struct rpc_message msg = {
- .rpc_proc = nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
- .rpc_argp = clp,
- };
- int status;
+ struct task_struct *t;
if (atomic_read(cb-cb_set))
return;
@@ -426,16 +452,11 @@ nfsd4_probe_callback(struct nfs4_client *clp)
/* the task holds a reference to the nfs4_client struct */
atomic_inc(clp-cl_count);
- msg.rpc_cred = nfsd4_lookupcred(clp,0);
- if (IS_ERR(msg.rpc_cred))
- goto out_release_clp;
- status = rpc_call_async(cb-cb_client, msg, RPC_TASK_ASYNC,
nfs4_cb_null_ops, NULL);
- put_rpccred(msg.rpc_cred);
+ t = kthread_run(do_probe_callback, clp, nfs4_cb_probe);
- if (status != 0) {
- dprintk(NFSD: asynchronous NFSPROC4_CB_NULL failed!\n);
+ if (IS_ERR(t))
goto out_release_clp;
- }
+
return;
out_release_clp:
@@ -447,30 +468,6 @@ out_err:
(int)clp-cl_name.len, clp-cl_name.data);
}
-static void
-nfs4_cb_null(struct rpc_task *task, void *dummy)
-{
- struct nfs4_client *clp = (struct nfs4_client *)task-tk_msg.rpc_argp;
- struct nfs4_callback *cb = clp-cl_callback;
- __be32 addr = htonl(cb-cb_addr);
-
- dprintk(NFSD: nfs4_cb_null task-tk_status %d\n, task-tk_status);
-
- if (task-tk_status 0) {
- dprintk(NFSD: callback establishment to client %.*s failed\n,
- (int)clp-cl_name.len, clp-cl_name.data);
- goto out;
- }
- atomic_set(cb-cb_set, 1);
- dprintk(NFSD: callback set to client %u.%u.%u.%u\n, NIPQUAD(addr));
-out:
- put_nfs4_client(clp);
-}
-
-static const struct rpc_call_ops nfs4_cb_null_ops = {
- .rpc_call_done = nfs4_cb_null,
-};
-
/*
* called with dp-dl_count inc'ed.
* nfs4_lock_state() may or may not have been called.
--
1.5.3.rc5.19.g0734d
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
On Monday August 27, [EMAIL PROTECTED] wrote: +/* Reference counting, callback cleanup, etc., all look racy as heck. + * And why is cb_set an atomic? */ Agreed so do we really want this code in mainline? is the old code so bad that this is better? - cb_set should not be atomic. - This looks like a job for async-rpc rather than a kernel thread - If you do use a thread, you at least want __module_get before starting the thread, and module_put_and_exit to terminate the thread. - Can you just use 'cb_client' rather than cb_set? If you move rpc_create into the thread, you don't need to set cb_client until the callback is successful. Then add a 'cb_active' flag bit so that you don't have two callbacks at the same time, and it should be less racy.. The other 14 patches all look ok. Thanks, NeilBrown - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/15] knfsd: spawn kernel thread to probe callback channel
On Tue, Aug 28, 2007 at 09:26:36AM +1000, Neil Brown wrote: On Monday August 27, [EMAIL PROTECTED] wrote: +/* Reference counting, callback cleanup, etc., all look racy as heck. + * And why is cb_set an atomic? */ Agreed so do we really want this code in mainline? is the old code so bad that this is better? The code in question is just moved, not rewritten, so those are preexisting problems that I was just adding a whiny comment about in hopes it would remind me some more patches were probably needed. (Thanks for the reminder) - cb_set should not be atomic. - This looks like a job for async-rpc rather than a kernel thread The problem isn't the wait for the rpc itself, it's the creation of the new cred that's needed in the gss case. That could also be made asynchronous, but as there's several other delicate changes in the pipeline (keyring stuff, etc.), I'd rather not touch it for now. - If you do use a thread, you at least want __module_get before starting the thread, and module_put_and_exit to terminate the thread. - Can you just use 'cb_client' rather than cb_set? If you move rpc_create into the thread, you don't need to set cb_client until the callback is successful. Then add a 'cb_active' flag bit so that you don't have two callbacks at the same time, and it should be less racy.. Yep, probably so. --b. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
