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 <yuch...@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; > } > >