Re: [f2fs-dev] 答复: [PATCH v4] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu <chao@kernel.org>]

On 2024/6/25 16:09, 牛志国 (Zhiguo Niu) wrote:
> 
> 
> -----邮件原件-----
> 发件人: Chao Yu <chao@kernel.org>
> 发送时间: 2024年6月25日 14:55
> 收件人: jaegeuk@kernel.org
> 抄送: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; Chao Yu <chao@kernel.org>; wangzijie <wangzijie1@honor.com>; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com>; Yunlei He <heyunlei@hihonor.com>
> 主题: [PATCH v4] f2fs: reduce expensive checkpoint trigger frequency
> 
> 
> 注意: 这封邮件来自于外部。除非你确定邮件内容安全，否则不要点击任何链接和附件。
> 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 may trigger high frequent checkpoint for below case:
> 1. mkdir /mnt/dir1; set dir1 encrypted
> 2. touch /mnt/file1; fsync /mnt/file1
> 3. mkdir /mnt/dir2; set dir2 encrypted
> 4. touch /mnt/file2; fsync /mnt/file2
> ...
> 
> Although, newly created dir and file are not related, due to commit bbf156f7afa7 ("f2fs: fix lost xattrs of directories"), we will trigger checkpoint whenever fsync() comes after a new encrypted dir created.
> 
> In order to avoid such performance regression issue, let's record an entry including directory's ino in global cache whenever we update directory's xattr data, and then triggerring checkpoint() only if xattr metadata of target file's parent was updated.
> 
> This patch updates to cover below no encryption case as well:
> 1) parent is checkpointed
> 2) set_xattr(dir) w/ new xnid
> 3) create(file)
> 4) fsync(file)
> 
> Fixes: bbf156f7afa7 ("f2fs: fix lost xattrs of directories")
> Reported-by: wangzijie <wangzijie1@honor.com>
> Reported-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Tested-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Reported-by: Yunlei He <heyunlei@hihonor.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>   fs/f2fs/f2fs.h              |  2 ++
>   fs/f2fs/file.c              |  3 +++
>   fs/f2fs/xattr.c             | 14 ++++++++++++--
>   include/trace/events/f2fs.h |  3 ++-
>   4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index f1d65ee3addf..f3c910b8983b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -284,6 +284,7 @@ enum {
>          APPEND_INO,             /* for append ino list */
>          UPDATE_INO,             /* for update ino list */
>          TRANS_DIR_INO,          /* for transactions dir ino list */
> +       ENC_DIR_INO,            /* for encrypted dir ino list */
>          FLUSH_INO,              /* for multiple device flushing */
>          MAX_INO_ENTRY,          /* max. list */
>   };
> @@ -1150,6 +1151,7 @@ enum cp_reason_type {
>          CP_FASTBOOT_MODE,
>          CP_SPEC_LOG_NUM,
>          CP_RECOVER_DIR,
> +       CP_ENC_DIR,
>   };
> 
>   enum iostat_type {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index a527de1e7a2f..278573974db4 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -217,6 +217,9 @@ static inline enum cp_reason_type need_do_checkpoint(struct inode *inode)
>                  f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>                                                          TRANS_DIR_INO))
>                  cp_reason = CP_RECOVER_DIR;
> +       else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
> +                                                       ENC_DIR_INO))
> +               cp_reason = CP_ENC_DIR;
> 
>          return cp_reason;
>   }
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c index f290fe9327c4..d04c0b47a4e4 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -629,6 +629,7 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>                          const char *name, const void *value, size_t size,
>                          struct page *ipage, int flags)  {
> +       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>          struct f2fs_xattr_entry *here, *last;
>          void *base_addr, *last_base_addr;
>          int found, newsize;
> @@ -772,9 +773,18 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>          if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>                          !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>                  f2fs_set_encrypted_inode(inode);
> -       if (S_ISDIR(inode->i_mode))
> -               set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
> 
> +       if (!S_ISDIR(inode->i_mode))
> +               goto same;
> +       /*
> +        * In restrict mode, fsync() always try to trigger checkpoint for all
> +        * metadata consistency, in other mode, it triggers checkpoint when
> +        * parent's xattr metadata was updated.
> +        */
> +       if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT)
> +               set_sbi_flag(sbi, SBI_NEED_CP);
> Hi Chao,
> For this case, will it also cause the same issue with original issue when fsync_mode == FSYNC_MODE_STRICT ?
> if ckpt thread is blocked by some reasons and SBI_NEED_CP is not cleared in time, Subsequent fsync will trigger cp?

Hi Zhiguo,

If there is no dirty data after previous CP, later CP triggered by subsequent
fsync will return directly?

	if (!is_sbi_flag_set(sbi, SBI_IS_DIRTY) &&
		((cpc->reason & CP_FASTBOOT) || (cpc->reason & CP_SYNC) ||
		((cpc->reason & CP_DISCARD) && !sbi->discard_blks)))
		goto out;

> +       else
> +               f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> This patch version regardless of whether dir is encrypted or not, so this name(ENC_DIR_INO) can be rename other for more accurate?

Thanks for pointing out this, let me fix it in v5.

Thanks,

> Thanks！
>   same:
>          if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>                  inode->i_mode = F2FS_I(inode)->i_acl_mode; diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h index ed794b5fefbe..e4a94995e9a8 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -139,7 +139,8 @@ TRACE_DEFINE_ENUM(EX_BLOCK_AGE);
>                  { CP_NODE_NEED_CP,      "node needs cp" },              \
>                  { CP_FASTBOOT_MODE,     "fastboot mode" },              \
>                  { CP_SPEC_LOG_NUM,      "log type is 2" },              \
> -               { CP_RECOVER_DIR,       "dir needs recovery" })
> +               { CP_RECOVER_DIR,       "dir needs recovery" },         \
> +               { CP_ENC_DIR,           "persist encryption policy" })
> 
>   #define show_shutdown_mode(type)                                       \
>          __print_symbolic(type,                                          \
> --
> 2.40.1
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel