* [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-16 9:58 ` Chao Yu 0 siblings, 0 replies; 11+ messages in thread From: Chao Yu @ 2021-04-16 9:58 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu 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(). Signed-off-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/f2fs.h | 2 ++ fs/f2fs/file.c | 3 +++ fs/f2fs/xattr.c | 6 ++++-- include/trace/events/f2fs.h | 3 ++- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 87d734f5589d..34487e527d12 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -246,6 +246,7 @@ enum { APPEND_INO, /* for append ino list */ UPDATE_INO, /* for update ino list */ TRANS_DIR_INO, /* for trasactions dir ino list */ + ENC_DIR_INO, /* for encrypted dir ino list */ FLUSH_INO, /* for multiple device flushing */ MAX_INO_ENTRY, /* max. list */ }; @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 --- a/fs/f2fs/xattr.c +++ b/fs/f2fs/xattr.c @@ -630,6 +630,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; @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) f2fs_set_encrypted_inode(inode); f2fs_mark_inode_dirty_sync(inode, true); - if (!error && S_ISDIR(inode->i_mode)) - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); + if (!error && S_ISDIR(inode->i_mode) && + 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 56b113e3cd6a..ca0cf12226e9 100644 --- a/include/trace/events/f2fs.h +++ b/include/trace/events/f2fs.h @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); { 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.29.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-16 9:58 ` Chao Yu 0 siblings, 0 replies; 11+ messages in thread From: Chao Yu @ 2021-04-16 9:58 UTC (permalink / raw) To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel 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(). Signed-off-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/f2fs.h | 2 ++ fs/f2fs/file.c | 3 +++ fs/f2fs/xattr.c | 6 ++++-- include/trace/events/f2fs.h | 3 ++- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 87d734f5589d..34487e527d12 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -246,6 +246,7 @@ enum { APPEND_INO, /* for append ino list */ UPDATE_INO, /* for update ino list */ TRANS_DIR_INO, /* for trasactions dir ino list */ + ENC_DIR_INO, /* for encrypted dir ino list */ FLUSH_INO, /* for multiple device flushing */ MAX_INO_ENTRY, /* max. list */ }; @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 --- a/fs/f2fs/xattr.c +++ b/fs/f2fs/xattr.c @@ -630,6 +630,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; @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) f2fs_set_encrypted_inode(inode); f2fs_mark_inode_dirty_sync(inode, true); - if (!error && S_ISDIR(inode->i_mode)) - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); + if (!error && S_ISDIR(inode->i_mode) && + 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 56b113e3cd6a..ca0cf12226e9 100644 --- a/include/trace/events/f2fs.h +++ b/include/trace/events/f2fs.h @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); { 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.29.2 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency 2021-04-16 9:58 ` [f2fs-dev] " Chao Yu (?) @ 2021-04-22 3:24 ` heyunlei 00015531 -1 siblings, 0 replies; 11+ messages in thread From: heyunlei 00015531 @ 2021-04-22 3:24 UTC (permalink / raw) To: Chao Yu, jaegeuk; +Cc: linux-kernel, linux-f2fs-devel, feng.han, bintian.wang 在 2021/4/16 17:58, Chao Yu 写道: > 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(). Ping! > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 6 ++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 87d734f5589d..34487e527d12 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -246,6 +246,7 @@ enum { > APPEND_INO, /* for append ino list */ > UPDATE_INO, /* for update ino list */ > TRANS_DIR_INO, /* for trasactions dir ino list */ > + ENC_DIR_INO, /* for encrypted dir ino list */ > FLUSH_INO, /* for multiple device flushing */ > MAX_INO_ENTRY, /* max. list */ > }; > @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 > --- a/fs/f2fs/xattr.c > +++ b/fs/f2fs/xattr.c > @@ -630,6 +630,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; > @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, > !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) > f2fs_set_encrypted_inode(inode); > f2fs_mark_inode_dirty_sync(inode, true); > - if (!error && S_ISDIR(inode->i_mode)) > - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + if (!error && S_ISDIR(inode->i_mode) && > + 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 56b113e3cd6a..ca0cf12226e9 100644 > --- a/include/trace/events/f2fs.h > +++ b/include/trace/events/f2fs.h > @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); > { 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, \ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency 2021-04-16 9:58 ` [f2fs-dev] " Chao Yu @ 2021-04-22 4:06 ` Jaegeuk Kim -1 siblings, 0 replies; 11+ messages in thread From: Jaegeuk Kim @ 2021-04-22 4:06 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao On 04/16, 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. It'll happen once? How much impact will we hit due to this? > > 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(). > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 6 ++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 87d734f5589d..34487e527d12 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -246,6 +246,7 @@ enum { > APPEND_INO, /* for append ino list */ > UPDATE_INO, /* for update ino list */ > TRANS_DIR_INO, /* for trasactions dir ino list */ > + ENC_DIR_INO, /* for encrypted dir ino list */ > FLUSH_INO, /* for multiple device flushing */ > MAX_INO_ENTRY, /* max. list */ > }; > @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 > --- a/fs/f2fs/xattr.c > +++ b/fs/f2fs/xattr.c > @@ -630,6 +630,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; > @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, > !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) > f2fs_set_encrypted_inode(inode); > f2fs_mark_inode_dirty_sync(inode, true); > - if (!error && S_ISDIR(inode->i_mode)) > - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + if (!error && S_ISDIR(inode->i_mode) && > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); Is it right to say ENC_DIR_INO in this case? > > 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 56b113e3cd6a..ca0cf12226e9 100644 > --- a/include/trace/events/f2fs.h > +++ b/include/trace/events/f2fs.h > @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); > { 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.29.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-22 4:06 ` Jaegeuk Kim 0 siblings, 0 replies; 11+ messages in thread From: Jaegeuk Kim @ 2021-04-22 4:06 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel On 04/16, 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. It'll happen once? How much impact will we hit due to this? > > 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(). > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/file.c | 3 +++ > fs/f2fs/xattr.c | 6 ++++-- > include/trace/events/f2fs.h | 3 ++- > 4 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 87d734f5589d..34487e527d12 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -246,6 +246,7 @@ enum { > APPEND_INO, /* for append ino list */ > UPDATE_INO, /* for update ino list */ > TRANS_DIR_INO, /* for trasactions dir ino list */ > + ENC_DIR_INO, /* for encrypted dir ino list */ > FLUSH_INO, /* for multiple device flushing */ > MAX_INO_ENTRY, /* max. list */ > }; > @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 > --- a/fs/f2fs/xattr.c > +++ b/fs/f2fs/xattr.c > @@ -630,6 +630,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; > @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, > !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) > f2fs_set_encrypted_inode(inode); > f2fs_mark_inode_dirty_sync(inode, true); > - if (!error && S_ISDIR(inode->i_mode)) > - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > + if (!error && S_ISDIR(inode->i_mode) && > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); Is it right to say ENC_DIR_INO in this case? > > 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 56b113e3cd6a..ca0cf12226e9 100644 > --- a/include/trace/events/f2fs.h > +++ b/include/trace/events/f2fs.h > @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); > { 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.29.2 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency 2021-04-22 4:06 ` [f2fs-dev] " Jaegeuk Kim @ 2021-04-22 7:14 ` Chao Yu -1 siblings, 0 replies; 11+ messages in thread From: Chao Yu @ 2021-04-22 7:14 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao, heyunlei On 2021/4/22 12:06, Jaegeuk Kim wrote: > On 04/16, 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. > > It'll happen once? How much impact will we hit due to this? Yunlei reports me this issue, the problems here in Honer device's specified environment, most fsync() on regular file triggers a checkpoint() when mkdir() happened concurrently, result in causing the performance issue. Yunlei could explain more about details of this issue. @Yunlei > >> >> 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(). >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> fs/f2fs/f2fs.h | 2 ++ >> fs/f2fs/file.c | 3 +++ >> fs/f2fs/xattr.c | 6 ++++-- >> include/trace/events/f2fs.h | 3 ++- >> 4 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 87d734f5589d..34487e527d12 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -246,6 +246,7 @@ enum { >> APPEND_INO, /* for append ino list */ >> UPDATE_INO, /* for update ino list */ >> TRANS_DIR_INO, /* for trasactions dir ino list */ >> + ENC_DIR_INO, /* for encrypted dir ino list */ >> FLUSH_INO, /* for multiple device flushing */ >> MAX_INO_ENTRY, /* max. list */ >> }; >> @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 >> --- a/fs/f2fs/xattr.c >> +++ b/fs/f2fs/xattr.c >> @@ -630,6 +630,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; >> @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, >> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) >> f2fs_set_encrypted_inode(inode); >> f2fs_mark_inode_dirty_sync(inode, true); >> - if (!error && S_ISDIR(inode->i_mode)) >> - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >> + if (!error && S_ISDIR(inode->i_mode) && >> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > Is it right to say ENC_DIR_INO in this case? Sorry, I didn't get it. Thanks, > >> >> 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 56b113e3cd6a..ca0cf12226e9 100644 >> --- a/include/trace/events/f2fs.h >> +++ b/include/trace/events/f2fs.h >> @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); >> { 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.29.2 > . > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-22 7:14 ` Chao Yu 0 siblings, 0 replies; 11+ messages in thread From: Chao Yu @ 2021-04-22 7:14 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel On 2021/4/22 12:06, Jaegeuk Kim wrote: > On 04/16, 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. > > It'll happen once? How much impact will we hit due to this? Yunlei reports me this issue, the problems here in Honer device's specified environment, most fsync() on regular file triggers a checkpoint() when mkdir() happened concurrently, result in causing the performance issue. Yunlei could explain more about details of this issue. @Yunlei > >> >> 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(). >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> fs/f2fs/f2fs.h | 2 ++ >> fs/f2fs/file.c | 3 +++ >> fs/f2fs/xattr.c | 6 ++++-- >> include/trace/events/f2fs.h | 3 ++- >> 4 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 87d734f5589d..34487e527d12 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -246,6 +246,7 @@ enum { >> APPEND_INO, /* for append ino list */ >> UPDATE_INO, /* for update ino list */ >> TRANS_DIR_INO, /* for trasactions dir ino list */ >> + ENC_DIR_INO, /* for encrypted dir ino list */ >> FLUSH_INO, /* for multiple device flushing */ >> MAX_INO_ENTRY, /* max. list */ >> }; >> @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 >> --- a/fs/f2fs/xattr.c >> +++ b/fs/f2fs/xattr.c >> @@ -630,6 +630,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; >> @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, >> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) >> f2fs_set_encrypted_inode(inode); >> f2fs_mark_inode_dirty_sync(inode, true); >> - if (!error && S_ISDIR(inode->i_mode)) >> - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >> + if (!error && S_ISDIR(inode->i_mode) && >> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > Is it right to say ENC_DIR_INO in this case? Sorry, I didn't get it. Thanks, > >> >> 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 56b113e3cd6a..ca0cf12226e9 100644 >> --- a/include/trace/events/f2fs.h >> +++ b/include/trace/events/f2fs.h >> @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); >> { 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.29.2 > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency 2021-04-22 7:14 ` [f2fs-dev] " Chao Yu @ 2021-04-22 12:35 ` heyunlei 00015531 -1 siblings, 0 replies; 11+ messages in thread From: heyunlei 00015531 @ 2021-04-22 12:35 UTC (permalink / raw) To: Chao Yu, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao 在 2021/4/22 15:14, Chao Yu 写道: > On 2021/4/22 12:06, Jaegeuk Kim wrote: >> On 04/16, 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. >> >> It'll happen once? How much impact will we hit due to this? > > Yunlei reports me this issue, the problems here in Honer device's > specified > environment, most fsync() on regular file triggers a checkpoint() when > mkdir() > happened concurrently, result in causing the performance issue. > > Yunlei could explain more about details of this issue. @Yunlei Hi all, Many apps launch will create new directories,which will set need checkpoint flag because inherited crypto info from their parent. During this time, any fsynced will write a new cp. For the worst case: new dir1 fsync file a (with no relations with dir1), and write cp n new dir 2 fsync file b (with no relations with dir2), and write cp n + 1 ... ... Thanks. > >> >>> >>> 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(). >>> >>> Signed-off-by: Chao Yu <yuchao0@huawei.com> >>> --- >>> fs/f2fs/f2fs.h | 2 ++ >>> fs/f2fs/file.c | 3 +++ >>> fs/f2fs/xattr.c | 6 ++++-- >>> include/trace/events/f2fs.h | 3 ++- >>> 4 files changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 87d734f5589d..34487e527d12 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -246,6 +246,7 @@ enum { >>> APPEND_INO, /* for append ino list */ >>> UPDATE_INO, /* for update ino list */ >>> TRANS_DIR_INO, /* for trasactions dir ino list */ >>> + ENC_DIR_INO, /* for encrypted dir ino list */ >>> FLUSH_INO, /* for multiple device flushing */ >>> MAX_INO_ENTRY, /* max. list */ >>> }; >>> @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 >>> --- a/fs/f2fs/xattr.c >>> +++ b/fs/f2fs/xattr.c >>> @@ -630,6 +630,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; >>> @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, >>> int index, >>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) >>> f2fs_set_encrypted_inode(inode); >>> f2fs_mark_inode_dirty_sync(inode, true); >>> - if (!error && S_ISDIR(inode->i_mode)) >>> - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >>> + if (!error && S_ISDIR(inode->i_mode) && >>> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >>> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); >> >> Is it right to say ENC_DIR_INO in this case? > > Sorry, I didn't get it. > > Thanks, > >> >>> 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 56b113e3cd6a..ca0cf12226e9 100644 >>> --- a/include/trace/events/f2fs.h >>> +++ b/include/trace/events/f2fs.h >>> @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); >>> { 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.29.2 >> . >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-22 12:35 ` heyunlei 00015531 0 siblings, 0 replies; 11+ messages in thread From: heyunlei 00015531 @ 2021-04-22 12:35 UTC (permalink / raw) To: Chao Yu, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 在 2021/4/22 15:14, Chao Yu 写道: > On 2021/4/22 12:06, Jaegeuk Kim wrote: >> On 04/16, 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. >> >> It'll happen once? How much impact will we hit due to this? > > Yunlei reports me this issue, the problems here in Honer device's > specified > environment, most fsync() on regular file triggers a checkpoint() when > mkdir() > happened concurrently, result in causing the performance issue. > > Yunlei could explain more about details of this issue. @Yunlei Hi all, Many apps launch will create new directories,which will set need checkpoint flag because inherited crypto info from their parent. During this time, any fsynced will write a new cp. For the worst case: new dir1 fsync file a (with no relations with dir1), and write cp n new dir 2 fsync file b (with no relations with dir2), and write cp n + 1 ... ... Thanks. > >> >>> >>> 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(). >>> >>> Signed-off-by: Chao Yu <yuchao0@huawei.com> >>> --- >>> fs/f2fs/f2fs.h | 2 ++ >>> fs/f2fs/file.c | 3 +++ >>> fs/f2fs/xattr.c | 6 ++++-- >>> include/trace/events/f2fs.h | 3 ++- >>> 4 files changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 87d734f5589d..34487e527d12 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -246,6 +246,7 @@ enum { >>> APPEND_INO, /* for append ino list */ >>> UPDATE_INO, /* for update ino list */ >>> TRANS_DIR_INO, /* for trasactions dir ino list */ >>> + ENC_DIR_INO, /* for encrypted dir ino list */ >>> FLUSH_INO, /* for multiple device flushing */ >>> MAX_INO_ENTRY, /* max. list */ >>> }; >>> @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 >>> --- a/fs/f2fs/xattr.c >>> +++ b/fs/f2fs/xattr.c >>> @@ -630,6 +630,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; >>> @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, >>> int index, >>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) >>> f2fs_set_encrypted_inode(inode); >>> f2fs_mark_inode_dirty_sync(inode, true); >>> - if (!error && S_ISDIR(inode->i_mode)) >>> - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); >>> + if (!error && S_ISDIR(inode->i_mode) && >>> + f2fs_is_checkpointed_node(sbi, inode->i_ino)) >>> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); >> >> Is it right to say ENC_DIR_INO in this case? > > Sorry, I didn't get it. > > Thanks, > >> >>> 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 56b113e3cd6a..ca0cf12226e9 100644 >>> --- a/include/trace/events/f2fs.h >>> +++ b/include/trace/events/f2fs.h >>> @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); >>> { 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.29.2 >> . >> _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency 2021-04-22 7:14 ` [f2fs-dev] " Chao Yu @ 2021-04-25 0:52 ` Jaegeuk Kim -1 siblings, 0 replies; 11+ messages in thread From: Jaegeuk Kim @ 2021-04-25 0:52 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao, heyunlei On 04/22, Chao Yu wrote: > On 2021/4/22 12:06, Jaegeuk Kim wrote: > > On 04/16, 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. > > > > It'll happen once? How much impact will we hit due to this? > > Yunlei reports me this issue, the problems here in Honer device's specified > environment, most fsync() on regular file triggers a checkpoint() when mkdir() > happened concurrently, result in causing the performance issue. > > Yunlei could explain more about details of this issue. @Yunlei > > > > > > > > > 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(). > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > > > --- > > > fs/f2fs/f2fs.h | 2 ++ > > > fs/f2fs/file.c | 3 +++ > > > fs/f2fs/xattr.c | 6 ++++-- > > > include/trace/events/f2fs.h | 3 ++- > > > 4 files changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index 87d734f5589d..34487e527d12 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -246,6 +246,7 @@ enum { > > > APPEND_INO, /* for append ino list */ > > > UPDATE_INO, /* for update ino list */ > > > TRANS_DIR_INO, /* for trasactions dir ino list */ > > > + ENC_DIR_INO, /* for encrypted dir ino list */ > > > FLUSH_INO, /* for multiple device flushing */ > > > MAX_INO_ENTRY, /* max. list */ > > > }; > > > @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 > > > --- a/fs/f2fs/xattr.c > > > +++ b/fs/f2fs/xattr.c > > > @@ -630,6 +630,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; > > > @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, > > > !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) > > > f2fs_set_encrypted_inode(inode); > > > f2fs_mark_inode_dirty_sync(inode, true); > > > - if (!error && S_ISDIR(inode->i_mode)) > > > - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > > > + if (!error && S_ISDIR(inode->i_mode) && > > > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > > > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > > > Is it right to say ENC_DIR_INO in this case? > > Sorry, I didn't get it. I mean, this case includes non-encrypted directory as well. > > Thanks, > > > > > > 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 56b113e3cd6a..ca0cf12226e9 100644 > > > --- a/include/trace/events/f2fs.h > > > +++ b/include/trace/events/f2fs.h > > > @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); > > > { 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.29.2 > > . > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency @ 2021-04-25 0:52 ` Jaegeuk Kim 0 siblings, 0 replies; 11+ messages in thread From: Jaegeuk Kim @ 2021-04-25 0:52 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel On 04/22, Chao Yu wrote: > On 2021/4/22 12:06, Jaegeuk Kim wrote: > > On 04/16, 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. > > > > It'll happen once? How much impact will we hit due to this? > > Yunlei reports me this issue, the problems here in Honer device's specified > environment, most fsync() on regular file triggers a checkpoint() when mkdir() > happened concurrently, result in causing the performance issue. > > Yunlei could explain more about details of this issue. @Yunlei > > > > > > > > > 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(). > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > > > --- > > > fs/f2fs/f2fs.h | 2 ++ > > > fs/f2fs/file.c | 3 +++ > > > fs/f2fs/xattr.c | 6 ++++-- > > > include/trace/events/f2fs.h | 3 ++- > > > 4 files changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index 87d734f5589d..34487e527d12 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -246,6 +246,7 @@ enum { > > > APPEND_INO, /* for append ino list */ > > > UPDATE_INO, /* for update ino list */ > > > TRANS_DIR_INO, /* for trasactions dir ino list */ > > > + ENC_DIR_INO, /* for encrypted dir ino list */ > > > FLUSH_INO, /* for multiple device flushing */ > > > MAX_INO_ENTRY, /* max. list */ > > > }; > > > @@ -1090,6 +1091,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 6284b2f4a60b..a6c38d8b1ec3 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 c8f34decbf8e..38796d488d15 100644 > > > --- a/fs/f2fs/xattr.c > > > +++ b/fs/f2fs/xattr.c > > > @@ -630,6 +630,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; > > > @@ -745,8 +746,9 @@ static int __f2fs_setxattr(struct inode *inode, int index, > > > !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) > > > f2fs_set_encrypted_inode(inode); > > > f2fs_mark_inode_dirty_sync(inode, true); > > > - if (!error && S_ISDIR(inode->i_mode)) > > > - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP); > > > + if (!error && S_ISDIR(inode->i_mode) && > > > + f2fs_is_checkpointed_node(sbi, inode->i_ino)) > > > + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO); > > > > Is it right to say ENC_DIR_INO in this case? > > Sorry, I didn't get it. I mean, this case includes non-encrypted directory as well. > > Thanks, > > > > > > 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 56b113e3cd6a..ca0cf12226e9 100644 > > > --- a/include/trace/events/f2fs.h > > > +++ b/include/trace/events/f2fs.h > > > @@ -145,7 +145,8 @@ TRACE_DEFINE_ENUM(CP_RESIZE); > > > { 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.29.2 > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-04-25 8:36 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-16 9:58 [RFC PATCH] f2fs: reduce expensive checkpoint trigger frequency Chao Yu 2021-04-16 9:58 ` [f2fs-dev] " Chao Yu 2021-04-22 3:24 ` heyunlei 00015531 2021-04-22 4:06 ` Jaegeuk Kim 2021-04-22 4:06 ` [f2fs-dev] " Jaegeuk Kim 2021-04-22 7:14 ` Chao Yu 2021-04-22 7:14 ` [f2fs-dev] " Chao Yu 2021-04-22 12:35 ` heyunlei 00015531 2021-04-22 12:35 ` [f2fs-dev] " heyunlei 00015531 2021-04-25 0:52 ` Jaegeuk Kim 2021-04-25 0:52 ` [f2fs-dev] " Jaegeuk Kim
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.