Re: 答复: [PATCH v2 1/2] f2fs: fix to shrink read extent node in batches

[Chao Yu <chao@kernel.org>]

On 2024/11/25 11:11, 王秀红 (Xiuhong Wang) wrote:
> Hi Chao,
> 
> after tested in this weekend with these patch base on the orginal case, no issue reproduced, so
> Tested-by: Xiuhong Wang <xiuhong.wang@unisoc.com>

Hi Xiuhong,

Thanks for helping to test.

Thanks,

> 
> thanks!
> 
> -----邮件原件-----
> 发件人: Chao Yu <chao@kernel.org>
> 发送时间: 2024年11月22日 14:50
> 收件人: jaegeuk@kernel.org
> 抄送: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; Chao Yu <chao@kernel.org>; 王秀红 (Xiuhong Wang) <Xiuhong.Wang@unisoc.com>; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com>
> 主题: [PATCH v2 1/2] f2fs: fix to shrink read extent node in batches
> 
> 
> 注意: 这封邮件来自于外部。除非你确定邮件内容安全，否则不要点击任何链接和附件。
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> We use rwlock to protect core structure data of extent tree during its shrink, however, if there is a huge number of extent nodes in extent tree, during shrink of extent tree, it may hold rwlock for a very long time, which may trigger kernel hang issue.
> 
> This patch fixes to shrink read extent node in batches, so that, critical region of the rwlock can be shrunk to avoid its extreme long time hold.
> 
> Reported-by: Xiuhong Wang <xiuhong.wang@unisoc.com>
> Closes: https://lore.kernel.org/linux-f2fs-devel/20241112110627.1314632-1-xiuhong.wang@unisoc.com/
> Signed-off-by: Xiuhong Wang <xiuhong.wang@unisoc.com>
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> v2:
> - no updates.
>   fs/f2fs/extent_cache.c | 69 +++++++++++++++++++++++++-----------------
>   1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c index 019c1f7b7fa5..b7a6817b44b0 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -379,21 +379,22 @@ static struct extent_tree *__grab_extent_tree(struct inode *inode,  }
> 
>   static unsigned int __free_extent_tree(struct f2fs_sb_info *sbi,
> -                                       struct extent_tree *et)
> +                               struct extent_tree *et, unsigned int
> + nr_shrink)
>   {
>          struct rb_node *node, *next;
>          struct extent_node *en;
> -       unsigned int count = atomic_read(&et->node_cnt);
> +       unsigned int count;
> 
>          node = rb_first_cached(&et->root);
> -       while (node) {
> +
> +       for (count = 0; node && count < nr_shrink; count++) {
>                  next = rb_next(node);
>                  en = rb_entry(node, struct extent_node, rb_node);
>                  __release_extent_node(sbi, et, en);
>                  node = next;
>          }
> 
> -       return count - atomic_read(&et->node_cnt);
> +       return count;
>   }
> 
>   static void __drop_largest_extent(struct extent_tree *et, @@ -622,6 +623,30 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
>          return en;
>   }
> 
> +static unsigned int __destroy_extent_node(struct inode *inode,
> +                                       enum extent_type type) {
> +       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +       struct extent_tree *et = F2FS_I(inode)->extent_tree[type];
> +       unsigned int nr_shrink = type == EX_READ ?
> +                               READ_EXTENT_CACHE_SHRINK_NUMBER :
> +                               AGE_EXTENT_CACHE_SHRINK_NUMBER;
> +       unsigned int node_cnt = 0;
> +
> +       if (!et || !atomic_read(&et->node_cnt))
> +               return 0;
> +
> +       while (atomic_read(&et->node_cnt)) {
> +               write_lock(&et->lock);
> +               node_cnt += __free_extent_tree(sbi, et, nr_shrink);
> +               write_unlock(&et->lock);
> +       }
> +
> +       f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> +
> +       return node_cnt;
> +}
> +
>   static void __update_extent_tree_range(struct inode *inode,
>                          struct extent_info *tei, enum extent_type type)  { @@ -760,9 +785,6 @@ static void __update_extent_tree_range(struct inode *inode,
>                  }
>          }
> 
> -       if (is_inode_flag_set(inode, FI_NO_EXTENT))
> -               __free_extent_tree(sbi, et);
> -
>          if (et->largest_updated) {
>                  et->largest_updated = false;
>                  updated = true;
> @@ -780,6 +802,9 @@ static void __update_extent_tree_range(struct inode *inode,
>   out_read_extent_cache:
>          write_unlock(&et->lock);
> 
> +       if (is_inode_flag_set(inode, FI_NO_EXTENT))
> +               __destroy_extent_node(inode, EX_READ);
> +
>          if (updated)
>                  f2fs_mark_inode_dirty_sync(inode, true);  } @@ -942,10 +967,14 @@ static unsigned int __shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink
>          list_for_each_entry_safe(et, next, &eti->zombie_list, list) {
>                  if (atomic_read(&et->node_cnt)) {
>                          write_lock(&et->lock);
> -                       node_cnt += __free_extent_tree(sbi, et);
> +                       node_cnt += __free_extent_tree(sbi, et,
> +                                       nr_shrink - node_cnt -
> + tree_cnt);
>                          write_unlock(&et->lock);
>                  }
> -               f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
> +
> +               if (atomic_read(&et->node_cnt))
> +                       goto unlock_out;
> +
>                  list_del_init(&et->list);
>                  radix_tree_delete(&eti->extent_tree_root, et->ino);
>                  kmem_cache_free(extent_tree_slab, et); @@ -1084,23 +1113,6 @@ unsigned int f2fs_shrink_age_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink
>          return __shrink_extent_tree(sbi, nr_shrink, EX_BLOCK_AGE);  }
> 
> -static unsigned int __destroy_extent_node(struct inode *inode,
> -                                       enum extent_type type)
> -{
> -       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> -       struct extent_tree *et = F2FS_I(inode)->extent_tree[type];
> -       unsigned int node_cnt = 0;
> -
> -       if (!et || !atomic_read(&et->node_cnt))
> -               return 0;
> -
> -       write_lock(&et->lock);
> -       node_cnt = __free_extent_tree(sbi, et);
> -       write_unlock(&et->lock);
> -
> -       return node_cnt;
> -}
> -
>   void f2fs_destroy_extent_node(struct inode *inode)  {
>          __destroy_extent_node(inode, EX_READ); @@ -1109,7 +1121,6 @@ void f2fs_destroy_extent_node(struct inode *inode)
> 
>   static void __drop_extent_tree(struct inode *inode, enum extent_type type)  {
> -       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>          struct extent_tree *et = F2FS_I(inode)->extent_tree[type];
>          bool updated = false;
> 
> @@ -1117,7 +1128,6 @@ static void __drop_extent_tree(struct inode *inode, enum extent_type type)
>                  return;
> 
>          write_lock(&et->lock);
> -       __free_extent_tree(sbi, et);
>          if (type == EX_READ) {
>                  set_inode_flag(inode, FI_NO_EXTENT);
>                  if (et->largest.len) {
> @@ -1126,6 +1136,9 @@ static void __drop_extent_tree(struct inode *inode, enum extent_type type)
>                  }
>          }
>          write_unlock(&et->lock);
> +
> +       __destroy_extent_node(inode, type);
> +
>          if (updated)
>                  f2fs_mark_inode_dirty_sync(inode, true);  }
> --
> 2.40.1
>