lkml.org 
[lkml]   [2018]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer
    From
    Date
    On 2018/5/5 18:02, Chao Yu wrote:
    > Thread A Thread B Thread C
    > - f2fs_remount
    > - stop_gc_thread
    > - f2fs_sbi_store
    > - issue_discard_thread
    > sbi->gc_thread = NULL;
    > sbi->gc_thread->gc_wake = 1
    > access sbi->gc_thread->gc_urgent
    >
    > Previously, we allocate memory for sbi->gc_thread based on background
    > gc thread mount option, the memory can be released if we turn off
    > that mount option, but still there are several places access gc_thread
    > pointer without considering race condition, result in NULL point
    > dereference.
    >
    > In order to fix this issue, use sb->s_umount to exclude those operations

    I encounter deadlock with this patch

    dump_stack+0x5f/0x86
    __lock_acquire+0xff7/0x10e0
    lock_acquire+0xae/0x220
    down_read+0x38/0x60 try lock s_umount again
    need_SSR+0x5d/0x160 [f2fs]
    allocate_segment_by_default+0xb7/0x1c0 [f2fs]
    allocate_data_block+0x183/0x4c0 [f2fs]
    do_write_page+0x52/0x80 [f2fs]
    write_data_page+0x4a/0xd0 [f2fs]
    do_write_data_page+0x327/0x630 [f2fs]
    __write_data_page+0x34b/0x800 [f2fs]
    __f2fs_write_data_pages+0x3f1/0x8e0 [f2fs]
    f2fs_write_data_pages+0x27/0x30 [f2fs]
    do_writepages+0x1a/0x70
    __writeback_single_inode+0x55/0x7e0
    writeback_sb_inodes+0x21b/0x490
    __writeback_inodes_wb+0x7c/0xb0 trylock_super has alread hold s_umount
    wb_writeback+0x3e2/0x580
    wb_workfn+0x251/0x6b0
    process_one_work+0x196/0x550
    worker_thread+0x31/0x360
    kthread+0xe3/0x110
    ret_from_fork+0x2e/0x38

    So, is it better to introduce private lock to avoid the race condition?

    Thanks,

    >
    > Signed-off-by: Chao Yu <yuchao0@huawei.com>
    > ---
    > v3:
    > - use sb->s_umount to make all race cases exclusive.
    > fs/f2fs/gc.c | 4 ++++
    > fs/f2fs/segment.c | 11 ++++++++++-
    > fs/f2fs/sysfs.c | 14 +++++++++++---
    > 3 files changed, 25 insertions(+), 4 deletions(-)
    >
    > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
    > index 9bb2ddbbed1e..d7d469f9be0a 100644
    > --- a/fs/f2fs/gc.c
    > +++ b/fs/f2fs/gc.c
    > @@ -187,17 +187,21 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
    > p->max_search = dirty_i->nr_dirty[type];
    > p->ofs_unit = 1;
    > } else {
    > + down_read(&sbi->sb->s_umount);
    > p->gc_mode = select_gc_type(sbi->gc_thread, gc_type);
    > + up_read(&sbi->sb->s_umount);
    > p->dirty_segmap = dirty_i->dirty_segmap[DIRTY];
    > p->max_search = dirty_i->nr_dirty[DIRTY];
    > p->ofs_unit = sbi->segs_per_sec;
    > }
    >
    > /* we need to check every dirty segments in the FG_GC case */
    > + down_read(&sbi->sb->s_umount);
    > if (gc_type != FG_GC &&
    > (sbi->gc_thread && !sbi->gc_thread->gc_urgent) &&
    > p->max_search > sbi->max_victim_search)
    > p->max_search = sbi->max_victim_search;
    > + up_read(&sbi->sb->s_umount);
    >
    > /* let's select beginning hot/small space first in no_heap mode*/
    > if (test_opt(sbi, NOHEAP) &&
    > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
    > index 320cc1c57246..74e184ab0544 100644
    > --- a/fs/f2fs/segment.c
    > +++ b/fs/f2fs/segment.c
    > @@ -174,11 +174,18 @@ bool need_SSR(struct f2fs_sb_info *sbi)
    > int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES);
    > int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS);
    > int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA);
    > + bool gc_urgent = false;
    >
    > if (test_opt(sbi, LFS))
    > return false;
    > +
    > + down_read(&sbi->sb->s_umount);
    > if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
    > - return true;
    > + gc_urgent = true;
    > + up_read(&sbi->sb->s_umount);
    > +
    > + if (gc_urgent)
    > + return false;
    >
    > return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs +
    > SM_I(sbi)->min_ssr_sections + reserved_sections(sbi));
    > @@ -1421,8 +1428,10 @@ static int issue_discard_thread(void *data)
    > if (dcc->discard_wake)
    > dcc->discard_wake = 0;
    >
    > + down_read(&sbi->sb->s_umount);
    > if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
    > init_discard_policy(&dpolicy, DPOLICY_FORCE, 1);
    > + up_read(&sbi->sb->s_umount);
    >
    > sb_start_intwrite(sbi->sb);
    >
    > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
    > index 6d8d8f41e517..1cba68812b32 100644
    > --- a/fs/f2fs/sysfs.c
    > +++ b/fs/f2fs/sysfs.c
    > @@ -173,6 +173,7 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
    > unsigned long t;
    > unsigned int *ui;
    > ssize_t ret;
    > + bool gc_entry = (a->struct_type == GC_THREAD);
    >
    > ptr = __struct_ptr(sbi, a->struct_type);
    > if (!ptr)
    > @@ -248,16 +249,23 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
    > if (!strcmp(a->attr.name, "trim_sections"))
    > return -EINVAL;
    >
    > - *ui = t;
    > -
    > - if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0)
    > + if (!strcmp(a->attr.name, "iostat_enable") && t == 0)
    > f2fs_reset_iostat(sbi);
    > +
    > + if (gc_entry)
    > + down_read(&sbi->sb->s_umount);
    > +
    > if (!strcmp(a->attr.name, "gc_urgent") && t == 1 && sbi->gc_thread) {
    > sbi->gc_thread->gc_wake = 1;
    > wake_up_interruptible_all(&sbi->gc_thread->gc_wait_queue_head);
    > wake_up_discard_thread(sbi, true);
    > }
    >
    > + *ui = t;
    > +
    > + if (gc_entry)
    > + up_read(&sbi->sb->s_umount);
    > +
    > return count;
    > }
    >
    >

    \
     
     \ /
      Last update: 2018-05-05 16:56    [W:4.172 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site