Messages in this thread | | | Date | Thu, 29 Nov 2018 09:02:39 +0530 | From | Sahitya Tummala <> | Subject | Re: [PATCH v2] f2fs: fix sbi->extent_list corruption issue |
| |
On Tue, Nov 27, 2018 at 09:42:39AM +0800, Chao Yu wrote: > On 2018/11/27 8:30, Jaegeuk Kim wrote: > > On 11/26, Sahitya Tummala wrote: > >> When there is a failure in f2fs_fill_super() after/during > >> the recovery of fsync'd nodes, it frees the current sbi and > >> retries again. This time the mount is successful, but the files > >> that got recovered before retry, still holds the extent tree, > >> whose extent nodes list is corrupted since sbi and sbi->extent_list > >> is freed up. The list_del corruption issue is observed when the > >> file system is getting unmounted and when those recoverd files extent > >> node is being freed up in the below context. > >> > >> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null) > >> <...> > >> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53! > >> task: fffffff1f46f2280 task.stack: ffffff8008068000 > >> lr : __list_del_entry_valid+0x94/0xb4 > >> pc : __list_del_entry_valid+0x94/0xb4 > >> <...> > >> Call trace: > >> __list_del_entry_valid+0x94/0xb4 > >> __release_extent_node+0xb0/0x114 > >> __free_extent_tree+0x58/0x7c > >> f2fs_shrink_extent_tree+0xdc/0x3b0 > >> f2fs_leave_shrinker+0x28/0x7c > >> f2fs_put_super+0xfc/0x1e0 > >> generic_shutdown_super+0x70/0xf4 > >> kill_block_super+0x2c/0x5c > >> kill_f2fs_super+0x44/0x50 > >> deactivate_locked_super+0x60/0x8c > >> deactivate_super+0x68/0x74 > >> cleanup_mnt+0x40/0x78 > >> __cleanup_mnt+0x1c/0x28 > >> task_work_run+0x48/0xd0 > >> do_notify_resume+0x678/0xe98 > >> work_pending+0x8/0x14 > >> > >> Fix this by cleaning up inodes, extent tree and nodes of those > >> recovered files before freeing up sbi and before next retry. > >> > >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> > >> --- > >> v2: > >> -call evict_inodes() and f2fs_shrink_extent_tree() to cleanup inodes > >> > >> fs/f2fs/f2fs.h | 1 + > >> fs/f2fs/shrinker.c | 2 +- > >> fs/f2fs/super.c | 13 ++++++++++++- > >> 3 files changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> index 1e03197..aaee63b 100644 > >> --- a/fs/f2fs/f2fs.h > >> +++ b/fs/f2fs/f2fs.h > >> @@ -3407,6 +3407,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct rb_root_cached *root, > >> bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi, > >> struct rb_root_cached *root); > >> unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink); > >> +unsigned long __count_extent_cache(struct f2fs_sb_info *sbi); > >> bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext); > >> void f2fs_drop_extent_tree(struct inode *inode); > >> unsigned int f2fs_destroy_extent_node(struct inode *inode); > >> diff --git a/fs/f2fs/shrinker.c b/fs/f2fs/shrinker.c > >> index 9e13db9..7e3c13b 100644 > >> --- a/fs/f2fs/shrinker.c > >> +++ b/fs/f2fs/shrinker.c > >> @@ -30,7 +30,7 @@ static unsigned long __count_free_nids(struct f2fs_sb_info *sbi) > >> return count > 0 ? count : 0; > >> } > >> > >> -static unsigned long __count_extent_cache(struct f2fs_sb_info *sbi) > >> +unsigned long __count_extent_cache(struct f2fs_sb_info *sbi) > >> { > >> return atomic_read(&sbi->total_zombie_tree) + > >> atomic_read(&sbi->total_ext_node); > >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >> index af58b2c..769e7b1 100644 > >> --- a/fs/f2fs/super.c > >> +++ b/fs/f2fs/super.c > >> @@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) > >> sbi->readdir_ra = 1; > >> } > >> > >> +static void f2fs_cleanup_inodes(struct f2fs_sb_info *sbi) > >> +{ > >> + struct super_block *sb = sbi->sb; > >> + > >> + sync_filesystem(sb); > > > > This writes another checkpoint, which would not be what this retrial intended. > > Actually, checkpoint will not be triggered due to SBI_POR_DOING flag check > as below: > > int f2fs_sync_fs(struct super_block *sb, int sync) > { > ... > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > return -EAGAIN; > ... > } > > And also all dirty data/node won't be persisted due to SBI_POR_DOING flag, > IIUC. >
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Thanks, Sahitya.
> Thanks, > > > How about adding a condition in f2fs_may_extent_tree() when adding extents? > > Likewise, if (shrinker is not registered) return false; > > > > > >> + shrink_dcache_sb(sb); > >> + evict_inodes(sb); > >> + f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); > >> +} > >> + > >> static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >> { > >> struct f2fs_sb_info *sbi; > >> @@ -3402,6 +3412,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >> * falls into an infinite loop in f2fs_sync_meta_pages(). > >> */ > >> truncate_inode_pages_final(META_MAPPING(sbi)); > >> + /* cleanup recovery and quota inodes */ > >> + f2fs_cleanup_inodes(sbi); > >> f2fs_unregister_sysfs(sbi); > >> free_root_inode: > >> dput(sb->s_root); > >> @@ -3445,7 +3457,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >> /* give only one another chance */ > >> if (retry) { > >> retry = false; > >> - shrink_dcache_sb(sb); > >> goto try_onemore; > >> } > >> return err; > >> -- > >> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. > >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. > > > > . > > >
-- -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
| |