| Message ID | 20240111081743.2999210-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,v3] f2fs: reduce expensive checkpoint trigger frequency | expand |
Jaegeuk, Any comments? On 2024/1/11 16:17, Chao Yu wrote: > 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 condition, let's record an entry including > directory's ino into global cache when we initialize encryption policy > in a checkpointed directory, and then only trigger checkpoint() when > target file's parent has non-persisted encryption policy, for the case > its parent is not checkpointed, need_do_checkpoint() has cover that > by verifying it with f2fs_is_checkpointed_node(). > > 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> > --- > v3: > - Recently, Zhiguo Niu reported the same issue, so I repost this > patch for comments. > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 16 ++++++++++++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index e2e0ca45f881..0094a8c85f4a 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -279,6 +279,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 */ > }; > @@ -1147,6 +1148,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 8198afb5fb9c..18b33b1f0c83 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { > + /* > + * In restrict mode, fsync() always tries triggering checkpoint > + * for all metadata consistency, in other mode, it only triggers > + * checkpoint when parent's encryption metadata updates. > + */ > + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + else if (IS_ENCRYPTED(inode) && > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > + } > > same: > if (is_inode_flag_set(inode, FI_ACL_MODE)) { > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h > index 7ed0fc430dc6..48f2e399e184 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, \
Hi Jaegeuk Sorry for disturbing you, Do you have any comments about this patch from Chao, I’ve met this issue several times on our platform when do monkey test. Thanks! -----邮件原件----- 发件人: Chao Yu <chao@kernel.org> 发送时间: 2024年2月19日 15:19 收件人: jaegeuk@kernel.org 抄送: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com> 主题: Re: [PATCH v3] 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. Jaegeuk, Any comments? On 2024/1/11 16:17, Chao Yu wrote: > 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 condition, let's record an entry including > directory's ino into global cache when we initialize encryption policy > in a checkpointed directory, and then only trigger checkpoint() when > target file's parent has non-persisted encryption policy, for the case > its parent is not checkpointed, need_do_checkpoint() has cover that by > verifying it with f2fs_is_checkpointed_node(). > > 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> > --- > v3: > - Recently, Zhiguo Niu reported the same issue, so I repost this patch > for comments. > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 16 ++++++++++++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index > e2e0ca45f881..0094a8c85f4a 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -279,6 +279,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 */ > }; > @@ -1147,6 +1148,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 > 8198afb5fb9c..18b33b1f0c83 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { > + /* > + * In restrict mode, fsync() always tries triggering checkpoint > + * for all metadata consistency, in other mode, it only triggers > + * checkpoint when parent's encryption metadata updates. > + */ > + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + else if (IS_ENCRYPTED(inode) && > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > + } > > same: > if (is_inode_flag_set(inode, FI_ACL_MODE)) { diff --git > a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h index > 7ed0fc430dc6..48f2e399e184 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, \
Dear Jaegeuk, Let me describe the problem process, it is reproduced by monkey stability test: 1.SBI_NEED_CP flag bit is set， set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); 2.Ckpt thread is blocked by IO busy when it is doing CP, it can not get request tag from block queue, PID: 505 TASK: ffffff80ed7f49c0 CPU: 4 COMMAND: "f2fs_ckpt-254:4" #0 [ffffffc015fcb330] __switch_to at ffffffc010196350 #1 [ffffffc015fcb390] __schedule at ffffffc01168e53c #2 [ffffffc015fcb3f0] schedule at ffffffc01168ed7c #3 [ffffffc015fcb450] io_schedule at ffffffc01168f7a0 #4 [ffffffc015fcb4c0] blk_mq_get_tag at ffffffc0101008a4 #5 [ffffffc015fcb530] blk_mq_get_request at ffffffc0109241b0 #6 [ffffffc015fcb5f0] blk_mq_make_request at ffffffc0109233bc #7 [ffffffc015fcb680] generic_make_request at ffffffc0100fc6ec #8 [ffffffc015fcb700] submit_bio at ffffffc0100fc3b8 #9 [ffffffc015fcb750] __submit_bio at ffffffc01081a2e0 #10 [ffffffc015fcb7d0] __submit_merged_bio at ffffffc01081b07c #11 [ffffffc015fcb8a0] f2fs_submit_page_write at ffffffc0100ecd3c #12 [ffffffc015fcb990] f2fs_do_write_meta_page at ffffffc010845738 #13 [ffffffc015fcb9d0] __f2fs_write_meta_page at ffffffc01080a8f4 #14 [ffffffc015fcbb60] f2fs_sync_meta_pages at ffffffc01080a684 #15 [ffffffc015fcbca0] do_checkpoint at ffffffc01080f0a8 #16 [ffffffc015fcbd10] f2fs_write_checkpoint at ffffffc01080e50c #17 [ffffffc015fcbdb0] __checkpoint_and_complete_reqs at ffffffc010810f54 #18 [ffffffc015fcbe40] issue_checkpoint_thread at ffffffc0108113ec #19 [ffffffc015fcbe80] kthread at ffffffc0102665b0 3.Subsequent regular file fsync will trigger ckpt because SBI_NEED_CP has not been cleared. In fact, these cases should not trigger ckpt. 4.If some processes that perform f2fs_do_sync_file are important processes in the system(such as init) and are blocked for too long, it will cause other problems in the system, ANR or android reboot PID: 287 TASK: ffffff80f9eb0ec0 CPU: 2 COMMAND: "init" #0 [ffffffc01389bab0] __switch_to at ffffffc010196350 #1 [ffffffc01389bb10] __schedule at ffffffc01168e53c #2 [ffffffc01389bb70] schedule at ffffffc01168ed7c #3 [ffffffc01389bbc0] wait_for_completion at ffffffc011692368 #4 [ffffffc01389bca0] f2fs_issue_checkpoint at ffffffc010810cb0 #5 [ffffffc01389bd00] f2fs_sync_fs at ffffffc0107f4e1c #6 [ffffffc01389bdc0] f2fs_do_sync_file at ffffffc0107d4d44 #7 [ffffffc01389be20] f2fs_sync_file at ffffffc0107d492c #8 [ffffffc01389be30] __arm64_sys_fsync at ffffffc0105d31d8 #9 [ffffffc01389be70] el0_svc_common at ffffffc0101aa550 #10 [ffffffc01389beb0] el0_svc_handler at ffffffc0100886fc and I tested Chao's patch can avoid the above case, please help consider this patch or any comment/suggestions about this? thanks! On Mon, Feb 26, 2024 at 9:22 AM 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com> wrote: > > > Hi Jaegeuk > > Sorry for disturbing you, Do you have any comments about this patch from Chao, I’ve met this issue several times on our platform when do monkey test. > Thanks! > > -----邮件原件----- > 发件人: Chao Yu <chao@kernel.org> > 发送时间: 2024年2月19日 15:19 > 收件人: jaegeuk@kernel.org > 抄送: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com> > 主题: Re: [PATCH v3] 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. > > > > Jaegeuk, > > Any comments? > > On 2024/1/11 16:17, Chao Yu wrote: > > 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 condition, let's record an entry including > > directory's ino into global cache when we initialize encryption policy > > in a checkpointed directory, and then only trigger checkpoint() when > > target file's parent has non-persisted encryption policy, for the case > > its parent is not checkpointed, need_do_checkpoint() has cover that by > > verifying it with f2fs_is_checkpointed_node(). > > > > 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> > > --- > > v3: > > - Recently, Zhiguo Niu reported the same issue, so I repost this patch > > for comments. > > fs/f2fs/f2fs.h | 2 ++ > > fs/f2fs/file.c | 3 +++ > > fs/f2fs/xattr.c | 16 ++++++++++++++-- > > include/trace/events/f2fs.h | 3 ++- > > 4 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index > > e2e0ca45f881..0094a8c85f4a 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -279,6 +279,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 */ > > }; > > @@ -1147,6 +1148,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 > > 8198afb5fb9c..18b33b1f0c83 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { > > + /* > > + * In restrict mode, fsync() always tries triggering checkpoint > > + * for all metadata consistency, in other mode, it only triggers > > + * checkpoint when parent's encryption metadata updates. > > + */ > > + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > > + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > > + else if (IS_ENCRYPTED(inode) && > > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > + } > > > > same: > > if (is_inode_flag_set(inode, FI_ACL_MODE)) { diff --git > > a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h index > > 7ed0fc430dc6..48f2e399e184 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, \
Sorry, move jaegeuk to the "To" list Dear Jaegeuk, Let me describe the problem process, it is reproduced by monkey stability test: 1.SBI_NEED_CP flag bit is set， set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); 2.Ckpt thread is blocked by IO busy when it is doing CP, it can not get request tag from block queue, PID: 505 TASK: ffffff80ed7f49c0 CPU: 4 COMMAND: "f2fs_ckpt-254:4" #0 [ffffffc015fcb330] __switch_to at ffffffc010196350 #1 [ffffffc015fcb390] __schedule at ffffffc01168e53c #2 [ffffffc015fcb3f0] schedule at ffffffc01168ed7c #3 [ffffffc015fcb450] io_schedule at ffffffc01168f7a0 #4 [ffffffc015fcb4c0] blk_mq_get_tag at ffffffc0101008a4 #5 [ffffffc015fcb530] blk_mq_get_request at ffffffc0109241b0 #6 [ffffffc015fcb5f0] blk_mq_make_request at ffffffc0109233bc #7 [ffffffc015fcb680] generic_make_request at ffffffc0100fc6ec #8 [ffffffc015fcb700] submit_bio at ffffffc0100fc3b8 #9 [ffffffc015fcb750] __submit_bio at ffffffc01081a2e0 #10 [ffffffc015fcb7d0] __submit_merged_bio at ffffffc01081b07c #11 [ffffffc015fcb8a0] f2fs_submit_page_write at ffffffc0100ecd3c #12 [ffffffc015fcb990] f2fs_do_write_meta_page at ffffffc010845738 #13 [ffffffc015fcb9d0] __f2fs_write_meta_page at ffffffc01080a8f4 #14 [ffffffc015fcbb60] f2fs_sync_meta_pages at ffffffc01080a684 #15 [ffffffc015fcbca0] do_checkpoint at ffffffc01080f0a8 #16 [ffffffc015fcbd10] f2fs_write_checkpoint at ffffffc01080e50c #17 [ffffffc015fcbdb0] __checkpoint_and_complete_reqs at ffffffc010810f54 #18 [ffffffc015fcbe40] issue_checkpoint_thread at ffffffc0108113ec #19 [ffffffc015fcbe80] kthread at ffffffc0102665b0 3.Subsequent regular file fsync will trigger ckpt because SBI_NEED_CP has not been cleared. In fact, these cases should not trigger ckpt. 4.If some processes that perform f2fs_do_sync_file are important processes in the system(such as init) and are blocked for too long, it will cause other problems in the system, ANR or android reboot PID: 287 TASK: ffffff80f9eb0ec0 CPU: 2 COMMAND: "init" #0 [ffffffc01389bab0] __switch_to at ffffffc010196350 #1 [ffffffc01389bb10] __schedule at ffffffc01168e53c #2 [ffffffc01389bb70] schedule at ffffffc01168ed7c #3 [ffffffc01389bbc0] wait_for_completion at ffffffc011692368 #4 [ffffffc01389bca0] f2fs_issue_checkpoint at ffffffc010810cb0 #5 [ffffffc01389bd00] f2fs_sync_fs at ffffffc0107f4e1c #6 [ffffffc01389bdc0] f2fs_do_sync_file at ffffffc0107d4d44 #7 [ffffffc01389be20] f2fs_sync_file at ffffffc0107d492c #8 [ffffffc01389be30] __arm64_sys_fsync at ffffffc0105d31d8 #9 [ffffffc01389be70] el0_svc_common at ffffffc0101aa550 #10 [ffffffc01389beb0] el0_svc_handler at ffffffc0100886fc and I tested Chao's patch can avoid the above case, please help consider this patch or any comment/suggestions about this? thanks! On Tue, Mar 5, 2024 at 9:56 AM Zhiguo Niu <niuzhiguo84@gmail.com> wrote: > > Dear Jaegeuk, > > Let me describe the problem process, it is reproduced by monkey stability test: > > 1.SBI_NEED_CP flag bit is set， > set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > > 2.Ckpt thread is blocked by IO busy when it is doing CP, it can not > get request tag from block queue, > PID: 505 TASK: ffffff80ed7f49c0 CPU: 4 COMMAND: "f2fs_ckpt-254:4" > #0 [ffffffc015fcb330] __switch_to at ffffffc010196350 > #1 [ffffffc015fcb390] __schedule at ffffffc01168e53c > #2 [ffffffc015fcb3f0] schedule at ffffffc01168ed7c > #3 [ffffffc015fcb450] io_schedule at ffffffc01168f7a0 > #4 [ffffffc015fcb4c0] blk_mq_get_tag at ffffffc0101008a4 > #5 [ffffffc015fcb530] blk_mq_get_request at ffffffc0109241b0 > #6 [ffffffc015fcb5f0] blk_mq_make_request at ffffffc0109233bc > #7 [ffffffc015fcb680] generic_make_request at ffffffc0100fc6ec > #8 [ffffffc015fcb700] submit_bio at ffffffc0100fc3b8 > #9 [ffffffc015fcb750] __submit_bio at ffffffc01081a2e0 > #10 [ffffffc015fcb7d0] __submit_merged_bio at ffffffc01081b07c > #11 [ffffffc015fcb8a0] f2fs_submit_page_write at ffffffc0100ecd3c > #12 [ffffffc015fcb990] f2fs_do_write_meta_page at ffffffc010845738 > #13 [ffffffc015fcb9d0] __f2fs_write_meta_page at ffffffc01080a8f4 > #14 [ffffffc015fcbb60] f2fs_sync_meta_pages at ffffffc01080a684 > #15 [ffffffc015fcbca0] do_checkpoint at ffffffc01080f0a8 > #16 [ffffffc015fcbd10] f2fs_write_checkpoint at ffffffc01080e50c > #17 [ffffffc015fcbdb0] __checkpoint_and_complete_reqs at ffffffc010810f54 > #18 [ffffffc015fcbe40] issue_checkpoint_thread at ffffffc0108113ec > #19 [ffffffc015fcbe80] kthread at ffffffc0102665b0 > > 3.Subsequent regular file fsync will trigger ckpt because SBI_NEED_CP > has not been cleared. > In fact, these cases should not trigger ckpt. > > 4.If some processes that perform f2fs_do_sync_file are important processes > in the system(such as init) and are blocked for too long, it will > cause other problems in the system, ANR or android reboot > PID: 287 TASK: ffffff80f9eb0ec0 CPU: 2 COMMAND: "init" > #0 [ffffffc01389bab0] __switch_to at ffffffc010196350 > #1 [ffffffc01389bb10] __schedule at ffffffc01168e53c > #2 [ffffffc01389bb70] schedule at ffffffc01168ed7c > #3 [ffffffc01389bbc0] wait_for_completion at ffffffc011692368 > #4 [ffffffc01389bca0] f2fs_issue_checkpoint at ffffffc010810cb0 > #5 [ffffffc01389bd00] f2fs_sync_fs at ffffffc0107f4e1c > #6 [ffffffc01389bdc0] f2fs_do_sync_file at ffffffc0107d4d44 > #7 [ffffffc01389be20] f2fs_sync_file at ffffffc0107d492c > #8 [ffffffc01389be30] __arm64_sys_fsync at ffffffc0105d31d8 > #9 [ffffffc01389be70] el0_svc_common at ffffffc0101aa550 > #10 [ffffffc01389beb0] el0_svc_handler at ffffffc0100886fc > > and I tested Chao's patch can avoid the above case, please help > consider this patch or > any comment/suggestions about this? > > thanks! > > On Mon, Feb 26, 2024 at 9:22 AM 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com> wrote: > > > > > > Hi Jaegeuk > > > > Sorry for disturbing you, Do you have any comments about this patch from Chao, I’ve met this issue several times on our platform when do monkey test. > > Thanks! > > > > -----邮件原件----- > > 发件人: Chao Yu <chao@kernel.org> > > 发送时间: 2024年2月19日 15:19 > > 收件人: jaegeuk@kernel.org > > 抄送: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com> > > 主题: Re: [PATCH v3] 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. > > > > > > > > Jaegeuk, > > > > Any comments? > > > > On 2024/1/11 16:17, Chao Yu wrote: > > > 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 condition, let's record an entry including > > > directory's ino into global cache when we initialize encryption policy > > > in a checkpointed directory, and then only trigger checkpoint() when > > > target file's parent has non-persisted encryption policy, for the case > > > its parent is not checkpointed, need_do_checkpoint() has cover that by > > > verifying it with f2fs_is_checkpointed_node(). > > > > > > 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> > > > --- > > > v3: > > > - Recently, Zhiguo Niu reported the same issue, so I repost this patch > > > for comments. > > > fs/f2fs/f2fs.h | 2 ++ > > > fs/f2fs/file.c | 3 +++ > > > fs/f2fs/xattr.c | 16 ++++++++++++++-- > > > include/trace/events/f2fs.h | 3 ++- > > > 4 files changed, 21 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index > > > e2e0ca45f881..0094a8c85f4a 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -279,6 +279,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 */ > > > }; > > > @@ -1147,6 +1148,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 > > > 8198afb5fb9c..18b33b1f0c83 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { > > > + /* > > > + * In restrict mode, fsync() always tries triggering checkpoint > > > + * for all metadata consistency, in other mode, it only triggers > > > + * checkpoint when parent's encryption metadata updates. > > > + */ > > > + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > > > + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > > > + else if (IS_ENCRYPTED(inode) && > > > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > > > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > > + } > > > > > > same: > > > if (is_inode_flag_set(inode, FI_ACL_MODE)) { diff --git > > > a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h index > > > 7ed0fc430dc6..48f2e399e184 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, \
> 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 condition, let's record an entry including > directory's ino into global cache when we initialize encryption policy > in a checkpointed directory, and then only trigger checkpoint() when > target file's parent has non-persisted encryption policy, for the case > its parent is not checkpointed, need_do_checkpoint() has cover that > by verifying it with f2fs_is_checkpointed_node(). > > 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> > --- > v3: > - Recently, Zhiguo Niu reported the same issue, so I repost this > patch for comments. > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 16 ++++++++++++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index e2e0ca45f881..0094a8c85f4a 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -279,6 +279,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 */ > }; > @@ -1147,6 +1148,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 8198afb5fb9c..18b33b1f0c83 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { > + /* > + * In restrict mode, fsync() always tries triggering checkpoint > + * for all metadata consistency, in other mode, it only triggers > + * checkpoint when parent's encryption metadata updates. > + */ > + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + else if (IS_ENCRYPTED(inode) && > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > + } > > same: > if (is_inode_flag_set(inode, FI_ACL_MODE)) { > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h > index 7ed0fc430dc6..48f2e399e184 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 Hi, Chao I noticed the discussion about patch v2, Jaegeuk mentioned no encryption case: 1) parent is checkpointed 2) set_xattr(dir) w/ new xnid 3) create(file) 4) fsync(file) We will not trigger checkpoint() after this change. So, how about adding a rule in need_do_checkpoint? We can judge if the parent has xnid being checkpointed or not, if not we can still trigger checkpoint() and keep the same behavior before this change.
On 2024/6/25 10:14, wangzijie wrote: >> 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 condition, let's record an entry including >> directory's ino into global cache when we initialize encryption policy >> in a checkpointed directory, and then only trigger checkpoint() when >> target file's parent has non-persisted encryption policy, for the case >> its parent is not checkpointed, need_do_checkpoint() has cover that >> by verifying it with f2fs_is_checkpointed_node(). >> >> 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> >> --- >> v3: >> - Recently, Zhiguo Niu reported the same issue, so I repost this >> patch for comments. >> fs/f2fs/f2fs.h | 2 ++ >> fs/f2fs/file.c | 3 +++ >> fs/f2fs/xattr.c | 16 ++++++++++++++-- >> include/trace/events/f2fs.h | 3 ++- >> 4 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index e2e0ca45f881..0094a8c85f4a 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -279,6 +279,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 */ >> }; >> @@ -1147,6 +1148,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 8198afb5fb9c..18b33b1f0c83 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { >> + /* >> + * In restrict mode, fsync() always tries triggering checkpoint >> + * for all metadata consistency, in other mode, it only triggers >> + * checkpoint when parent's encryption metadata updates. >> + */ >> + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) >> + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >> + else if (IS_ENCRYPTED(inode) && >> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); >> + } >> >> same: >> if (is_inode_flag_set(inode, FI_ACL_MODE)) { >> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h >> index 7ed0fc430dc6..48f2e399e184 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 > > Hi, Chao > I noticed the discussion about patch v2, Jaegeuk mentioned no encryption case: > 1) parent is checkpointed > 2) set_xattr(dir) w/ new xnid > 3) create(file) > 4) fsync(file) > We will not trigger checkpoint() after this change. > So, how about adding a rule in need_do_checkpoint? We can judge if the parent has xnid > being checkpointed or not, if not we can still trigger checkpoint() and keep the same > behavior before this change. Hi wangzijie, It may cause performance overhead to get parent's xnid by parent ino, so I prefer to tag ENC_DIR_INO for the inode if its parent's xnid is not checkpointed, it that fine to you? Thanks, > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >
>On 2024/6/25 10:14, wangzijie wrote: >>> 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 condition, let's record an entry including >>> directory's ino into global cache when we initialize encryption policy >>> in a checkpointed directory, and then only trigger checkpoint() when >>> target file's parent has non-persisted encryption policy, for the case >>> its parent is not checkpointed, need_do_checkpoint() has cover that >>> by verifying it with f2fs_is_checkpointed_node(). >>> >>> 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> >>> --- >>> v3: >>> - Recently, Zhiguo Niu reported the same issue, so I repost this >>> patch for comments. >>> fs/f2fs/f2fs.h | 2 ++ >>> fs/f2fs/file.c | 3 +++ >>> fs/f2fs/xattr.c | 16 ++++++++++++++-- >>> include/trace/events/f2fs.h | 3 ++- >>> 4 files changed, 21 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index e2e0ca45f881..0094a8c85f4a 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -279,6 +279,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 */ >>> }; >>> @@ -1147,6 +1148,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 8198afb5fb9c..18b33b1f0c83 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { >>> + /* >>> + * In restrict mode, fsync() always tries triggering checkpoint >>> + * for all metadata consistency, in other mode, it only triggers >>> + * checkpoint when parent's encryption metadata updates. >>> + */ >>> + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) >>> + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >>> + else if (IS_ENCRYPTED(inode) && >>> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >>> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); >>> + } >>> >>> same: >>> if (is_inode_flag_set(inode, FI_ACL_MODE)) { >>> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h >>> index 7ed0fc430dc6..48f2e399e184 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 >> >> Hi, Chao >> I noticed the discussion about patch v2, Jaegeuk mentioned no encryption case: >> 1) parent is checkpointed >> 2) set_xattr(dir) w/ new xnid >> 3) create(file) >> 4) fsync(file) >> We will not trigger checkpoint() after this change. >> So, how about adding a rule in need_do_checkpoint? We can judge if the parent has xnid >> being checkpointed or not, if not we can still trigger checkpoint() and keep the same >> behavior before this change. > >Hi wangzijie, > >It may cause performance overhead to get parent's xnid by parent ino, >so I prefer to tag ENC_DIR_INO for the inode if its parent's xnid is >not checkpointed, it that fine to you? > >Thanks, Yes, I've also met this reported problem. Thank you for your reply.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index e2e0ca45f881..0094a8c85f4a 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -279,6 +279,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 */ }; @@ -1147,6 +1148,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 8198afb5fb9c..18b33b1f0c83 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -218,6 +218,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..cbd1b88297fe 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,8 +773,19 @@ 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)) { + /* + * In restrict mode, fsync() always tries triggering checkpoint + * for all metadata consistency, in other mode, it only triggers + * checkpoint when parent's encryption metadata updates. + */ + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); + else if (IS_ENCRYPTED(inode) && + f2fs_is_checkpointed_node(sbi, inode->i_ino)) + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); + } same: if (is_inode_flag_set(inode, FI_ACL_MODE)) { diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h index 7ed0fc430dc6..48f2e399e184 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, \
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 condition, let's record an entry including directory's ino into global cache when we initialize encryption policy in a checkpointed directory, and then only trigger checkpoint() when target file's parent has non-persisted encryption policy, for the case its parent is not checkpointed, need_do_checkpoint() has cover that by verifying it with f2fs_is_checkpointed_node(). 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> --- v3: - Recently, Zhiguo Niu reported the same issue, so I repost this patch for comments. fs/f2fs/f2fs.h | 2 ++ fs/f2fs/file.c | 3 +++ fs/f2fs/xattr.c | 16 ++++++++++++++-- include/trace/events/f2fs.h | 3 ++- 4 files changed, 21 insertions(+), 3 deletions(-)