lkml.org 
[lkml]   [2006]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: please revert kthread from loop.c
Quoting Andrew Morton (akpm@osdl.org):
> On Tue, 11 Jul 2006 22:26:47 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
>
> > > If so, this should plug it. The same race is not possible against the
> > > loop_set_fd() wakeup because the thread isn't running at that stage, yes?
> >
> > Right, it's not yet running at loop_set_fd(). However what about
> > kthread_stop() called from loop_clr_fd()? Unfortunately fixing
> > that seems hairy. Need to think about it...
>
> Yes, there does seem to be a little race there.
>
> I think it would be sufficient to do
>
>
> diff -puN drivers/block/loop.c~a drivers/block/loop.c
> --- a/drivers/block/loop.c~a
> +++ a/drivers/block/loop.c
> @@ -602,7 +602,8 @@ static int loop_thread(void *data)
> }
> __set_current_state(TASK_INTERRUPTIBLE);
> spin_unlock_irq(&lo->lo_lock);
> - schedule();
> + if (lo->state != Lo_rundown)
> + schedule();
> }
>
> return 0;
> @@ -888,12 +889,11 @@ static int loop_clr_fd(struct loop_devic
> if (filp == NULL)
> return -EINVAL;
>
> + kthread_stop(lo->lo_thread);
> spin_lock_irq(&lo->lo_lock);
> lo->lo_state = Lo_rundown;
> spin_unlock_irq(&lo->lo_lock);
>
> - kthread_stop(lo->lo_thread);
> -
> lo->lo_backing_file = NULL;
>
> loop_release_xfer(lo);
> _
>
> where the tweak to loop_clr_fd() is just there to prevent loop_thread()
> from going into a very brief busyloop.

Why does this fix the problem? Can't the wake_up_process() in
kthread_stop() still happen right before loop_thread's schedule()?

This also means that after loop_thread() has decided to stop,
make_request() has a chance to make a few more requests. It
will see lo->lo_state as bound, assume all is well, but when it goes to
wake_up_thread(), the thread will have been put_task_struct()d.

If I'm not entirely wrong above, how about the following alternate fix?
Unfortunately I guess it doesn't stop the brief busyloop...

> I'm not sure why it's all so tricky in there, really. Loop is doing a
> pretty conventional stop, wakeup, stick-things-on-lists operation and we do
> that all over the kernel using pretty well-understood idioms. But for some
> reason, loop is all difficult about it. I wonder why. hm.

Perhaps I should give completions another go.

thanks,
-serge

Subject: [PATCH 3/3] kthread: fix loop.c race at thread stop

The wake_up_process() from kthread_stop() could happen
between loop_thread's __set_current_state(TASK_INTERRUPTIBLE)
and schedule(). But we can't put kthread_stop() under the
spin_lock like we did the wake_up_process() in make_request().

So turn the thread stopping into a two-phase process. Do
a wake_up_process() under spin_lock after setting the
lo_state to Lo_rundown, after which the loop_thread no
long sleeps.

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

---
drivers/block/loop.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
e972f09b6ca27a7ac3421ab49bde6dba33fca62c
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f944536..df38e05 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -600,9 +600,13 @@ static int loop_thread(void *data)
spin_unlock_irq(&lo->lo_lock);
break;
}
- __set_current_state(TASK_INTERRUPTIBLE);
+ if (lo->lo_state != Lo_rundown)
+ __set_current_state(TASK_INTERRUPTIBLE);
spin_unlock_irq(&lo->lo_lock);
- schedule();
+ if (lo->lo_state != Lo_rundown)
+ schedule();
+ else
+ __set_current_state(TASK_UNINTERRUPTIBLE);
}

return 0;
@@ -896,6 +900,7 @@ static int loop_clr_fd(struct loop_devic

spin_lock_irq(&lo->lo_lock);
lo->lo_state = Lo_rundown;
+ wake_up_process(lo->lo_thread);
spin_unlock_irq(&lo->lo_lock);

kthread_stop(lo->lo_thread);
--
1.1.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2006-07-12 16:35    [from the cache]
©2003-2011 Jasper Spaans