[PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[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().

Reported-by: Yunlei He <heyunlei@hihonor.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
- fix to set ENC_DIR_INO only for encrypted directory
 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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(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 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

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Jaegeuk Kim]

On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> - fix to set ENC_DIR_INO only for encrypted directory
>  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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);

What will happen, if we need to checkpoint xattr_nid on this directory?

>  
>  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

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu]

On 2021/4/27 1:09, Jaegeuk Kim wrote:
> On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> v2:
>> - fix to set ENC_DIR_INO only for encrypted directory
>>   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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
>> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
>> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> 
> What will happen, if we need to checkpoint xattr_nid on this directory?

need_do_checkpoint()

a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
		cp_reason = CP_NODE_NEED_CP;

b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
							ENC_DIR_INO))
		cp_reason = CP_ENC_DIR;

If parent is not checkpointed, after converting parent to encrypted directory
and create the file in parent, fsync this file will trigger checkpoint() due
to a)

If parent is checkpointed, after converting parent to encrypted directory
and create the file in parent, fsync this file will trigger checkpoint() due
to b)

Am I missing any cases?

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
> .
> 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Jaegeuk Kim]

On 04/27, Chao Yu wrote:
> On 2021/4/27 1:09, Jaegeuk Kim wrote:
> > On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > > v2:
> > > - fix to set ENC_DIR_INO only for encrypted directory
> > >   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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
> > > +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
> > > +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> > 
> > What will happen, if we need to checkpoint xattr_nid on this directory?
> 
> need_do_checkpoint()
> 
> a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
> 		cp_reason = CP_NODE_NEED_CP;

This will change the current behavior which does checkpoint regardless of the
parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
get xnid being checkpointed?

> 
> b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
> 							ENC_DIR_INO))
> 		cp_reason = CP_ENC_DIR;
> 
> If parent is not checkpointed, after converting parent to encrypted directory
> and create the file in parent, fsync this file will trigger checkpoint() due
> to a)
> 
> If parent is checkpointed, after converting parent to encrypted directory
> and create the file in parent, fsync this file will trigger checkpoint() due
> to b)
> 
> Am I missing any cases?
> 
> 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
> > .
> > 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu]

On 2021/5/4 22:36, Jaegeuk Kim wrote:
> On 04/27, Chao Yu wrote:
>> On 2021/4/27 1:09, Jaegeuk Kim wrote:
>>> On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>> v2:
>>>> - fix to set ENC_DIR_INO only for encrypted directory
>>>>    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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
>>>> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
>>>> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
>>>
>>> What will happen, if we need to checkpoint xattr_nid on this directory?
>>
>> need_do_checkpoint()
>>
>> a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>> 		cp_reason = CP_NODE_NEED_CP;
> 
> This will change the current behavior which does checkpoint regardless of the
> parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
> get xnid being checkpointed?

Yes,

 >> If parent is checkpointed, after converting parent to encrypted directory
 >> and create the file in parent, fsync this file will trigger checkpoint() due
 >> to b)

If i_pino was checkpointed, but xnid wasn't due to enable encryption on this
directory, fsync() this file will trigger checkpoint() to make sure xnid
checkpointed due to b) case.

Thanks,

> 
>>
>> b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>> 							ENC_DIR_INO))
>> 		cp_reason = CP_ENC_DIR;
>>
>> If parent is not checkpointed, after converting parent to encrypted directory
>> and create the file in parent, fsync this file will trigger checkpoint() due
>> to a)
>>
>> If parent is checkpointed, after converting parent to encrypted directory
>> and create the file in parent, fsync this file will trigger checkpoint() due
>> to b)
>>
>> Am I missing any cases?
>>
>> 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
>>> .
>>>
> .
> 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Jaegeuk Kim]

On 05/06, Chao Yu wrote:
> On 2021/5/4 22:36, Jaegeuk Kim wrote:
> > On 04/27, Chao Yu wrote:
> > > On 2021/4/27 1:09, Jaegeuk Kim wrote:
> > > > On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
> > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > ---
> > > > > v2:
> > > > > - fix to set ENC_DIR_INO only for encrypted directory
> > > > >    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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
> > > > > +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
> > > > > +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> > > > 
> > > > What will happen, if we need to checkpoint xattr_nid on this directory?
> > > 
> > > need_do_checkpoint()
> > > 
> > > a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
> > > 		cp_reason = CP_NODE_NEED_CP;
> > 
> > This will change the current behavior which does checkpoint regardless of the
> > parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
> > get xnid being checkpointed?
> 
> Yes,
> 
> >> If parent is checkpointed, after converting parent to encrypted directory
> >> and create the file in parent, fsync this file will trigger checkpoint() due
> >> to b)
> 
> If i_pino was checkpointed, but xnid wasn't due to enable encryption on this

I keep asking no encryption case where
1) parent is checkpointed
2) set_xattr(dir) w/ new new xnid
3) create(file)
4) fsync(file)

In that case, previousely we do checkpoint, but this change does not. Yes?

> directory, fsync() this file will trigger checkpoint() to make sure xnid
> checkpointed due to b) case.
> 
> Thanks,
> 
> > 
> > > 
> > > b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
> > > 							ENC_DIR_INO))
> > > 		cp_reason = CP_ENC_DIR;
> > > 
> > > If parent is not checkpointed, after converting parent to encrypted directory
> > > and create the file in parent, fsync this file will trigger checkpoint() due
> > > to a)
> > > 
> > > If parent is checkpointed, after converting parent to encrypted directory
> > > and create the file in parent, fsync this file will trigger checkpoint() due
> > > to b)
> > > 
> > > Am I missing any cases?
> > > 
> > > 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
> > > > .
> > > > 
> > .
> > 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu]

On 2021/5/6 12:43, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> On 2021/5/4 22:36, Jaegeuk Kim wrote:
>>> On 04/27, Chao Yu wrote:
>>>> On 2021/4/27 1:09, Jaegeuk Kim wrote:
>>>>> On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - fix to set ENC_DIR_INO only for encrypted directory
>>>>>>     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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
>>>>>> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
>>>>>> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
>>>>>
>>>>> What will happen, if we need to checkpoint xattr_nid on this directory?
>>>>
>>>> need_do_checkpoint()
>>>>
>>>> a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>>>> 		cp_reason = CP_NODE_NEED_CP;
>>>
>>> This will change the current behavior which does checkpoint regardless of the
>>> parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
>>> get xnid being checkpointed?
>>
>> Yes,
>>
>>>> If parent is checkpointed, after converting parent to encrypted directory
>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>> to b)
>>
>> If i_pino was checkpointed, but xnid wasn't due to enable encryption on this
> 
> I keep asking no encryption case where
> 1) parent is checkpointed
> 2) set_xattr(dir) w/ new new xnid
> 3) create(file)
> 4) fsync(file)
> 
> In that case, previousely we do checkpoint, but this change does not. Yes?

In this case, we won't checkpoint xnid, instead, just flushing file's data/node.

So yes, actually this patch will change the policy, which looks posix compliance,
that mean we only persist the file's meta/data after fsync(file).

How about keeping original policy in FSYNC_MODE_STRICT mode, and using current
policy in FSYNC_MODE_POSIX mode?

Thanks,

> 
>> directory, fsync() this file will trigger checkpoint() to make sure xnid
>> checkpointed due to b) case.
>>
>> Thanks,
>>
>>>
>>>>
>>>> b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>>>> 							ENC_DIR_INO))
>>>> 		cp_reason = CP_ENC_DIR;
>>>>
>>>> If parent is not checkpointed, after converting parent to encrypted directory
>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>> to a)
>>>>
>>>> If parent is checkpointed, after converting parent to encrypted directory
>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>> to b)
>>>>
>>>> Am I missing any cases?
>>>>
>>>> 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
>>>>> .
>>>>>
>>> .
>>>
> .
> 

Re: [f2fs-dev] [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Eric Biggers]

On Sun, Apr 25, 2021 at 09:10:53AM +0800, Chao Yu wrote:
> +	if (!error && S_ISDIR(inode->i_mode) && f2fs_encrypted_file(inode) &&
> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);

This will never be true, since S_ISDIR() and f2fs_encrypted_file() are logically
contradictory (as f2fs_encrypted_file() only returns true when S_ISREG()).

How did you test this change?

- Eric

Re: [f2fs-dev] [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu]

On 2021/5/7 14:33, Eric Biggers wrote:
> On Sun, Apr 25, 2021 at 09:10:53AM +0800, Chao Yu wrote:
>> +	if (!error && S_ISDIR(inode->i_mode) && f2fs_encrypted_file(inode) &&
>> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
>> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> 
> This will never be true, since S_ISDIR() and f2fs_encrypted_file() are logically
> contradictory (as f2fs_encrypted_file() only returns true when S_ISREG()).
> 
> How did you test this change?

I should add RFC tag in this v2 as v1, since I haven't test both
with specified case because this idea should be discussed first.

Thanks,

> 
> - Eric
> .
> 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Jaegeuk Kim]

On 05/06, Chao Yu wrote:
> On 2021/5/6 12:43, Jaegeuk Kim wrote:
> > On 05/06, Chao Yu wrote:
> > > On 2021/5/4 22:36, Jaegeuk Kim wrote:
> > > > On 04/27, Chao Yu wrote:
> > > > > On 2021/4/27 1:09, Jaegeuk Kim wrote:
> > > > > > On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
> > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > ---
> > > > > > > v2:
> > > > > > > - fix to set ENC_DIR_INO only for encrypted directory
> > > > > > >     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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
> > > > > > > +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
> > > > > > > +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
> > > > > > 
> > > > > > What will happen, if we need to checkpoint xattr_nid on this directory?
> > > > > 
> > > > > need_do_checkpoint()
> > > > > 
> > > > > a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
> > > > > 		cp_reason = CP_NODE_NEED_CP;
> > > > 
> > > > This will change the current behavior which does checkpoint regardless of the
> > > > parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
> > > > get xnid being checkpointed?
> > > 
> > > Yes,
> > > 
> > > > > If parent is checkpointed, after converting parent to encrypted directory
> > > > > and create the file in parent, fsync this file will trigger checkpoint() due
> > > > > to b)
> > > 
> > > If i_pino was checkpointed, but xnid wasn't due to enable encryption on this
> > 
> > I keep asking no encryption case where
> > 1) parent is checkpointed
> > 2) set_xattr(dir) w/ new new xnid
> > 3) create(file)
> > 4) fsync(file)
> > 
> > In that case, previousely we do checkpoint, but this change does not. Yes?
> 
> In this case, we won't checkpoint xnid, instead, just flushing file's data/node.
> 
> So yes, actually this patch will change the policy, which looks posix compliance,
> that mean we only persist the file's meta/data after fsync(file).
> 
> How about keeping original policy in FSYNC_MODE_STRICT mode, and using current
> policy in FSYNC_MODE_POSIX mode?

IIRC, it'd be much important to keep directory's xnid, so worry about stability
regression. Is that case a really performance blocker?

> 
> Thanks,
> 
> > 
> > > directory, fsync() this file will trigger checkpoint() to make sure xnid
> > > checkpointed due to b) case.
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
> > > > > 							ENC_DIR_INO))
> > > > > 		cp_reason = CP_ENC_DIR;
> > > > > 
> > > > > If parent is not checkpointed, after converting parent to encrypted directory
> > > > > and create the file in parent, fsync this file will trigger checkpoint() due
> > > > > to a)
> > > > > 
> > > > > If parent is checkpointed, after converting parent to encrypted directory
> > > > > and create the file in parent, fsync this file will trigger checkpoint() due
> > > > > to b)
> > > > > 
> > > > > Am I missing any cases?
> > > > > 
> > > > > 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
> > > > > > .
> > > > > > 
> > > > .
> > > > 
> > .
> > 

Re: [PATCH v2] f2fs: reduce expensive checkpoint trigger frequency

[Chao Yu]

On 2021/5/10 23:36, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> On 2021/5/6 12:43, Jaegeuk Kim wrote:
>>> On 05/06, Chao Yu wrote:
>>>> On 2021/5/4 22:36, Jaegeuk Kim wrote:
>>>>> On 04/27, Chao Yu wrote:
>>>>>> On 2021/4/27 1:09, Jaegeuk Kim wrote:
>>>>>>> On 04/25, 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: Yunlei He <heyunlei@hihonor.com>
>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> - fix to set ENC_DIR_INO only for encrypted directory
>>>>>>>>      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 b9d5317db0a7..0fe881309a20 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 a595050c56d3..62af29ec0879 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..70615d504f7e 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_encrypted_file(inode) &&
>>>>>>>> +			f2fs_is_checkpointed_node(sbi, inode->i_ino))
>>>>>>>> +		f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
>>>>>>>
>>>>>>> What will happen, if we need to checkpoint xattr_nid on this directory?
>>>>>>
>>>>>> need_do_checkpoint()
>>>>>>
>>>>>> a)	else if (!f2fs_is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>>>>>> 		cp_reason = CP_NODE_NEED_CP;
>>>>>
>>>>> This will change the current behavior which does checkpoint regardless of the
>>>>> parent being checkpointed. If i_pino was checkpointed but xnid wasn't, can we
>>>>> get xnid being checkpointed?
>>>>
>>>> Yes,
>>>>
>>>>>> If parent is checkpointed, after converting parent to encrypted directory
>>>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>>>> to b)
>>>>
>>>> If i_pino was checkpointed, but xnid wasn't due to enable encryption on this
>>>
>>> I keep asking no encryption case where
>>> 1) parent is checkpointed
>>> 2) set_xattr(dir) w/ new new xnid
>>> 3) create(file)
>>> 4) fsync(file)
>>>
>>> In that case, previousely we do checkpoint, but this change does not. Yes?
>>
>> In this case, we won't checkpoint xnid, instead, just flushing file's data/node.
>>
>> So yes, actually this patch will change the policy, which looks posix compliance,
>> that mean we only persist the file's meta/data after fsync(file).
>>
>> How about keeping original policy in FSYNC_MODE_STRICT mode, and using current
>> policy in FSYNC_MODE_POSIX mode?
> 
> IIRC, it'd be much important to keep directory's xnid, so worry about stability

Understood your concern.

> regression. Is that case a really performance blocker?

Hi, Yunlei, does this performance issue block anything?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>> directory, fsync() this file will trigger checkpoint() to make sure xnid
>>>> checkpointed due to b) case.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> b)	else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>>>>>> 							ENC_DIR_INO))
>>>>>> 		cp_reason = CP_ENC_DIR;
>>>>>>
>>>>>> If parent is not checkpointed, after converting parent to encrypted directory
>>>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>>>> to a)
>>>>>>
>>>>>> If parent is checkpointed, after converting parent to encrypted directory
>>>>>> and create the file in parent, fsync this file will trigger checkpoint() due
>>>>>> to b)
>>>>>>
>>>>>> Am I missing any cases?
>>>>>>
>>>>>> 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
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>> .
>>>
> .
>