* [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer @ 2018-05-07 12:26 Chao Yu 2018-05-07 21:36 ` Jaegeuk Kim 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2018-05-07 12:26 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu 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, introduce gc_rwsem to exclude those operations. Signed-off-by: Chao Yu <yuchao0@huawei.com> --- v4: - use introduced sbi.gc_rwsem lock instead of sb.s_umount. fs/f2fs/f2fs.h | 1 + fs/f2fs/gc.c | 4 ++++ fs/f2fs/segment.c | 11 ++++++++++- fs/f2fs/super.c | 19 ++++++++++++++----- fs/f2fs/sysfs.c | 14 +++++++++++--- 5 files changed, 40 insertions(+), 9 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 80490a7991a7..e238d0ea0be7 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1191,6 +1191,7 @@ struct f2fs_sb_info { /* for cleaning operations */ struct mutex gc_mutex; /* mutex for GC */ + struct rw_semaphore gc_rwsem; /* rw semaphore for gc_thread */ struct f2fs_gc_kthread *gc_thread; /* GC thread */ unsigned int cur_victim_sec; /* current victim section num */ diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 9bb2ddbbed1e..b74714be7be7 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->gc_rwsem); p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); + up_read(&sbi->gc_rwsem); 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->gc_rwsem); 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->gc_rwsem); /* 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..33d146939048 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->gc_rwsem); if (sbi->gc_thread && sbi->gc_thread->gc_urgent) - return true; + gc_urgent = true; + up_read(&sbi->gc_rwsem); + + 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->gc_rwsem); if (sbi->gc_thread && sbi->gc_thread->gc_urgent) init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); + up_read(&sbi->gc_rwsem); sb_start_intwrite(sbi->sb); diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index c8e5fe5d71fe..294be9e92aee 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1476,15 +1476,23 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) * option. Also sync the filesystem. */ if ((*flags & SB_RDONLY) || !test_opt(sbi, BG_GC)) { + down_write(&sbi->gc_rwsem); if (sbi->gc_thread) { stop_gc_thread(sbi); need_restart_gc = true; } - } else if (!sbi->gc_thread) { - err = start_gc_thread(sbi); - if (err) - goto restore_opts; - need_stop_gc = true; + up_write(&sbi->gc_rwsem); + } else { + down_write(&sbi->gc_rwsem); + if (!sbi->gc_thread) { + err = start_gc_thread(sbi); + if (err) { + up_write(&sbi->gc_rwsem); + goto restore_opts; + } + need_stop_gc = true; + } + up_write(&sbi->gc_rwsem); } if (*flags & SB_RDONLY || @@ -2802,6 +2810,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) } init_rwsem(&sbi->cp_rwsem); + init_rwsem(&sbi->gc_rwsem); init_waitqueue_head(&sbi->cp_wait); init_sb_info(sbi); diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c index 6d8d8f41e517..34c542dbc459 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->gc_rwsem); + 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->gc_rwsem); + return count; } -- 2.17.0.391.g1f1cddd558b5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer 2018-05-07 12:26 [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer Chao Yu @ 2018-05-07 21:36 ` Jaegeuk Kim 2018-05-08 1:56 ` Chao Yu 2018-05-18 1:51 ` Chao Yu 0 siblings, 2 replies; 6+ messages in thread From: Jaegeuk Kim @ 2018-05-07 21:36 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao On 05/07, 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, introduce gc_rwsem to exclude those operations. > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > v4: > - use introduced sbi.gc_rwsem lock instead of sb.s_umount. We can use this first. >From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim <jaegeuk@kernel.org> Date: Mon, 7 May 2018 14:22:40 -0700 Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy This is to avoid sbi->gc_thread pointer access. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/f2fs.h | 8 ++++++++ fs/f2fs/gc.c | 28 ++++++++++++---------------- fs/f2fs/gc.h | 2 -- fs/f2fs/segment.c | 4 ++-- fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++-------- 5 files changed, 47 insertions(+), 28 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 80490a7991a7..779d8b26878c 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1065,6 +1065,13 @@ enum { MAX_TIME, }; +enum { + GC_NORMAL, + GC_IDLE_CB, + GC_IDLE_GREEDY, + GC_URGENT, +}; + enum { WHINT_MODE_OFF, /* not pass down write hints */ WHINT_MODE_USER, /* try to pass down hints given by users */ @@ -1193,6 +1200,7 @@ struct f2fs_sb_info { struct mutex gc_mutex; /* mutex for GC */ struct f2fs_gc_kthread *gc_thread; /* GC thread */ unsigned int cur_victim_sec; /* current victim section num */ + unsigned int gc_mode; /* current GC state */ /* threshold for gc trials on pinned files */ u64 gc_pin_file_threshold; diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 9bb2ddbbed1e..7ec8ea75dfde 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -76,7 +76,7 @@ static int gc_thread_func(void *data) * invalidated soon after by user update or deletion. * So, I'd like to wait some time to collect dirty segments. */ - if (gc_th->gc_urgent) { + if (sbi->gc_mode == GC_URGENT) { wait_ms = gc_th->urgent_sleep_time; mutex_lock(&sbi->gc_mutex); goto do_gc; @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi) gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME; gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME; - gc_th->gc_idle = 0; - gc_th->gc_urgent = 0; gc_th->gc_wake= 0; sbi->gc_thread = gc_th; @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi) sbi->gc_thread = NULL; } -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type) +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type) { int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY; - if (!gc_th) - return gc_mode; - - if (gc_th->gc_idle) { - if (gc_th->gc_idle == 1) - gc_mode = GC_CB; - else if (gc_th->gc_idle == 2) - gc_mode = GC_GREEDY; - } - if (gc_th->gc_urgent) + switch (sbi->gc_mode) { + case GC_IDLE_CB: + gc_mode = GC_CB; + break; + case GC_IDLE_GREEDY: + case GC_URGENT: gc_mode = GC_GREEDY; + break; + } return gc_mode; } @@ -187,7 +183,7 @@ 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 { - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); + p->gc_mode = select_gc_type(sbi, gc_type); p->dirty_segmap = dirty_i->dirty_segmap[DIRTY]; p->max_search = dirty_i->nr_dirty[DIRTY]; p->ofs_unit = sbi->segs_per_sec; @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type, /* we need to check every dirty segments in the FG_GC case */ if (gc_type != FG_GC && - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) && + (sbi->gc_mode != GC_URGENT) && p->max_search > sbi->max_victim_search) p->max_search = sbi->max_victim_search; diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h index b0045d4c8d1e..c8619e408009 100644 --- a/fs/f2fs/gc.h +++ b/fs/f2fs/gc.h @@ -36,8 +36,6 @@ struct f2fs_gc_kthread { unsigned int no_gc_sleep_time; /* for changing gc mode */ - unsigned int gc_idle; - unsigned int gc_urgent; unsigned int gc_wake; }; diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 503a98abdb2f..5a7ed06291e0 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi) if (test_opt(sbi, LFS)) return false; - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) + if (sbi->gc_mode == GC_URGENT) return true; return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data) if (dcc->discard_wake) dcc->discard_wake = 0; - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) + if (sbi->gc_mode == GC_URGENT) init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); sb_start_intwrite(sbi->sb); diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c index 6d8d8f41e517..dd940d156af6 100644 --- a/fs/f2fs/sysfs.c +++ b/fs/f2fs/sysfs.c @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a, if (!strcmp(a->attr.name, "trim_sections")) return -EINVAL; + if (!strcmp(a->attr.name, "gc_urgent")) { + if (t >= 1) { + sbi->gc_mode = GC_URGENT; + if (sbi->gc_thread) { + wake_up_interruptible_all( + &sbi->gc_thread->gc_wait_queue_head); + wake_up_discard_thread(sbi, true); + } + } else { + sbi->gc_mode = GC_NORMAL; + } + return count; + } + if (!strcmp(a->attr.name, "gc_idle")) { + if (t == GC_IDLE_CB) + sbi->gc_mode = GC_IDLE_CB; + else if (t == GC_IDLE_GREEDY) + sbi->gc_mode = GC_IDLE_GREEDY; + else + sbi->gc_mode = GC_NORMAL; + return count; + } + *ui = t; if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0) f2fs_reset_iostat(sbi); - 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); - } - return count; } @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time, F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time); F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time); F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time); -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle); -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent); +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode); +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments); F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards); F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity); -- 2.17.0.484.g0c8726318c-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer 2018-05-07 21:36 ` Jaegeuk Kim @ 2018-05-08 1:56 ` Chao Yu 2018-05-08 3:17 ` Jaegeuk Kim 2018-05-18 1:51 ` Chao Yu 1 sibling, 1 reply; 6+ messages in thread From: Chao Yu @ 2018-05-08 1:56 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao On 2018/5/8 5:36, Jaegeuk Kim wrote: > On 05/07, 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, introduce gc_rwsem to exclude those operations. >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> v4: >> - use introduced sbi.gc_rwsem lock instead of sb.s_umount. > > We can use this first. > >>From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Mon, 7 May 2018 14:22:40 -0700 > Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy > > This is to avoid sbi->gc_thread pointer access. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/f2fs.h | 8 ++++++++ > fs/f2fs/gc.c | 28 ++++++++++++---------------- > fs/f2fs/gc.h | 2 -- > fs/f2fs/segment.c | 4 ++-- > fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++-------- > 5 files changed, 47 insertions(+), 28 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 80490a7991a7..779d8b26878c 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1065,6 +1065,13 @@ enum { > MAX_TIME, > }; > > +enum { > + GC_NORMAL, > + GC_IDLE_CB, > + GC_IDLE_GREEDY, > + GC_URGENT, > +}; > + > enum { > WHINT_MODE_OFF, /* not pass down write hints */ > WHINT_MODE_USER, /* try to pass down hints given by users */ > @@ -1193,6 +1200,7 @@ struct f2fs_sb_info { > struct mutex gc_mutex; /* mutex for GC */ > struct f2fs_gc_kthread *gc_thread; /* GC thread */ > unsigned int cur_victim_sec; /* current victim section num */ > + unsigned int gc_mode; /* current GC state */ > > /* threshold for gc trials on pinned files */ > u64 gc_pin_file_threshold; > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 9bb2ddbbed1e..7ec8ea75dfde 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -76,7 +76,7 @@ static int gc_thread_func(void *data) > * invalidated soon after by user update or deletion. > * So, I'd like to wait some time to collect dirty segments. > */ > - if (gc_th->gc_urgent) { > + if (sbi->gc_mode == GC_URGENT) { > wait_ms = gc_th->urgent_sleep_time; > mutex_lock(&sbi->gc_mutex); > goto do_gc; > @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi) > gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME; > gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME; > > - gc_th->gc_idle = 0; > - gc_th->gc_urgent = 0; > gc_th->gc_wake= 0; > > sbi->gc_thread = gc_th; > @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi) > sbi->gc_thread = NULL; > } > > -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type) > +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type) > { > int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY; > > - if (!gc_th) > - return gc_mode; > - > - if (gc_th->gc_idle) { > - if (gc_th->gc_idle == 1) > - gc_mode = GC_CB; > - else if (gc_th->gc_idle == 2) > - gc_mode = GC_GREEDY; > - } > - if (gc_th->gc_urgent) > + switch (sbi->gc_mode) { > + case GC_IDLE_CB: > + gc_mode = GC_CB; > + break; > + case GC_IDLE_GREEDY: > + case GC_URGENT: > gc_mode = GC_GREEDY; > + break; > + } > return gc_mode; > } > > @@ -187,7 +183,7 @@ 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 { > - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); > + p->gc_mode = select_gc_type(sbi, gc_type); > p->dirty_segmap = dirty_i->dirty_segmap[DIRTY]; > p->max_search = dirty_i->nr_dirty[DIRTY]; > p->ofs_unit = sbi->segs_per_sec; > @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type, > > /* we need to check every dirty segments in the FG_GC case */ > if (gc_type != FG_GC && > - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) && > + (sbi->gc_mode != GC_URGENT) && > p->max_search > sbi->max_victim_search) > p->max_search = sbi->max_victim_search; > > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > index b0045d4c8d1e..c8619e408009 100644 > --- a/fs/f2fs/gc.h > +++ b/fs/f2fs/gc.h > @@ -36,8 +36,6 @@ struct f2fs_gc_kthread { > unsigned int no_gc_sleep_time; > > /* for changing gc mode */ > - unsigned int gc_idle; > - unsigned int gc_urgent; > unsigned int gc_wake; > }; > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 503a98abdb2f..5a7ed06291e0 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi) > > if (test_opt(sbi, LFS)) > return false; > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > + if (sbi->gc_mode == GC_URGENT) > return true; > > return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + > @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data) > if (dcc->discard_wake) > dcc->discard_wake = 0; > > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > + if (sbi->gc_mode == GC_URGENT) > init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); > > sb_start_intwrite(sbi->sb); > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c > index 6d8d8f41e517..dd940d156af6 100644 > --- a/fs/f2fs/sysfs.c > +++ b/fs/f2fs/sysfs.c > @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a, > if (!strcmp(a->attr.name, "trim_sections")) > return -EINVAL; > > + if (!strcmp(a->attr.name, "gc_urgent")) { > + if (t >= 1) { > + sbi->gc_mode = GC_URGENT; > + if (sbi->gc_thread) { > + wake_up_interruptible_all( > + &sbi->gc_thread->gc_wait_queue_head); > + wake_up_discard_thread(sbi, true); > + } > + } else { > + sbi->gc_mode = GC_NORMAL; > + } > + return count; > + } > + if (!strcmp(a->attr.name, "gc_idle")) { > + if (t == GC_IDLE_CB) > + sbi->gc_mode = GC_IDLE_CB; > + else if (t == GC_IDLE_GREEDY) > + sbi->gc_mode = GC_IDLE_GREEDY; > + else > + sbi->gc_mode = GC_NORMAL; > + return count; > + } > + > *ui = t; > > if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0) > f2fs_reset_iostat(sbi); > - 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); > - } > - > return count; > } > > @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time, > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time); > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time); > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time); All above parameters will be accessed via '*ui = t;', in order to avoid that, we need to move all gc_* configuration fields into sbi, so leaving f2fs_gc_kthread all most empty. IMO, just merge my v1 patch is OK? since that patch makes better encapsulation, so that background GC related parameter can still locate in independent f2fs_gc_kthread structure rather in common f2fs_sb_info structure. Thanks, > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle); > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent); > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode); > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); > F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments); > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards); > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer 2018-05-08 1:56 ` Chao Yu @ 2018-05-08 3:17 ` Jaegeuk Kim 2018-05-08 3:21 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Jaegeuk Kim @ 2018-05-08 3:17 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao On 05/08, Chao Yu wrote: > On 2018/5/8 5:36, Jaegeuk Kim wrote: > > On 05/07, 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, introduce gc_rwsem to exclude those operations. > >> > >> Signed-off-by: Chao Yu <yuchao0@huawei.com> > >> --- > >> v4: > >> - use introduced sbi.gc_rwsem lock instead of sb.s_umount. > > > > We can use this first. > > > >>From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001 > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > Date: Mon, 7 May 2018 14:22:40 -0700 > > Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy > > > > This is to avoid sbi->gc_thread pointer access. > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/f2fs.h | 8 ++++++++ > > fs/f2fs/gc.c | 28 ++++++++++++---------------- > > fs/f2fs/gc.h | 2 -- > > fs/f2fs/segment.c | 4 ++-- > > fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++-------- > > 5 files changed, 47 insertions(+), 28 deletions(-) > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 80490a7991a7..779d8b26878c 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1065,6 +1065,13 @@ enum { > > MAX_TIME, > > }; > > > > +enum { > > + GC_NORMAL, > > + GC_IDLE_CB, > > + GC_IDLE_GREEDY, > > + GC_URGENT, > > +}; > > + > > enum { > > WHINT_MODE_OFF, /* not pass down write hints */ > > WHINT_MODE_USER, /* try to pass down hints given by users */ > > @@ -1193,6 +1200,7 @@ struct f2fs_sb_info { > > struct mutex gc_mutex; /* mutex for GC */ > > struct f2fs_gc_kthread *gc_thread; /* GC thread */ > > unsigned int cur_victim_sec; /* current victim section num */ > > + unsigned int gc_mode; /* current GC state */ > > > > /* threshold for gc trials on pinned files */ > > u64 gc_pin_file_threshold; > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > index 9bb2ddbbed1e..7ec8ea75dfde 100644 > > --- a/fs/f2fs/gc.c > > +++ b/fs/f2fs/gc.c > > @@ -76,7 +76,7 @@ static int gc_thread_func(void *data) > > * invalidated soon after by user update or deletion. > > * So, I'd like to wait some time to collect dirty segments. > > */ > > - if (gc_th->gc_urgent) { > > + if (sbi->gc_mode == GC_URGENT) { > > wait_ms = gc_th->urgent_sleep_time; > > mutex_lock(&sbi->gc_mutex); > > goto do_gc; > > @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi) > > gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME; > > gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME; > > > > - gc_th->gc_idle = 0; > > - gc_th->gc_urgent = 0; > > gc_th->gc_wake= 0; > > > > sbi->gc_thread = gc_th; > > @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi) > > sbi->gc_thread = NULL; > > } > > > > -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type) > > +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type) > > { > > int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY; > > > > - if (!gc_th) > > - return gc_mode; > > - > > - if (gc_th->gc_idle) { > > - if (gc_th->gc_idle == 1) > > - gc_mode = GC_CB; > > - else if (gc_th->gc_idle == 2) > > - gc_mode = GC_GREEDY; > > - } > > - if (gc_th->gc_urgent) > > + switch (sbi->gc_mode) { > > + case GC_IDLE_CB: > > + gc_mode = GC_CB; > > + break; > > + case GC_IDLE_GREEDY: > > + case GC_URGENT: > > gc_mode = GC_GREEDY; > > + break; > > + } > > return gc_mode; > > } > > > > @@ -187,7 +183,7 @@ 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 { > > - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); > > + p->gc_mode = select_gc_type(sbi, gc_type); > > p->dirty_segmap = dirty_i->dirty_segmap[DIRTY]; > > p->max_search = dirty_i->nr_dirty[DIRTY]; > > p->ofs_unit = sbi->segs_per_sec; > > @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type, > > > > /* we need to check every dirty segments in the FG_GC case */ > > if (gc_type != FG_GC && > > - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) && > > + (sbi->gc_mode != GC_URGENT) && > > p->max_search > sbi->max_victim_search) > > p->max_search = sbi->max_victim_search; > > > > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > > index b0045d4c8d1e..c8619e408009 100644 > > --- a/fs/f2fs/gc.h > > +++ b/fs/f2fs/gc.h > > @@ -36,8 +36,6 @@ struct f2fs_gc_kthread { > > unsigned int no_gc_sleep_time; > > > > /* for changing gc mode */ > > - unsigned int gc_idle; > > - unsigned int gc_urgent; > > unsigned int gc_wake; > > }; > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > index 503a98abdb2f..5a7ed06291e0 100644 > > --- a/fs/f2fs/segment.c > > +++ b/fs/f2fs/segment.c > > @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi) > > > > if (test_opt(sbi, LFS)) > > return false; > > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > > + if (sbi->gc_mode == GC_URGENT) > > return true; > > > > return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + > > @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data) > > if (dcc->discard_wake) > > dcc->discard_wake = 0; > > > > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > > + if (sbi->gc_mode == GC_URGENT) > > init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); > > > > sb_start_intwrite(sbi->sb); > > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c > > index 6d8d8f41e517..dd940d156af6 100644 > > --- a/fs/f2fs/sysfs.c > > +++ b/fs/f2fs/sysfs.c > > @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a, > > if (!strcmp(a->attr.name, "trim_sections")) > > return -EINVAL; > > > > + if (!strcmp(a->attr.name, "gc_urgent")) { > > + if (t >= 1) { > > + sbi->gc_mode = GC_URGENT; > > + if (sbi->gc_thread) { > > + wake_up_interruptible_all( > > + &sbi->gc_thread->gc_wait_queue_head); > > + wake_up_discard_thread(sbi, true); > > + } > > + } else { > > + sbi->gc_mode = GC_NORMAL; > > + } > > + return count; > > + } > > + if (!strcmp(a->attr.name, "gc_idle")) { > > + if (t == GC_IDLE_CB) > > + sbi->gc_mode = GC_IDLE_CB; > > + else if (t == GC_IDLE_GREEDY) > > + sbi->gc_mode = GC_IDLE_GREEDY; > > + else > > + sbi->gc_mode = GC_NORMAL; > > + return count; > > + } > > + > > *ui = t; > > > > if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0) > > f2fs_reset_iostat(sbi); > > - 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); > > - } > > - > > return count; > > } > > > > @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time, > > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time); > > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time); > > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time); > > All above parameters will be accessed via '*ui = t;', in order to avoid that, we > need to move all gc_* configuration fields into sbi, so leaving f2fs_gc_kthread > all most empty. What I said was to use down_read(&sbi->sb->s_umount); in sysfs.c on top of this. > IMO, just merge my v1 patch is OK? since that patch makes better > encapsulation, so that background GC related parameter can still locate in > independent f2fs_gc_kthread structure rather in common f2fs_sb_info structure. > > Thanks, > > > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle); > > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent); > > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode); > > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); > > F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments); > > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards); > > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer 2018-05-08 3:17 ` Jaegeuk Kim @ 2018-05-08 3:21 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2018-05-08 3:21 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao On 2018/5/8 11:17, Jaegeuk Kim wrote: > On 05/08, Chao Yu wrote: >> On 2018/5/8 5:36, Jaegeuk Kim wrote: >>> On 05/07, 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, introduce gc_rwsem to exclude those operations. >>>> >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com> >>>> --- >>>> v4: >>>> - use introduced sbi.gc_rwsem lock instead of sb.s_umount. >>> >>> We can use this first. >>> >>> >From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001 >>> From: Jaegeuk Kim <jaegeuk@kernel.org> >>> Date: Mon, 7 May 2018 14:22:40 -0700 >>> Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy >>> >>> This is to avoid sbi->gc_thread pointer access. >>> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/f2fs.h | 8 ++++++++ >>> fs/f2fs/gc.c | 28 ++++++++++++---------------- >>> fs/f2fs/gc.h | 2 -- >>> fs/f2fs/segment.c | 4 ++-- >>> fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++-------- >>> 5 files changed, 47 insertions(+), 28 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 80490a7991a7..779d8b26878c 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -1065,6 +1065,13 @@ enum { >>> MAX_TIME, >>> }; >>> >>> +enum { >>> + GC_NORMAL, >>> + GC_IDLE_CB, >>> + GC_IDLE_GREEDY, >>> + GC_URGENT, >>> +}; >>> + >>> enum { >>> WHINT_MODE_OFF, /* not pass down write hints */ >>> WHINT_MODE_USER, /* try to pass down hints given by users */ >>> @@ -1193,6 +1200,7 @@ struct f2fs_sb_info { >>> struct mutex gc_mutex; /* mutex for GC */ >>> struct f2fs_gc_kthread *gc_thread; /* GC thread */ >>> unsigned int cur_victim_sec; /* current victim section num */ >>> + unsigned int gc_mode; /* current GC state */ >>> >>> /* threshold for gc trials on pinned files */ >>> u64 gc_pin_file_threshold; >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>> index 9bb2ddbbed1e..7ec8ea75dfde 100644 >>> --- a/fs/f2fs/gc.c >>> +++ b/fs/f2fs/gc.c >>> @@ -76,7 +76,7 @@ static int gc_thread_func(void *data) >>> * invalidated soon after by user update or deletion. >>> * So, I'd like to wait some time to collect dirty segments. >>> */ >>> - if (gc_th->gc_urgent) { >>> + if (sbi->gc_mode == GC_URGENT) { >>> wait_ms = gc_th->urgent_sleep_time; >>> mutex_lock(&sbi->gc_mutex); >>> goto do_gc; >>> @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi) >>> gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME; >>> gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME; >>> >>> - gc_th->gc_idle = 0; >>> - gc_th->gc_urgent = 0; >>> gc_th->gc_wake= 0; >>> >>> sbi->gc_thread = gc_th; >>> @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi) >>> sbi->gc_thread = NULL; >>> } >>> >>> -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type) >>> +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type) >>> { >>> int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY; >>> >>> - if (!gc_th) >>> - return gc_mode; >>> - >>> - if (gc_th->gc_idle) { >>> - if (gc_th->gc_idle == 1) >>> - gc_mode = GC_CB; >>> - else if (gc_th->gc_idle == 2) >>> - gc_mode = GC_GREEDY; >>> - } >>> - if (gc_th->gc_urgent) >>> + switch (sbi->gc_mode) { >>> + case GC_IDLE_CB: >>> + gc_mode = GC_CB; >>> + break; >>> + case GC_IDLE_GREEDY: >>> + case GC_URGENT: >>> gc_mode = GC_GREEDY; >>> + break; >>> + } >>> return gc_mode; >>> } >>> >>> @@ -187,7 +183,7 @@ 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 { >>> - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); >>> + p->gc_mode = select_gc_type(sbi, gc_type); >>> p->dirty_segmap = dirty_i->dirty_segmap[DIRTY]; >>> p->max_search = dirty_i->nr_dirty[DIRTY]; >>> p->ofs_unit = sbi->segs_per_sec; >>> @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type, >>> >>> /* we need to check every dirty segments in the FG_GC case */ >>> if (gc_type != FG_GC && >>> - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) && >>> + (sbi->gc_mode != GC_URGENT) && >>> p->max_search > sbi->max_victim_search) >>> p->max_search = sbi->max_victim_search; >>> >>> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h >>> index b0045d4c8d1e..c8619e408009 100644 >>> --- a/fs/f2fs/gc.h >>> +++ b/fs/f2fs/gc.h >>> @@ -36,8 +36,6 @@ struct f2fs_gc_kthread { >>> unsigned int no_gc_sleep_time; >>> >>> /* for changing gc mode */ >>> - unsigned int gc_idle; >>> - unsigned int gc_urgent; >>> unsigned int gc_wake; >>> }; >>> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index 503a98abdb2f..5a7ed06291e0 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi) >>> >>> if (test_opt(sbi, LFS)) >>> return false; >>> - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) >>> + if (sbi->gc_mode == GC_URGENT) >>> return true; >>> >>> return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + >>> @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data) >>> if (dcc->discard_wake) >>> dcc->discard_wake = 0; >>> >>> - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) >>> + if (sbi->gc_mode == GC_URGENT) >>> init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); >>> >>> sb_start_intwrite(sbi->sb); >>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c >>> index 6d8d8f41e517..dd940d156af6 100644 >>> --- a/fs/f2fs/sysfs.c >>> +++ b/fs/f2fs/sysfs.c >>> @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a, >>> if (!strcmp(a->attr.name, "trim_sections")) >>> return -EINVAL; >>> >>> + if (!strcmp(a->attr.name, "gc_urgent")) { >>> + if (t >= 1) { >>> + sbi->gc_mode = GC_URGENT; >>> + if (sbi->gc_thread) { >>> + wake_up_interruptible_all( >>> + &sbi->gc_thread->gc_wait_queue_head); >>> + wake_up_discard_thread(sbi, true); >>> + } >>> + } else { >>> + sbi->gc_mode = GC_NORMAL; >>> + } >>> + return count; >>> + } >>> + if (!strcmp(a->attr.name, "gc_idle")) { >>> + if (t == GC_IDLE_CB) >>> + sbi->gc_mode = GC_IDLE_CB; >>> + else if (t == GC_IDLE_GREEDY) >>> + sbi->gc_mode = GC_IDLE_GREEDY; >>> + else >>> + sbi->gc_mode = GC_NORMAL; >>> + return count; >>> + } >>> + >>> *ui = t; >>> >>> if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0) >>> f2fs_reset_iostat(sbi); >>> - 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); >>> - } >>> - >>> return count; >>> } >>> >>> @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time, >>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time); >>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time); >>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time); >> >> All above parameters will be accessed via '*ui = t;', in order to avoid that, we >> need to move all gc_* configuration fields into sbi, so leaving f2fs_gc_kthread >> all most empty. > > What I said was to use down_read(&sbi->sb->s_umount); in sysfs.c on top of this. Oh, sorry, I didn't realize that, let me update my patch. Thanks, > >> IMO, just merge my v1 patch is OK? since that patch makes better >> encapsulation, so that background GC related parameter can still locate in >> independent f2fs_gc_kthread structure rather in common f2fs_sb_info structure. >> >> Thanks, >> >>> -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle); >>> -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent); >>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode); >>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); >>> F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments); >>> F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards); >>> F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity); >>> > > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer 2018-05-07 21:36 ` Jaegeuk Kim 2018-05-08 1:56 ` Chao Yu @ 2018-05-18 1:51 ` Chao Yu 1 sibling, 0 replies; 6+ messages in thread From: Chao Yu @ 2018-05-18 1:51 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao On 2018/5/8 5:36, Jaegeuk Kim wrote: > On 05/07, 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, introduce gc_rwsem to exclude those operations. >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> v4: >> - use introduced sbi.gc_rwsem lock instead of sb.s_umount. > > We can use this first. > >>From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Mon, 7 May 2018 14:22:40 -0700 > Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy > > This is to avoid sbi->gc_thread pointer access. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/f2fs.h | 8 ++++++++ > fs/f2fs/gc.c | 28 ++++++++++++---------------- > fs/f2fs/gc.h | 2 -- > fs/f2fs/segment.c | 4 ++-- > fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++-------- > 5 files changed, 47 insertions(+), 28 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 80490a7991a7..779d8b26878c 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1065,6 +1065,13 @@ enum { > MAX_TIME, > }; > > +enum { > + GC_NORMAL, > + GC_IDLE_CB, > + GC_IDLE_GREEDY, > + GC_URGENT, > +}; > + > enum { > WHINT_MODE_OFF, /* not pass down write hints */ > WHINT_MODE_USER, /* try to pass down hints given by users */ > @@ -1193,6 +1200,7 @@ struct f2fs_sb_info { > struct mutex gc_mutex; /* mutex for GC */ > struct f2fs_gc_kthread *gc_thread; /* GC thread */ > unsigned int cur_victim_sec; /* current victim section num */ > + unsigned int gc_mode; /* current GC state */ > > /* threshold for gc trials on pinned files */ > u64 gc_pin_file_threshold; > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 9bb2ddbbed1e..7ec8ea75dfde 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -76,7 +76,7 @@ static int gc_thread_func(void *data) > * invalidated soon after by user update or deletion. > * So, I'd like to wait some time to collect dirty segments. > */ > - if (gc_th->gc_urgent) { > + if (sbi->gc_mode == GC_URGENT) { > wait_ms = gc_th->urgent_sleep_time; > mutex_lock(&sbi->gc_mutex); > goto do_gc; > @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi) > gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME; > gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME; > > - gc_th->gc_idle = 0; > - gc_th->gc_urgent = 0; > gc_th->gc_wake= 0; > > sbi->gc_thread = gc_th; > @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi) > sbi->gc_thread = NULL; > } > > -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type) > +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type) > { > int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY; > > - if (!gc_th) > - return gc_mode; > - > - if (gc_th->gc_idle) { > - if (gc_th->gc_idle == 1) > - gc_mode = GC_CB; > - else if (gc_th->gc_idle == 2) > - gc_mode = GC_GREEDY; > - } > - if (gc_th->gc_urgent) > + switch (sbi->gc_mode) { > + case GC_IDLE_CB: > + gc_mode = GC_CB; > + break; > + case GC_IDLE_GREEDY: > + case GC_URGENT: > gc_mode = GC_GREEDY; > + break; > + } > return gc_mode; > } > > @@ -187,7 +183,7 @@ 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 { > - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type); > + p->gc_mode = select_gc_type(sbi, gc_type); > p->dirty_segmap = dirty_i->dirty_segmap[DIRTY]; > p->max_search = dirty_i->nr_dirty[DIRTY]; > p->ofs_unit = sbi->segs_per_sec; > @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type, > > /* we need to check every dirty segments in the FG_GC case */ > if (gc_type != FG_GC && > - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) && > + (sbi->gc_mode != GC_URGENT) && > p->max_search > sbi->max_victim_search) > p->max_search = sbi->max_victim_search; > > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > index b0045d4c8d1e..c8619e408009 100644 > --- a/fs/f2fs/gc.h > +++ b/fs/f2fs/gc.h > @@ -36,8 +36,6 @@ struct f2fs_gc_kthread { > unsigned int no_gc_sleep_time; > > /* for changing gc mode */ > - unsigned int gc_idle; > - unsigned int gc_urgent; > unsigned int gc_wake; > }; > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 503a98abdb2f..5a7ed06291e0 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi) > > if (test_opt(sbi, LFS)) > return false; > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > + if (sbi->gc_mode == GC_URGENT) > return true; > > return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + > @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data) > if (dcc->discard_wake) > dcc->discard_wake = 0; > > - if (sbi->gc_thread && sbi->gc_thread->gc_urgent) > + if (sbi->gc_mode == GC_URGENT) > init_discard_policy(&dpolicy, DPOLICY_FORCE, 1); > > sb_start_intwrite(sbi->sb); > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c > index 6d8d8f41e517..dd940d156af6 100644 > --- a/fs/f2fs/sysfs.c > +++ b/fs/f2fs/sysfs.c > @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a, > if (!strcmp(a->attr.name, "trim_sections")) > return -EINVAL; > > + if (!strcmp(a->attr.name, "gc_urgent")) { > + if (t >= 1) { > + sbi->gc_mode = GC_URGENT; > + if (sbi->gc_thread) { > + wake_up_interruptible_all( > + &sbi->gc_thread->gc_wait_queue_head); F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); We have moved gc_urgent into F2FS_SBI set, but still we will try to access gc_wait_queue_head queue head after verifying sbi->gc_thread pointer. So we need to add s_umount here to avoid racing with remount? Thanks, > + wake_up_discard_thread(sbi, true); > + } > + } else { > + sbi->gc_mode = GC_NORMAL; > + } > + return count; > + } > + if (!strcmp(a->attr.name, "gc_idle")) { > + if (t == GC_IDLE_CB) > + sbi->gc_mode = GC_IDLE_CB; > + else if (t == GC_IDLE_GREEDY) > + sbi->gc_mode = GC_IDLE_GREEDY; > + else > + sbi->gc_mode = GC_NORMAL; > + return count; > + } > + > *ui = t; > > if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0) > f2fs_reset_iostat(sbi); > - 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); > - } > - > return count; > } > > @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time, > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time); > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time); > F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time); > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle); > -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent); > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode); > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode); > F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments); > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards); > F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity); > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-18 1:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-07 12:26 [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer Chao Yu 2018-05-07 21:36 ` Jaegeuk Kim 2018-05-08 1:56 ` Chao Yu 2018-05-08 3:17 ` Jaegeuk Kim 2018-05-08 3:21 ` Chao Yu 2018-05-18 1:51 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).