Re: [f2fs-dev] [PATCH v2] f2fs: avoid stucking GC due to atomic write
On 04/27, Chao Yu wrote: > On 2018/4/26 23:54, Jaegeuk Kim wrote: > > On 04/24, Chao Yu wrote: > >> f2fs doesn't allow abuse on atomic write class interface, so except > >> limiting in-mem pages' total memory usage capacity, we need to limit > >> atomic-write usage as well when filesystem is seriously fragmented, > >> otherwise we may run into infinite loop during foreground GC because > >> target blocks in victim segment are belong to atomic opened file for > >> long time. > > > > How about using fi->i_gc_failure likewise pin_file? > > OK, how about changing it to array fi->i_gc_failure[MAX_GC_FAILURE], and > change > the type to unsigned long long to avoid overflow? It'd be enough to share i_gc_failure between the types, IMO. > > enum { > GC_FAILURE_PIN, > GC_FAILURE_ATOMIC, > MAX_GC_FAILURE > } > > Thanks, > > > > >> > >> Now, we will detect failure due to atomic write in foreground GC, if > >> the count exceeds threshold, we will drop all atomic written data in > >> cache, by this, I expect it can keep our system running safely to > >> prevent Dos attack. > >> > >> In addition, his patch adds to show GC skip information in debugfs, > >> now it just shows count of skipped caused by atomic write. > >> > >> Signed-off-by: Chao Yu > >> --- > >> v2: > >> - add to show skip info in debugfs. > >> fs/f2fs/debug.c | 8 > >> fs/f2fs/f2fs.h| 2 ++ > >> fs/f2fs/file.c| 5 + > >> fs/f2fs/gc.c | 29 + > >> fs/f2fs/gc.h | 3 +++ > >> fs/f2fs/segment.c | 1 + > >> fs/f2fs/segment.h | 2 ++ > >> 7 files changed, 46 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c > >> index 0fbd674c66fb..607b258a9b61 100644 > >> --- a/fs/f2fs/debug.c > >> +++ b/fs/f2fs/debug.c > >> @@ -104,6 +104,10 @@ static void update_general_status(struct f2fs_sb_info > >> *sbi) > >>si->avail_nids = NM_I(sbi)->available_nids; > >>si->alloc_nids = NM_I(sbi)->nid_cnt[PREALLOC_NID]; > >>si->bg_gc = sbi->bg_gc; > >> + si->skipped_atomic_files[BG_GC] = > >> + sbi->gc_thread->skipped_atomic_files[BG_GC]; > >> + si->skipped_atomic_files[FG_GC] = > >> + sbi->gc_thread->skipped_atomic_files[FG_GC]; > >>si->util_free = (int)(free_user_blocks(sbi) >> sbi->log_blocks_per_seg) > >>* 100 / (int)(sbi->user_block_count >> sbi->log_blocks_per_seg) > >>/ 2; > >> @@ -341,6 +345,10 @@ static int stat_show(struct seq_file *s, void *v) > >>si->bg_data_blks); > >>seq_printf(s, " - node blocks : %d (%d)\n", si->node_blks, > >>si->bg_node_blks); > >> + seq_printf(s, "Skipped : atomic write %llu (%llu)\n", > >> + si->skipped_atomic_files[BG_GC] + > >> + si->skipped_atomic_files[FG_GC], > >> + si->skipped_atomic_files[BG_GC]); > >>seq_puts(s, "\nExtent Cache:\n"); > >>seq_printf(s, " - Hit Count: L1-1:%llu L1-2:%llu L2:%llu\n", > >>si->hit_largest, si->hit_cached, > >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> index 75d3b4875429..c2b92cb377c6 100644 > >> --- a/fs/f2fs/f2fs.h > >> +++ b/fs/f2fs/f2fs.h > >> @@ -2254,6 +2254,7 @@ enum { > >>FI_EXTRA_ATTR, /* indicate file has extra attribute */ > >>FI_PROJ_INHERIT,/* indicate file inherits projectid */ > >>FI_PIN_FILE,/* indicate file should not be gced */ > >> + FI_ATOMIC_REVOKE_REQUEST,/* indicate atomic committed data has been > >> dropped */ > >> }; > >> > >> static inline void __mark_inode_dirty_flag(struct inode *inode, > >> @@ -3010,6 +3011,7 @@ struct f2fs_stat_info { > >>int bg_node_segs, bg_data_segs; > >>int tot_blks, data_blks, node_blks; > >>int bg_data_blks, bg_node_blks; > >> + unsigned long long skipped_atomic_files[2]; > >>int curseg[NR_CURSEG_TYPE]; > >>int cursec[NR_CURSEG_TYPE]; > >>int curzone[NR_CURSEG_TYPE]; > >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >> index a352804af244..0cfa65c21d3f 100644 > >> --- a/fs/f2fs/file.c > >> +++ b/fs/f2fs/file.c > >> @@ -1702,6 +1702,7 @@ static int f2fs_ioc_start_atomic_write(struct file > >> *filp) > >> skip_flush: > >>set_inode_flag(inode, FI_HOT_DATA); > >>set_inode_flag(inode, FI_ATOMIC_FILE); > >> + clear_inode_flag(inode, FI_ATOMIC_REVOKE_REQUEST); > >>f2fs_update_time(F2FS_I_SB(inode), REQ_TIME); > >> > >>F2FS_I(inode)->inmem_task = current; > >> @@ -1750,6 +1751,10 @@ static int f2fs_ioc_commit_atomic_write(struct file > >> *filp) > >>ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false); > >>} > >> err_out: > >> + if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST)) { > >> + clear_inode_flag(inode, FI_ATOMIC_REVOKE_REQUEST); > >> + ret = -EINVAL; > >> + } > >>up_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
Re: [f2fs-dev] [PATCH v2] f2fs: fix to avoid race during access gc_thread pointer
On 04/27, Chao Yu wrote: > On 2018/4/27 0:03, Jaegeuk Kim wrote: > > On 04/24, Chao Yu wrote: > >> Thread A Thread BThread 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. > > > > Do we just need to check &sb->s_umount in f2fs_sbi_store() and > > Better, > > > issue_discard_thread? > > There is more cases can be called from different scenarios: > - select_policy() > - need_SSR() No? They should be blocked by remount(2). > > Thanks, > > > > >> > >> In order to fix this issue, keep gc_thread structure valid in sbi all > >> the time instead of alloc/free it dynamically. > >> > >> In addition, add a rw semaphore to exclude rw operation in fields of > >> gc_thread. > >> > >> Signed-off-by: Chao Yu > >> --- > >> v2: > >> - add a rw semaphore to exclude rw operation suggested by Jaegeuk. > >> fs/f2fs/debug.c | 3 +-- > >> fs/f2fs/f2fs.h| 9 ++- > >> fs/f2fs/gc.c | 78 > >> --- > >> fs/f2fs/gc.h | 1 + > >> fs/f2fs/segment.c | 10 +-- > >> fs/f2fs/super.c | 26 +-- > >> fs/f2fs/sysfs.c | 26 ++- > >> 7 files changed, 107 insertions(+), 46 deletions(-) > >> > >> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c > >> index a66107b5cfff..0fbd674c66fb 100644 > >> --- a/fs/f2fs/debug.c > >> +++ b/fs/f2fs/debug.c > >> @@ -221,8 +221,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi) > >>si->cache_mem = 0; > >> > >>/* build gc */ > >> - if (sbi->gc_thread) > >> - si->cache_mem += sizeof(struct f2fs_gc_kthread); > >> + si->cache_mem += sizeof(struct f2fs_gc_kthread); > >> > >>/* build merge flush thread */ > >>if (SM_I(sbi)->fcc_info) > >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> index 8f3ad9662d13..75d3b4875429 100644 > >> --- a/fs/f2fs/f2fs.h > >> +++ b/fs/f2fs/f2fs.h > >> @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct > >> f2fs_sb_info *sbi) > >>return (struct sit_info *)(SM_I(sbi)->sit_info); > >> } > >> > >> +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi) > >> +{ > >> + return (struct f2fs_gc_kthread *)(sbi->gc_thread); > >> +} > >> + > >> static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi) > >> { > >>return (struct free_segmap_info *)(SM_I(sbi)->free_info); > >> @@ -2954,8 +2959,10 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t > >> pos, size_t len); > >> /* > >> * gc.c > >> */ > >> +int init_gc_context(struct f2fs_sb_info *sbi); > >> +void destroy_gc_context(struct f2fs_sb_info * sbi); > >> int start_gc_thread(struct f2fs_sb_info *sbi); > >> -void stop_gc_thread(struct f2fs_sb_info *sbi); > >> +bool stop_gc_thread(struct f2fs_sb_info *sbi); > >> block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode); > >> int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, bool background, > >>unsigned int segno); > >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > >> index 70418b34c5f6..2febb84b2fd6 100644 > >> --- a/fs/f2fs/gc.c > >> +++ b/fs/f2fs/gc.c > >> @@ -26,8 +26,8 @@ > >> static int gc_thread_func(void *data) > >> { > >>struct f2fs_sb_info *sbi = data; > >> - struct f2fs_gc_kthread *gc_th = sbi->gc_thread; > >> - wait_queue_head_t *wq = &sbi->gc_thread->gc_wait_queue_head; > >> + struct f2fs_gc_kthread *gc_th = GC_I(sbi); > >> + wait_queue_head_t *wq = &gc_th->gc_wait_queue_head; > >>unsigned int wait_ms; > >> > >>wait_ms = gc_th->min_sleep_time; > >> @@ -114,17 +114,15 @@ static int gc_thread_func(void *data) > >>return 0; > >> } > >> > >> -int start_gc_thread(struct f2fs_sb_info *sbi) > >> +int init_gc_context(struct f2fs_sb_info *sbi) > >> { > >>struct f2fs_gc_kthread *gc_th; > >> - dev_t dev = sbi->sb->s_bdev->bd_dev; > >> - int err = 0; > >> > >>gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL); > >> - if (!gc_th) { > >> - err = -ENOMEM; > >> - goto out; > >> - } > >> + if (!gc_th) > >> + return -ENOMEM; > >> + > >> + gc_th->f2fs_gc_task = NULL; > >> > >>gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME; > >>gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME; > >> @@ -135,35 +133,59 @@ int start_gc_thread(struct f2fs_sb_info *sbi) > >>gc_th->gc_urgent = 0; > >>
Re: [f2fs-dev] [PATCH v2] f2fs: fix to avoid race during access gc_thread pointer
On 04/27, Chao Yu wrote: > On 2018/4/27 10:04, Chao Yu wrote: > > On 2018/4/27 0:03, Jaegeuk Kim wrote: > >> On 04/24, Chao Yu wrote: > >>> Thread A Thread BThread 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. > >> > >> Do we just need to check &sb->s_umount in f2fs_sbi_store() and > > > > Better, > > > >> issue_discard_thread? > > > > There is more cases can be called from different scenarios: > > - select_policy() > > - need_SSR() > > BTW, do you agree with introducing {init,destroy}_gc_context as the patch > does? I don't think so. The issue only requires mutex() in sysfs/discard_thread. > > Thanks, > > > > > Thanks, > > > >> > >>> > >>> In order to fix this issue, keep gc_thread structure valid in sbi all > >>> the time instead of alloc/free it dynamically. > >>> > >>> In addition, add a rw semaphore to exclude rw operation in fields of > >>> gc_thread. > >>> > >>> Signed-off-by: Chao Yu > >>> --- > >>> v2: > >>> - add a rw semaphore to exclude rw operation suggested by Jaegeuk. > >>> fs/f2fs/debug.c | 3 +-- > >>> fs/f2fs/f2fs.h| 9 ++- > >>> fs/f2fs/gc.c | 78 > >>> --- > >>> fs/f2fs/gc.h | 1 + > >>> fs/f2fs/segment.c | 10 +-- > >>> fs/f2fs/super.c | 26 +-- > >>> fs/f2fs/sysfs.c | 26 ++- > >>> 7 files changed, 107 insertions(+), 46 deletions(-) > >>> > >>> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c > >>> index a66107b5cfff..0fbd674c66fb 100644 > >>> --- a/fs/f2fs/debug.c > >>> +++ b/fs/f2fs/debug.c > >>> @@ -221,8 +221,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi) > >>> si->cache_mem = 0; > >>> > >>> /* build gc */ > >>> - if (sbi->gc_thread) > >>> - si->cache_mem += sizeof(struct f2fs_gc_kthread); > >>> + si->cache_mem += sizeof(struct f2fs_gc_kthread); > >>> > >>> /* build merge flush thread */ > >>> if (SM_I(sbi)->fcc_info) > >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >>> index 8f3ad9662d13..75d3b4875429 100644 > >>> --- a/fs/f2fs/f2fs.h > >>> +++ b/fs/f2fs/f2fs.h > >>> @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct > >>> f2fs_sb_info *sbi) > >>> return (struct sit_info *)(SM_I(sbi)->sit_info); > >>> } > >>> > >>> +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi) > >>> +{ > >>> + return (struct f2fs_gc_kthread *)(sbi->gc_thread); > >>> +} > >>> + > >>> static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi) > >>> { > >>> return (struct free_segmap_info *)(SM_I(sbi)->free_info); > >>> @@ -2954,8 +2959,10 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t > >>> pos, size_t len); > >>> /* > >>> * gc.c > >>> */ > >>> +int init_gc_context(struct f2fs_sb_info *sbi); > >>> +void destroy_gc_context(struct f2fs_sb_info * sbi); > >>> int start_gc_thread(struct f2fs_sb_info *sbi); > >>> -void stop_gc_thread(struct f2fs_sb_info *sbi); > >>> +bool stop_gc_thread(struct f2fs_sb_info *sbi); > >>> block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode); > >>> int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, bool background, > >>> unsigned int segno); > >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > >>> index 70418b34c5f6..2febb84b2fd6 100644 > >>> --- a/fs/f2fs/gc.c > >>> +++ b/fs/f2fs/gc.c > >>> @@ -26,8 +26,8 @@ > >>> static int gc_thread_func(void *data) > >>> { > >>> struct f2fs_sb_info *sbi = data; > >>> - struct f2fs_gc_kthread *gc_th = sbi->gc_thread; > >>> - wait_queue_head_t *wq = &sbi->gc_thread->gc_wait_queue_head; > >>> + struct f2fs_gc_kthread *gc_th = GC_I(sbi); > >>> + wait_queue_head_t *wq = &gc_th->gc_wait_queue_head; > >>> unsigned int wait_ms; > >>> > >>> wait_ms = gc_th->min_sleep_time; > >>> @@ -114,17 +114,15 @@ static int gc_thread_func(void *data) > >>> return 0; > >>> } > >>> > >>> -int start_gc_thread(struct f2fs_sb_info *sbi) > >>> +int init_gc_context(struct f2fs_sb_info *sbi) > >>> { > >>> struct f2fs_gc_kthread *gc_th; > >>> - dev_t dev = sbi->sb->s_bdev->bd_dev; > >>> - int err = 0; > >>> > >>> gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL); > >>> - if (!gc_th) { > >>> - err = -ENOMEM; > >>> - goto out; > >>> - } > >>> + if (!gc_th) > >>> + return -ENOMEM; > >>> + > >>>
Re: [f2fs-dev] [RFC PATCH] f2fs-tools: introduce tune.f2fs
On 04/27, Chao Yu wrote: > On 2018/4/27 0:13, Jaegeuk Kim wrote: > > On 04/26, Junling Zheng wrote: > >> Ping... > >> > >> On 2018/4/23 15:32, Junling Zheng wrote: > >>> Introduce tune.f2fs tool to change the f2fs parameters. > >>> Currently this tool only supports adding or removing encrypt > >>> feature bit in superblock. > > > > What is the purpose of this empty tune.f2fs? How can we say we have this > > tool to users? You have to design what kind of things to support first. > > I checked very initial tune2fs.c, it only supports very few parameters tuning > functionality, but, can not say that is a bad start to introduce the misc > tool. > > + fprintf (stderr, "Usage: %s [-c max-mounts-count] [-e > errors-behavior] " > +"[-i interval[d|m]]\n" > +"\t[-l] [-m reserved-blocks-percent] device\n", > program_name); I don't think We have to follow that. > > Maybe tuning 1. extension list, 2. multi device name later? just guess. First of all, does it make sense to unset feature bits? I don't think so. Agreed to your suggestion where: 1. feature set - enable system quota 2. extension list 3. multi device name -- which may be really big trial > > Using fsck.f2fs to tune parameter, IMO, maybe a little confuse for user to > understand the tool's functionality. > > Thanks, -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel