[PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[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 the extent tree of those recovered files
before freeing up sbi and before next retry.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
---
 fs/f2fs/extent_cache.c |  6 +++++-
 fs/f2fs/f2fs.h         |  2 +-
 fs/f2fs/inode.c        |  2 +-
 fs/f2fs/super.c        | 10 ++++++++++
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 1cb0fcc..763ba83 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -743,7 +743,7 @@ void f2fs_drop_extent_tree(struct inode *inode)
 		f2fs_mark_inode_dirty_sync(inode, true);
 }
 
-void f2fs_destroy_extent_tree(struct inode *inode)
+void f2fs_destroy_extent_tree(struct inode *inode, bool force)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct extent_tree *et = F2FS_I(inode)->extent_tree;
@@ -752,6 +752,9 @@ void f2fs_destroy_extent_tree(struct inode *inode)
 	if (!et)
 		return;
 
+	if (force)
+		goto destroy_et;
+
 	if (inode->i_nlink && !is_bad_inode(inode) &&
 					atomic_read(&et->node_cnt)) {
 		mutex_lock(&sbi->extent_tree_lock);
@@ -761,6 +764,7 @@ void f2fs_destroy_extent_tree(struct inode *inode)
 		return;
 	}
 
+destroy_et:
 	/* free all extent info belong to this extent tree */
 	node_cnt = f2fs_destroy_extent_node(inode);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 1e03197..db8a919 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3410,7 +3410,7 @@ bool f2fs_check_rb_tree_consistence(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);
-void f2fs_destroy_extent_tree(struct inode *inode);
+void f2fs_destroy_extent_tree(struct inode *inode, bool force);
 bool f2fs_lookup_extent_cache(struct inode *inode, pgoff_t pgofs,
 			struct extent_info *ei);
 void f2fs_update_extent_cache(struct dnode_of_data *dn);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 91ceee0..39e3ade3 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -649,7 +649,7 @@ void f2fs_evict_inode(struct inode *inode)
 	f2fs_bug_on(sbi, get_dirty_pages(inode));
 	f2fs_remove_dirty_inode(inode);
 
-	f2fs_destroy_extent_tree(inode);
+	f2fs_destroy_extent_tree(inode, false);
 
 	if (inode->i_nlink || is_bad_inode(inode))
 		goto no_delete;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index af58b2c..f41ac43 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3016,6 +3016,15 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
 	sbi->readdir_ra = 1;
 }
 
+void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
+{
+	struct super_block *sb = sbi->sb;
+	struct inode *inode, *next;
+
+	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
+		f2fs_destroy_extent_tree(inode, true);
+}
+
 static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct f2fs_sb_info *sbi;
@@ -3402,6 +3411,7 @@ 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));
+	f2fs_cleanup_extent_cache(sbi);
 	f2fs_unregister_sysfs(sbi);
 free_root_inode:
 	dput(sb->s_root);
-- 
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.

[PATCH 2/2] f2fs: fix memory leak of quota files extent tree and it's nodes

[Sahitya Tummala]

When there is a failure in f2fs_fill_super() after the quota is
enabled, then f2fs_quota_off_umount() is called in the error handling.
Then sbi is freed up and f2fs_fill_super() retries again.
But f2fs_quota_off_umount() doesn't guarantee that quota file's extent
tree/nodes are removed/freed. It will just add to sbi->zombie_list,
if those files are referenced. In the next retry, quota is enabled
again with the new extent tree and nodes, causing memory leak for the
previously allocated memory.

Fix this by cleaning up the sbi->zombie_list before freeing sbi and
before the next retry.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
---
 fs/f2fs/extent_cache.c | 21 +++++++++++++++++++++
 fs/f2fs/f2fs.h         |  1 +
 fs/f2fs/super.c        |  5 +++++
 3 files changed, 27 insertions(+)

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 763ba83..c2bcd88 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -629,6 +629,27 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
 		f2fs_mark_inode_dirty_sync(inode, true);
 }
 
+void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi)
+{
+	struct extent_tree *et, *next;
+
+	mutex_lock(&sbi->extent_tree_lock);
+	list_for_each_entry_safe(et, next, &sbi->zombie_list, list) {
+		if (atomic_read(&et->node_cnt)) {
+			write_lock(&et->lock);
+			__free_extent_tree(sbi, et);
+			write_unlock(&et->lock);
+		}
+		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
+		list_del_init(&et->list);
+		radix_tree_delete(&sbi->extent_tree_root, et->ino);
+		kmem_cache_free(extent_tree_slab, et);
+		atomic_dec(&sbi->total_ext_tree);
+		atomic_dec(&sbi->total_zombie_tree);
+	}
+	mutex_unlock(&sbi->extent_tree_lock);
+}
+
 unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
 {
 	struct extent_tree *et, *next;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index db8a919..6807815 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3419,6 +3419,7 @@ void f2fs_update_extent_cache_range(struct dnode_of_data *dn,
 void f2fs_init_extent_cache_info(struct f2fs_sb_info *sbi);
 int __init f2fs_create_extent_cache(void);
 void f2fs_destroy_extent_cache(void);
+void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi);
 
 /*
  * sysfs.c
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f41ac43..521fe3f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3023,6 +3023,11 @@ void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
 
 	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
 		f2fs_destroy_extent_tree(inode, true);
+
+	f2fs_cleanup_zombie_list(sbi);
+
+	f2fs_bug_on(sbi, !list_empty(&sbi->zombie_list));
+	f2fs_bug_on(sbi, !list_empty(&sbi->extent_list));
 }
 
 static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
-- 
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.

Re: [PATCH 2/2] f2fs: fix memory leak of quota files extent tree and it's nodes

[Chao Yu]

On 2018/11/22 18:59, Sahitya Tummala wrote:
> When there is a failure in f2fs_fill_super() after the quota is
> enabled, then f2fs_quota_off_umount() is called in the error handling.
> Then sbi is freed up and f2fs_fill_super() retries again.
> But f2fs_quota_off_umount() doesn't guarantee that quota file's extent
> tree/nodes are removed/freed. It will just add to sbi->zombie_list,
> if those files are referenced. In the next retry, quota is enabled
> again with the new extent tree and nodes, causing memory leak for the
> previously allocated memory.
> 
> Fix this by cleaning up the sbi->zombie_list before freeing sbi and
> before the next retry.

I guess we can fix this issue with the same way as the comment I add in
your previous patch, how do you think?

Thanks,

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/extent_cache.c | 21 +++++++++++++++++++++
>  fs/f2fs/f2fs.h         |  1 +
>  fs/f2fs/super.c        |  5 +++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 763ba83..c2bcd88 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -629,6 +629,27 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
>  		f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> +void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi)
> +{
> +	struct extent_tree *et, *next;
> +
> +	mutex_lock(&sbi->extent_tree_lock);
> +	list_for_each_entry_safe(et, next, &sbi->zombie_list, list) {
> +		if (atomic_read(&et->node_cnt)) {
> +			write_lock(&et->lock);
> +			__free_extent_tree(sbi, et);
> +			write_unlock(&et->lock);
> +		}
> +		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> +		list_del_init(&et->list);
> +		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> +		kmem_cache_free(extent_tree_slab, et);
> +		atomic_dec(&sbi->total_ext_tree);
> +		atomic_dec(&sbi->total_zombie_tree);
> +	}
> +	mutex_unlock(&sbi->extent_tree_lock);
> +}
> +
>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>  {
>  	struct extent_tree *et, *next;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index db8a919..6807815 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3419,6 +3419,7 @@ void f2fs_update_extent_cache_range(struct dnode_of_data *dn,
>  void f2fs_init_extent_cache_info(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_extent_cache(void);
>  void f2fs_destroy_extent_cache(void);
> +void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi);
>  
>  /*
>   * sysfs.c
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index f41ac43..521fe3f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3023,6 +3023,11 @@ void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
>  
>  	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
>  		f2fs_destroy_extent_tree(inode, true);
> +
> +	f2fs_cleanup_zombie_list(sbi);
> +
> +	f2fs_bug_on(sbi, !list_empty(&sbi->zombie_list));
> +	f2fs_bug_on(sbi, !list_empty(&sbi->extent_list));
>  }
>  
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> 

Re: [PATCH 2/2] f2fs: fix memory leak of quota files extent tree and it's nodes

[Chao Yu]

On 2018/11/22 18:59, Sahitya Tummala wrote:
> When there is a failure in f2fs_fill_super() after the quota is
> enabled, then f2fs_quota_off_umount() is called in the error handling.
> Then sbi is freed up and f2fs_fill_super() retries again.
> But f2fs_quota_off_umount() doesn't guarantee that quota file's extent
> tree/nodes are removed/freed. It will just add to sbi->zombie_list,
> if those files are referenced. In the next retry, quota is enabled
> again with the new extent tree and nodes, causing memory leak for the
> previously allocated memory.
> 
> Fix this by cleaning up the sbi->zombie_list before freeing sbi and
> before the next retry.

I guess we can fix this issue with the same way as the comment I add in
your previous patch, how do you think?

Thanks,

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/extent_cache.c | 21 +++++++++++++++++++++
>  fs/f2fs/f2fs.h         |  1 +
>  fs/f2fs/super.c        |  5 +++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 763ba83..c2bcd88 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -629,6 +629,27 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
>  		f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> +void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi)
> +{
> +	struct extent_tree *et, *next;
> +
> +	mutex_lock(&sbi->extent_tree_lock);
> +	list_for_each_entry_safe(et, next, &sbi->zombie_list, list) {
> +		if (atomic_read(&et->node_cnt)) {
> +			write_lock(&et->lock);
> +			__free_extent_tree(sbi, et);
> +			write_unlock(&et->lock);
> +		}
> +		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> +		list_del_init(&et->list);
> +		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> +		kmem_cache_free(extent_tree_slab, et);
> +		atomic_dec(&sbi->total_ext_tree);
> +		atomic_dec(&sbi->total_zombie_tree);
> +	}
> +	mutex_unlock(&sbi->extent_tree_lock);
> +}
> +
>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>  {
>  	struct extent_tree *et, *next;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index db8a919..6807815 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3419,6 +3419,7 @@ void f2fs_update_extent_cache_range(struct dnode_of_data *dn,
>  void f2fs_init_extent_cache_info(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_extent_cache(void);
>  void f2fs_destroy_extent_cache(void);
> +void f2fs_cleanup_zombie_list(struct f2fs_sb_info *sbi);
>  
>  /*
>   * sysfs.c
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index f41ac43..521fe3f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3023,6 +3023,11 @@ void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
>  
>  	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
>  		f2fs_destroy_extent_tree(inode, true);
> +
> +	f2fs_cleanup_zombie_list(sbi);
> +
> +	f2fs_bug_on(sbi, !list_empty(&sbi->zombie_list));
> +	f2fs_bug_on(sbi, !list_empty(&sbi->extent_list));
>  }
>  
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/22 18:59, Sahitya Tummala wrote:
> When there is a failure in f2fs_fill_super() after/during
> the recovery of fsync'd nodes, it frees the current sbi and
> retries again. This time the mount is successful, but the files
> that got recovered before retry, still holds the extent tree,
> whose extent nodes list is corrupted since sbi and sbi->extent_list
> is freed up. The list_del corruption issue is observed when the
> file system is getting unmounted and when those recoverd files extent
> node is being freed up in the below context.
> 
> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> <...>
> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> task: fffffff1f46f2280 task.stack: ffffff8008068000
> lr : __list_del_entry_valid+0x94/0xb4
> pc : __list_del_entry_valid+0x94/0xb4
> <...>
> Call trace:
> __list_del_entry_valid+0x94/0xb4
> __release_extent_node+0xb0/0x114
> __free_extent_tree+0x58/0x7c
> f2fs_shrink_extent_tree+0xdc/0x3b0
> f2fs_leave_shrinker+0x28/0x7c
> f2fs_put_super+0xfc/0x1e0
> generic_shutdown_super+0x70/0xf4
> kill_block_super+0x2c/0x5c
> kill_f2fs_super+0x44/0x50
> deactivate_locked_super+0x60/0x8c
> deactivate_super+0x68/0x74
> cleanup_mnt+0x40/0x78
> __cleanup_mnt+0x1c/0x28
> task_work_run+0x48/0xd0
> do_notify_resume+0x678/0xe98
> work_pending+0x8/0x14
> 
> Fix this by cleaning up the extent tree of those recovered files
> before freeing up sbi and before next retry.

Would it be more clear to call shrink_dcache_sb earlier to invalid all
inodes and call f2fs_shrink_extent_tree release cached entries and trees in
error path?

BTW, I don't see any benefit of retry flow in fill_super, I guess we can
avoid it to simply fill_super flow?

To Jaegeuk, how do you think?

Thanks,

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/extent_cache.c |  6 +++++-
>  fs/f2fs/f2fs.h         |  2 +-
>  fs/f2fs/inode.c        |  2 +-
>  fs/f2fs/super.c        | 10 ++++++++++
>  4 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 1cb0fcc..763ba83 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -743,7 +743,7 @@ void f2fs_drop_extent_tree(struct inode *inode)
>  		f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> -void f2fs_destroy_extent_tree(struct inode *inode)
> +void f2fs_destroy_extent_tree(struct inode *inode, bool force)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	struct extent_tree *et = F2FS_I(inode)->extent_tree;
> @@ -752,6 +752,9 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  	if (!et)
>  		return;
>  
> +	if (force)
> +		goto destroy_et;
> +
>  	if (inode->i_nlink && !is_bad_inode(inode) &&
>  					atomic_read(&et->node_cnt)) {
>  		mutex_lock(&sbi->extent_tree_lock);
> @@ -761,6 +764,7 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  		return;
>  	}
>  
> +destroy_et:
>  	/* free all extent info belong to this extent tree */
>  	node_cnt = f2fs_destroy_extent_node(inode);
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 1e03197..db8a919 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3410,7 +3410,7 @@ bool f2fs_check_rb_tree_consistence(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);
> -void f2fs_destroy_extent_tree(struct inode *inode);
> +void f2fs_destroy_extent_tree(struct inode *inode, bool force);
>  bool f2fs_lookup_extent_cache(struct inode *inode, pgoff_t pgofs,
>  			struct extent_info *ei);
>  void f2fs_update_extent_cache(struct dnode_of_data *dn);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 91ceee0..39e3ade3 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -649,7 +649,7 @@ void f2fs_evict_inode(struct inode *inode)
>  	f2fs_bug_on(sbi, get_dirty_pages(inode));
>  	f2fs_remove_dirty_inode(inode);
>  
> -	f2fs_destroy_extent_tree(inode);
> +	f2fs_destroy_extent_tree(inode, false);
>  
>  	if (inode->i_nlink || is_bad_inode(inode))
>  		goto no_delete;
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index af58b2c..f41ac43 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3016,6 +3016,15 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
>  	sbi->readdir_ra = 1;
>  }
>  
> +void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
> +{
> +	struct super_block *sb = sbi->sb;
> +	struct inode *inode, *next;
> +
> +	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
> +		f2fs_destroy_extent_tree(inode, true);
> +}
> +
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  {
>  	struct f2fs_sb_info *sbi;
> @@ -3402,6 +3411,7 @@ 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));
> +	f2fs_cleanup_extent_cache(sbi);
>  	f2fs_unregister_sysfs(sbi);
>  free_root_inode:
>  	dput(sb->s_root);
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/22 18:59, Sahitya Tummala wrote:
> When there is a failure in f2fs_fill_super() after/during
> the recovery of fsync'd nodes, it frees the current sbi and
> retries again. This time the mount is successful, but the files
> that got recovered before retry, still holds the extent tree,
> whose extent nodes list is corrupted since sbi and sbi->extent_list
> is freed up. The list_del corruption issue is observed when the
> file system is getting unmounted and when those recoverd files extent
> node is being freed up in the below context.
> 
> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> <...>
> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> task: fffffff1f46f2280 task.stack: ffffff8008068000
> lr : __list_del_entry_valid+0x94/0xb4
> pc : __list_del_entry_valid+0x94/0xb4
> <...>
> Call trace:
> __list_del_entry_valid+0x94/0xb4
> __release_extent_node+0xb0/0x114
> __free_extent_tree+0x58/0x7c
> f2fs_shrink_extent_tree+0xdc/0x3b0
> f2fs_leave_shrinker+0x28/0x7c
> f2fs_put_super+0xfc/0x1e0
> generic_shutdown_super+0x70/0xf4
> kill_block_super+0x2c/0x5c
> kill_f2fs_super+0x44/0x50
> deactivate_locked_super+0x60/0x8c
> deactivate_super+0x68/0x74
> cleanup_mnt+0x40/0x78
> __cleanup_mnt+0x1c/0x28
> task_work_run+0x48/0xd0
> do_notify_resume+0x678/0xe98
> work_pending+0x8/0x14
> 
> Fix this by cleaning up the extent tree of those recovered files
> before freeing up sbi and before next retry.

Would it be more clear to call shrink_dcache_sb earlier to invalid all
inodes and call f2fs_shrink_extent_tree release cached entries and trees in
error path?

BTW, I don't see any benefit of retry flow in fill_super, I guess we can
avoid it to simply fill_super flow?

To Jaegeuk, how do you think?

Thanks,

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/extent_cache.c |  6 +++++-
>  fs/f2fs/f2fs.h         |  2 +-
>  fs/f2fs/inode.c        |  2 +-
>  fs/f2fs/super.c        | 10 ++++++++++
>  4 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 1cb0fcc..763ba83 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -743,7 +743,7 @@ void f2fs_drop_extent_tree(struct inode *inode)
>  		f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> -void f2fs_destroy_extent_tree(struct inode *inode)
> +void f2fs_destroy_extent_tree(struct inode *inode, bool force)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	struct extent_tree *et = F2FS_I(inode)->extent_tree;
> @@ -752,6 +752,9 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  	if (!et)
>  		return;
>  
> +	if (force)
> +		goto destroy_et;
> +
>  	if (inode->i_nlink && !is_bad_inode(inode) &&
>  					atomic_read(&et->node_cnt)) {
>  		mutex_lock(&sbi->extent_tree_lock);
> @@ -761,6 +764,7 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  		return;
>  	}
>  
> +destroy_et:
>  	/* free all extent info belong to this extent tree */
>  	node_cnt = f2fs_destroy_extent_node(inode);
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 1e03197..db8a919 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3410,7 +3410,7 @@ bool f2fs_check_rb_tree_consistence(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);
> -void f2fs_destroy_extent_tree(struct inode *inode);
> +void f2fs_destroy_extent_tree(struct inode *inode, bool force);
>  bool f2fs_lookup_extent_cache(struct inode *inode, pgoff_t pgofs,
>  			struct extent_info *ei);
>  void f2fs_update_extent_cache(struct dnode_of_data *dn);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 91ceee0..39e3ade3 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -649,7 +649,7 @@ void f2fs_evict_inode(struct inode *inode)
>  	f2fs_bug_on(sbi, get_dirty_pages(inode));
>  	f2fs_remove_dirty_inode(inode);
>  
> -	f2fs_destroy_extent_tree(inode);
> +	f2fs_destroy_extent_tree(inode, false);
>  
>  	if (inode->i_nlink || is_bad_inode(inode))
>  		goto no_delete;
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index af58b2c..f41ac43 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3016,6 +3016,15 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
>  	sbi->readdir_ra = 1;
>  }
>  
> +void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
> +{
> +	struct super_block *sb = sbi->sb;
> +	struct inode *inode, *next;
> +
> +	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
> +		f2fs_destroy_extent_tree(inode, true);
> +}
> +
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  {
>  	struct f2fs_sb_info *sbi;
> @@ -3402,6 +3411,7 @@ 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));
> +	f2fs_cleanup_extent_cache(sbi);
>  	f2fs_unregister_sysfs(sbi);
>  free_root_inode:
>  	dput(sb->s_root);
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Jaegeuk Kim]

On 11/22, Chao Yu wrote:
> On 2018/11/22 18:59, Sahitya Tummala wrote:
> > When there is a failure in f2fs_fill_super() after/during
> > the recovery of fsync'd nodes, it frees the current sbi and
> > retries again. This time the mount is successful, but the files
> > that got recovered before retry, still holds the extent tree,
> > whose extent nodes list is corrupted since sbi and sbi->extent_list
> > is freed up. The list_del corruption issue is observed when the
> > file system is getting unmounted and when those recoverd files extent
> > node is being freed up in the below context.
> > 
> > list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> > <...>
> > kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> > task: fffffff1f46f2280 task.stack: ffffff8008068000
> > lr : __list_del_entry_valid+0x94/0xb4
> > pc : __list_del_entry_valid+0x94/0xb4
> > <...>
> > Call trace:
> > __list_del_entry_valid+0x94/0xb4
> > __release_extent_node+0xb0/0x114
> > __free_extent_tree+0x58/0x7c
> > f2fs_shrink_extent_tree+0xdc/0x3b0
> > f2fs_leave_shrinker+0x28/0x7c
> > f2fs_put_super+0xfc/0x1e0
> > generic_shutdown_super+0x70/0xf4
> > kill_block_super+0x2c/0x5c
> > kill_f2fs_super+0x44/0x50
> > deactivate_locked_super+0x60/0x8c
> > deactivate_super+0x68/0x74
> > cleanup_mnt+0x40/0x78
> > __cleanup_mnt+0x1c/0x28
> > task_work_run+0x48/0xd0
> > do_notify_resume+0x678/0xe98
> > work_pending+0x8/0x14
> > 
> > Fix this by cleaning up the extent tree of those recovered files
> > before freeing up sbi and before next retry.
> 
> Would it be more clear to call shrink_dcache_sb earlier to invalid all
> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> error path?

Agreed.

> 
> BTW, I don't see any benefit of retry flow in fill_super, I guess we can
> avoid it to simply fill_super flow?

I expect it can avoid another mount failure.

> 
> To Jaegeuk, how do you think?
> 
> Thanks,
> 
> > 
> > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > ---
> >  fs/f2fs/extent_cache.c |  6 +++++-
> >  fs/f2fs/f2fs.h         |  2 +-
> >  fs/f2fs/inode.c        |  2 +-
> >  fs/f2fs/super.c        | 10 ++++++++++
> >  4 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> > index 1cb0fcc..763ba83 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -743,7 +743,7 @@ void f2fs_drop_extent_tree(struct inode *inode)
> >  		f2fs_mark_inode_dirty_sync(inode, true);
> >  }
> >  
> > -void f2fs_destroy_extent_tree(struct inode *inode)
> > +void f2fs_destroy_extent_tree(struct inode *inode, bool force)
> >  {
> >  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >  	struct extent_tree *et = F2FS_I(inode)->extent_tree;
> > @@ -752,6 +752,9 @@ void f2fs_destroy_extent_tree(struct inode *inode)
> >  	if (!et)
> >  		return;
> >  
> > +	if (force)
> > +		goto destroy_et;
> > +
> >  	if (inode->i_nlink && !is_bad_inode(inode) &&
> >  					atomic_read(&et->node_cnt)) {
> >  		mutex_lock(&sbi->extent_tree_lock);
> > @@ -761,6 +764,7 @@ void f2fs_destroy_extent_tree(struct inode *inode)
> >  		return;
> >  	}
> >  
> > +destroy_et:
> >  	/* free all extent info belong to this extent tree */
> >  	node_cnt = f2fs_destroy_extent_node(inode);
> >  
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 1e03197..db8a919 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3410,7 +3410,7 @@ bool f2fs_check_rb_tree_consistence(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);
> > -void f2fs_destroy_extent_tree(struct inode *inode);
> > +void f2fs_destroy_extent_tree(struct inode *inode, bool force);
> >  bool f2fs_lookup_extent_cache(struct inode *inode, pgoff_t pgofs,
> >  			struct extent_info *ei);
> >  void f2fs_update_extent_cache(struct dnode_of_data *dn);
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 91ceee0..39e3ade3 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -649,7 +649,7 @@ void f2fs_evict_inode(struct inode *inode)
> >  	f2fs_bug_on(sbi, get_dirty_pages(inode));
> >  	f2fs_remove_dirty_inode(inode);
> >  
> > -	f2fs_destroy_extent_tree(inode);
> > +	f2fs_destroy_extent_tree(inode, false);
> >  
> >  	if (inode->i_nlink || is_bad_inode(inode))
> >  		goto no_delete;
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index af58b2c..f41ac43 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -3016,6 +3016,15 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
> >  	sbi->readdir_ra = 1;
> >  }
> >  
> > +void f2fs_cleanup_extent_cache(struct f2fs_sb_info *sbi)
> > +{
> > +	struct super_block *sb = sbi->sb;
> > +	struct inode *inode, *next;
> > +
> > +	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list)
> > +		f2fs_destroy_extent_tree(inode, true);
> > +}
> > +
> >  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  {
> >  	struct f2fs_sb_info *sbi;
> > @@ -3402,6 +3411,7 @@ 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));
> > +	f2fs_cleanup_extent_cache(sbi);
> >  	f2fs_unregister_sysfs(sbi);
> >  free_root_inode:
> >  	dput(sb->s_root);
> > 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Sahitya Tummala]

On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
> On 11/22, Chao Yu wrote:
> > On 2018/11/22 18:59, Sahitya Tummala wrote:
> > > When there is a failure in f2fs_fill_super() after/during
> > > the recovery of fsync'd nodes, it frees the current sbi and
> > > retries again. This time the mount is successful, but the files
> > > that got recovered before retry, still holds the extent tree,
> > > whose extent nodes list is corrupted since sbi and sbi->extent_list
> > > is freed up. The list_del corruption issue is observed when the
> > > file system is getting unmounted and when those recoverd files extent
> > > node is being freed up in the below context.
> > > 
> > > list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> > > <...>
> > > kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> > > task: fffffff1f46f2280 task.stack: ffffff8008068000
> > > lr : __list_del_entry_valid+0x94/0xb4
> > > pc : __list_del_entry_valid+0x94/0xb4
> > > <...>
> > > Call trace:
> > > __list_del_entry_valid+0x94/0xb4
> > > __release_extent_node+0xb0/0x114
> > > __free_extent_tree+0x58/0x7c
> > > f2fs_shrink_extent_tree+0xdc/0x3b0
> > > f2fs_leave_shrinker+0x28/0x7c
> > > f2fs_put_super+0xfc/0x1e0
> > > generic_shutdown_super+0x70/0xf4
> > > kill_block_super+0x2c/0x5c
> > > kill_f2fs_super+0x44/0x50
> > > deactivate_locked_super+0x60/0x8c
> > > deactivate_super+0x68/0x74
> > > cleanup_mnt+0x40/0x78
> > > __cleanup_mnt+0x1c/0x28
> > > task_work_run+0x48/0xd0
> > > do_notify_resume+0x678/0xe98
> > > work_pending+0x8/0x14
> > > 
> > > Fix this by cleaning up the extent tree of those recovered files
> > > before freeing up sbi and before next retry.
> > 
> > Would it be more clear to call shrink_dcache_sb earlier to invalid all
> > inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> > error path?
> 
> Agreed.
> 
I have tried doing shrink_dcache_sb() earlier but that doesn't call
f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
the cached extent nodes. However, I see that extent tree is left intact for
those recovered files, which should not be a problem as it gets freed as part
of next umount/rm. Only one small problem I see with this is - during rm/umount when
those previoulsy recovered files are being evicted, extent tree memory gets
free'd but the counter sbi->total_ext_tree gets invalid as these recovered
files are not present as part of current sbi->extent_tree_root. So i have come
up with this patch below to fix this. Let me know if this looks good?

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 1cb0fcc..3e4801e 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
 		}
 		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
 		list_del_init(&et->list);
-		radix_tree_delete(&sbi->extent_tree_root, et->ino);
+		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
+			atomic_dec(&sbi->total_ext_tree);
 		kmem_cache_free(extent_tree_slab, et);
-		atomic_dec(&sbi->total_ext_tree);
 		atomic_dec(&sbi->total_zombie_tree);
 		tree_cnt++;
 
@@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
 	/* delete extent tree entry in radix tree */
 	mutex_lock(&sbi->extent_tree_lock);
 	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
-	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
+	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
+		atomic_dec(&sbi->total_ext_tree);
 	kmem_cache_free(extent_tree_slab, et);
 	atomic_dec(&sbi->total_ext_tree);
 	mutex_unlock(&sbi->extent_tree_lock);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index af58b2c..3e5588f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto free_root_inode;
 
+	f2fs_join_shrinker(sbi);
 #ifdef CONFIG_QUOTA
 	/* Enable quota usage during mount */
 	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
@@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 			sbi->valid_super_block ? 1 : 2, err);
 	}
 
-	f2fs_join_shrinker(sbi);
-
 	f2fs_tuning_parameters(sbi);
 
 	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
@@ -3402,6 +3401,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));
+	shrink_dcache_sb(sb);
+	f2fs_leave_shrinker(sbi);
 	f2fs_unregister_sysfs(sbi);
 free_root_inode:
 	dput(sb->s_root);
@@ -3445,7 +3446,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;

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

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Sahitya Tummala]

On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
> On 11/22, Chao Yu wrote:
> > On 2018/11/22 18:59, Sahitya Tummala wrote:
> > > When there is a failure in f2fs_fill_super() after/during
> > > the recovery of fsync'd nodes, it frees the current sbi and
> > > retries again. This time the mount is successful, but the files
> > > that got recovered before retry, still holds the extent tree,
> > > whose extent nodes list is corrupted since sbi and sbi->extent_list
> > > is freed up. The list_del corruption issue is observed when the
> > > file system is getting unmounted and when those recoverd files extent
> > > node is being freed up in the below context.
> > > 
> > > list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> > > <...>
> > > kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> > > task: fffffff1f46f2280 task.stack: ffffff8008068000
> > > lr : __list_del_entry_valid+0x94/0xb4
> > > pc : __list_del_entry_valid+0x94/0xb4
> > > <...>
> > > Call trace:
> > > __list_del_entry_valid+0x94/0xb4
> > > __release_extent_node+0xb0/0x114
> > > __free_extent_tree+0x58/0x7c
> > > f2fs_shrink_extent_tree+0xdc/0x3b0
> > > f2fs_leave_shrinker+0x28/0x7c
> > > f2fs_put_super+0xfc/0x1e0
> > > generic_shutdown_super+0x70/0xf4
> > > kill_block_super+0x2c/0x5c
> > > kill_f2fs_super+0x44/0x50
> > > deactivate_locked_super+0x60/0x8c
> > > deactivate_super+0x68/0x74
> > > cleanup_mnt+0x40/0x78
> > > __cleanup_mnt+0x1c/0x28
> > > task_work_run+0x48/0xd0
> > > do_notify_resume+0x678/0xe98
> > > work_pending+0x8/0x14
> > > 
> > > Fix this by cleaning up the extent tree of those recovered files
> > > before freeing up sbi and before next retry.
> > 
> > Would it be more clear to call shrink_dcache_sb earlier to invalid all
> > inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> > error path?
> 
> Agreed.
> 
I have tried doing shrink_dcache_sb() earlier but that doesn't call
f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
the cached extent nodes. However, I see that extent tree is left intact for
those recovered files, which should not be a problem as it gets freed as part
of next umount/rm. Only one small problem I see with this is - during rm/umount when
those previoulsy recovered files are being evicted, extent tree memory gets
free'd but the counter sbi->total_ext_tree gets invalid as these recovered
files are not present as part of current sbi->extent_tree_root. So i have come
up with this patch below to fix this. Let me know if this looks good?

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 1cb0fcc..3e4801e 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
 		}
 		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
 		list_del_init(&et->list);
-		radix_tree_delete(&sbi->extent_tree_root, et->ino);
+		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
+			atomic_dec(&sbi->total_ext_tree);
 		kmem_cache_free(extent_tree_slab, et);
-		atomic_dec(&sbi->total_ext_tree);
 		atomic_dec(&sbi->total_zombie_tree);
 		tree_cnt++;
 
@@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
 	/* delete extent tree entry in radix tree */
 	mutex_lock(&sbi->extent_tree_lock);
 	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
-	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
+	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
+		atomic_dec(&sbi->total_ext_tree);
 	kmem_cache_free(extent_tree_slab, et);
 	atomic_dec(&sbi->total_ext_tree);
 	mutex_unlock(&sbi->extent_tree_lock);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index af58b2c..3e5588f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto free_root_inode;
 
+	f2fs_join_shrinker(sbi);
 #ifdef CONFIG_QUOTA
 	/* Enable quota usage during mount */
 	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
@@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 			sbi->valid_super_block ? 1 : 2, err);
 	}
 
-	f2fs_join_shrinker(sbi);
-
 	f2fs_tuning_parameters(sbi);
 
 	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
@@ -3402,6 +3401,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));
+	shrink_dcache_sb(sb);
+	f2fs_leave_shrinker(sbi);
 	f2fs_unregister_sysfs(sbi);
 free_root_inode:
 	dput(sb->s_root);
@@ -3445,7 +3446,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;

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

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/23 11:42, Sahitya Tummala wrote:
> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
>> On 11/22, Chao Yu wrote:
>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
>>>> When there is a failure in f2fs_fill_super() after/during
>>>> the recovery of fsync'd nodes, it frees the current sbi and
>>>> retries again. This time the mount is successful, but the files
>>>> that got recovered before retry, still holds the extent tree,
>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
>>>> is freed up. The list_del corruption issue is observed when the
>>>> file system is getting unmounted and when those recoverd files extent
>>>> node is being freed up in the below context.
>>>>
>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
>>>> <...>
>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
>>>> lr : __list_del_entry_valid+0x94/0xb4
>>>> pc : __list_del_entry_valid+0x94/0xb4
>>>> <...>
>>>> Call trace:
>>>> __list_del_entry_valid+0x94/0xb4
>>>> __release_extent_node+0xb0/0x114
>>>> __free_extent_tree+0x58/0x7c
>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
>>>> f2fs_leave_shrinker+0x28/0x7c
>>>> f2fs_put_super+0xfc/0x1e0
>>>> generic_shutdown_super+0x70/0xf4
>>>> kill_block_super+0x2c/0x5c
>>>> kill_f2fs_super+0x44/0x50
>>>> deactivate_locked_super+0x60/0x8c
>>>> deactivate_super+0x68/0x74
>>>> cleanup_mnt+0x40/0x78
>>>> __cleanup_mnt+0x1c/0x28
>>>> task_work_run+0x48/0xd0
>>>> do_notify_resume+0x678/0xe98
>>>> work_pending+0x8/0x14
>>>>
>>>> Fix this by cleaning up the extent tree of those recovered files
>>>> before freeing up sbi and before next retry.
>>>
>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
>>> error path?
>>
>> Agreed.
>>
> I have tried doing shrink_dcache_sb() earlier but that doesn't call
> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
> the cached extent nodes. However, I see that extent tree is left intact for

I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
f2fs_leave_shrinker, for those recovered files, their extent nodes were
evicted, but their extent trees are still in cache?

> those recovered files, which should not be a problem as it gets freed as part
> of next umount/rm. Only one small problem I see with this is - during rm/umount when
> those previoulsy recovered files are being evicted, extent tree memory gets
> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
> files are not present as part of current sbi->extent_tree_root. So i have come
> up with this patch below to fix this. Let me know if this looks good?
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 1cb0fcc..3e4801e 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>  		}
>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>  		list_del_init(&et->list);
> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
> +			atomic_dec(&sbi->total_ext_tree);
>  		kmem_cache_free(extent_tree_slab, et);
> -		atomic_dec(&sbi->total_ext_tree);
>  		atomic_dec(&sbi->total_zombie_tree);
>  		tree_cnt++;
>  
> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  	/* delete extent tree entry in radix tree */
>  	mutex_lock(&sbi->extent_tree_lock);
>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
> +		atomic_dec(&sbi->total_ext_tree);
>  	kmem_cache_free(extent_tree_slab, et);
>  	atomic_dec(&sbi->total_ext_tree);
>  	mutex_unlock(&sbi->extent_tree_lock);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index af58b2c..3e5588f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (err)
>  		goto free_root_inode;
>  
> +	f2fs_join_shrinker(sbi);
>  #ifdef CONFIG_QUOTA
>  	/* Enable quota usage during mount */
>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  			sbi->valid_super_block ? 1 : 2, err);
>  	}
>  
> -	f2fs_join_shrinker(sbi);
> -
>  	f2fs_tuning_parameters(sbi);
>  
>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
> @@ -3402,6 +3401,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));
> +	shrink_dcache_sb(sb);
> +	f2fs_leave_shrinker(sbi);

Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?

Thanks,

>  	f2fs_unregister_sysfs(sbi);
>  free_root_inode:
>  	dput(sb->s_root);
> @@ -3445,7 +3446,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;
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/23 11:42, Sahitya Tummala wrote:
> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
>> On 11/22, Chao Yu wrote:
>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
>>>> When there is a failure in f2fs_fill_super() after/during
>>>> the recovery of fsync'd nodes, it frees the current sbi and
>>>> retries again. This time the mount is successful, but the files
>>>> that got recovered before retry, still holds the extent tree,
>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
>>>> is freed up. The list_del corruption issue is observed when the
>>>> file system is getting unmounted and when those recoverd files extent
>>>> node is being freed up in the below context.
>>>>
>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
>>>> <...>
>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
>>>> lr : __list_del_entry_valid+0x94/0xb4
>>>> pc : __list_del_entry_valid+0x94/0xb4
>>>> <...>
>>>> Call trace:
>>>> __list_del_entry_valid+0x94/0xb4
>>>> __release_extent_node+0xb0/0x114
>>>> __free_extent_tree+0x58/0x7c
>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
>>>> f2fs_leave_shrinker+0x28/0x7c
>>>> f2fs_put_super+0xfc/0x1e0
>>>> generic_shutdown_super+0x70/0xf4
>>>> kill_block_super+0x2c/0x5c
>>>> kill_f2fs_super+0x44/0x50
>>>> deactivate_locked_super+0x60/0x8c
>>>> deactivate_super+0x68/0x74
>>>> cleanup_mnt+0x40/0x78
>>>> __cleanup_mnt+0x1c/0x28
>>>> task_work_run+0x48/0xd0
>>>> do_notify_resume+0x678/0xe98
>>>> work_pending+0x8/0x14
>>>>
>>>> Fix this by cleaning up the extent tree of those recovered files
>>>> before freeing up sbi and before next retry.
>>>
>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
>>> error path?
>>
>> Agreed.
>>
> I have tried doing shrink_dcache_sb() earlier but that doesn't call
> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
> the cached extent nodes. However, I see that extent tree is left intact for

I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
f2fs_leave_shrinker, for those recovered files, their extent nodes were
evicted, but their extent trees are still in cache?

> those recovered files, which should not be a problem as it gets freed as part
> of next umount/rm. Only one small problem I see with this is - during rm/umount when
> those previoulsy recovered files are being evicted, extent tree memory gets
> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
> files are not present as part of current sbi->extent_tree_root. So i have come
> up with this patch below to fix this. Let me know if this looks good?
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 1cb0fcc..3e4801e 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>  		}
>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>  		list_del_init(&et->list);
> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
> +			atomic_dec(&sbi->total_ext_tree);
>  		kmem_cache_free(extent_tree_slab, et);
> -		atomic_dec(&sbi->total_ext_tree);
>  		atomic_dec(&sbi->total_zombie_tree);
>  		tree_cnt++;
>  
> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>  	/* delete extent tree entry in radix tree */
>  	mutex_lock(&sbi->extent_tree_lock);
>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
> +		atomic_dec(&sbi->total_ext_tree);
>  	kmem_cache_free(extent_tree_slab, et);
>  	atomic_dec(&sbi->total_ext_tree);
>  	mutex_unlock(&sbi->extent_tree_lock);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index af58b2c..3e5588f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (err)
>  		goto free_root_inode;
>  
> +	f2fs_join_shrinker(sbi);
>  #ifdef CONFIG_QUOTA
>  	/* Enable quota usage during mount */
>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  			sbi->valid_super_block ? 1 : 2, err);
>  	}
>  
> -	f2fs_join_shrinker(sbi);
> -
>  	f2fs_tuning_parameters(sbi);
>  
>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
> @@ -3402,6 +3401,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));
> +	shrink_dcache_sb(sb);
> +	f2fs_leave_shrinker(sbi);

Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?

Thanks,

>  	f2fs_unregister_sysfs(sbi);
>  free_root_inode:
>  	dput(sb->s_root);
> @@ -3445,7 +3446,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;
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Sahitya Tummala]

On Fri, Nov 23, 2018 at 05:52:16PM +0800, Chao Yu wrote:
> On 2018/11/23 11:42, Sahitya Tummala wrote:
> > On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
> >> On 11/22, Chao Yu wrote:
> >>> On 2018/11/22 18:59, Sahitya Tummala wrote:
> >>>> When there is a failure in f2fs_fill_super() after/during
> >>>> the recovery of fsync'd nodes, it frees the current sbi and
> >>>> retries again. This time the mount is successful, but the files
> >>>> that got recovered before retry, still holds the extent tree,
> >>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
> >>>> is freed up. The list_del corruption issue is observed when the
> >>>> file system is getting unmounted and when those recoverd files extent
> >>>> node is being freed up in the below context.
> >>>>
> >>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> >>>> <...>
> >>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> >>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
> >>>> lr : __list_del_entry_valid+0x94/0xb4
> >>>> pc : __list_del_entry_valid+0x94/0xb4
> >>>> <...>
> >>>> Call trace:
> >>>> __list_del_entry_valid+0x94/0xb4
> >>>> __release_extent_node+0xb0/0x114
> >>>> __free_extent_tree+0x58/0x7c
> >>>> f2fs_shrink_extent_tree+0xdc/0x3b0
> >>>> f2fs_leave_shrinker+0x28/0x7c
> >>>> f2fs_put_super+0xfc/0x1e0
> >>>> generic_shutdown_super+0x70/0xf4
> >>>> kill_block_super+0x2c/0x5c
> >>>> kill_f2fs_super+0x44/0x50
> >>>> deactivate_locked_super+0x60/0x8c
> >>>> deactivate_super+0x68/0x74
> >>>> cleanup_mnt+0x40/0x78
> >>>> __cleanup_mnt+0x1c/0x28
> >>>> task_work_run+0x48/0xd0
> >>>> do_notify_resume+0x678/0xe98
> >>>> work_pending+0x8/0x14
> >>>>
> >>>> Fix this by cleaning up the extent tree of those recovered files
> >>>> before freeing up sbi and before next retry.
> >>>
> >>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
> >>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> >>> error path?
> >>
> >> Agreed.
> >>
> > I have tried doing shrink_dcache_sb() earlier but that doesn't call
> > f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
> > tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
> > the cached extent nodes. However, I see that extent tree is left intact for
> 
> I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
> f2fs_leave_shrinker, for those recovered files, their extent nodes were
> evicted, but their extent trees are still in cache?
> 

Yes, only extent tree is present with zero extent nodes as
f2fs_leave_shrinker() is only clearing the exntent nodes from
sbi->extent_list.

> > those recovered files, which should not be a problem as it gets freed as part
> > of next umount/rm. Only one small problem I see with this is - during rm/umount when
> > those previoulsy recovered files are being evicted, extent tree memory gets
> > free'd but the counter sbi->total_ext_tree gets invalid as these recovered
> > files are not present as part of current sbi->extent_tree_root. So i have come
> > up with this patch below to fix this. Let me know if this looks good?
> > 
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> > index 1cb0fcc..3e4801e 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
> >  		}
> >  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> >  		list_del_init(&et->list);
> > -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> > +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
> > +			atomic_dec(&sbi->total_ext_tree);
> >  		kmem_cache_free(extent_tree_slab, et);
> > -		atomic_dec(&sbi->total_ext_tree);
> >  		atomic_dec(&sbi->total_zombie_tree);
> >  		tree_cnt++;
> >  
> > @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
> >  	/* delete extent tree entry in radix tree */
> >  	mutex_lock(&sbi->extent_tree_lock);
> >  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> > -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
> > +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
> > +		atomic_dec(&sbi->total_ext_tree);
> >  	kmem_cache_free(extent_tree_slab, et);
> >  	atomic_dec(&sbi->total_ext_tree);
> >  	mutex_unlock(&sbi->extent_tree_lock);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index af58b2c..3e5588f 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  	if (err)
> >  		goto free_root_inode;
> >  
> > +	f2fs_join_shrinker(sbi);
> >  #ifdef CONFIG_QUOTA
> >  	/* Enable quota usage during mount */
> >  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
> > @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  			sbi->valid_super_block ? 1 : 2, err);
> >  	}
> >  
> > -	f2fs_join_shrinker(sbi);
> > -
> >  	f2fs_tuning_parameters(sbi);
> >  
> >  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
> > @@ -3402,6 +3401,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));
> > +	shrink_dcache_sb(sb);
> > +	f2fs_leave_shrinker(sbi);
> 
> Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?
> 

Sure, will update in the next patchset.

> Thanks,
> 
> >  	f2fs_unregister_sysfs(sbi);
> >  free_root_inode:
> >  	dput(sb->s_root);
> > @@ -3445,7 +3446,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;
> > 
> 

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

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/23 18:19, Sahitya Tummala wrote:
> On Fri, Nov 23, 2018 at 05:52:16PM +0800, Chao Yu wrote:
>> On 2018/11/23 11:42, Sahitya Tummala wrote:
>>> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
>>>> On 11/22, Chao Yu wrote:
>>>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
>>>>>> When there is a failure in f2fs_fill_super() after/during
>>>>>> the recovery of fsync'd nodes, it frees the current sbi and
>>>>>> retries again. This time the mount is successful, but the files
>>>>>> that got recovered before retry, still holds the extent tree,
>>>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
>>>>>> is freed up. The list_del corruption issue is observed when the
>>>>>> file system is getting unmounted and when those recoverd files extent
>>>>>> node is being freed up in the below context.
>>>>>>
>>>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
>>>>>> <...>
>>>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
>>>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
>>>>>> lr : __list_del_entry_valid+0x94/0xb4
>>>>>> pc : __list_del_entry_valid+0x94/0xb4
>>>>>> <...>
>>>>>> Call trace:
>>>>>> __list_del_entry_valid+0x94/0xb4
>>>>>> __release_extent_node+0xb0/0x114
>>>>>> __free_extent_tree+0x58/0x7c
>>>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
>>>>>> f2fs_leave_shrinker+0x28/0x7c
>>>>>> f2fs_put_super+0xfc/0x1e0
>>>>>> generic_shutdown_super+0x70/0xf4
>>>>>> kill_block_super+0x2c/0x5c
>>>>>> kill_f2fs_super+0x44/0x50
>>>>>> deactivate_locked_super+0x60/0x8c
>>>>>> deactivate_super+0x68/0x74
>>>>>> cleanup_mnt+0x40/0x78
>>>>>> __cleanup_mnt+0x1c/0x28
>>>>>> task_work_run+0x48/0xd0
>>>>>> do_notify_resume+0x678/0xe98
>>>>>> work_pending+0x8/0x14
>>>>>>
>>>>>> Fix this by cleaning up the extent tree of those recovered files
>>>>>> before freeing up sbi and before next retry.
>>>>>
>>>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
>>>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
>>>>> error path?
>>>>
>>>> Agreed.
>>>>
>>> I have tried doing shrink_dcache_sb() earlier but that doesn't call
>>> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
>>> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
>>> the cached extent nodes. However, I see that extent tree is left intact for
>>
>> I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
>> f2fs_leave_shrinker, for those recovered files, their extent nodes were
>> evicted, but their extent trees are still in cache?
>>
> 
> Yes, only extent tree is present with zero extent nodes as
> f2fs_leave_shrinker() is only clearing the exntent nodes from
> sbi->extent_list.

Oh, recovered inodes are in cache due to we didn't call evict_inodes, so
they are still referenced to extent tree...

How about calling evict_inodes after shrink_dcache_sb?

Thanks,

> 
>>> those recovered files, which should not be a problem as it gets freed as part
>>> of next umount/rm. Only one small problem I see with this is - during rm/umount when
>>> those previoulsy recovered files are being evicted, extent tree memory gets
>>> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
>>> files are not present as part of current sbi->extent_tree_root. So i have come
>>> up with this patch below to fix this. Let me know if this looks good?
>>>
>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>> index 1cb0fcc..3e4801e 100644
>>> --- a/fs/f2fs/extent_cache.c
>>> +++ b/fs/f2fs/extent_cache.c
>>> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>>>  		}
>>>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>>>  		list_del_init(&et->list);
>>> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
>>> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
>>> +			atomic_dec(&sbi->total_ext_tree);
>>>  		kmem_cache_free(extent_tree_slab, et);
>>> -		atomic_dec(&sbi->total_ext_tree);
>>>  		atomic_dec(&sbi->total_zombie_tree);
>>>  		tree_cnt++;
>>>  
>>> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>>>  	/* delete extent tree entry in radix tree */
>>>  	mutex_lock(&sbi->extent_tree_lock);
>>>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>>> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
>>> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
>>> +		atomic_dec(&sbi->total_ext_tree);
>>>  	kmem_cache_free(extent_tree_slab, et);
>>>  	atomic_dec(&sbi->total_ext_tree);
>>>  	mutex_unlock(&sbi->extent_tree_lock);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index af58b2c..3e5588f 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  	if (err)
>>>  		goto free_root_inode;
>>>  
>>> +	f2fs_join_shrinker(sbi);
>>>  #ifdef CONFIG_QUOTA
>>>  	/* Enable quota usage during mount */
>>>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
>>> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  			sbi->valid_super_block ? 1 : 2, err);
>>>  	}
>>>  
>>> -	f2fs_join_shrinker(sbi);
>>> -
>>>  	f2fs_tuning_parameters(sbi);
>>>  
>>>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
>>> @@ -3402,6 +3401,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));
>>> +	shrink_dcache_sb(sb);
>>> +	f2fs_leave_shrinker(sbi);
>>
>> Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?
>>
> 
> Sure, will update in the next patchset.
> 
>> Thanks,
>>
>>>  	f2fs_unregister_sysfs(sbi);
>>>  free_root_inode:
>>>  	dput(sb->s_root);
>>> @@ -3445,7 +3446,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;
>>>
>>
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Chao Yu]

On 2018/11/23 18:19, Sahitya Tummala wrote:
> On Fri, Nov 23, 2018 at 05:52:16PM +0800, Chao Yu wrote:
>> On 2018/11/23 11:42, Sahitya Tummala wrote:
>>> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
>>>> On 11/22, Chao Yu wrote:
>>>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
>>>>>> When there is a failure in f2fs_fill_super() after/during
>>>>>> the recovery of fsync'd nodes, it frees the current sbi and
>>>>>> retries again. This time the mount is successful, but the files
>>>>>> that got recovered before retry, still holds the extent tree,
>>>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
>>>>>> is freed up. The list_del corruption issue is observed when the
>>>>>> file system is getting unmounted and when those recoverd files extent
>>>>>> node is being freed up in the below context.
>>>>>>
>>>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
>>>>>> <...>
>>>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
>>>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
>>>>>> lr : __list_del_entry_valid+0x94/0xb4
>>>>>> pc : __list_del_entry_valid+0x94/0xb4
>>>>>> <...>
>>>>>> Call trace:
>>>>>> __list_del_entry_valid+0x94/0xb4
>>>>>> __release_extent_node+0xb0/0x114
>>>>>> __free_extent_tree+0x58/0x7c
>>>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
>>>>>> f2fs_leave_shrinker+0x28/0x7c
>>>>>> f2fs_put_super+0xfc/0x1e0
>>>>>> generic_shutdown_super+0x70/0xf4
>>>>>> kill_block_super+0x2c/0x5c
>>>>>> kill_f2fs_super+0x44/0x50
>>>>>> deactivate_locked_super+0x60/0x8c
>>>>>> deactivate_super+0x68/0x74
>>>>>> cleanup_mnt+0x40/0x78
>>>>>> __cleanup_mnt+0x1c/0x28
>>>>>> task_work_run+0x48/0xd0
>>>>>> do_notify_resume+0x678/0xe98
>>>>>> work_pending+0x8/0x14
>>>>>>
>>>>>> Fix this by cleaning up the extent tree of those recovered files
>>>>>> before freeing up sbi and before next retry.
>>>>>
>>>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
>>>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
>>>>> error path?
>>>>
>>>> Agreed.
>>>>
>>> I have tried doing shrink_dcache_sb() earlier but that doesn't call
>>> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
>>> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
>>> the cached extent nodes. However, I see that extent tree is left intact for
>>
>> I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
>> f2fs_leave_shrinker, for those recovered files, their extent nodes were
>> evicted, but their extent trees are still in cache?
>>
> 
> Yes, only extent tree is present with zero extent nodes as
> f2fs_leave_shrinker() is only clearing the exntent nodes from
> sbi->extent_list.

Oh, recovered inodes are in cache due to we didn't call evict_inodes, so
they are still referenced to extent tree...

How about calling evict_inodes after shrink_dcache_sb?

Thanks,

> 
>>> those recovered files, which should not be a problem as it gets freed as part
>>> of next umount/rm. Only one small problem I see with this is - during rm/umount when
>>> those previoulsy recovered files are being evicted, extent tree memory gets
>>> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
>>> files are not present as part of current sbi->extent_tree_root. So i have come
>>> up with this patch below to fix this. Let me know if this looks good?
>>>
>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>> index 1cb0fcc..3e4801e 100644
>>> --- a/fs/f2fs/extent_cache.c
>>> +++ b/fs/f2fs/extent_cache.c
>>> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
>>>  		}
>>>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>>>  		list_del_init(&et->list);
>>> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
>>> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
>>> +			atomic_dec(&sbi->total_ext_tree);
>>>  		kmem_cache_free(extent_tree_slab, et);
>>> -		atomic_dec(&sbi->total_ext_tree);
>>>  		atomic_dec(&sbi->total_zombie_tree);
>>>  		tree_cnt++;
>>>  
>>> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
>>>  	/* delete extent tree entry in radix tree */
>>>  	mutex_lock(&sbi->extent_tree_lock);
>>>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>>> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
>>> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
>>> +		atomic_dec(&sbi->total_ext_tree);
>>>  	kmem_cache_free(extent_tree_slab, et);
>>>  	atomic_dec(&sbi->total_ext_tree);
>>>  	mutex_unlock(&sbi->extent_tree_lock);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index af58b2c..3e5588f 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  	if (err)
>>>  		goto free_root_inode;
>>>  
>>> +	f2fs_join_shrinker(sbi);
>>>  #ifdef CONFIG_QUOTA
>>>  	/* Enable quota usage during mount */
>>>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
>>> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  			sbi->valid_super_block ? 1 : 2, err);
>>>  	}
>>>  
>>> -	f2fs_join_shrinker(sbi);
>>> -
>>>  	f2fs_tuning_parameters(sbi);
>>>  
>>>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
>>> @@ -3402,6 +3401,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));
>>> +	shrink_dcache_sb(sb);
>>> +	f2fs_leave_shrinker(sbi);
>>
>> Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?
>>
> 
> Sure, will update in the next patchset.
> 
>> Thanks,
>>
>>>  	f2fs_unregister_sysfs(sbi);
>>>  free_root_inode:
>>>  	dput(sb->s_root);
>>> @@ -3445,7 +3446,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;
>>>
>>
> 

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Sahitya Tummala]

On Sat, Nov 24, 2018 at 05:36:53PM +0800, Chao Yu wrote:
> On 2018/11/23 18:19, Sahitya Tummala wrote:
> > On Fri, Nov 23, 2018 at 05:52:16PM +0800, Chao Yu wrote:
> >> On 2018/11/23 11:42, Sahitya Tummala wrote:
> >>> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
> >>>> On 11/22, Chao Yu wrote:
> >>>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
> >>>>>> When there is a failure in f2fs_fill_super() after/during
> >>>>>> the recovery of fsync'd nodes, it frees the current sbi and
> >>>>>> retries again. This time the mount is successful, but the files
> >>>>>> that got recovered before retry, still holds the extent tree,
> >>>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
> >>>>>> is freed up. The list_del corruption issue is observed when the
> >>>>>> file system is getting unmounted and when those recoverd files extent
> >>>>>> node is being freed up in the below context.
> >>>>>>
> >>>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> >>>>>> <...>
> >>>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> >>>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
> >>>>>> lr : __list_del_entry_valid+0x94/0xb4
> >>>>>> pc : __list_del_entry_valid+0x94/0xb4
> >>>>>> <...>
> >>>>>> Call trace:
> >>>>>> __list_del_entry_valid+0x94/0xb4
> >>>>>> __release_extent_node+0xb0/0x114
> >>>>>> __free_extent_tree+0x58/0x7c
> >>>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
> >>>>>> f2fs_leave_shrinker+0x28/0x7c
> >>>>>> f2fs_put_super+0xfc/0x1e0
> >>>>>> generic_shutdown_super+0x70/0xf4
> >>>>>> kill_block_super+0x2c/0x5c
> >>>>>> kill_f2fs_super+0x44/0x50
> >>>>>> deactivate_locked_super+0x60/0x8c
> >>>>>> deactivate_super+0x68/0x74
> >>>>>> cleanup_mnt+0x40/0x78
> >>>>>> __cleanup_mnt+0x1c/0x28
> >>>>>> task_work_run+0x48/0xd0
> >>>>>> do_notify_resume+0x678/0xe98
> >>>>>> work_pending+0x8/0x14
> >>>>>>
> >>>>>> Fix this by cleaning up the extent tree of those recovered files
> >>>>>> before freeing up sbi and before next retry.
> >>>>>
> >>>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
> >>>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> >>>>> error path?
> >>>>
> >>>> Agreed.
> >>>>
> >>> I have tried doing shrink_dcache_sb() earlier but that doesn't call
> >>> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
> >>> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
> >>> the cached extent nodes. However, I see that extent tree is left intact for
> >>
> >> I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
> >> f2fs_leave_shrinker, for those recovered files, their extent nodes were
> >> evicted, but their extent trees are still in cache?
> >>
> > 
> > Yes, only extent tree is present with zero extent nodes as
> > f2fs_leave_shrinker() is only clearing the exntent nodes from
> > sbi->extent_list.
> 
> Oh, recovered inodes are in cache due to we didn't call evict_inodes, so
> they are still referenced to extent tree...
> 
> How about calling evict_inodes after shrink_dcache_sb?
> 

Thanks Chao for your suggestion. Yes, calling evict_inodes() is working fine.
But before doing evict_inodes(), we should also do sync_filesystem() otherwise
we hit this f2fs_bug_on in f2fs_evict_inode():
        if (likely(!is_set_ckpt_flags(sbi, CP_ERROR_FLAG)))
	                f2fs_bug_on(sbi, is_inode_flag_set(inode,
			FI_DIRTY_INODE));

I will post the updated patchset for review.

Thanks.

> Thanks,
> 
> > 
> >>> those recovered files, which should not be a problem as it gets freed as part
> >>> of next umount/rm. Only one small problem I see with this is - during rm/umount when
> >>> those previoulsy recovered files are being evicted, extent tree memory gets
> >>> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
> >>> files are not present as part of current sbi->extent_tree_root. So i have come
> >>> up with this patch below to fix this. Let me know if this looks good?
> >>>
> >>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >>> index 1cb0fcc..3e4801e 100644
> >>> --- a/fs/f2fs/extent_cache.c
> >>> +++ b/fs/f2fs/extent_cache.c
> >>> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
> >>>  		}
> >>>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> >>>  		list_del_init(&et->list);
> >>> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> >>> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
> >>> +			atomic_dec(&sbi->total_ext_tree);
> >>>  		kmem_cache_free(extent_tree_slab, et);
> >>> -		atomic_dec(&sbi->total_ext_tree);
> >>>  		atomic_dec(&sbi->total_zombie_tree);
> >>>  		tree_cnt++;
> >>>  
> >>> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
> >>>  	/* delete extent tree entry in radix tree */
> >>>  	mutex_lock(&sbi->extent_tree_lock);
> >>>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> >>> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
> >>> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
> >>> +		atomic_dec(&sbi->total_ext_tree);
> >>>  	kmem_cache_free(extent_tree_slab, et);
> >>>  	atomic_dec(&sbi->total_ext_tree);
> >>>  	mutex_unlock(&sbi->extent_tree_lock);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index af58b2c..3e5588f 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >>>  	if (err)
> >>>  		goto free_root_inode;
> >>>  
> >>> +	f2fs_join_shrinker(sbi);
> >>>  #ifdef CONFIG_QUOTA
> >>>  	/* Enable quota usage during mount */
> >>>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
> >>> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >>>  			sbi->valid_super_block ? 1 : 2, err);
> >>>  	}
> >>>  
> >>> -	f2fs_join_shrinker(sbi);
> >>> -
> >>>  	f2fs_tuning_parameters(sbi);
> >>>  
> >>>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
> >>> @@ -3402,6 +3401,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));
> >>> +	shrink_dcache_sb(sb);
> >>> +	f2fs_leave_shrinker(sbi);
> >>
> >> Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?
> >>
> > 
> > Sure, will update in the next patchset.
> > 
> >> Thanks,
> >>
> >>>  	f2fs_unregister_sysfs(sbi);
> >>>  free_root_inode:
> >>>  	dput(sb->s_root);
> >>> @@ -3445,7 +3446,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;
> >>>
> >>
> > 
> 

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

Re: [PATCH 1/2] f2fs: fix sbi->extent_list corruption issue

[Sahitya Tummala]

On Sat, Nov 24, 2018 at 05:36:53PM +0800, Chao Yu wrote:
> On 2018/11/23 18:19, Sahitya Tummala wrote:
> > On Fri, Nov 23, 2018 at 05:52:16PM +0800, Chao Yu wrote:
> >> On 2018/11/23 11:42, Sahitya Tummala wrote:
> >>> On Thu, Nov 22, 2018 at 04:11:07AM -0800, Jaegeuk Kim wrote:
> >>>> On 11/22, Chao Yu wrote:
> >>>>> On 2018/11/22 18:59, Sahitya Tummala wrote:
> >>>>>> When there is a failure in f2fs_fill_super() after/during
> >>>>>> the recovery of fsync'd nodes, it frees the current sbi and
> >>>>>> retries again. This time the mount is successful, but the files
> >>>>>> that got recovered before retry, still holds the extent tree,
> >>>>>> whose extent nodes list is corrupted since sbi and sbi->extent_list
> >>>>>> is freed up. The list_del corruption issue is observed when the
> >>>>>> file system is getting unmounted and when those recoverd files extent
> >>>>>> node is being freed up in the below context.
> >>>>>>
> >>>>>> list_del corruption. prev->next should be fffffff1e1ef5480, but was (null)
> >>>>>> <...>
> >>>>>> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> >>>>>> task: fffffff1f46f2280 task.stack: ffffff8008068000
> >>>>>> lr : __list_del_entry_valid+0x94/0xb4
> >>>>>> pc : __list_del_entry_valid+0x94/0xb4
> >>>>>> <...>
> >>>>>> Call trace:
> >>>>>> __list_del_entry_valid+0x94/0xb4
> >>>>>> __release_extent_node+0xb0/0x114
> >>>>>> __free_extent_tree+0x58/0x7c
> >>>>>> f2fs_shrink_extent_tree+0xdc/0x3b0
> >>>>>> f2fs_leave_shrinker+0x28/0x7c
> >>>>>> f2fs_put_super+0xfc/0x1e0
> >>>>>> generic_shutdown_super+0x70/0xf4
> >>>>>> kill_block_super+0x2c/0x5c
> >>>>>> kill_f2fs_super+0x44/0x50
> >>>>>> deactivate_locked_super+0x60/0x8c
> >>>>>> deactivate_super+0x68/0x74
> >>>>>> cleanup_mnt+0x40/0x78
> >>>>>> __cleanup_mnt+0x1c/0x28
> >>>>>> task_work_run+0x48/0xd0
> >>>>>> do_notify_resume+0x678/0xe98
> >>>>>> work_pending+0x8/0x14
> >>>>>>
> >>>>>> Fix this by cleaning up the extent tree of those recovered files
> >>>>>> before freeing up sbi and before next retry.
> >>>>>
> >>>>> Would it be more clear to call shrink_dcache_sb earlier to invalid all
> >>>>> inodes and call f2fs_shrink_extent_tree release cached entries and trees in
> >>>>> error path?
> >>>>
> >>>> Agreed.
> >>>>
> >>> I have tried doing shrink_dcache_sb() earlier but that doesn't call
> >>> f2fs_shrink_extent_tree(). So I have moved f2fs_join_shrinker() earlier and 
> >>> tried calling f2fs_leave_shrinker() in the error path. That helps to clean up
> >>> the cached extent nodes. However, I see that extent tree is left intact for
> >>
> >> I didn't get it, you mean, in error path, after we call shrink_dcache_sb &
> >> f2fs_leave_shrinker, for those recovered files, their extent nodes were
> >> evicted, but their extent trees are still in cache?
> >>
> > 
> > Yes, only extent tree is present with zero extent nodes as
> > f2fs_leave_shrinker() is only clearing the exntent nodes from
> > sbi->extent_list.
> 
> Oh, recovered inodes are in cache due to we didn't call evict_inodes, so
> they are still referenced to extent tree...
> 
> How about calling evict_inodes after shrink_dcache_sb?
> 

Thanks Chao for your suggestion. Yes, calling evict_inodes() is working fine.
But before doing evict_inodes(), we should also do sync_filesystem() otherwise
we hit this f2fs_bug_on in f2fs_evict_inode():
        if (likely(!is_set_ckpt_flags(sbi, CP_ERROR_FLAG)))
	                f2fs_bug_on(sbi, is_inode_flag_set(inode,
			FI_DIRTY_INODE));

I will post the updated patchset for review.

Thanks.

> Thanks,
> 
> > 
> >>> those recovered files, which should not be a problem as it gets freed as part
> >>> of next umount/rm. Only one small problem I see with this is - during rm/umount when
> >>> those previoulsy recovered files are being evicted, extent tree memory gets
> >>> free'd but the counter sbi->total_ext_tree gets invalid as these recovered
> >>> files are not present as part of current sbi->extent_tree_root. So i have come
> >>> up with this patch below to fix this. Let me know if this looks good?
> >>>
> >>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >>> index 1cb0fcc..3e4801e 100644
> >>> --- a/fs/f2fs/extent_cache.c
> >>> +++ b/fs/f2fs/extent_cache.c
> >>> @@ -654,9 +654,9 @@ unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
> >>>  		}
> >>>  		f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> >>>  		list_del_init(&et->list);
> >>> -		radix_tree_delete(&sbi->extent_tree_root, et->ino);
> >>> +		if (radix_tree_delete(&sbi->extent_tree_root, et->ino))
> >>> +			atomic_dec(&sbi->total_ext_tree);
> >>>  		kmem_cache_free(extent_tree_slab, et);
> >>> -		atomic_dec(&sbi->total_ext_tree);
> >>>  		atomic_dec(&sbi->total_zombie_tree);
> >>>  		tree_cnt++;
> >>>  
> >>> @@ -767,7 +767,8 @@ void f2fs_destroy_extent_tree(struct inode *inode)
> >>>  	/* delete extent tree entry in radix tree */
> >>>  	mutex_lock(&sbi->extent_tree_lock);
> >>>  	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> >>> -	radix_tree_delete(&sbi->extent_tree_root, inode->i_ino);
> >>> +	if (radix_tree_delete(&sbi->extent_tree_root, inode->i_ino))
> >>> +		atomic_dec(&sbi->total_ext_tree);
> >>>  	kmem_cache_free(extent_tree_slab, et);
> >>>  	atomic_dec(&sbi->total_ext_tree);
> >>>  	mutex_unlock(&sbi->extent_tree_lock);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index af58b2c..3e5588f 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -3295,6 +3295,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >>>  	if (err)
> >>>  		goto free_root_inode;
> >>>  
> >>> +	f2fs_join_shrinker(sbi);
> >>>  #ifdef CONFIG_QUOTA
> >>>  	/* Enable quota usage during mount */
> >>>  	if (f2fs_sb_has_quota_ino(sb) && !f2fs_readonly(sb)) {
> >>> @@ -3379,8 +3380,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >>>  			sbi->valid_super_block ? 1 : 2, err);
> >>>  	}
> >>>  
> >>> -	f2fs_join_shrinker(sbi);
> >>> -
> >>>  	f2fs_tuning_parameters(sbi);
> >>>  
> >>>  	f2fs_msg(sbi->sb, KERN_NOTICE, "Mounted with checkpoint version = %llx",
> >>> @@ -3402,6 +3401,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));
> >>> +	shrink_dcache_sb(sb);
> >>> +	f2fs_leave_shrinker(sbi);
> >>
> >> Why not just calling f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi)); ?
> >>
> > 
> > Sure, will update in the next patchset.
> > 
> >> Thanks,
> >>
> >>>  	f2fs_unregister_sysfs(sbi);
> >>>  free_root_inode:
> >>>  	dput(sb->s_root);
> >>> @@ -3445,7 +3446,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;
> >>>
> >>
> > 
> 

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