* [PATCH] RPC: killing RPC tasks races fixed
@ 2011-03-17 12:16 Stanislav Kinsbursky
[not found] ` <20110317121638.15035.39410.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Stanislav Kinsbursky @ 2011-03-17 12:16 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem,
skinsbursky
task->tk_waitqueue must be checked for NULL before trying to wake up task in
rpc_killall_tasks() because it can be NULL.
Here is an example:
CPU 0 CPU 1 CPU 2
-------------------- --------------------- --------------------------
nfs4_run_open_task
rpc_run_task
rpc_execute
rpc_set_active
rpc_make_runnable
(waiting)
rpc_async_schedule
nfs4_open_prepare
nfs_wait_on_sequence
nfs_umount_begin
rpc_killall_tasks
rpc_wake_up_task
rpc_wake_up_queued_task
spin_lock(tk_waitqueue == NULL)
BUG()
rpc_sleep_on
spin_lock(&q->lock)
__rpc_sleep_on
task->tk_waitqueue = q
Signed-off-by: Stanislav Kinsbursky <skinsbursky@openvz.org>
---
net/sunrpc/clnt.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 57d344c..24039fe 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
if (!(rovr->tk_flags & RPC_TASK_KILLED)) {
rovr->tk_flags |= RPC_TASK_KILLED;
rpc_exit(rovr, -EIO);
- rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
+ if (rovr->tk_waitqueue)
+ rpc_wake_up_queued_task(rovr->tk_waitqueue,
+ rovr);
}
}
spin_unlock(&clnt->cl_lock);
^ permalink raw reply related [flat|nested] 5+ messages in thread[parent not found: <20110317121638.15035.39410.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>]
* Re: [PATCH] RPC: killing RPC tasks races fixed [not found] ` <20110317121638.15035.39410.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org> @ 2011-03-17 13:01 ` Trond Myklebust 2011-03-17 15:43 ` Stanislav Kinsbursky 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2011-03-17 13:01 UTC (permalink / raw) To: Stanislav Kinsbursky Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, xemul-bzQdu9zFT3WakBO8gow8eQ, neilb-l3A5Bk7waGM, netdev-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, bfields-uC3wQj2KruNg9hUCZPvPmw, davem-fT/PcQaiUtIeIZ0/mPfg9Q, skinsbursky-GEFAQzZX7r8dnm+yROfE0A On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote: > task->tk_waitqueue must be checked for NULL before trying to wake up task in > rpc_killall_tasks() because it can be NULL. > > Here is an example: > > CPU 0 CPU 1 CPU 2 > -------------------- --------------------- -------------------------- > nfs4_run_open_task > rpc_run_task > rpc_execute > rpc_set_active > rpc_make_runnable > (waiting) > rpc_async_schedule > nfs4_open_prepare > nfs_wait_on_sequence > nfs_umount_begin > rpc_killall_tasks > rpc_wake_up_task > rpc_wake_up_queued_task > spin_lock(tk_waitqueue == NULL) > BUG() > rpc_sleep_on > spin_lock(&q->lock) > __rpc_sleep_on > task->tk_waitqueue = q > > Signed-off-by: Stanislav Kinsbursky <skinsbursky-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> > > --- > net/sunrpc/clnt.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index 57d344c..24039fe 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt) > if (!(rovr->tk_flags & RPC_TASK_KILLED)) { > rovr->tk_flags |= RPC_TASK_KILLED; > rpc_exit(rovr, -EIO); > - rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr); > + if (rovr->tk_waitqueue) > + rpc_wake_up_queued_task(rovr->tk_waitqueue, > + rovr); Testing for RPC_IS_QUEUED(rovr) would be better, since that would optimise away the call to rpc_wake_up_queued_task() altogether for those tasks that aren't queued. > } > } > spin_unlock(&clnt->cl_lock); > -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RPC: killing RPC tasks races fixed 2011-03-17 13:01 ` Trond Myklebust @ 2011-03-17 15:43 ` Stanislav Kinsbursky [not found] ` <4D822C3C.9090100-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Stanislav Kinsbursky @ 2011-03-17 15:43 UTC (permalink / raw) To: Trond Myklebust Cc: linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bfields@fieldses.org, davem@davemloft.net, skinsbursky@openvz.org 17.03.2011 16:01, Trond Myklebust пишет: > On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote: >> task->tk_waitqueue must be checked for NULL before trying to wake up task in >> rpc_killall_tasks() because it can be NULL. >> >> Here is an example: >> >> CPU 0 CPU 1 CPU 2 >> -------------------- --------------------- -------------------------- >> nfs4_run_open_task >> rpc_run_task >> rpc_execute >> rpc_set_active >> rpc_make_runnable >> (waiting) >> rpc_async_schedule >> nfs4_open_prepare >> nfs_wait_on_sequence >> nfs_umount_begin >> rpc_killall_tasks >> rpc_wake_up_task >> rpc_wake_up_queued_task >> spin_lock(tk_waitqueue == NULL) >> BUG() >> rpc_sleep_on >> spin_lock(&q->lock) >> __rpc_sleep_on >> task->tk_waitqueue = q >> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@openvz.org> >> >> --- >> net/sunrpc/clnt.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >> index 57d344c..24039fe 100644 >> --- a/net/sunrpc/clnt.c >> +++ b/net/sunrpc/clnt.c >> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt) >> if (!(rovr->tk_flags& RPC_TASK_KILLED)) { >> rovr->tk_flags |= RPC_TASK_KILLED; >> rpc_exit(rovr, -EIO); >> - rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr); >> + if (rovr->tk_waitqueue) >> + rpc_wake_up_queued_task(rovr->tk_waitqueue, >> + rovr); > > Testing for RPC_IS_QUEUED(rovr) would be better, since that would > optimise away the call to rpc_wake_up_queued_task() altogether for those > tasks that aren't queued. > Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks clearer and in 2.6.38 tk_waitqueue is initialized prior to set RPC_TASK_QUEUED bit. But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed. Will send fixed version soon. >> } >> } >> spin_unlock(&clnt->cl_lock); >> > -- Best regards, Stanislav Kinsbursky ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <4D822C3C.9090100-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH] RPC: killing RPC tasks races fixed [not found] ` <4D822C3C.9090100-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-03-17 16:44 ` Trond Myklebust 2011-03-17 17:04 ` Stanislav Kinsbursky 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2011-03-17 16:44 UTC (permalink / raw) To: Stanislav Kinsbursky Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org, skinsbursky-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org On Thu, 2011-03-17 at 18:43 +0300, Stanislav Kinsbursky wrote: > 17.03.2011 16:01, Trond Myklebust пишет: > > On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote: > >> task->tk_waitqueue must be checked for NULL before trying to wake up task in > >> rpc_killall_tasks() because it can be NULL. > >> > >> Here is an example: > >> > >> CPU 0 CPU 1 CPU 2 > >> -------------------- --------------------- -------------------------- > >> nfs4_run_open_task > >> rpc_run_task > >> rpc_execute > >> rpc_set_active > >> rpc_make_runnable > >> (waiting) > >> rpc_async_schedule > >> nfs4_open_prepare > >> nfs_wait_on_sequence > >> nfs_umount_begin > >> rpc_killall_tasks > >> rpc_wake_up_task > >> rpc_wake_up_queued_task > >> spin_lock(tk_waitqueue == NULL) > >> BUG() > >> rpc_sleep_on > >> spin_lock(&q->lock) > >> __rpc_sleep_on > >> task->tk_waitqueue = q > >> > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> > >> > >> --- > >> net/sunrpc/clnt.c | 4 +++- > >> 1 files changed, 3 insertions(+), 1 deletions(-) > >> > >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > >> index 57d344c..24039fe 100644 > >> --- a/net/sunrpc/clnt.c > >> +++ b/net/sunrpc/clnt.c > >> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt) > >> if (!(rovr->tk_flags& RPC_TASK_KILLED)) { > >> rovr->tk_flags |= RPC_TASK_KILLED; > >> rpc_exit(rovr, -EIO); > >> - rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr); > >> + if (rovr->tk_waitqueue) > >> + rpc_wake_up_queued_task(rovr->tk_waitqueue, > >> + rovr); > > > > Testing for RPC_IS_QUEUED(rovr) would be better, since that would > > optimise away the call to rpc_wake_up_queued_task() altogether for those > > tasks that aren't queued. > > > > Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks > clearer and in 2.6.38 tk_waitqueue is initialized prior to set > RPC_TASK_QUEUED bit. > But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed. > Will send fixed version soon. Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline 2.6.32 kernel in this respect? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RPC: killing RPC tasks races fixed 2011-03-17 16:44 ` Trond Myklebust @ 2011-03-17 17:04 ` Stanislav Kinsbursky 0 siblings, 0 replies; 5+ messages in thread From: Stanislav Kinsbursky @ 2011-03-17 17:04 UTC (permalink / raw) To: Trond Myklebust Cc: linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bfields@fieldses.org, davem@davemloft.net, skinsbursky@openvz.org 17.03.2011 19:44, Trond Myklebust пишет: > > Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline > 2.6.32 kernel in this respect? > Checked again and realized, that was wrong. This set sequence is the same in 2.6.32 rhel, 2.6.32 and 2.6.38 kernels. -- Best regards, Stanislav Kinsbursky ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-03-17 17:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-17 12:16 [PATCH] RPC: killing RPC tasks races fixed Stanislav Kinsbursky
[not found] ` <20110317121638.15035.39410.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
2011-03-17 13:01 ` Trond Myklebust
2011-03-17 15:43 ` Stanislav Kinsbursky
[not found] ` <4D822C3C.9090100-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-03-17 16:44 ` Trond Myklebust
2011-03-17 17:04 ` Stanislav Kinsbursky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).