f2fs: fix sbi->extent_list corruption issue

PATCH Superseded Sahitya Tummala 3 participants 21 messages Nov 26, 2018 – Jan 10, 2019 F2FS file system

Share & tools

Sahitya Tummala

author

2018-11-26 04:47 UTC #0 lore raw

PATCH v2 1/1 f2fs: fix sbi->extent_list corruption issue

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 <[email protected]>

---
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(-)

fs/f2fs/f2fs.h +1 -0

index 1e03197..aaee63b 100644
@@ -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);

fs/f2fs/shrinker.c +1 -1

index 9e13db9..7e3c13b 100644
@@ -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);

fs/f2fs/super.c +12 -1

index af58b2c..769e7b1 100644
@@ -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);
+	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;

Signature · 2 lines

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.

Chao Yu

Reviewed-by on v2 1/1

2018-11-26 07:17 UTC #1 lore raw

Quoted 34 lines · Sahitya Tummala

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

Quoted 4 lines

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 <[email protected]>

Looks good to me now. :)

Reviewed-by Chao Yu <[email protected]>

Thanks,

Jaegeuk Kim

maintainer

2018-11-27 00:30 UTC #2 lore raw

In reply to Sahitya Tummala

Quoted 77 lines · Sahitya Tummala

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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644

fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023

Quoted 8 lines

fs/f2fs/super.c +5 -0

@@ -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. How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 28 lines

fs/f2fs/super.c +7 -1

+	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;

Signature · 2 lines

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.

Chao Yu

2018-11-27 01:42 UTC #3 lore raw

Quoted 81 lines · Jaegeuk Kim

Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 

Quoted 7 lines

fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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,

Quoted 35 lines

How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 28 lines

fs/f2fs/super.c +7 -1

+	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;

Signature · 2 lines

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.

Sahitya Tummala

author

2018-11-29 03:32 UTC #4 lore raw

Quoted 96 lines · Chao Yu

Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...

Quoted 8 lines

	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.

Quoted 38 lines

Thanks,

Quoted 35 lines

How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 28 lines

fs/f2fs/super.c +7 -1

+	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;

Signature · 2 lines

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.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Jaegeuk Kim

maintainer

2018-11-30 20:33 UTC #5 lore raw

Quoted 107 lines · Sahitya Tummala

Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.

Quoted 5 lines

Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?

Could you try the below first?

-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;

If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.

Let me consider this as a final resolution.

Thanks,

Quoted 47 lines

Thanks,
Sahitya.

Quoted 38 lines

Thanks,

Quoted 35 lines

How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 28 lines

fs/f2fs/super.c +7 -1

+	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;

Signature · 2 lines

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.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Chao Yu

2018-12-07 09:47 UTC #6 lore raw

Quoted 119 lines · Jaegeuk Kim

Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;

Quoted 6 lines

If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.

Jaegeuk,

Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.

Thanks,

Quoted 53 lines

Thanks,

Quoted 34 lines

Thanks,
Sahitya.

Quoted 30 lines

Thanks,

Quoted 28 lines

How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 24 lines

fs/f2fs/super.c +7 -1

+	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;
 	}

Quoted 13 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

fs/f2fs/super.c +0 -0

 	return err;

Signature · 2 lines

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.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Sahitya Tummala

author

2018-12-12 03:17 UTC #7 lore raw

Quoted 125 lines · Chao Yu

Quoted 124 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.

Quoted 9 lines

Quoted 1 line
Let me consider this as a final resolution.
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.

Hi Chao,

In my case, the first boot up has a failure in recovery as below -

F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22

But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.

Thanks,

Quoted 56 lines

Thanks,

Quoted 53 lines

Thanks,

Quoted 34 lines

Thanks,
Sahitya.

Quoted 30 lines

Thanks,

Quoted 28 lines

How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;

Quoted 24 lines

fs/f2fs/super.c +7 -1

+	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;
 	}

Quoted 13 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

fs/f2fs/super.c +0 -0

 	return err;

Signature · 2 lines

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.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Chao Yu

2018-12-12 03:36 UTC #8 lore raw

Quoted 128 lines · Sahitya Tummala

Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.

Quoted 8 lines

Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,

Hi Sahitya,

Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:

commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2

Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700

f2fs: give a chance to mount again when encountering errors

This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.

But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...

Thanks,

Quoted 69 lines

In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 56 lines
Thanks,
Quoted 53 lines
Thanks,
Quoted 34 lines
Thanks,
Sahitya.
Quoted 30 lines
Thanks,
Quoted 28 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 24 lines
fs/f2fs/super.c +7 -1
+	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;
 	}
Quoted 13 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
fs/f2fs/super.c +0 -0
 	return err;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.

Sahitya Tummala

author

2018-12-14 07:56 UTC #9 lore raw

Quoted 149 lines · Chao Yu

Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors

Quoted 8 lines

This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...

Hi Jaegeuk,

How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.

Thanks,

Quoted 72 lines

Thanks,
Quoted 69 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 56 lines
Thanks,
Quoted 53 lines
Thanks,
Quoted 34 lines
Thanks,
Sahitya.
Quoted 30 lines
Thanks,
Quoted 28 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 24 lines
fs/f2fs/super.c +7 -1
+	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;
 	}
Quoted 13 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
fs/f2fs/super.c +0 -0
 	return err;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Jaegeuk Kim

maintainer

2018-12-14 14:25 UTC #10 lore raw

Quoted 150 lines · Sahitya Tummala

Quoted 149 lines · Chao Yu
Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors

Quoted 7 lines

Quoted 7 lines
This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...

The problem was there was no way to recover roll-forward area by fsck. IOWs, mount was failing all the time. I don't think roll-forward itself can corrupt the image more. Please report, if you have any issue on this.

Quoted 7 lines

Quoted 1 line
Hi Jaegeuk,
How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.

I'd like to keep retry logic, so could you please test what I suggested above?

Thanks,

Quoted 80 lines

Thanks,
Quoted 72 lines
Thanks,
Quoted 69 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 56 lines
Thanks,
Quoted 53 lines
Thanks,
Quoted 34 lines
Thanks,
Sahitya.
Quoted 30 lines
Thanks,
Quoted 28 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 24 lines
fs/f2fs/super.c +7 -1
+	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;
 	}
Quoted 13 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
fs/f2fs/super.c +0 -0
 	return err;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Chao Yu

2018-12-18 10:28 UTC #11 lore raw

Quoted 155 lines · Jaegeuk Kim

Quoted 154 lines · Sahitya Tummala
Quoted 153 lines · Chao Yu
Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors
This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...
The problem was there was no way to recover roll-forward area by fsck. IOWs, mount was failing all the time. I don't think roll-forward itself can corrupt

I got your concern, IMO, if mount fails, it will be better to let user decide how to handle it.

If mount fails due to:

1) recovery, user can run fsck and/or try disable_roll_forward or norecovery option in another mount;
2) -EINVAL caused by sanity, user can run fsck and retry mount.
3) -ENOMEM caused low memory in system, user can add more memory and retry mount.
...

Thanks,

Quoted 96 lines

the image more. Please report, if you have any issue on this.>
Quoted 7 lines
Quoted 1 line
Hi Jaegeuk,
How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.
I'd like to keep retry logic, so could you please test what I suggested above?
Thanks,
Quoted 70 lines
Thanks,
Quoted 67 lines
Thanks,
Quoted 65 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 53 lines
Thanks,
Quoted 51 lines
Thanks,
Quoted 47 lines
Thanks,
Sahitya.
Quoted 38 lines
Thanks,
Quoted 35 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 28 lines
fs/f2fs/super.c +7 -1
+	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;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 10 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.

Jaegeuk Kim

maintainer

2018-12-18 22:47 UTC #12 lore raw

Quoted 160 lines · Chao Yu

Quoted 159 lines · Jaegeuk Kim
Quoted 157 lines · Sahitya Tummala
Quoted 156 lines · Chao Yu
Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors
This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...

Quoted 5 lines

Quoted 2 lines
The problem was there was no way to recover roll-forward area by fsck. IOWs, mount was failing all the time. I don't think roll-forward itself can corrupt
I got your concern, IMO, if mount fails, it will be better to let user decide how to handle it.

Roll-forward is not based on user decision, but f2fs does internally. So, I'm in doubt we have to ask users on any failed case here.
I don't catch why we need to revert this which has been landed for a long time.

Quoted 108 lines

If mount fails due to:
1) recovery, user can run fsck and/or try disable_roll_forward or norecovery option in another mount;
2) -EINVAL caused by sanity, user can run fsck and retry mount.
3) -ENOMEM caused low memory in system, user can add more memory and retry mount.
...
Thanks,
Quoted 96 lines
the image more. Please report, if you have any issue on this.>
Quoted 7 lines
Quoted 1 line
Hi Jaegeuk,
How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.
I'd like to keep retry logic, so could you please test what I suggested above?
Thanks,
Quoted 70 lines
Thanks,
Quoted 67 lines
Thanks,
Quoted 65 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 53 lines
Thanks,
Quoted 51 lines
Thanks,
Quoted 47 lines
Thanks,
Sahitya.
Quoted 38 lines
Thanks,
Quoted 35 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 28 lines
fs/f2fs/super.c +7 -1
+	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;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 10 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.

Chao Yu

2018-12-19 03:43 UTC #13 lore raw

Quoted 161 lines · Jaegeuk Kim

Quoted 160 lines · Chao Yu
Quoted 159 lines · Jaegeuk Kim
Quoted 157 lines · Sahitya Tummala
Quoted 156 lines · Chao Yu
Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
v2:
(quoted diff) +0 -1
-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(-)
fs/f2fs/f2fs.h +1 -0
index 1e03197..aaee63b 100644
@@ -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);
fs/f2fs/shrinker.c +1 -1
index 9e13db9..7e3c13b 100644
@@ -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);
fs/f2fs/super.c +0 -0
index af58b2c..769e7b1 100644
@@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
Quoted 7 lines
fs/f2fs/super.c static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi) · line 3023
Quoted 5 lines
fs/f2fs/super.c +5 -0
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors
This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
The problem was there was no way to recover roll-forward area by fsck. IOWs, mount was failing all the time. I don't think roll-forward itself can corrupt
I got your concern, IMO, if mount fails, it will be better to let user decide how to handle it.
Roll-forward is not based on user decision, but f2fs does internally. So, I'm

Yup, IMO without roll-forward, data may lose, and posix compliance can be corrupted, f2fs should do roll-forward internally as possible as it can.

Quoted 2 lines

in doubt we have to ask users on any failed case here.
I don't catch why we need to revert this which has been landed for a long time.

Actually, what I mean is mount can fail due to different reason, but we handle it with fixed retry method by dropping recovery, it may be not flexible.

For example, first fill_super fails due to no memory, then second fill_super runs w/o recovery, if we succeed, we may lose fsynced data. I don't think it make sense.

Thanks,

Quoted 112 lines

Quoted 4 lines
If mount fails due to:
Quoted 105 lines
1) recovery, user can run fsck and/or try disable_roll_forward or norecovery option in another mount;
2) -EINVAL caused by sanity, user can run fsck and retry mount.
3) -ENOMEM caused low memory in system, user can add more memory and retry mount.
...
Thanks,
Quoted 96 lines
the image more. Please report, if you have any issue on this.>
Quoted 7 lines
Quoted 1 line
Hi Jaegeuk,
How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.
I'd like to keep retry logic, so could you please test what I suggested above?
Thanks,
Quoted 70 lines
Thanks,
Quoted 67 lines
Thanks,
Quoted 65 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 53 lines
Thanks,
Quoted 51 lines
Thanks,
Quoted 47 lines
Thanks,
Sahitya.
Quoted 38 lines
Thanks,
Quoted 35 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 28 lines
fs/f2fs/super.c +7 -1
+	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;
Signature · 2 lines
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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 10 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.
.

Jaegeuk Kim

maintainer

2018-12-19 23:42 UTC #14 lore raw

Quoted 176 lines · Chao Yu

Quoted 161 lines · Jaegeuk Kim
Quoted 160 lines · Chao Yu
Quoted 159 lines · Jaegeuk Kim
Quoted 157 lines · Sahitya Tummala
Quoted 156 lines · Chao Yu
Quoted 128 lines · Sahitya Tummala
Quoted 127 lines · Chao Yu
Quoted 125 lines · Jaegeuk Kim
Quoted 107 lines · Sahitya Tummala
Quoted 96 lines · Chao Yu
Quoted 81 lines · Jaegeuk Kim
Quoted 80 lines · Sahitya Tummala
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 <[email protected]>
---
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;
}
Quoted 7 lines
Quoted 5 lines
+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) {
...
Quoted 8 lines
	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.
Quoted 5 lines
Thanks Chao for the clarification.
Hi Jaegeuk,
Do you still have any further concerns or comments on this patch?
Could you try the below first?
-- How about adding a condition in f2fs_may_extent_tree() when adding extents? -- Likewise, if (shrinker is not registered) return false;
If we can fix what you described directly, I don't want to rely on such the assumptions saying we won't do checkpoint. This flow literally says syncing and evicting cached objects, which opposed to what we'd like to drop all caches and restart fill_super again.
Let me consider this as a final resolution.
Quoted 8 lines
Quoted 7 lines
Jaegeuk,
Still I want to ask, what kind of scenario we have to add retry logic in fill_super for? As in android scenario, it must be extreme rare case that system runs out-of-memory in boot time...at least, I didn't get any kind of report like that.
Hi Chao,
Hi Sahitya,
Thanks for letting me know that, I git-blamed the code, and found the original intention is like what you described:
commit ed2e621a95d704e6a4e904cc00524e8cbddda0c2
Author: Jaegeuk Kim <[email protected]>
Date:   Fri Aug 8 15:37:41 2014 -0700
f2fs: give a chance to mount again when encountering errors
This patch gives another chance to try mount process when we encounter
an error.
This makes an effect on the roll-forward recovery failures as well.
But I doubt that if we failed in recovery, maybe there is corruption in this image, would it be better to fail the mount, and let user fsck it and retry the mount? otherwise, the corruption may be expanded...
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
The problem was there was no way to recover roll-forward area by fsck. IOWs, mount was failing all the time. I don't think roll-forward itself can corrupt
I got your concern, IMO, if mount fails, it will be better to let user decide how to handle it.
Roll-forward is not based on user decision, but f2fs does internally. So, I'm
Yup, IMO without roll-forward, data may lose, and posix compliance can be corrupted, f2fs should do roll-forward internally as possible as it can.
Quoted 2 lines
in doubt we have to ask users on any failed case here.
I don't catch why we need to revert this which has been landed for a long time.

Quoted 6 lines

Actually, what I mean is mount can fail due to different reason, but we handle it with fixed retry method by dropping recovery, it may be not flexible.
For example, first fill_super fails due to no memory, then second fill_super runs w/o recovery, if we succeed, we may lose fsynced data. I don't think it make sense.

Then, that's another issue, and yeah, we can prepare a patch for that.

Quoted 115 lines

Thanks,
Quoted 112 lines
Quoted 4 lines
If mount fails due to:
Quoted 105 lines
1) recovery, user can run fsck and/or try disable_roll_forward or norecovery option in another mount;
2) -EINVAL caused by sanity, user can run fsck and retry mount.
3) -ENOMEM caused low memory in system, user can add more memory and retry mount.
...
Thanks,
Quoted 96 lines
the image more. Please report, if you have any issue on this.>
Quoted 7 lines
Quoted 1 line
Hi Jaegeuk,
How do you think about this? If you think it is okay, then I will fix the sbi->extent_list corruption issue, by removing the retry logic. Otherwise, I will fix it in the extent handling as you have suggested earlier.
I'd like to keep retry logic, so could you please test what I suggested above?
Thanks,
Quoted 70 lines
Thanks,
Quoted 67 lines
Thanks,
Quoted 65 lines
In my case, the first boot up has a failure in recovery as below -
F2FS-fs (mmcblk0p75): find_fsync_dnodes: detect looped node chain, blkaddr:1979471, next:1979472 F2FS-fs (mmcblk0p75): Cannot recover all fsync data errno=-22
But in the second attempt, retry will be set to false and because of that recover_fsync_data() is skipped. This helped mount to be successful in the second attempt.
Thanks,
Quoted 53 lines
Thanks,
Quoted 51 lines
Thanks,
Quoted 47 lines
Thanks,
Sahitya.
Quoted 38 lines
Thanks,
Quoted 35 lines
How about adding a condition in f2fs_may_extent_tree() when adding extents? Likewise, if (shrinker is not registered) return false;
Quoted 28 lines
+ 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.
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 10 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
.
Signature · 3 lines
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
.
.

Sahitya Tummala

author

2019-01-04 08:05 UTC #15 lore raw

In reply to Sahitya Tummala

Quoted 34 lines · Sahitya Tummala

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

Quoted 3 lines

Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.

Hi Jaegeuk, Chao,

I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?

Please share your comments.

Thanks,

Quoted 77 lines

Signed-off-by Sahitya Tummala <[email protected]>

---
v2:

(quoted diff) +0 -1

-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(-)

fs/f2fs/f2fs.h +1 -0

index 1e03197..aaee63b 100644
@@ -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);

fs/f2fs/shrinker.c +1 -1

index 9e13db9..7e3c13b 100644
@@ -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);

fs/f2fs/super.c +12 -1

index af58b2c..769e7b1 100644
@@ -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);
+	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;

Signature · 2 lines

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.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Jaegeuk Kim

maintainer

2019-01-04 20:33 UTC #16 lore raw

Quoted 38 lines · Sahitya Tummala

Quoted 34 lines · Sahitya Tummala
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
Quoted 3 lines
Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.

Quoted 8 lines

Hi Jaegeuk, Chao,
I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.

recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops its dirty list and call iput(), when there is an error. So, after then, there'd be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages in dirty inodes as well. Can we check where this dirty inode came from?

Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.

Quoted 17 lines

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Quoted 3 lines

I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?

The patch was merged in mainline already.
Could you take a look at this patch?

Quoted 1 line

From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery

Some works after roll-forward recovery can get an error which will release all the data structures. Let's flush them in order to make it clean.

One possible corruption came from:

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Reported-by Sahitya Tummala <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/f2fs/super.c +6 -2

index 547cb7459be7..bb02186293a3 100644
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	} else if (is_set_ckpt_flags(sbi, CP_DISABLED_FLAG)) {
 		f2fs_enable_checkpoint(sbi);
 	}

@@ -3370,7 +3370,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		/* After POR, we can run background GC thread.*/
 		err = f2fs_start_gc_thread(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	}
 	kvfree(options);

@@ -3392,6 +3392,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	f2fs_update_time(sbi, REQ_TIME);
 	return 0;
 
+sync_free_meta:
+	/* safe to flush all the data */
+	sync_filesystem(sbi->sb);
+
 free_meta:
 	/* flush dirty orphan inode objects */
 	f2fs_sync_inode_meta(sbi);

Signature · 1 line

2.19.0.605.g01d371f741-goog

Chao Yu

2019-01-07 02:25 UTC #17 lore raw

Quoted 48 lines · Jaegeuk Kim

Quoted 38 lines · Sahitya Tummala
Quoted 34 lines · Sahitya Tummala
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
Quoted 3 lines
Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.
Quoted 8 lines
Hi Jaegeuk, Chao,
I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.

Quoted 4 lines

recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops its dirty list and call iput(), when there is an error. So, after then, there'd be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages in dirty inodes as well. Can we check where this dirty inode came from?

I guess it comes from:

f2fs_recover_fsync_data()

	/* Needed for iput() to work correctly and not trash data */
	sbi->sb->s_flags |= SB_ACTIVE;

iput_final()

	if (!drop && (sb->s_flags & SB_ACTIVE)) {
		inode_add_lru(inode);
		spin_unlock(&inode->i_lock);
		return;
	}

So dirty data in those inode can be remained after iput(), then meta/node can be persisted during next checkpoint, if checkpoint failed due to error, dirty inode remain in system. IIUC.

Quoted 24 lines

Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.

Quoted 17 lines

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Quoted 3 lines

I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?

Quoted 7 lines

The patch was merged in mainline already.
Could you take a look at this patch?
Quoted 1 line
From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery

You mean android kernel mainline?

Thanks,

Quoted 62 lines

Some works after roll-forward recovery can get an error which will release all the data structures. Let's flush them in order to make it clean.

One possible corruption came from:

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Reported-by Sahitya Tummala <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/f2fs/super.c +6 -2

index 547cb7459be7..bb02186293a3 100644
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	} else if (is_set_ckpt_flags(sbi, CP_DISABLED_FLAG)) {
 		f2fs_enable_checkpoint(sbi);
 	}

@@ -3370,7 +3370,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		/* After POR, we can run background GC thread.*/
 		err = f2fs_start_gc_thread(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	}
 	kvfree(options);

@@ -3392,6 +3392,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	f2fs_update_time(sbi, REQ_TIME);
 	return 0;
 
+sync_free_meta:
+	/* safe to flush all the data */
+	sync_filesystem(sbi->sb);
+
 free_meta:
 	/* flush dirty orphan inode objects */
 	f2fs_sync_inode_meta(sbi);

Jaegeuk Kim

maintainer

2019-01-09 04:38 UTC #18 lore raw

Quoted 69 lines · Chao Yu

Quoted 48 lines · Jaegeuk Kim
Quoted 38 lines · Sahitya Tummala
Quoted 34 lines · Sahitya Tummala
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
Quoted 3 lines
Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.
Quoted 8 lines
Hi Jaegeuk, Chao,
I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.
Quoted 4 lines
recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops its dirty list and call iput(), when there is an error. So, after then, there'd be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages in dirty inodes as well. Can we check where this dirty inode came from?
I guess it comes from:
f2fs_recover_fsync_data()
	/* Needed for iput() to work correctly and not trash data */
	sbi->sb->s_flags |= SB_ACTIVE;
iput_final()
	if (!drop && (sb->s_flags & SB_ACTIVE)) {
		inode_add_lru(inode);
		spin_unlock(&inode->i_lock);
		return;
	}

Quoted 3 lines

So dirty data in those inode can be remained after iput(), then meta/node can be persisted during next checkpoint, if checkpoint failed due to error, dirty inode remain in system. IIUC.

749 err = recover_data(sbi, &inode_list, &tmp_inode_list, &dir_list);
750 if (!err)
751 f2fs_bug_on(sbi, !list_empty(&inode_list));
752 else {
753 /* restore s_flags to let iput() trash data */
754 sbi->sb->s_flags = s_flags;
755 }

We deactivate sb before iput?

Quoted 28 lines

Quoted 28 lines
Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.
Quoted 17 lines
[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18
Quoted 3 lines
I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?
The patch was merged in mainline already.
Could you take a look at this patch?

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery
You mean android kernel mainline?

I meant the previous patch was upstreamed. We need another patch to address this second issue.

Thanks,

Quoted 65 lines

Thanks,

Quoted 62 lines

Some works after roll-forward recovery can get an error which will release all the data structures. Let's flush them in order to make it clean.

One possible corruption came from:

[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Reported-by Sahitya Tummala <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/f2fs/super.c +6 -2

index 547cb7459be7..bb02186293a3 100644
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	} else if (is_set_ckpt_flags(sbi, CP_DISABLED_FLAG)) {
 		f2fs_enable_checkpoint(sbi);
 	}

@@ -3370,7 +3370,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		/* After POR, we can run background GC thread.*/
 		err = f2fs_start_gc_thread(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	}
 	kvfree(options);

@@ -3392,6 +3392,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	f2fs_update_time(sbi, REQ_TIME);
 	return 0;
 
+sync_free_meta:
+	/* safe to flush all the data */
+	sync_filesystem(sbi->sb);
+
 free_meta:
 	/* flush dirty orphan inode objects */
 	f2fs_sync_inode_meta(sbi);

Chao Yu

2019-01-09 06:13 UTC #19 lore raw

Quoted 83 lines · Jaegeuk Kim

Quoted 69 lines · Chao Yu
Quoted 48 lines · Jaegeuk Kim
Quoted 38 lines · Sahitya Tummala
Quoted 34 lines · Sahitya Tummala
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
Quoted 3 lines
Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.
Quoted 8 lines
Hi Jaegeuk, Chao,
I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.
Quoted 4 lines
recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops its dirty list and call iput(), when there is an error. So, after then, there'd be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages in dirty inodes as well. Can we check where this dirty inode came from?
I guess it comes from:
f2fs_recover_fsync_data()
	/* Needed for iput() to work correctly and not trash data */
	sbi->sb->s_flags |= SB_ACTIVE;
iput_final()
	if (!drop && (sb->s_flags & SB_ACTIVE)) {
		inode_add_lru(inode);
		spin_unlock(&inode->i_lock);
		return;
	}
Quoted 3 lines
So dirty data in those inode can be remained after iput(), then meta/node can be persisted during next checkpoint, if checkpoint failed due to error, dirty inode remain in system. IIUC.
749 err = recover_data(sbi, &inode_list, &tmp_inode_list, &dir_list);
750 if (!err)
751 f2fs_bug_on(sbi, !list_empty(&inode_list));
752 else {
753 /* restore s_flags to let iput() trash data */
754 sbi->sb->s_flags = s_flags;
755 }

Quoted 1 line

We deactivate sb before iput?

We only restore s_flags in error path of recover_data? I remember Sahitya said his case is encountering error in later checkpoint()?

Thanks,

Quoted 109 lines

Quoted 29 lines
Quoted 29 lines
Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.
Quoted 17 lines
[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18
Quoted 3 lines
I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?
The patch was merged in mainline already.
Could you take a look at this patch?
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery
You mean android kernel mainline?
I meant the previous patch was upstreamed. We need another patch to address this second issue.
Thanks,
Quoted 57 lines
Thanks,
Quoted 54 lines
Some works after roll-forward recovery can get an error which will release all the data structures. Let's flush them in order to make it clean.
One possible corruption came from:
[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18
Reported-by Sahitya Tummala <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
fs/f2fs/super.c +2 -2
index 547cb7459be7..bb02186293a3 100644
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	} else if (is_set_ckpt_flags(sbi, CP_DISABLED_FLAG)) {
 		f2fs_enable_checkpoint(sbi);
 	}
@@ -3370,7 +3370,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		/* After POR, we can run background GC thread.*/
 		err = f2fs_start_gc_thread(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	}
 	kvfree(options);
 
@@ -3392,6 +3392,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	f2fs_update_time(sbi, REQ_TIME);
 	return 0;
 
Quoted 8 lines
fs/f2fs/super.c static int f2fs_fill_super(struct super_block *sb, void *data, int silent) · line 3401
Quoted 8 lines
fs/f2fs/super.c +4 -0
+sync_free_meta:
+	/* safe to flush all the data */
+	sync_filesystem(sbi->sb);
+
 free_meta:
 	/* flush dirty orphan inode objects */
 	f2fs_sync_inode_meta(sbi);
.

Sahitya Tummala

author

2019-01-10 06:39 UTC #20 lore raw

Quoted 84 lines · Chao Yu

Quoted 83 lines · Jaegeuk Kim
Quoted 69 lines · Chao Yu
Quoted 48 lines · Jaegeuk Kim
Quoted 38 lines · Sahitya Tummala
Quoted 34 lines · Sahitya Tummala
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
Quoted 3 lines
Fix this by cleaning up inodes, extent tree and nodes of those recovered files before freeing up sbi and before next retry.
Quoted 8 lines
Hi Jaegeuk, Chao,
I have observed another scenario where the similar list corruption issue can happen with sbi->inode_list as well. If recover_fsync_data() fails at some point in write_checkpoint() due to some error and if those recovered inodes are still dirty, then after the mount is successful, this issue is observed when that dirty inode is under writeback.
Quoted 4 lines
recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops its dirty list and call iput(), when there is an error. So, after then, there'd be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages in dirty inodes as well. Can we check where this dirty inode came from?
I guess it comes from:
f2fs_recover_fsync_data()
	/* Needed for iput() to work correctly and not trash data */
	sbi->sb->s_flags |= SB_ACTIVE;
iput_final()
	if (!drop && (sb->s_flags & SB_ACTIVE)) {
		inode_add_lru(inode);
		spin_unlock(&inode->i_lock);
		return;
	}
Quoted 3 lines
So dirty data in those inode can be remained after iput(), then meta/node can be persisted during next checkpoint, if checkpoint failed due to error, dirty inode remain in system. IIUC.
749 err = recover_data(sbi, &inode_list, &tmp_inode_list, &dir_list);
750 if (!err)
751 f2fs_bug_on(sbi, !list_empty(&inode_list));
752 else {
753 /* restore s_flags to let iput() trash data */
754 sbi->sb->s_flags = s_flags;
755 }

Quoted 5 lines

Quoted 1 line
We deactivate sb before iput?
We only restore s_flags in error path of recover_data? I remember Sahitya said his case is encountering error in later checkpoint()?

Thanks Chao and Jaegeuk for taking a look at this. I might be wrong earlier on the point of error.
The case that Jaegeuk has addressed below looks good for now.
https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=cd2dcebde8b8088b8ad77184ee3a1e81fd3da6ba

Quoted 112 lines

Thanks,
Quoted 109 lines
Quoted 29 lines
Quoted 29 lines
Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.
Quoted 17 lines
[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18
Quoted 3 lines
I think it is better to cleanup those inodes completely before freeing sbi and before next retry as done in this patch. Would you like to re-consider this patch for this new issue?
The patch was merged in mainline already.
Could you take a look at this patch?
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery
You mean android kernel mainline?
I meant the previous patch was upstreamed. We need another patch to address this second issue.
Thanks,
Quoted 57 lines
Thanks,
Quoted 54 lines
Some works after roll-forward recovery can get an error which will release all the data structures. Let's flush them in order to make it clean.
One possible corruption came from:
[   90.400500] list_del corruption. prev->next should be ffffffed1f566208, but was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18
Reported-by Sahitya Tummala <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
fs/f2fs/super.c +2 -2
index 547cb7459be7..bb02186293a3 100644
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	} else if (is_set_ckpt_flags(sbi, CP_DISABLED_FLAG)) {
 		f2fs_enable_checkpoint(sbi);
 	}
@@ -3370,7 +3370,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		/* After POR, we can run background GC thread.*/
 		err = f2fs_start_gc_thread(sbi);
 		if (err)
-			goto free_meta;
+			goto sync_free_meta;
 	}
 	kvfree(options);
 
@@ -3392,6 +3392,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	f2fs_update_time(sbi, REQ_TIME);
 	return 0;
 
Quoted 8 lines
fs/f2fs/super.c static int f2fs_fill_super(struct super_block *sb, void *data, int silent) · line 3401
Quoted 8 lines
fs/f2fs/super.c +4 -0
+sync_free_meta:
+	/* safe to flush all the data */
+	sync_filesystem(sbi->sb);
+
 free_meta:
 	/* flush dirty orphan inode objects */
 	f2fs_sync_inode_meta(sbi);
.

Signature · 3 lines

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.