lkml.org 
[lkml]   [2018]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH AUTOSEL for 4.15 160/189] bcache: properly set task state in bch_writeback_thread()
    Date
    From: Coly Li <colyli@suse.de>

    [ Upstream commit 99361bbf26337186f02561109c17a4c4b1a7536a ]

    Kernel thread routine bch_writeback_thread() has the following code block,

    447 down_write(&dc->writeback_lock);
    448~450 if (check conditions) {
    451 up_write(&dc->writeback_lock);
    452 set_current_state(TASK_INTERRUPTIBLE);
    453
    454 if (kthread_should_stop())
    455 return 0;
    456
    457 schedule();
    458 continue;
    459 }

    If condition check is true, its task state is set to TASK_INTERRUPTIBLE
    and call schedule() to wait for others to wake up it.

    There are 2 issues in current code,
    1, Task state is set to TASK_INTERRUPTIBLE after the condition checks, if
    another process changes the condition and call wake_up_process(dc->
    writeback_thread), then at line 452 task state is set back to
    TASK_INTERRUPTIBLE, the writeback kernel thread will lose a chance to be
    waken up.
    2, At line 454 if kthread_should_stop() is true, writeback kernel thread
    will return to kernel/kthread.c:kthread() with TASK_INTERRUPTIBLE and
    call do_exit(). It is not good to enter do_exit() with task state
    TASK_INTERRUPTIBLE, in following code path might_sleep() is called and a
    warning message is reported by __might_sleep(): "WARNING: do not call
    blocking ops when !TASK_RUNNING; state=1 set at [xxxx]".

    For the first issue, task state should be set before condition checks.
    Ineed because dc->writeback_lock is required when modifying all the
    conditions, calling set_current_state() inside code block where dc->
    writeback_lock is hold is safe. But this is quite implicit, so I still move
    set_current_state() before all the condition checks.

    For the second issue, frankley speaking it does not hurt when kernel thread
    exits with TASK_INTERRUPTIBLE state, but this warning message scares users,
    makes them feel there might be something risky with bcache and hurt their
    data. Setting task state to TASK_RUNNING before returning fixes this
    problem.

    In alloc.c:allocator_wait(), there is also a similar issue, and is also
    fixed in this patch.

    Changelog:
    v3: merge two similar fixes into one patch
    v2: fix the race issue in v1 patch.
    v1: initial buggy fix.

    Signed-off-by: Coly Li <colyli@suse.de>
    Reviewed-by: Hannes Reinecke <hare@suse.de>
    Reviewed-by: Michael Lyle <mlyle@lyle.org>
    Cc: Michael Lyle <mlyle@lyle.org>
    Cc: Junhui Tang <tang.junhui@zte.com.cn>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>
    Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
    ---
    drivers/md/bcache/alloc.c | 4 +++-
    drivers/md/bcache/writeback.c | 7 +++++--
    2 files changed, 8 insertions(+), 3 deletions(-)

    diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
    index a0cc1bc6d884..aa2b657e507a 100644
    --- a/drivers/md/bcache/alloc.c
    +++ b/drivers/md/bcache/alloc.c
    @@ -287,8 +287,10 @@ do { \
    break; \
    \
    mutex_unlock(&(ca)->set->bucket_lock); \
    - if (kthread_should_stop()) \
    + if (kthread_should_stop()) { \
    + set_current_state(TASK_RUNNING); \
    return 0; \
    + } \
    \
    schedule(); \
    mutex_lock(&(ca)->set->bucket_lock); \
    diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
    index 56a37884ca8b..2259f78e9434 100644
    --- a/drivers/md/bcache/writeback.c
    +++ b/drivers/md/bcache/writeback.c
    @@ -445,18 +445,21 @@ static int bch_writeback_thread(void *arg)

    while (!kthread_should_stop()) {
    down_write(&dc->writeback_lock);
    + set_current_state(TASK_INTERRUPTIBLE);
    if (!atomic_read(&dc->has_dirty) ||
    (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) &&
    !dc->writeback_running)) {
    up_write(&dc->writeback_lock);
    - set_current_state(TASK_INTERRUPTIBLE);

    - if (kthread_should_stop())
    + if (kthread_should_stop()) {
    + set_current_state(TASK_RUNNING);
    return 0;
    + }

    schedule();
    continue;
    }
    + set_current_state(TASK_RUNNING);

    searched_full_index = refill_dirty(dc);

    --
    2.15.1
    \
     
     \ /
      Last update: 2018-04-09 05:34    [W:5.034 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site