* [PATCH] RPC: killing RPC tasks races fixed
@ 2011-03-17 12:16 Stanislav Kinsbursky
2011-03-17 13:01 ` Trond Myklebust
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* Re: [PATCH] RPC: killing RPC tasks races fixed
2011-03-17 12:16 [PATCH] RPC: killing RPC tasks races fixed Stanislav Kinsbursky
@ 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, xemul, neilb, netdev, linux-kernel, bfields, davem,
skinsbursky
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.
> }
> }
> spin_unlock(&clnt->cl_lock);
>
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
^ 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
2011-03-17 16:44 ` Trond Myklebust
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* Re: [PATCH] RPC: killing RPC tasks races fixed
2011-03-17 15:43 ` Stanislav Kinsbursky
@ 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@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
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@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.
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@netapp.com
www.netapp.com
^ 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
2011-03-17 13:01 ` Trond Myklebust
2011-03-17 15:43 ` Stanislav Kinsbursky
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