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    [W:0.026 / U:90.004 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site