2022-11-11 10:19:13

by Sheng Yong

[permalink] [raw]
Subject: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
---
fs/f2fs/namei.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)

---
v1->v2: add filename parameter for f2fs_new_inode, and move
set_compress_inode into f2fs_new_inode

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e5..36e251f438568 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,12 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
+ const unsigned char *name);
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -119,6 +123,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
f2fs_may_compress(inode))
set_compress_context(inode);
+ if (name)
+ set_compress_inode(sbi, inode, name);
}

/* Should enable inline_data after compression set */
@@ -293,8 +299,7 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
int i, cold_count, hot_count;

- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
+ if (F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
!f2fs_may_compress(inode) ||
(!ext_cnt && !noext_cnt))
return;
@@ -326,10 +331,6 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
for (i = 0; i < ext_cnt; i++) {
if (!is_extension_exist(name, ext[i], false))
continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
set_compress_context(inode);
return;
}
@@ -352,15 +353,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +688,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +759,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +816,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +855,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.25.1



2022-11-11 10:43:25

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/11 18:08, Sheng Yong wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,

2022-11-12 01:56:39

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

Does thes make sense?

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=608460dfae20b9d23aa222f7448710a086778222
https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=962379487b5cb9f3b85ea367b130c2c6ca584edf

Second one is needed to address build error.

On 11/11, Sheng Yong wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> ---
> fs/f2fs/namei.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> ---
> v1->v2: add filename parameter for f2fs_new_inode, and move
> set_compress_inode into f2fs_new_inode
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e5..36e251f438568 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,12 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> + const unsigned char *name);
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -119,6 +123,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> f2fs_may_compress(inode))
> set_compress_context(inode);
> + if (name)
> + set_compress_inode(sbi, inode, name);
> }
>
> /* Should enable inline_data after compression set */
> @@ -293,8 +299,7 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> int i, cold_count, hot_count;
>
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> + if (F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> !f2fs_may_compress(inode) ||
> (!ext_cnt && !noext_cnt))
> return;
> @@ -326,10 +331,6 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> for (i = 0; i < ext_cnt; i++) {
> if (!is_extension_exist(name, ext[i], false))
> continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> set_compress_context(inode);
> return;
> }
> @@ -352,15 +353,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +688,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +759,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +816,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +855,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> --
> 2.25.1

2022-11-14 01:59:12

by Sheng Yong

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches



On 2022/11/12 9:27, Jaegeuk Kim wrote:
> Does thes make sense?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=608460dfae20b9d23aa222f7448710a086778222
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=962379487b5cb9f3b85ea367b130c2c6ca584edf
>
Hi, Jaegeuk,

Absolutely. Thanks for addressing it.

> Second one is needed to address build error.

Sorry for missing adding a hunk of that patch :(
The above 2 commits are already tested, shall I resend a new patchset?

thanks,
shengyong
>
> On 11/11, Sheng Yong wrote:
>> If compress_extension is set, and a newly created file matches the
>> extension, the file could be marked as compression file. However,
>> if inline_data is also enabled, there is no chance to check its
>> extension since f2fs_should_compress() always returns false.
>>
>> This patch moves set_compress_inode(), which do extension check, in
>> f2fs_should_compress() to check extensions before setting inline
>> data flag.
>>
>> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
>> Signed-off-by: Sheng Yong <[email protected]>
>> ---
>> fs/f2fs/namei.c | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> ---
>> v1->v2: add filename parameter for f2fs_new_inode, and move
>> set_compress_inode into f2fs_new_inode
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index e104409c3a0e5..36e251f438568 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -22,8 +22,12 @@
>> #include "acl.h"
>> #include <trace/events/f2fs.h>
>>
>> +static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> + const unsigned char *name);
>> +
>> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> - struct inode *dir, umode_t mode)
>> + struct inode *dir, umode_t mode,
>> + const char *name)
>> {
>> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>> nid_t ino;
>> @@ -119,6 +123,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
>> f2fs_may_compress(inode))
>> set_compress_context(inode);
>> + if (name)
>> + set_compress_inode(sbi, inode, name);
>> }
>>
>> /* Should enable inline_data after compression set */
>> @@ -293,8 +299,7 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> int i, cold_count, hot_count;
>>
>> - if (!f2fs_sb_has_compression(sbi) ||
>> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> + if (F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> !f2fs_may_compress(inode) ||
>> (!ext_cnt && !noext_cnt))
>> return;
>> @@ -326,10 +331,6 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> for (i = 0; i < ext_cnt; i++) {
>> if (!is_extension_exist(name, ext[i], false))
>> continue;
>> -
>> - /* Do not use inline_data with compression */
>> - stat_dec_inline_inode(inode);
>> - clear_inode_flag(inode, FI_INLINE_DATA);
>> set_compress_context(inode);
>> return;
>> }
>> @@ -352,15 +353,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
>> set_file_temperature(sbi, inode, dentry->d_name.name);
>>
>> - set_compress_inode(sbi, inode, dentry->d_name.name);
>> -
>> inode->i_op = &f2fs_file_inode_operations;
>> inode->i_fop = &f2fs_file_operations;
>> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> @@ -689,7 +688,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -760,7 +759,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -817,7 +816,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -856,7 +855,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> --
>> 2.25.1

2022-11-14 16:21:43

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/12 9:27, Jaegeuk Kim wrote:
> Does thes make sense?

Jaegeuk,

Could you please send modified patches to mailing list, otherwise,
I can not add comments on specified line.

Thanks,

>
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=608460dfae20b9d23aa222f7448710a086778222
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test&id=962379487b5cb9f3b85ea367b130c2c6ca584edf
>
> Second one is needed to address build error.
>
> On 11/11, Sheng Yong wrote:
>> If compress_extension is set, and a newly created file matches the
>> extension, the file could be marked as compression file. However,
>> if inline_data is also enabled, there is no chance to check its
>> extension since f2fs_should_compress() always returns false.
>>
>> This patch moves set_compress_inode(), which do extension check, in
>> f2fs_should_compress() to check extensions before setting inline
>> data flag.
>>
>> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
>> Signed-off-by: Sheng Yong <[email protected]>
>> ---
>> fs/f2fs/namei.c | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> ---
>> v1->v2: add filename parameter for f2fs_new_inode, and move
>> set_compress_inode into f2fs_new_inode
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index e104409c3a0e5..36e251f438568 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -22,8 +22,12 @@
>> #include "acl.h"
>> #include <trace/events/f2fs.h>
>>
>> +static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> + const unsigned char *name);
>> +
>> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> - struct inode *dir, umode_t mode)
>> + struct inode *dir, umode_t mode,
>> + const char *name)
>> {
>> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>> nid_t ino;
>> @@ -119,6 +123,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
>> f2fs_may_compress(inode))
>> set_compress_context(inode);
>> + if (name)
>> + set_compress_inode(sbi, inode, name);
>> }
>>
>> /* Should enable inline_data after compression set */
>> @@ -293,8 +299,7 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> int i, cold_count, hot_count;
>>
>> - if (!f2fs_sb_has_compression(sbi) ||
>> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> + if (F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> !f2fs_may_compress(inode) ||
>> (!ext_cnt && !noext_cnt))
>> return;
>> @@ -326,10 +331,6 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> for (i = 0; i < ext_cnt; i++) {
>> if (!is_extension_exist(name, ext[i], false))
>> continue;
>> -
>> - /* Do not use inline_data with compression */
>> - stat_dec_inline_inode(inode);
>> - clear_inode_flag(inode, FI_INLINE_DATA);
>> set_compress_context(inode);
>> return;
>> }
>> @@ -352,15 +353,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
>> set_file_temperature(sbi, inode, dentry->d_name.name);
>>
>> - set_compress_inode(sbi, inode, dentry->d_name.name);
>> -
>> inode->i_op = &f2fs_file_inode_operations;
>> inode->i_fop = &f2fs_file_operations;
>> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> @@ -689,7 +688,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -760,7 +759,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -817,7 +816,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -856,7 +855,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> --
>> 2.25.1

2022-11-14 22:46:21

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/namei.c | 326 ++++++++++++++++++++++++------------------------
1 file changed, 160 insertions(+), 166 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e..43b721d8e491 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,160 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static inline int is_extension_exist(const unsigned char *s, const char *sub,
+ bool tmp_ext)
+{
+ size_t slen = strlen(s);
+ size_t sublen = strlen(sub);
+ int i;
+
+ if (sublen == 1 && *sub == '*')
+ return 1;
+
+ /*
+ * filename format of multimedia file should be defined as:
+ * "filename + '.' + extension + (optional: '.' + temp extension)".
+ */
+ if (slen < sublen + 2)
+ return 0;
+
+ if (!tmp_ext) {
+ /* file has no temp extension */
+ if (s[slen - sublen - 1] != '.')
+ return 0;
+ return !strncasecmp(s + slen - sublen, sub, sublen);
+ }
+
+ for (i = 1; i < slen - sublen; i++) {
+ if (s[i] != '.')
+ continue;
+ if (!strncasecmp(s + i + 1, sub, sublen))
+ return 1;
+ }
+
+ return 0;
+}
+
+int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
+ bool hot, bool set)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ int hot_count = sbi->raw_super->hot_ext_count;
+ int total_count = cold_count + hot_count;
+ int start, count;
+ int i;
+
+ if (set) {
+ if (total_count == F2FS_MAX_EXTENSION)
+ return -EINVAL;
+ } else {
+ if (!hot && !cold_count)
+ return -EINVAL;
+ if (hot && !hot_count)
+ return -EINVAL;
+ }
+
+ if (hot) {
+ start = cold_count;
+ count = total_count;
+ } else {
+ start = 0;
+ count = cold_count;
+ }
+
+ for (i = start; i < count; i++) {
+ if (strcmp(name, extlist[i]))
+ continue;
+
+ if (set)
+ return -EINVAL;
+
+ memcpy(extlist[i], extlist[i + 1],
+ F2FS_EXTENSION_LEN * (total_count - i - 1));
+ memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
+ if (hot)
+ sbi->raw_super->hot_ext_count = hot_count - 1;
+ else
+ sbi->raw_super->extension_count =
+ cpu_to_le32(cold_count - 1);
+ return 0;
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ if (hot) {
+ memcpy(extlist[count], name, strlen(name));
+ sbi->raw_super->hot_ext_count = hot_count + 1;
+ } else {
+ char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
+
+ memcpy(buf, &extlist[cold_count],
+ F2FS_EXTENSION_LEN * hot_count);
+ memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
+ memcpy(extlist[cold_count], name, strlen(name));
+ memcpy(&extlist[cold_count + 1], buf,
+ F2FS_EXTENSION_LEN * hot_count);
+ sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
+ }
+ return 0;
+}
+
+static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
+ struct inode *inode, const unsigned char *name)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ unsigned char (*noext)[F2FS_EXTENSION_LEN] =
+ F2FS_OPTION(sbi).noextensions;
+ unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
+ unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
+ unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
+ int i, cold_count, hot_count;
+
+ if (!f2fs_sb_has_compression(sbi) || !name)
+ return;
+ if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+ return;
+
+ /* Inherit the compression flag in directory */
+ if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL)) {
+ set_compress_context(inode);
+ return;
+ }
+
+ /* Start to check extension list */
+ if (!ext_cnt)
+ return;
+
+ /* Don't compress hot files. */
+ f2fs_down_read(&sbi->sb_lock);
+ cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ hot_count = sbi->raw_super->hot_ext_count;
+ for (i = cold_count; i < cold_count + hot_count; i++)
+ if (is_extension_exist(name, extlist[i], false))
+ break;
+ f2fs_up_read(&sbi->sb_lock);
+ if (i < (cold_count + hot_count))
+ return;
+
+ /* Don't compress unallowed extension. */
+ for (i = 0; i < noext_cnt; i++)
+ if (is_extension_exist(name, noext[i], false))
+ return;
+
+ /* Compress wanting extension. */
+ for (i = 0; i < ext_cnt; i++) {
+ if (is_extension_exist(name, ext[i], false)) {
+ set_compress_context(inode);
+ return;
+ }
+ }
+}
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -114,12 +266,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
set_inode_flag(inode, FI_PROJ_INHERIT);

- if (f2fs_sb_has_compression(sbi)) {
- /* Inherit the compression flag in directory */
- if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
- f2fs_may_compress(inode))
- set_compress_context(inode);
- }
+ /* Check compression first. */
+ set_compress_new_inode(sbi, dir, inode, name);

/* Should enable inline_data after compression set */
if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
@@ -153,40 +301,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
return ERR_PTR(err);
}

-static inline int is_extension_exist(const unsigned char *s, const char *sub,
- bool tmp_ext)
-{
- size_t slen = strlen(s);
- size_t sublen = strlen(sub);
- int i;
-
- if (sublen == 1 && *sub == '*')
- return 1;
-
- /*
- * filename format of multimedia file should be defined as:
- * "filename + '.' + extension + (optional: '.' + temp extension)".
- */
- if (slen < sublen + 2)
- return 0;
-
- if (!tmp_ext) {
- /* file has no temp extension */
- if (s[slen - sublen - 1] != '.')
- return 0;
- return !strncasecmp(s + slen - sublen, sub, sublen);
- }
-
- for (i = 1; i < slen - sublen; i++) {
- if (s[i] != '.')
- continue;
- if (!strncasecmp(s + i + 1, sub, sublen))
- return 1;
- }
-
- return 0;
-}
-
/*
* Set file's temperature for hot/cold data separation
*/
@@ -217,124 +331,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
file_set_hot(inode);
}

-int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
- bool hot, bool set)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- int hot_count = sbi->raw_super->hot_ext_count;
- int total_count = cold_count + hot_count;
- int start, count;
- int i;
-
- if (set) {
- if (total_count == F2FS_MAX_EXTENSION)
- return -EINVAL;
- } else {
- if (!hot && !cold_count)
- return -EINVAL;
- if (hot && !hot_count)
- return -EINVAL;
- }
-
- if (hot) {
- start = cold_count;
- count = total_count;
- } else {
- start = 0;
- count = cold_count;
- }
-
- for (i = start; i < count; i++) {
- if (strcmp(name, extlist[i]))
- continue;
-
- if (set)
- return -EINVAL;
-
- memcpy(extlist[i], extlist[i + 1],
- F2FS_EXTENSION_LEN * (total_count - i - 1));
- memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
- if (hot)
- sbi->raw_super->hot_ext_count = hot_count - 1;
- else
- sbi->raw_super->extension_count =
- cpu_to_le32(cold_count - 1);
- return 0;
- }
-
- if (!set)
- return -EINVAL;
-
- if (hot) {
- memcpy(extlist[count], name, strlen(name));
- sbi->raw_super->hot_ext_count = hot_count + 1;
- } else {
- char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
-
- memcpy(buf, &extlist[cold_count],
- F2FS_EXTENSION_LEN * hot_count);
- memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
- memcpy(extlist[cold_count], name, strlen(name));
- memcpy(&extlist[cold_count + 1], buf,
- F2FS_EXTENSION_LEN * hot_count);
- sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
- }
- return 0;
-}
-
-static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
- const unsigned char *name)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
- unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
- unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
- unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
- int i, cold_count, hot_count;
-
- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
- !f2fs_may_compress(inode) ||
- (!ext_cnt && !noext_cnt))
- return;
-
- f2fs_down_read(&sbi->sb_lock);
-
- cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- hot_count = sbi->raw_super->hot_ext_count;
-
- for (i = cold_count; i < cold_count + hot_count; i++) {
- if (is_extension_exist(name, extlist[i], false)) {
- f2fs_up_read(&sbi->sb_lock);
- return;
- }
- }
-
- f2fs_up_read(&sbi->sb_lock);
-
- for (i = 0; i < noext_cnt; i++) {
- if (is_extension_exist(name, noext[i], false)) {
- f2fs_disable_compressed_file(inode);
- return;
- }
- }
-
- if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
- return;
-
- for (i = 0; i < ext_cnt; i++) {
- if (!is_extension_exist(name, ext[i], false))
- continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
- set_compress_context(inode);
- return;
- }
-}
-
static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
@@ -352,15 +348,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +683,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +754,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +811,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +850,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.38.1.493.g58b659f92b-goog


2022-11-15 01:00:24

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

On 11/14, Jaegeuk Kim wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/namei.c | 326 ++++++++++++++++++++++++------------------------
> 1 file changed, 160 insertions(+), 166 deletions(-)
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e..43b721d8e491 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,160 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + int i, cold_count, hot_count;
> +
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return;
> +
> + /* Inherit the compression flag in directory */
> + if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL)) {
> + set_compress_context(inode);
> + return;
> + }
> +
> + /* Start to check extension list */
> + if (!ext_cnt)
> + return;
> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false))
> + break;
> + f2fs_up_read(&sbi->sb_lock);
> + if (i < (cold_count + hot_count))
> + return;
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++)
> + if (is_extension_exist(name, noext[i], false))
> + return;
> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + set_compress_context(inode);
> + return;
> + }
> + }
> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +266,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +301,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +331,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +348,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +683,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +754,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
+ dentry->d_name.name);

Should pass dentry.

> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +811,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +850,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> --
> 2.38.1.493.g58b659f92b-goog
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2022-11-15 02:06:31

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/15 6:39, Jaegeuk Kim wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/namei.c | 326 ++++++++++++++++++++++++------------------------
> 1 file changed, 160 insertions(+), 166 deletions(-)
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e..43b721d8e491 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,160 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + int i, cold_count, hot_count;
> +
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return;
> +
> + /* Inherit the compression flag in directory */
> + if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL)) {
> + set_compress_context(inode);
> + return;
> + }
> +
> + /* Start to check extension list */
> + if (!ext_cnt)
> + return;
> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false))
> + break;
> + f2fs_up_read(&sbi->sb_lock);
> + if (i < (cold_count + hot_count))
> + return;
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++)
> + if (is_extension_exist(name, noext[i], false))
> + return;

Should check noext before inheritting F2FS_COMPR_FL?

Thanks,

> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + set_compress_context(inode);
> + return;
> + }
> + }
> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +266,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +301,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +331,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +348,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +683,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +754,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +811,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +850,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>

2022-11-15 02:35:57

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/15 8:20, Jaegeuk Kim wrote:
> On 11/14, Jaegeuk Kim wrote:
>> If compress_extension is set, and a newly created file matches the
>> extension, the file could be marked as compression file. However,
>> if inline_data is also enabled, there is no chance to check its
>> extension since f2fs_should_compress() always returns false.
>>
>> This patch moves set_compress_inode(), which do extension check, in
>> f2fs_should_compress() to check extensions before setting inline
>> data flag.
>>
>> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
>> Signed-off-by: Sheng Yong <[email protected]>
>> Signed-off-by: Jaegeuk Kim <[email protected]>
>> ---
>> fs/f2fs/namei.c | 326 ++++++++++++++++++++++++------------------------
>> 1 file changed, 160 insertions(+), 166 deletions(-)
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index e104409c3a0e..43b721d8e491 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -22,8 +22,160 @@
>> #include "acl.h"
>> #include <trace/events/f2fs.h>
>>
>> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
>> + bool tmp_ext)
>> +{
>> + size_t slen = strlen(s);
>> + size_t sublen = strlen(sub);
>> + int i;
>> +
>> + if (sublen == 1 && *sub == '*')
>> + return 1;
>> +
>> + /*
>> + * filename format of multimedia file should be defined as:
>> + * "filename + '.' + extension + (optional: '.' + temp extension)".
>> + */
>> + if (slen < sublen + 2)
>> + return 0;
>> +
>> + if (!tmp_ext) {
>> + /* file has no temp extension */
>> + if (s[slen - sublen - 1] != '.')
>> + return 0;
>> + return !strncasecmp(s + slen - sublen, sub, sublen);
>> + }
>> +
>> + for (i = 1; i < slen - sublen; i++) {
>> + if (s[i] != '.')
>> + continue;
>> + if (!strncasecmp(s + i + 1, sub, sublen))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
>> + bool hot, bool set)
>> +{
>> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> + int hot_count = sbi->raw_super->hot_ext_count;
>> + int total_count = cold_count + hot_count;
>> + int start, count;
>> + int i;
>> +
>> + if (set) {
>> + if (total_count == F2FS_MAX_EXTENSION)
>> + return -EINVAL;
>> + } else {
>> + if (!hot && !cold_count)
>> + return -EINVAL;
>> + if (hot && !hot_count)
>> + return -EINVAL;
>> + }
>> +
>> + if (hot) {
>> + start = cold_count;
>> + count = total_count;
>> + } else {
>> + start = 0;
>> + count = cold_count;
>> + }
>> +
>> + for (i = start; i < count; i++) {
>> + if (strcmp(name, extlist[i]))
>> + continue;
>> +
>> + if (set)
>> + return -EINVAL;
>> +
>> + memcpy(extlist[i], extlist[i + 1],
>> + F2FS_EXTENSION_LEN * (total_count - i - 1));
>> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
>> + if (hot)
>> + sbi->raw_super->hot_ext_count = hot_count - 1;
>> + else
>> + sbi->raw_super->extension_count =
>> + cpu_to_le32(cold_count - 1);
>> + return 0;
>> + }
>> +
>> + if (!set)
>> + return -EINVAL;
>> +
>> + if (hot) {
>> + memcpy(extlist[count], name, strlen(name));
>> + sbi->raw_super->hot_ext_count = hot_count + 1;
>> + } else {
>> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
>> +
>> + memcpy(buf, &extlist[cold_count],
>> + F2FS_EXTENSION_LEN * hot_count);
>> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
>> + memcpy(extlist[cold_count], name, strlen(name));
>> + memcpy(&extlist[cold_count + 1], buf,
>> + F2FS_EXTENSION_LEN * hot_count);
>> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
>> + }
>> + return 0;
>> +}
>> +
>> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
>> + struct inode *inode, const unsigned char *name)
>> +{
>> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
>> + F2FS_OPTION(sbi).noextensions;
>> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
>> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
>> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> + int i, cold_count, hot_count;
>> +
>> + if (!f2fs_sb_has_compression(sbi) || !name)
>> + return;
>> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>> + return;
>> +
>> + /* Inherit the compression flag in directory */
>> + if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL)) {
>> + set_compress_context(inode);
>> + return;
>> + }
>> +
>> + /* Start to check extension list */
>> + if (!ext_cnt)
>> + return;
>> +
>> + /* Don't compress hot files. */
>> + f2fs_down_read(&sbi->sb_lock);
>> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> + hot_count = sbi->raw_super->hot_ext_count;
>> + for (i = cold_count; i < cold_count + hot_count; i++)
>> + if (is_extension_exist(name, extlist[i], false))
>> + break;
>> + f2fs_up_read(&sbi->sb_lock);
>> + if (i < (cold_count + hot_count))
>> + return;
>> +
>> + /* Don't compress unallowed extension. */
>> + for (i = 0; i < noext_cnt; i++)
>> + if (is_extension_exist(name, noext[i], false))
>> + return;
>> +
>> + /* Compress wanting extension. */
>> + for (i = 0; i < ext_cnt; i++) {
>> + if (is_extension_exist(name, ext[i], false)) {
>> + set_compress_context(inode);
>> + return;
>> + }
>> + }
>> +}
>> +
>> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> - struct inode *dir, umode_t mode)
>> + struct inode *dir, umode_t mode,
>> + const char *name)
>> {
>> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>> nid_t ino;
>> @@ -114,12 +266,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
>> set_inode_flag(inode, FI_PROJ_INHERIT);
>>
>> - if (f2fs_sb_has_compression(sbi)) {
>> - /* Inherit the compression flag in directory */
>> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
>> - f2fs_may_compress(inode))
>> - set_compress_context(inode);
>> - }
>> + /* Check compression first. */
>> + set_compress_new_inode(sbi, dir, inode, name);
>>
>> /* Should enable inline_data after compression set */
>> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> @@ -153,40 +301,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> return ERR_PTR(err);
>> }
>>
>> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
>> - bool tmp_ext)
>> -{
>> - size_t slen = strlen(s);
>> - size_t sublen = strlen(sub);
>> - int i;
>> -
>> - if (sublen == 1 && *sub == '*')
>> - return 1;
>> -
>> - /*
>> - * filename format of multimedia file should be defined as:
>> - * "filename + '.' + extension + (optional: '.' + temp extension)".
>> - */
>> - if (slen < sublen + 2)
>> - return 0;
>> -
>> - if (!tmp_ext) {
>> - /* file has no temp extension */
>> - if (s[slen - sublen - 1] != '.')
>> - return 0;
>> - return !strncasecmp(s + slen - sublen, sub, sublen);
>> - }
>> -
>> - for (i = 1; i < slen - sublen; i++) {
>> - if (s[i] != '.')
>> - continue;
>> - if (!strncasecmp(s + i + 1, sub, sublen))
>> - return 1;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * Set file's temperature for hot/cold data separation
>> */
>> @@ -217,124 +331,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
>> file_set_hot(inode);
>> }
>>
>> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
>> - bool hot, bool set)
>> -{
>> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> - int hot_count = sbi->raw_super->hot_ext_count;
>> - int total_count = cold_count + hot_count;
>> - int start, count;
>> - int i;
>> -
>> - if (set) {
>> - if (total_count == F2FS_MAX_EXTENSION)
>> - return -EINVAL;
>> - } else {
>> - if (!hot && !cold_count)
>> - return -EINVAL;
>> - if (hot && !hot_count)
>> - return -EINVAL;
>> - }
>> -
>> - if (hot) {
>> - start = cold_count;
>> - count = total_count;
>> - } else {
>> - start = 0;
>> - count = cold_count;
>> - }
>> -
>> - for (i = start; i < count; i++) {
>> - if (strcmp(name, extlist[i]))
>> - continue;
>> -
>> - if (set)
>> - return -EINVAL;
>> -
>> - memcpy(extlist[i], extlist[i + 1],
>> - F2FS_EXTENSION_LEN * (total_count - i - 1));
>> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
>> - if (hot)
>> - sbi->raw_super->hot_ext_count = hot_count - 1;
>> - else
>> - sbi->raw_super->extension_count =
>> - cpu_to_le32(cold_count - 1);
>> - return 0;
>> - }
>> -
>> - if (!set)
>> - return -EINVAL;
>> -
>> - if (hot) {
>> - memcpy(extlist[count], name, strlen(name));
>> - sbi->raw_super->hot_ext_count = hot_count + 1;
>> - } else {
>> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
>> -
>> - memcpy(buf, &extlist[cold_count],
>> - F2FS_EXTENSION_LEN * hot_count);
>> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
>> - memcpy(extlist[cold_count], name, strlen(name));
>> - memcpy(&extlist[cold_count + 1], buf,
>> - F2FS_EXTENSION_LEN * hot_count);
>> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
>> - }
>> - return 0;
>> -}
>> -
>> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> - const unsigned char *name)
>> -{
>> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
>> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
>> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
>> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> - int i, cold_count, hot_count;
>> -
>> - if (!f2fs_sb_has_compression(sbi) ||
>> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> - !f2fs_may_compress(inode) ||
>> - (!ext_cnt && !noext_cnt))
>> - return;
>> -
>> - f2fs_down_read(&sbi->sb_lock);
>> -
>> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> - hot_count = sbi->raw_super->hot_ext_count;
>> -
>> - for (i = cold_count; i < cold_count + hot_count; i++) {
>> - if (is_extension_exist(name, extlist[i], false)) {
>> - f2fs_up_read(&sbi->sb_lock);
>> - return;
>> - }
>> - }
>> -
>> - f2fs_up_read(&sbi->sb_lock);
>> -
>> - for (i = 0; i < noext_cnt; i++) {
>> - if (is_extension_exist(name, noext[i], false)) {
>> - f2fs_disable_compressed_file(inode);
>> - return;
>> - }
>> - }
>> -
>> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
>> - return;
>> -
>> - for (i = 0; i < ext_cnt; i++) {
>> - if (!is_extension_exist(name, ext[i], false))
>> - continue;
>> -
>> - /* Do not use inline_data with compression */
>> - stat_dec_inline_inode(inode);
>> - clear_inode_flag(inode, FI_INLINE_DATA);
>> - set_compress_context(inode);
>> - return;
>> - }
>> -}
>> -
>> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>> struct dentry *dentry, umode_t mode, bool excl)
>> {
>> @@ -352,15 +348,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
>> set_file_temperature(sbi, inode, dentry->d_name.name);
>>
>> - set_compress_inode(sbi, inode, dentry->d_name.name);
>> -
>> inode->i_op = &f2fs_file_inode_operations;
>> inode->i_fop = &f2fs_file_operations;
>> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> @@ -689,7 +683,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -760,7 +754,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
> + dentry->d_name.name);
>
> Should pass dentry.

It looks F2FS_COMPR_FL should be inherited for both directory and regular inode,
and the flag should be set/unset due to matched {,no}ext only for regular inode.

Thanks,

>
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -817,7 +811,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> @@ -856,7 +850,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
>> if (err)
>> return err;
>>
>> - inode = f2fs_new_inode(mnt_userns, dir, mode);
>> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>> if (IS_ERR(inode))
>> return PTR_ERR(inode);
>>
>> --
>> 2.38.1.493.g58b659f92b-goog
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2022-11-15 16:47:15

by Sheng Yong

[permalink] [raw]
Subject: [PATCH v3] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/f2fs.h | 1 +
fs/f2fs/namei.c | 336 ++++++++++++++++++++++++------------------------
2 files changed, 171 insertions(+), 166 deletions(-)
---

Hi, Jaegeuk, Chao,

How about adding a bool `may_compress' in set_compress_new_inode, set
`my_compress` according to several conditions. If it is false, clear
F2FS_COMPR_FL.

And set_compress_context is also changed to clear F2FS_NOCOMP_FL,
otherwise, if F2FS_NOCOMP_FL is inherited from parent and hit
compress_extension, both F2FS_NOCOMP_FL and F2FS_COMPR_FL are set.

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6a8cbf5bb1871..a3420fbb29214 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4355,6 +4355,7 @@ static inline int set_compress_context(struct inode *inode)
F2FS_I(inode)->i_compress_flag |=
F2FS_OPTION(sbi).compress_level <<
COMPRESS_LEVEL_OFFSET;
+ F2FS_I(inode)->i_flags &= ~F2FS_NOCOMP_FL;
F2FS_I(inode)->i_flags |= F2FS_COMPR_FL;
set_inode_flag(inode, FI_COMPRESSED_FILE);
stat_inc_compr_inode(inode);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e5..36ec5cf7cf859 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,170 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static inline int is_extension_exist(const unsigned char *s, const char *sub,
+ bool tmp_ext)
+{
+ size_t slen = strlen(s);
+ size_t sublen = strlen(sub);
+ int i;
+
+ if (sublen == 1 && *sub == '*')
+ return 1;
+
+ /*
+ * filename format of multimedia file should be defined as:
+ * "filename + '.' + extension + (optional: '.' + temp extension)".
+ */
+ if (slen < sublen + 2)
+ return 0;
+
+ if (!tmp_ext) {
+ /* file has no temp extension */
+ if (s[slen - sublen - 1] != '.')
+ return 0;
+ return !strncasecmp(s + slen - sublen, sub, sublen);
+ }
+
+ for (i = 1; i < slen - sublen; i++) {
+ if (s[i] != '.')
+ continue;
+ if (!strncasecmp(s + i + 1, sub, sublen))
+ return 1;
+ }
+
+ return 0;
+}
+
+int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
+ bool hot, bool set)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ int hot_count = sbi->raw_super->hot_ext_count;
+ int total_count = cold_count + hot_count;
+ int start, count;
+ int i;
+
+ if (set) {
+ if (total_count == F2FS_MAX_EXTENSION)
+ return -EINVAL;
+ } else {
+ if (!hot && !cold_count)
+ return -EINVAL;
+ if (hot && !hot_count)
+ return -EINVAL;
+ }
+
+ if (hot) {
+ start = cold_count;
+ count = total_count;
+ } else {
+ start = 0;
+ count = cold_count;
+ }
+
+ for (i = start; i < count; i++) {
+ if (strcmp(name, extlist[i]))
+ continue;
+
+ if (set)
+ return -EINVAL;
+
+ memcpy(extlist[i], extlist[i + 1],
+ F2FS_EXTENSION_LEN * (total_count - i - 1));
+ memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
+ if (hot)
+ sbi->raw_super->hot_ext_count = hot_count - 1;
+ else
+ sbi->raw_super->extension_count =
+ cpu_to_le32(cold_count - 1);
+ return 0;
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ if (hot) {
+ memcpy(extlist[count], name, strlen(name));
+ sbi->raw_super->hot_ext_count = hot_count + 1;
+ } else {
+ char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
+
+ memcpy(buf, &extlist[cold_count],
+ F2FS_EXTENSION_LEN * hot_count);
+ memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
+ memcpy(extlist[cold_count], name, strlen(name));
+ memcpy(&extlist[cold_count + 1], buf,
+ F2FS_EXTENSION_LEN * hot_count);
+ sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
+ }
+ return 0;
+}
+
+static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
+ struct inode *inode, const unsigned char *name)
+{
+ struct f2fs_inode_info *fi = F2FS_I(inode);
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ unsigned char (*noext)[F2FS_EXTENSION_LEN] =
+ F2FS_OPTION(sbi).noextensions;
+ unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
+ unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
+ unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
+ bool may_compress = false;
+ int i, cold_count, hot_count;
+
+ if (!f2fs_sb_has_compression(sbi) || !name)
+ return;
+ if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+ return;
+
+ /* Inherit the compression flag in directory */
+ if (fi->i_flags & FS_COMPR_FL)
+ may_compress = true;
+
+ /* Start to check extension list for regular file */
+ if ((!ext_cnt && !noext_cnt) || S_ISDIR(inode->i_mode))
+ goto set_compress;
+
+ /* Don't compress hot files. */
+ f2fs_down_read(&sbi->sb_lock);
+ cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ hot_count = sbi->raw_super->hot_ext_count;
+ for (i = cold_count; i < cold_count + hot_count; i++)
+ if (is_extension_exist(name, extlist[i], false)) {
+ may_compress = false;
+ f2fs_up_read(&sbi->sb_lock);
+ goto set_compress;
+ }
+ f2fs_up_read(&sbi->sb_lock);
+
+ /* Don't compress unallowed extension. */
+ for (i = 0; i < noext_cnt; i++) {
+ if (is_extension_exist(name, noext[i], false)) {
+ may_compress = false;
+ goto set_compress;
+ }
+ }
+
+ /* Compress wanting extension. */
+ for (i = 0; i < ext_cnt; i++) {
+ if (is_extension_exist(name, ext[i], false)) {
+ may_compress = true;
+ goto set_compress;
+ }
+ }
+
+set_compress:
+ if (may_compress)
+ set_compress_context(inode);
+ else if (fi->i_flags & F2FS_COMPR_FL)
+ fi->i_flags &= ~F2FS_COMPR_FL;
+}
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -114,12 +276,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
set_inode_flag(inode, FI_PROJ_INHERIT);

- if (f2fs_sb_has_compression(sbi)) {
- /* Inherit the compression flag in directory */
- if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
- f2fs_may_compress(inode))
- set_compress_context(inode);
- }
+ /* Check compression first. */
+ set_compress_new_inode(sbi, dir, inode, name);

/* Should enable inline_data after compression set */
if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
@@ -153,40 +311,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
return ERR_PTR(err);
}

-static inline int is_extension_exist(const unsigned char *s, const char *sub,
- bool tmp_ext)
-{
- size_t slen = strlen(s);
- size_t sublen = strlen(sub);
- int i;
-
- if (sublen == 1 && *sub == '*')
- return 1;
-
- /*
- * filename format of multimedia file should be defined as:
- * "filename + '.' + extension + (optional: '.' + temp extension)".
- */
- if (slen < sublen + 2)
- return 0;
-
- if (!tmp_ext) {
- /* file has no temp extension */
- if (s[slen - sublen - 1] != '.')
- return 0;
- return !strncasecmp(s + slen - sublen, sub, sublen);
- }
-
- for (i = 1; i < slen - sublen; i++) {
- if (s[i] != '.')
- continue;
- if (!strncasecmp(s + i + 1, sub, sublen))
- return 1;
- }
-
- return 0;
-}
-
/*
* Set file's temperature for hot/cold data separation
*/
@@ -217,124 +341,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
file_set_hot(inode);
}

-int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
- bool hot, bool set)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- int hot_count = sbi->raw_super->hot_ext_count;
- int total_count = cold_count + hot_count;
- int start, count;
- int i;
-
- if (set) {
- if (total_count == F2FS_MAX_EXTENSION)
- return -EINVAL;
- } else {
- if (!hot && !cold_count)
- return -EINVAL;
- if (hot && !hot_count)
- return -EINVAL;
- }
-
- if (hot) {
- start = cold_count;
- count = total_count;
- } else {
- start = 0;
- count = cold_count;
- }
-
- for (i = start; i < count; i++) {
- if (strcmp(name, extlist[i]))
- continue;
-
- if (set)
- return -EINVAL;
-
- memcpy(extlist[i], extlist[i + 1],
- F2FS_EXTENSION_LEN * (total_count - i - 1));
- memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
- if (hot)
- sbi->raw_super->hot_ext_count = hot_count - 1;
- else
- sbi->raw_super->extension_count =
- cpu_to_le32(cold_count - 1);
- return 0;
- }
-
- if (!set)
- return -EINVAL;
-
- if (hot) {
- memcpy(extlist[count], name, strlen(name));
- sbi->raw_super->hot_ext_count = hot_count + 1;
- } else {
- char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
-
- memcpy(buf, &extlist[cold_count],
- F2FS_EXTENSION_LEN * hot_count);
- memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
- memcpy(extlist[cold_count], name, strlen(name));
- memcpy(&extlist[cold_count + 1], buf,
- F2FS_EXTENSION_LEN * hot_count);
- sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
- }
- return 0;
-}
-
-static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
- const unsigned char *name)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
- unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
- unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
- unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
- int i, cold_count, hot_count;
-
- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
- !f2fs_may_compress(inode) ||
- (!ext_cnt && !noext_cnt))
- return;
-
- f2fs_down_read(&sbi->sb_lock);
-
- cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- hot_count = sbi->raw_super->hot_ext_count;
-
- for (i = cold_count; i < cold_count + hot_count; i++) {
- if (is_extension_exist(name, extlist[i], false)) {
- f2fs_up_read(&sbi->sb_lock);
- return;
- }
- }
-
- f2fs_up_read(&sbi->sb_lock);
-
- for (i = 0; i < noext_cnt; i++) {
- if (is_extension_exist(name, noext[i], false)) {
- f2fs_disable_compressed_file(inode);
- return;
- }
- }
-
- if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
- return;
-
- for (i = 0; i < ext_cnt; i++) {
- if (!is_extension_exist(name, ext[i], false))
- continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
- set_compress_context(inode);
- return;
- }
-}
-
static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
@@ -352,15 +358,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +693,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +764,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +821,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +860,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.25.1


2022-11-15 22:10:57

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v3] f2fs: fix to enable compress for newly created file if extension matches

On 11/16, Sheng Yong wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/namei.c | 336 ++++++++++++++++++++++++------------------------
> 2 files changed, 171 insertions(+), 166 deletions(-)
> ---
>
> Hi, Jaegeuk, Chao,
>
> How about adding a bool `may_compress' in set_compress_new_inode, set
> `my_compress` according to several conditions. If it is false, clear
> F2FS_COMPR_FL.
>
> And set_compress_context is also changed to clear F2FS_NOCOMP_FL,
> otherwise, if F2FS_NOCOMP_FL is inherited from parent and hit
> compress_extension, both F2FS_NOCOMP_FL and F2FS_COMPR_FL are set.
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6a8cbf5bb1871..a3420fbb29214 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4355,6 +4355,7 @@ static inline int set_compress_context(struct inode *inode)
> F2FS_I(inode)->i_compress_flag |=
> F2FS_OPTION(sbi).compress_level <<
> COMPRESS_LEVEL_OFFSET;
> + F2FS_I(inode)->i_flags &= ~F2FS_NOCOMP_FL;
> F2FS_I(inode)->i_flags |= F2FS_COMPR_FL;
> set_inode_flag(inode, FI_COMPRESSED_FILE);
> stat_inc_compr_inode(inode);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e5..36ec5cf7cf859 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,170 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + bool may_compress = false;
> + int i, cold_count, hot_count;
> +
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return;
> +
> + /* Inherit the compression flag in directory */
> + if (fi->i_flags & FS_COMPR_FL)
> + may_compress = true;
> +
> + /* Start to check extension list for regular file */
> + if ((!ext_cnt && !noext_cnt) || S_ISDIR(inode->i_mode))

This doesn't address the Chao's point. It seems not much motivation to add
may_compress. Let me try to combine some with the previous patch.

> + goto set_compress;
> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false)) {
> + may_compress = false;
> + f2fs_up_read(&sbi->sb_lock);
> + goto set_compress;
> + }
> + f2fs_up_read(&sbi->sb_lock);
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++) {
> + if (is_extension_exist(name, noext[i], false)) {
> + may_compress = false;
> + goto set_compress;
> + }
> + }
> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + may_compress = true;
> + goto set_compress;
> + }
> + }
> +
> +set_compress:
> + if (may_compress)
> + set_compress_context(inode);
> + else if (fi->i_flags & F2FS_COMPR_FL)
> + fi->i_flags &= ~F2FS_COMPR_FL;
> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +276,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +311,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +341,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +358,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +693,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +764,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +821,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +860,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> --
> 2.25.1

2022-11-15 23:13:43

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---

Checking this version.

fs/f2fs/f2fs.h | 2 +-
fs/f2fs/namei.c | 328 ++++++++++++++++++++++++------------------------
2 files changed, 163 insertions(+), 167 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b89b5d755ce0..dedac413bf64 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
/* Flags that should be inherited by new inodes from their parent. */
#define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
- F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
+ F2FS_CASEFOLD_FL)

/* Flags that are appropriate for regular files (all but dir-specific ones). */
#define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e..cb7441a19d22 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,161 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static inline int is_extension_exist(const unsigned char *s, const char *sub,
+ bool tmp_ext)
+{
+ size_t slen = strlen(s);
+ size_t sublen = strlen(sub);
+ int i;
+
+ if (sublen == 1 && *sub == '*')
+ return 1;
+
+ /*
+ * filename format of multimedia file should be defined as:
+ * "filename + '.' + extension + (optional: '.' + temp extension)".
+ */
+ if (slen < sublen + 2)
+ return 0;
+
+ if (!tmp_ext) {
+ /* file has no temp extension */
+ if (s[slen - sublen - 1] != '.')
+ return 0;
+ return !strncasecmp(s + slen - sublen, sub, sublen);
+ }
+
+ for (i = 1; i < slen - sublen; i++) {
+ if (s[i] != '.')
+ continue;
+ if (!strncasecmp(s + i + 1, sub, sublen))
+ return 1;
+ }
+
+ return 0;
+}
+
+int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
+ bool hot, bool set)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ int hot_count = sbi->raw_super->hot_ext_count;
+ int total_count = cold_count + hot_count;
+ int start, count;
+ int i;
+
+ if (set) {
+ if (total_count == F2FS_MAX_EXTENSION)
+ return -EINVAL;
+ } else {
+ if (!hot && !cold_count)
+ return -EINVAL;
+ if (hot && !hot_count)
+ return -EINVAL;
+ }
+
+ if (hot) {
+ start = cold_count;
+ count = total_count;
+ } else {
+ start = 0;
+ count = cold_count;
+ }
+
+ for (i = start; i < count; i++) {
+ if (strcmp(name, extlist[i]))
+ continue;
+
+ if (set)
+ return -EINVAL;
+
+ memcpy(extlist[i], extlist[i + 1],
+ F2FS_EXTENSION_LEN * (total_count - i - 1));
+ memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
+ if (hot)
+ sbi->raw_super->hot_ext_count = hot_count - 1;
+ else
+ sbi->raw_super->extension_count =
+ cpu_to_le32(cold_count - 1);
+ return 0;
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ if (hot) {
+ memcpy(extlist[count], name, strlen(name));
+ sbi->raw_super->hot_ext_count = hot_count + 1;
+ } else {
+ char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
+
+ memcpy(buf, &extlist[cold_count],
+ F2FS_EXTENSION_LEN * hot_count);
+ memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
+ memcpy(extlist[cold_count], name, strlen(name));
+ memcpy(&extlist[cold_count + 1], buf,
+ F2FS_EXTENSION_LEN * hot_count);
+ sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
+ }
+ return 0;
+}
+
+static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
+ struct inode *inode, const unsigned char *name)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ unsigned char (*noext)[F2FS_EXTENSION_LEN] =
+ F2FS_OPTION(sbi).noextensions;
+ unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
+ unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
+ unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
+ int i, cold_count, hot_count;
+
+ /* Caller should give the name of regular file or directory. */
+ if (!f2fs_sb_has_compression(sbi) || !name)
+ return;
+
+ if (S_ISDIR(inode->i_mode))
+ goto inherit_comp;
+
+ /* Don't compress hot files. */
+ f2fs_down_read(&sbi->sb_lock);
+ cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ hot_count = sbi->raw_super->hot_ext_count;
+ for (i = cold_count; i < cold_count + hot_count; i++)
+ if (is_extension_exist(name, extlist[i], false))
+ break;
+ f2fs_up_read(&sbi->sb_lock);
+ if (i < (cold_count + hot_count))
+ return;
+
+ /* Don't compress unallowed extension. */
+ for (i = 0; i < noext_cnt; i++)
+ if (is_extension_exist(name, noext[i], false))
+ return;
+inherit_comp:
+ /* Inherit the {no-}compression flag in directory */
+ if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL) {
+ F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
+ return;
+ } else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL) {
+ set_compress_context(inode);
+ return;
+ }
+
+ /* Compress wanting extension. */
+ for (i = 0; i < ext_cnt; i++) {
+ if (is_extension_exist(name, ext[i], false)) {
+ set_compress_context(inode);
+ return;
+ }
+ }
+}
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -114,12 +267,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
set_inode_flag(inode, FI_PROJ_INHERIT);

- if (f2fs_sb_has_compression(sbi)) {
- /* Inherit the compression flag in directory */
- if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
- f2fs_may_compress(inode))
- set_compress_context(inode);
- }
+ /* Check compression first. */
+ set_compress_new_inode(sbi, dir, inode, name);

/* Should enable inline_data after compression set */
if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
@@ -153,40 +302,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
return ERR_PTR(err);
}

-static inline int is_extension_exist(const unsigned char *s, const char *sub,
- bool tmp_ext)
-{
- size_t slen = strlen(s);
- size_t sublen = strlen(sub);
- int i;
-
- if (sublen == 1 && *sub == '*')
- return 1;
-
- /*
- * filename format of multimedia file should be defined as:
- * "filename + '.' + extension + (optional: '.' + temp extension)".
- */
- if (slen < sublen + 2)
- return 0;
-
- if (!tmp_ext) {
- /* file has no temp extension */
- if (s[slen - sublen - 1] != '.')
- return 0;
- return !strncasecmp(s + slen - sublen, sub, sublen);
- }
-
- for (i = 1; i < slen - sublen; i++) {
- if (s[i] != '.')
- continue;
- if (!strncasecmp(s + i + 1, sub, sublen))
- return 1;
- }
-
- return 0;
-}
-
/*
* Set file's temperature for hot/cold data separation
*/
@@ -217,124 +332,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
file_set_hot(inode);
}

-int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
- bool hot, bool set)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- int hot_count = sbi->raw_super->hot_ext_count;
- int total_count = cold_count + hot_count;
- int start, count;
- int i;
-
- if (set) {
- if (total_count == F2FS_MAX_EXTENSION)
- return -EINVAL;
- } else {
- if (!hot && !cold_count)
- return -EINVAL;
- if (hot && !hot_count)
- return -EINVAL;
- }
-
- if (hot) {
- start = cold_count;
- count = total_count;
- } else {
- start = 0;
- count = cold_count;
- }
-
- for (i = start; i < count; i++) {
- if (strcmp(name, extlist[i]))
- continue;
-
- if (set)
- return -EINVAL;
-
- memcpy(extlist[i], extlist[i + 1],
- F2FS_EXTENSION_LEN * (total_count - i - 1));
- memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
- if (hot)
- sbi->raw_super->hot_ext_count = hot_count - 1;
- else
- sbi->raw_super->extension_count =
- cpu_to_le32(cold_count - 1);
- return 0;
- }
-
- if (!set)
- return -EINVAL;
-
- if (hot) {
- memcpy(extlist[count], name, strlen(name));
- sbi->raw_super->hot_ext_count = hot_count + 1;
- } else {
- char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
-
- memcpy(buf, &extlist[cold_count],
- F2FS_EXTENSION_LEN * hot_count);
- memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
- memcpy(extlist[cold_count], name, strlen(name));
- memcpy(&extlist[cold_count + 1], buf,
- F2FS_EXTENSION_LEN * hot_count);
- sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
- }
- return 0;
-}
-
-static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
- const unsigned char *name)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
- unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
- unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
- unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
- int i, cold_count, hot_count;
-
- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
- !f2fs_may_compress(inode) ||
- (!ext_cnt && !noext_cnt))
- return;
-
- f2fs_down_read(&sbi->sb_lock);
-
- cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- hot_count = sbi->raw_super->hot_ext_count;
-
- for (i = cold_count; i < cold_count + hot_count; i++) {
- if (is_extension_exist(name, extlist[i], false)) {
- f2fs_up_read(&sbi->sb_lock);
- return;
- }
- }
-
- f2fs_up_read(&sbi->sb_lock);
-
- for (i = 0; i < noext_cnt; i++) {
- if (is_extension_exist(name, noext[i], false)) {
- f2fs_disable_compressed_file(inode);
- return;
- }
- }
-
- if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
- return;
-
- for (i = 0; i < ext_cnt; i++) {
- if (!is_extension_exist(name, ext[i], false))
- continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
- set_compress_context(inode);
- return;
- }
-}
-
static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
@@ -352,15 +349,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +684,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +755,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
+ dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +813,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +852,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.38.1.493.g58b659f92b-goog


2022-11-15 23:48:31

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v3] f2fs: fix to enable compress for newly created file if extension matches

On 11/16, Sheng Yong wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/namei.c | 336 ++++++++++++++++++++++++------------------------
> 2 files changed, 171 insertions(+), 166 deletions(-)
> ---
>
> Hi, Jaegeuk, Chao,
>
> How about adding a bool `may_compress' in set_compress_new_inode, set
> `my_compress` according to several conditions. If it is false, clear
> F2FS_COMPR_FL.
>
> And set_compress_context is also changed to clear F2FS_NOCOMP_FL,
> otherwise, if F2FS_NOCOMP_FL is inherited from parent and hit
> compress_extension, both F2FS_NOCOMP_FL and F2FS_COMPR_FL are set.



>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6a8cbf5bb1871..a3420fbb29214 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4355,6 +4355,7 @@ static inline int set_compress_context(struct inode *inode)
> F2FS_I(inode)->i_compress_flag |=
> F2FS_OPTION(sbi).compress_level <<
> COMPRESS_LEVEL_OFFSET;
> + F2FS_I(inode)->i_flags &= ~F2FS_NOCOMP_FL;
> F2FS_I(inode)->i_flags |= F2FS_COMPR_FL;
> set_inode_flag(inode, FI_COMPRESSED_FILE);
> stat_inc_compr_inode(inode);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e5..36ec5cf7cf859 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,170 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + bool may_compress = false;
> + int i, cold_count, hot_count;
> +
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return;
> +
> + /* Inherit the compression flag in directory */
> + if (fi->i_flags & FS_COMPR_FL)
> + may_compress = true;
> +
> + /* Start to check extension list for regular file */
> + if ((!ext_cnt && !noext_cnt) || S_ISDIR(inode->i_mode))
> + goto set_compress;
> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false)) {
> + may_compress = false;
> + f2fs_up_read(&sbi->sb_lock);
> + goto set_compress;
> + }
> + f2fs_up_read(&sbi->sb_lock);
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++) {
> + if (is_extension_exist(name, noext[i], false)) {
> + may_compress = false;
> + goto set_compress;
> + }
> + }
> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + may_compress = true;
> + goto set_compress;
> + }
> + }
> +
> +set_compress:
> + if (may_compress)
> + set_compress_context(inode);
> + else if (fi->i_flags & F2FS_COMPR_FL)
> + fi->i_flags &= ~F2FS_COMPR_FL;

Let's inherit F2FS_COMPR_FL and F2FS_NOCOMP_FL here instead of getting it
before.

> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +276,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +311,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +341,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +358,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +693,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +764,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +821,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +860,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> --
> 2.25.1

2022-11-16 02:53:16

by Sheng Yong

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches



On 2022/11/16 7:00, Jaegeuk Kim wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
>
> Checking this version.
>
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/namei.c | 328 ++++++++++++++++++++++++------------------------
> 2 files changed, 163 insertions(+), 167 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b89b5d755ce0..dedac413bf64 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
> /* Flags that should be inherited by new inodes from their parent. */
> #define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
> F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> - F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
> + F2FS_CASEFOLD_FL)
>
> /* Flags that are appropriate for regular files (all but dir-specific ones). */
> #define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e..cb7441a19d22 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,161 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + int i, cold_count, hot_count;
> +
> + /* Caller should give the name of regular file or directory. */
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> +
> + if (S_ISDIR(inode->i_mode))
> + goto inherit_comp;
> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false))
> + break;
> + f2fs_up_read(&sbi->sb_lock);
> + if (i < (cold_count + hot_count))
> + return;
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++)
> + if (is_extension_exist(name, noext[i], false))
> + return;
> +inherit_comp:
> + /* Inherit the {no-}compression flag in directory */
> + if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL) {
> + F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
> + return;

Hi, Jaegeuk,
It should not return immediately here. The scenario here is:

mount -o compress_extension=txt,compress_mode=user
mkdir dir
f2fs_io setflags nocompression dir # set dir nocompression
touch dir/file.txt
f2fs_io getflags compression dir/file.txt # file.txt should have
compression flag

According to f2fs.rst, if dir is set as nocompression, but file's
extension hit compress_extension, then the file should be compressed.
And before set_compress_context later, F2FS_NOCOMP_FL should be
cleared first.

thanks,
shengyong

> + } else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL) {
> + set_compress_context(inode);
> + return;
> + }
> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + set_compress_context(inode);
> + return;
> + }
> + }
> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +267,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +302,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +332,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +349,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +684,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +755,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
> + dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +813,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +852,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>

2022-11-17 01:25:36

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/f2fs.h | 2 +-
fs/f2fs/namei.c | 325 +++++++++++++++++++++++-------------------------
2 files changed, 160 insertions(+), 167 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b89b5d755ce0..dedac413bf64 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
/* Flags that should be inherited by new inodes from their parent. */
#define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
- F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
+ F2FS_CASEFOLD_FL)

/* Flags that are appropriate for regular files (all but dir-specific ones). */
#define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e..c25009bb72f2 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,158 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static inline int is_extension_exist(const unsigned char *s, const char *sub,
+ bool tmp_ext)
+{
+ size_t slen = strlen(s);
+ size_t sublen = strlen(sub);
+ int i;
+
+ if (sublen == 1 && *sub == '*')
+ return 1;
+
+ /*
+ * filename format of multimedia file should be defined as:
+ * "filename + '.' + extension + (optional: '.' + temp extension)".
+ */
+ if (slen < sublen + 2)
+ return 0;
+
+ if (!tmp_ext) {
+ /* file has no temp extension */
+ if (s[slen - sublen - 1] != '.')
+ return 0;
+ return !strncasecmp(s + slen - sublen, sub, sublen);
+ }
+
+ for (i = 1; i < slen - sublen; i++) {
+ if (s[i] != '.')
+ continue;
+ if (!strncasecmp(s + i + 1, sub, sublen))
+ return 1;
+ }
+
+ return 0;
+}
+
+int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
+ bool hot, bool set)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ int hot_count = sbi->raw_super->hot_ext_count;
+ int total_count = cold_count + hot_count;
+ int start, count;
+ int i;
+
+ if (set) {
+ if (total_count == F2FS_MAX_EXTENSION)
+ return -EINVAL;
+ } else {
+ if (!hot && !cold_count)
+ return -EINVAL;
+ if (hot && !hot_count)
+ return -EINVAL;
+ }
+
+ if (hot) {
+ start = cold_count;
+ count = total_count;
+ } else {
+ start = 0;
+ count = cold_count;
+ }
+
+ for (i = start; i < count; i++) {
+ if (strcmp(name, extlist[i]))
+ continue;
+
+ if (set)
+ return -EINVAL;
+
+ memcpy(extlist[i], extlist[i + 1],
+ F2FS_EXTENSION_LEN * (total_count - i - 1));
+ memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
+ if (hot)
+ sbi->raw_super->hot_ext_count = hot_count - 1;
+ else
+ sbi->raw_super->extension_count =
+ cpu_to_le32(cold_count - 1);
+ return 0;
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ if (hot) {
+ memcpy(extlist[count], name, strlen(name));
+ sbi->raw_super->hot_ext_count = hot_count + 1;
+ } else {
+ char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
+
+ memcpy(buf, &extlist[cold_count],
+ F2FS_EXTENSION_LEN * hot_count);
+ memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
+ memcpy(extlist[cold_count], name, strlen(name));
+ memcpy(&extlist[cold_count + 1], buf,
+ F2FS_EXTENSION_LEN * hot_count);
+ sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
+ }
+ return 0;
+}
+
+static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
+ struct inode *inode, const unsigned char *name)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ unsigned char (*noext)[F2FS_EXTENSION_LEN] =
+ F2FS_OPTION(sbi).noextensions;
+ unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
+ unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
+ unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
+ int i, cold_count, hot_count;
+
+ /* Caller should give the name of regular file or directory. */
+ if (!f2fs_sb_has_compression(sbi) || !name)
+ return;
+
+ if (S_ISDIR(inode->i_mode))
+ goto inherit_comp;
+
+ /* Don't compress hot files. */
+ f2fs_down_read(&sbi->sb_lock);
+ cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ hot_count = sbi->raw_super->hot_ext_count;
+ for (i = cold_count; i < cold_count + hot_count; i++)
+ if (is_extension_exist(name, extlist[i], false))
+ break;
+ f2fs_up_read(&sbi->sb_lock);
+ if (i < (cold_count + hot_count))
+ return;
+
+ /* Don't compress unallowed extension. */
+ for (i = 0; i < noext_cnt; i++)
+ if (is_extension_exist(name, noext[i], false))
+ return;
+
+ /* Compress wanting extension. */
+ for (i = 0; i < ext_cnt; i++) {
+ if (is_extension_exist(name, ext[i], false)) {
+ set_compress_context(inode);
+ return;
+ }
+ }
+inherit_comp:
+ /* Inherit the {no-}compression flag in directory */
+ if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL)
+ F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
+ else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL)
+ set_compress_context(inode);
+}
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -114,12 +264,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
set_inode_flag(inode, FI_PROJ_INHERIT);

- if (f2fs_sb_has_compression(sbi)) {
- /* Inherit the compression flag in directory */
- if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
- f2fs_may_compress(inode))
- set_compress_context(inode);
- }
+ /* Check compression first. */
+ set_compress_new_inode(sbi, dir, inode, name);

/* Should enable inline_data after compression set */
if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
@@ -153,40 +299,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
return ERR_PTR(err);
}

-static inline int is_extension_exist(const unsigned char *s, const char *sub,
- bool tmp_ext)
-{
- size_t slen = strlen(s);
- size_t sublen = strlen(sub);
- int i;
-
- if (sublen == 1 && *sub == '*')
- return 1;
-
- /*
- * filename format of multimedia file should be defined as:
- * "filename + '.' + extension + (optional: '.' + temp extension)".
- */
- if (slen < sublen + 2)
- return 0;
-
- if (!tmp_ext) {
- /* file has no temp extension */
- if (s[slen - sublen - 1] != '.')
- return 0;
- return !strncasecmp(s + slen - sublen, sub, sublen);
- }
-
- for (i = 1; i < slen - sublen; i++) {
- if (s[i] != '.')
- continue;
- if (!strncasecmp(s + i + 1, sub, sublen))
- return 1;
- }
-
- return 0;
-}
-
/*
* Set file's temperature for hot/cold data separation
*/
@@ -217,124 +329,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
file_set_hot(inode);
}

-int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
- bool hot, bool set)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- int hot_count = sbi->raw_super->hot_ext_count;
- int total_count = cold_count + hot_count;
- int start, count;
- int i;
-
- if (set) {
- if (total_count == F2FS_MAX_EXTENSION)
- return -EINVAL;
- } else {
- if (!hot && !cold_count)
- return -EINVAL;
- if (hot && !hot_count)
- return -EINVAL;
- }
-
- if (hot) {
- start = cold_count;
- count = total_count;
- } else {
- start = 0;
- count = cold_count;
- }
-
- for (i = start; i < count; i++) {
- if (strcmp(name, extlist[i]))
- continue;
-
- if (set)
- return -EINVAL;
-
- memcpy(extlist[i], extlist[i + 1],
- F2FS_EXTENSION_LEN * (total_count - i - 1));
- memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
- if (hot)
- sbi->raw_super->hot_ext_count = hot_count - 1;
- else
- sbi->raw_super->extension_count =
- cpu_to_le32(cold_count - 1);
- return 0;
- }
-
- if (!set)
- return -EINVAL;
-
- if (hot) {
- memcpy(extlist[count], name, strlen(name));
- sbi->raw_super->hot_ext_count = hot_count + 1;
- } else {
- char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
-
- memcpy(buf, &extlist[cold_count],
- F2FS_EXTENSION_LEN * hot_count);
- memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
- memcpy(extlist[cold_count], name, strlen(name));
- memcpy(&extlist[cold_count + 1], buf,
- F2FS_EXTENSION_LEN * hot_count);
- sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
- }
- return 0;
-}
-
-static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
- const unsigned char *name)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
- unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
- unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
- unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
- int i, cold_count, hot_count;
-
- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
- !f2fs_may_compress(inode) ||
- (!ext_cnt && !noext_cnt))
- return;
-
- f2fs_down_read(&sbi->sb_lock);
-
- cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- hot_count = sbi->raw_super->hot_ext_count;
-
- for (i = cold_count; i < cold_count + hot_count; i++) {
- if (is_extension_exist(name, extlist[i], false)) {
- f2fs_up_read(&sbi->sb_lock);
- return;
- }
- }
-
- f2fs_up_read(&sbi->sb_lock);
-
- for (i = 0; i < noext_cnt; i++) {
- if (is_extension_exist(name, noext[i], false)) {
- f2fs_disable_compressed_file(inode);
- return;
- }
- }
-
- if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
- return;
-
- for (i = 0; i < ext_cnt; i++) {
- if (!is_extension_exist(name, ext[i], false))
- continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
- set_compress_context(inode);
- return;
- }
-}
-
static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
@@ -352,15 +346,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +681,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +752,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
+ dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +810,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +849,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.38.1.584.g0f3c55d4c2-goog


2022-11-17 01:40:29

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

On 11/16, Sheng Yong wrote:
>
>
> On 2022/11/16 7:00, Jaegeuk Kim wrote:
> > If compress_extension is set, and a newly created file matches the
> > extension, the file could be marked as compression file. However,
> > if inline_data is also enabled, there is no chance to check its
> > extension since f2fs_should_compress() always returns false.
> >
> > This patch moves set_compress_inode(), which do extension check, in
> > f2fs_should_compress() to check extensions before setting inline
> > data flag.
> >
> > Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> > Signed-off-by: Sheng Yong <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> >
> > Checking this version.
> >
> > fs/f2fs/f2fs.h | 2 +-
> > fs/f2fs/namei.c | 328 ++++++++++++++++++++++++------------------------
> > 2 files changed, 163 insertions(+), 167 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index b89b5d755ce0..dedac413bf64 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
> > /* Flags that should be inherited by new inodes from their parent. */
> > #define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
> > F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> > - F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
> > + F2FS_CASEFOLD_FL)
> > /* Flags that are appropriate for regular files (all but dir-specific ones). */
> > #define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > index e104409c3a0e..cb7441a19d22 100644
> > --- a/fs/f2fs/namei.c
> > +++ b/fs/f2fs/namei.c
> > @@ -22,8 +22,161 @@
> > #include "acl.h"
> > #include <trace/events/f2fs.h>
> > +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> > + bool tmp_ext)
> > +{
> > + size_t slen = strlen(s);
> > + size_t sublen = strlen(sub);
> > + int i;
> > +
> > + if (sublen == 1 && *sub == '*')
> > + return 1;
> > +
> > + /*
> > + * filename format of multimedia file should be defined as:
> > + * "filename + '.' + extension + (optional: '.' + temp extension)".
> > + */
> > + if (slen < sublen + 2)
> > + return 0;
> > +
> > + if (!tmp_ext) {
> > + /* file has no temp extension */
> > + if (s[slen - sublen - 1] != '.')
> > + return 0;
> > + return !strncasecmp(s + slen - sublen, sub, sublen);
> > + }
> > +
> > + for (i = 1; i < slen - sublen; i++) {
> > + if (s[i] != '.')
> > + continue;
> > + if (!strncasecmp(s + i + 1, sub, sublen))
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> > + bool hot, bool set)
> > +{
> > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > + int hot_count = sbi->raw_super->hot_ext_count;
> > + int total_count = cold_count + hot_count;
> > + int start, count;
> > + int i;
> > +
> > + if (set) {
> > + if (total_count == F2FS_MAX_EXTENSION)
> > + return -EINVAL;
> > + } else {
> > + if (!hot && !cold_count)
> > + return -EINVAL;
> > + if (hot && !hot_count)
> > + return -EINVAL;
> > + }
> > +
> > + if (hot) {
> > + start = cold_count;
> > + count = total_count;
> > + } else {
> > + start = 0;
> > + count = cold_count;
> > + }
> > +
> > + for (i = start; i < count; i++) {
> > + if (strcmp(name, extlist[i]))
> > + continue;
> > +
> > + if (set)
> > + return -EINVAL;
> > +
> > + memcpy(extlist[i], extlist[i + 1],
> > + F2FS_EXTENSION_LEN * (total_count - i - 1));
> > + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> > + if (hot)
> > + sbi->raw_super->hot_ext_count = hot_count - 1;
> > + else
> > + sbi->raw_super->extension_count =
> > + cpu_to_le32(cold_count - 1);
> > + return 0;
> > + }
> > +
> > + if (!set)
> > + return -EINVAL;
> > +
> > + if (hot) {
> > + memcpy(extlist[count], name, strlen(name));
> > + sbi->raw_super->hot_ext_count = hot_count + 1;
> > + } else {
> > + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> > +
> > + memcpy(buf, &extlist[cold_count],
> > + F2FS_EXTENSION_LEN * hot_count);
> > + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> > + memcpy(extlist[cold_count], name, strlen(name));
> > + memcpy(&extlist[cold_count + 1], buf,
> > + F2FS_EXTENSION_LEN * hot_count);
> > + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> > + }
> > + return 0;
> > +}
> > +
> > +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> > + struct inode *inode, const unsigned char *name)
> > +{
> > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> > + F2FS_OPTION(sbi).noextensions;
> > + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> > + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > + int i, cold_count, hot_count;
> > +
> > + /* Caller should give the name of regular file or directory. */
> > + if (!f2fs_sb_has_compression(sbi) || !name)
> > + return;
> > +
> > + if (S_ISDIR(inode->i_mode))
> > + goto inherit_comp;
> > +
> > + /* Don't compress hot files. */
> > + f2fs_down_read(&sbi->sb_lock);
> > + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > + hot_count = sbi->raw_super->hot_ext_count;
> > + for (i = cold_count; i < cold_count + hot_count; i++)
> > + if (is_extension_exist(name, extlist[i], false))
> > + break;
> > + f2fs_up_read(&sbi->sb_lock);
> > + if (i < (cold_count + hot_count))
> > + return;
> > +
> > + /* Don't compress unallowed extension. */
> > + for (i = 0; i < noext_cnt; i++)
> > + if (is_extension_exist(name, noext[i], false))
> > + return;
> > +inherit_comp:
> > + /* Inherit the {no-}compression flag in directory */
> > + if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL) {
> > + F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
> > + return;
>
> Hi, Jaegeuk,
> It should not return immediately here. The scenario here is:
>
> mount -o compress_extension=txt,compress_mode=user
> mkdir dir
> f2fs_io setflags nocompression dir # set dir nocompression
> touch dir/file.txt
> f2fs_io getflags compression dir/file.txt # file.txt should have
> compression flag

I see. Sent out v4.

>
> According to f2fs.rst, if dir is set as nocompression, but file's
> extension hit compress_extension, then the file should be compressed.
> And before set_compress_context later, F2FS_NOCOMP_FL should be
> cleared first.
>
> thanks,
> shengyong
>
> > + } else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL) {
> > + set_compress_context(inode);
> > + return;
> > + }
> > +
> > + /* Compress wanting extension. */
> > + for (i = 0; i < ext_cnt; i++) {
> > + if (is_extension_exist(name, ext[i], false)) {
> > + set_compress_context(inode);
> > + return;
> > + }
> > + }
> > +}
> > +
> > static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > - struct inode *dir, umode_t mode)
> > + struct inode *dir, umode_t mode,
> > + const char *name)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> > nid_t ino;
> > @@ -114,12 +267,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> > set_inode_flag(inode, FI_PROJ_INHERIT);
> > - if (f2fs_sb_has_compression(sbi)) {
> > - /* Inherit the compression flag in directory */
> > - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> > - f2fs_may_compress(inode))
> > - set_compress_context(inode);
> > - }
> > + /* Check compression first. */
> > + set_compress_new_inode(sbi, dir, inode, name);
> > /* Should enable inline_data after compression set */
> > if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> > @@ -153,40 +302,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > return ERR_PTR(err);
> > }
> > -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> > - bool tmp_ext)
> > -{
> > - size_t slen = strlen(s);
> > - size_t sublen = strlen(sub);
> > - int i;
> > -
> > - if (sublen == 1 && *sub == '*')
> > - return 1;
> > -
> > - /*
> > - * filename format of multimedia file should be defined as:
> > - * "filename + '.' + extension + (optional: '.' + temp extension)".
> > - */
> > - if (slen < sublen + 2)
> > - return 0;
> > -
> > - if (!tmp_ext) {
> > - /* file has no temp extension */
> > - if (s[slen - sublen - 1] != '.')
> > - return 0;
> > - return !strncasecmp(s + slen - sublen, sub, sublen);
> > - }
> > -
> > - for (i = 1; i < slen - sublen; i++) {
> > - if (s[i] != '.')
> > - continue;
> > - if (!strncasecmp(s + i + 1, sub, sublen))
> > - return 1;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * Set file's temperature for hot/cold data separation
> > */
> > @@ -217,124 +332,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> > file_set_hot(inode);
> > }
> > -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> > - bool hot, bool set)
> > -{
> > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > - int hot_count = sbi->raw_super->hot_ext_count;
> > - int total_count = cold_count + hot_count;
> > - int start, count;
> > - int i;
> > -
> > - if (set) {
> > - if (total_count == F2FS_MAX_EXTENSION)
> > - return -EINVAL;
> > - } else {
> > - if (!hot && !cold_count)
> > - return -EINVAL;
> > - if (hot && !hot_count)
> > - return -EINVAL;
> > - }
> > -
> > - if (hot) {
> > - start = cold_count;
> > - count = total_count;
> > - } else {
> > - start = 0;
> > - count = cold_count;
> > - }
> > -
> > - for (i = start; i < count; i++) {
> > - if (strcmp(name, extlist[i]))
> > - continue;
> > -
> > - if (set)
> > - return -EINVAL;
> > -
> > - memcpy(extlist[i], extlist[i + 1],
> > - F2FS_EXTENSION_LEN * (total_count - i - 1));
> > - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> > - if (hot)
> > - sbi->raw_super->hot_ext_count = hot_count - 1;
> > - else
> > - sbi->raw_super->extension_count =
> > - cpu_to_le32(cold_count - 1);
> > - return 0;
> > - }
> > -
> > - if (!set)
> > - return -EINVAL;
> > -
> > - if (hot) {
> > - memcpy(extlist[count], name, strlen(name));
> > - sbi->raw_super->hot_ext_count = hot_count + 1;
> > - } else {
> > - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> > -
> > - memcpy(buf, &extlist[cold_count],
> > - F2FS_EXTENSION_LEN * hot_count);
> > - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> > - memcpy(extlist[cold_count], name, strlen(name));
> > - memcpy(&extlist[cold_count + 1], buf,
> > - F2FS_EXTENSION_LEN * hot_count);
> > - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> > - }
> > - return 0;
> > -}
> > -
> > -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> > - const unsigned char *name)
> > -{
> > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> > - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> > - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > - int i, cold_count, hot_count;
> > -
> > - if (!f2fs_sb_has_compression(sbi) ||
> > - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> > - !f2fs_may_compress(inode) ||
> > - (!ext_cnt && !noext_cnt))
> > - return;
> > -
> > - f2fs_down_read(&sbi->sb_lock);
> > -
> > - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > - hot_count = sbi->raw_super->hot_ext_count;
> > -
> > - for (i = cold_count; i < cold_count + hot_count; i++) {
> > - if (is_extension_exist(name, extlist[i], false)) {
> > - f2fs_up_read(&sbi->sb_lock);
> > - return;
> > - }
> > - }
> > -
> > - f2fs_up_read(&sbi->sb_lock);
> > -
> > - for (i = 0; i < noext_cnt; i++) {
> > - if (is_extension_exist(name, noext[i], false)) {
> > - f2fs_disable_compressed_file(inode);
> > - return;
> > - }
> > - }
> > -
> > - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> > - return;
> > -
> > - for (i = 0; i < ext_cnt; i++) {
> > - if (!is_extension_exist(name, ext[i], false))
> > - continue;
> > -
> > - /* Do not use inline_data with compression */
> > - stat_dec_inline_inode(inode);
> > - clear_inode_flag(inode, FI_INLINE_DATA);
> > - set_compress_context(inode);
> > - return;
> > - }
> > -}
> > -
> > static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> > struct dentry *dentry, umode_t mode, bool excl)
> > {
> > @@ -352,15 +349,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> > set_file_temperature(sbi, inode, dentry->d_name.name);
> > - set_compress_inode(sbi, inode, dentry->d_name.name);
> > -
> > inode->i_op = &f2fs_file_inode_operations;
> > inode->i_fop = &f2fs_file_operations;
> > inode->i_mapping->a_ops = &f2fs_dblock_aops;
> > @@ -689,7 +684,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> > + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -760,7 +755,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
> > + dentry->d_name.name);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -817,7 +813,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -856,7 +852,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);

2022-11-23 15:03:37

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/17 9:12, Jaegeuk Kim wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/namei.c | 325 +++++++++++++++++++++++-------------------------
> 2 files changed, 160 insertions(+), 167 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b89b5d755ce0..dedac413bf64 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
> /* Flags that should be inherited by new inodes from their parent. */
> #define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
> F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> - F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
> + F2FS_CASEFOLD_FL)
>
> /* Flags that are appropriate for regular files (all but dir-specific ones). */
> #define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index e104409c3a0e..c25009bb72f2 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -22,8 +22,158 @@
> #include "acl.h"
> #include <trace/events/f2fs.h>
>
> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> + bool tmp_ext)
> +{
> + size_t slen = strlen(s);
> + size_t sublen = strlen(sub);
> + int i;
> +
> + if (sublen == 1 && *sub == '*')
> + return 1;
> +
> + /*
> + * filename format of multimedia file should be defined as:
> + * "filename + '.' + extension + (optional: '.' + temp extension)".
> + */
> + if (slen < sublen + 2)
> + return 0;
> +
> + if (!tmp_ext) {
> + /* file has no temp extension */
> + if (s[slen - sublen - 1] != '.')
> + return 0;
> + return !strncasecmp(s + slen - sublen, sub, sublen);
> + }
> +
> + for (i = 1; i < slen - sublen; i++) {
> + if (s[i] != '.')
> + continue;
> + if (!strncasecmp(s + i + 1, sub, sublen))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> + bool hot, bool set)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + int hot_count = sbi->raw_super->hot_ext_count;
> + int total_count = cold_count + hot_count;
> + int start, count;
> + int i;
> +
> + if (set) {
> + if (total_count == F2FS_MAX_EXTENSION)
> + return -EINVAL;
> + } else {
> + if (!hot && !cold_count)
> + return -EINVAL;
> + if (hot && !hot_count)
> + return -EINVAL;
> + }
> +
> + if (hot) {
> + start = cold_count;
> + count = total_count;
> + } else {
> + start = 0;
> + count = cold_count;
> + }
> +
> + for (i = start; i < count; i++) {
> + if (strcmp(name, extlist[i]))
> + continue;
> +
> + if (set)
> + return -EINVAL;
> +
> + memcpy(extlist[i], extlist[i + 1],
> + F2FS_EXTENSION_LEN * (total_count - i - 1));
> + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> + if (hot)
> + sbi->raw_super->hot_ext_count = hot_count - 1;
> + else
> + sbi->raw_super->extension_count =
> + cpu_to_le32(cold_count - 1);
> + return 0;
> + }
> +
> + if (!set)
> + return -EINVAL;
> +
> + if (hot) {
> + memcpy(extlist[count], name, strlen(name));
> + sbi->raw_super->hot_ext_count = hot_count + 1;
> + } else {
> + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> +
> + memcpy(buf, &extlist[cold_count],
> + F2FS_EXTENSION_LEN * hot_count);
> + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> + memcpy(extlist[cold_count], name, strlen(name));
> + memcpy(&extlist[cold_count + 1], buf,
> + F2FS_EXTENSION_LEN * hot_count);
> + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> + }
> + return 0;
> +}
> +
> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> + struct inode *inode, const unsigned char *name)
> +{
> + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> + F2FS_OPTION(sbi).noextensions;
> + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> + int i, cold_count, hot_count;
> +
> + /* Caller should give the name of regular file or directory. */
> + if (!f2fs_sb_has_compression(sbi) || !name)
> + return;
> +
> + if (S_ISDIR(inode->i_mode))
> + goto inherit_comp;

Documentation/filesystems/f2fs.rst

- Priority in between FS_COMPR_FL, FS_NOCOMP_FS, extensions:

* compress_extension=so; nocompress_extension=zip; chattr +c dir; touch
dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so and baz.txt
should be compresse, bar.zip should be non-compressed. chattr +c dir/bar.zip
can enable compress on bar.zip.

It looks nocompress_extension has higher priority than flag inheriting?

> +
> + /* Don't compress hot files. */
> + f2fs_down_read(&sbi->sb_lock);
> + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> + hot_count = sbi->raw_super->hot_ext_count;
> + for (i = cold_count; i < cold_count + hot_count; i++)
> + if (is_extension_exist(name, extlist[i], false))
> + break;
> + f2fs_up_read(&sbi->sb_lock);
> + if (i < (cold_count + hot_count))
> + return;
> +
> + /* Don't compress unallowed extension. */
> + for (i = 0; i < noext_cnt; i++)
> + if (is_extension_exist(name, noext[i], false))
> + return;
> +
> + /* Compress wanting extension. */
> + for (i = 0; i < ext_cnt; i++) {
> + if (is_extension_exist(name, ext[i], false)) {
> + set_compress_context(inode);
> + return;
> + }
> + }
> +inherit_comp:
> + /* Inherit the {no-}compression flag in directory */
> + if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL)
> + F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;

f2fs_mark_inode_dirty_sync(, true)?

> + else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL)
> + set_compress_context(inode);
> +}
> +
> static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> - struct inode *dir, umode_t mode)
> + struct inode *dir, umode_t mode,
> + const char *name)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> nid_t ino;
> @@ -114,12 +264,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> set_inode_flag(inode, FI_PROJ_INHERIT);
>
> - if (f2fs_sb_has_compression(sbi)) {
> - /* Inherit the compression flag in directory */
> - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> - f2fs_may_compress(inode))
> - set_compress_context(inode);
> - }
> + /* Check compression first. */
> + set_compress_new_inode(sbi, dir, inode, name);
>
> /* Should enable inline_data after compression set */
> if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> @@ -153,40 +299,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> return ERR_PTR(err);
> }
>
> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> - bool tmp_ext)
> -{
> - size_t slen = strlen(s);
> - size_t sublen = strlen(sub);
> - int i;
> -
> - if (sublen == 1 && *sub == '*')
> - return 1;
> -
> - /*
> - * filename format of multimedia file should be defined as:
> - * "filename + '.' + extension + (optional: '.' + temp extension)".
> - */
> - if (slen < sublen + 2)
> - return 0;
> -
> - if (!tmp_ext) {
> - /* file has no temp extension */
> - if (s[slen - sublen - 1] != '.')
> - return 0;
> - return !strncasecmp(s + slen - sublen, sub, sublen);
> - }
> -
> - for (i = 1; i < slen - sublen; i++) {
> - if (s[i] != '.')
> - continue;
> - if (!strncasecmp(s + i + 1, sub, sublen))
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Set file's temperature for hot/cold data separation
> */
> @@ -217,124 +329,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> file_set_hot(inode);
> }
>
> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> - bool hot, bool set)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - int hot_count = sbi->raw_super->hot_ext_count;
> - int total_count = cold_count + hot_count;
> - int start, count;
> - int i;
> -
> - if (set) {
> - if (total_count == F2FS_MAX_EXTENSION)
> - return -EINVAL;
> - } else {
> - if (!hot && !cold_count)
> - return -EINVAL;
> - if (hot && !hot_count)
> - return -EINVAL;
> - }
> -
> - if (hot) {
> - start = cold_count;
> - count = total_count;
> - } else {
> - start = 0;
> - count = cold_count;
> - }
> -
> - for (i = start; i < count; i++) {
> - if (strcmp(name, extlist[i]))
> - continue;
> -
> - if (set)
> - return -EINVAL;
> -
> - memcpy(extlist[i], extlist[i + 1],
> - F2FS_EXTENSION_LEN * (total_count - i - 1));
> - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> - if (hot)
> - sbi->raw_super->hot_ext_count = hot_count - 1;
> - else
> - sbi->raw_super->extension_count =
> - cpu_to_le32(cold_count - 1);
> - return 0;
> - }
> -
> - if (!set)
> - return -EINVAL;
> -
> - if (hot) {
> - memcpy(extlist[count], name, strlen(name));
> - sbi->raw_super->hot_ext_count = hot_count + 1;
> - } else {
> - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> -
> - memcpy(buf, &extlist[cold_count],
> - F2FS_EXTENSION_LEN * hot_count);
> - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> - memcpy(extlist[cold_count], name, strlen(name));
> - memcpy(&extlist[cold_count + 1], buf,
> - F2FS_EXTENSION_LEN * hot_count);
> - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> - }
> - return 0;
> -}
> -
> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> - const unsigned char *name)
> -{
> - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> - int i, cold_count, hot_count;
> -
> - if (!f2fs_sb_has_compression(sbi) ||
> - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> - !f2fs_may_compress(inode) ||
> - (!ext_cnt && !noext_cnt))
> - return;
> -
> - f2fs_down_read(&sbi->sb_lock);
> -
> - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> - hot_count = sbi->raw_super->hot_ext_count;
> -
> - for (i = cold_count; i < cold_count + hot_count; i++) {
> - if (is_extension_exist(name, extlist[i], false)) {
> - f2fs_up_read(&sbi->sb_lock);
> - return;
> - }
> - }
> -
> - f2fs_up_read(&sbi->sb_lock);
> -
> - for (i = 0; i < noext_cnt; i++) {
> - if (is_extension_exist(name, noext[i], false)) {
> - f2fs_disable_compressed_file(inode);
> - return;
> - }
> - }
> -
> - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> - return;
> -
> - for (i = 0; i < ext_cnt; i++) {
> - if (!is_extension_exist(name, ext[i], false))
> - continue;
> -
> - /* Do not use inline_data with compression */
> - stat_dec_inline_inode(inode);
> - clear_inode_flag(inode, FI_INLINE_DATA);
> - set_compress_context(inode);
> - return;
> - }
> -}
> -
> static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> struct dentry *dentry, umode_t mode, bool excl)
> {
> @@ -352,15 +346,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> set_file_temperature(sbi, inode, dentry->d_name.name);
>
> - set_compress_inode(sbi, inode, dentry->d_name.name);
> -
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> @@ -689,7 +681,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -760,7 +752,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
> + dentry->d_name.name);

Why we need to pass directory's name to set_compress_new_inode()?

Could we just check S_IFDIR in child inode?

Thanks,

> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -817,7 +810,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>
> @@ -856,7 +849,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - inode = f2fs_new_inode(mnt_userns, dir, mode);
> + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
>

2022-11-23 17:26:43

by Sheng Yong

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches



On 2022/11/23 22:54, Chao Yu wrote:
> On 2022/11/17 9:12, Jaegeuk Kim wrote:
>> If compress_extension is set, and a newly created file matches the
>> extension, the file could be marked as compression file. However,
>> if inline_data is also enabled, there is no chance to check its
>> extension since f2fs_should_compress() always returns false.
>>
>> This patch moves set_compress_inode(), which do extension check, in
>> f2fs_should_compress() to check extensions before setting inline
>> data flag.
>>
>> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
>> Signed-off-by: Sheng Yong <[email protected]>
>> Signed-off-by: Jaegeuk Kim <[email protected]>
>> ---
>>   fs/f2fs/f2fs.h  |   2 +-
>>   fs/f2fs/namei.c | 325 +++++++++++++++++++++++-------------------------
>>   2 files changed, 160 insertions(+), 167 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index b89b5d755ce0..dedac413bf64 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
>>   /* Flags that should be inherited by new inodes from their parent. */
>>   #define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
>>                  F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
>> -               F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
>> +               F2FS_CASEFOLD_FL)
>>   /* Flags that are appropriate for regular files (all but dir-specific ones). */
>>   #define F2FS_REG_FLMASK        (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index e104409c3a0e..c25009bb72f2 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -22,8 +22,158 @@
>>   #include "acl.h"
>>   #include <trace/events/f2fs.h>
>> +static inline int is_extension_exist(const unsigned char *s, const char *sub,
>> +                        bool tmp_ext)
>> +{
>> +    size_t slen = strlen(s);
>> +    size_t sublen = strlen(sub);
>> +    int i;
>> +
>> +    if (sublen == 1 && *sub == '*')
>> +        return 1;
>> +
>> +    /*
>> +     * filename format of multimedia file should be defined as:
>> +     * "filename + '.' + extension + (optional: '.' + temp extension)".
>> +     */
>> +    if (slen < sublen + 2)
>> +        return 0;
>> +
>> +    if (!tmp_ext) {
>> +        /* file has no temp extension */
>> +        if (s[slen - sublen - 1] != '.')
>> +            return 0;
>> +        return !strncasecmp(s + slen - sublen, sub, sublen);
>> +    }
>> +
>> +    for (i = 1; i < slen - sublen; i++) {
>> +        if (s[i] != '.')
>> +            continue;
>> +        if (!strncasecmp(s + i + 1, sub, sublen))
>> +            return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
>> +                            bool hot, bool set)
>> +{
>> +    __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> +    int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> +    int hot_count = sbi->raw_super->hot_ext_count;
>> +    int total_count = cold_count + hot_count;
>> +    int start, count;
>> +    int i;
>> +
>> +    if (set) {
>> +        if (total_count == F2FS_MAX_EXTENSION)
>> +            return -EINVAL;
>> +    } else {
>> +        if (!hot && !cold_count)
>> +            return -EINVAL;
>> +        if (hot && !hot_count)
>> +            return -EINVAL;
>> +    }
>> +
>> +    if (hot) {
>> +        start = cold_count;
>> +        count = total_count;
>> +    } else {
>> +        start = 0;
>> +        count = cold_count;
>> +    }
>> +
>> +    for (i = start; i < count; i++) {
>> +        if (strcmp(name, extlist[i]))
>> +            continue;
>> +
>> +        if (set)
>> +            return -EINVAL;
>> +
>> +        memcpy(extlist[i], extlist[i + 1],
>> +                F2FS_EXTENSION_LEN * (total_count - i - 1));
>> +        memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
>> +        if (hot)
>> +            sbi->raw_super->hot_ext_count = hot_count - 1;
>> +        else
>> +            sbi->raw_super->extension_count =
>> +                        cpu_to_le32(cold_count - 1);
>> +        return 0;
>> +    }
>> +
>> +    if (!set)
>> +        return -EINVAL;
>> +
>> +    if (hot) {
>> +        memcpy(extlist[count], name, strlen(name));
>> +        sbi->raw_super->hot_ext_count = hot_count + 1;
>> +    } else {
>> +        char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
>> +
>> +        memcpy(buf, &extlist[cold_count],
>> +                F2FS_EXTENSION_LEN * hot_count);
>> +        memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
>> +        memcpy(extlist[cold_count], name, strlen(name));
>> +        memcpy(&extlist[cold_count + 1], buf,
>> +                F2FS_EXTENSION_LEN * hot_count);
>> +        sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
>> +                struct inode *inode, const unsigned char *name)
>> +{
>> +    __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> +    unsigned char (*noext)[F2FS_EXTENSION_LEN] =
>> +                        F2FS_OPTION(sbi).noextensions;
>> +    unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
>> +    unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
>> +    unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> +    int i, cold_count, hot_count;
>> +
>> +    /* Caller should give the name of regular file or directory. */
>> +    if (!f2fs_sb_has_compression(sbi) || !name)
>> +        return;
>> +
>> +    if (S_ISDIR(inode->i_mode))
>> +        goto inherit_comp;
>
> Documentation/filesystems/f2fs.rst
>
> - Priority in between FS_COMPR_FL, FS_NOCOMP_FS, extensions:
>
>   * compress_extension=so; nocompress_extension=zip; chattr +c dir; touch
>     dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so and baz.txt
>     should be compresse, bar.zip should be non-compressed. chattr +c dir/bar.zip
>     can enable compress on bar.zip.
>
> It looks nocompress_extension has higher priority than flag inheriting?

Hi, Chao,

Yes, nocompress_extension has higher priority for regular files. The following code
checks ext[] and noext[] before setting compression flag. While for directories, we
skip checking ext and noext. So the above `if (S_ISDIR(inode)) goto inherit_comp`
does not break the rule.

thanks,
shengyong
>
>> +
>> +    /* Don't compress hot files. */
>> +    f2fs_down_read(&sbi->sb_lock);
>> +    cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> +    hot_count = sbi->raw_super->hot_ext_count;
>> +    for (i = cold_count; i < cold_count + hot_count; i++)
>> +        if (is_extension_exist(name, extlist[i], false))
>> +            break;
>> +    f2fs_up_read(&sbi->sb_lock);
>> +    if (i < (cold_count + hot_count))
>> +        return;
>> +
>> +    /* Don't compress unallowed extension. */
>> +    for (i = 0; i < noext_cnt; i++)
>> +        if (is_extension_exist(name, noext[i], false))
>> +            return;
>> +
>> +    /* Compress wanting extension. */
>> +    for (i = 0; i < ext_cnt; i++) {
>> +        if (is_extension_exist(name, ext[i], false)) {
>> +            set_compress_context(inode);
>> +            return;
>> +        }
>> +    }
>> +inherit_comp:
>> +    /* Inherit the {no-}compression flag in directory */
>> +    if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL)
>> +        F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
>
> f2fs_mark_inode_dirty_sync(, true)?
>
>> +    else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL)
>> +        set_compress_context(inode);
>> +}
>> +
>>   static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>> -                        struct inode *dir, umode_t mode)
>> +                        struct inode *dir, umode_t mode,
>> +                        const char *name)
>>   {
>>       struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>>       nid_t ino;
>> @@ -114,12 +264,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>       if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
>>           set_inode_flag(inode, FI_PROJ_INHERIT);
>> -    if (f2fs_sb_has_compression(sbi)) {
>> -        /* Inherit the compression flag in directory */
>> -        if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
>> -                    f2fs_may_compress(inode))
>> -            set_compress_context(inode);
>> -    }
>> +    /* Check compression first. */
>> +    set_compress_new_inode(sbi, dir, inode, name);
>>       /* Should enable inline_data after compression set */
>>       if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> @@ -153,40 +299,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>       return ERR_PTR(err);
>>   }
>> -static inline int is_extension_exist(const unsigned char *s, const char *sub,
>> -                        bool tmp_ext)
>> -{
>> -    size_t slen = strlen(s);
>> -    size_t sublen = strlen(sub);
>> -    int i;
>> -
>> -    if (sublen == 1 && *sub == '*')
>> -        return 1;
>> -
>> -    /*
>> -     * filename format of multimedia file should be defined as:
>> -     * "filename + '.' + extension + (optional: '.' + temp extension)".
>> -     */
>> -    if (slen < sublen + 2)
>> -        return 0;
>> -
>> -    if (!tmp_ext) {
>> -        /* file has no temp extension */
>> -        if (s[slen - sublen - 1] != '.')
>> -            return 0;
>> -        return !strncasecmp(s + slen - sublen, sub, sublen);
>> -    }
>> -
>> -    for (i = 1; i < slen - sublen; i++) {
>> -        if (s[i] != '.')
>> -            continue;
>> -        if (!strncasecmp(s + i + 1, sub, sublen))
>> -            return 1;
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>>   /*
>>    * Set file's temperature for hot/cold data separation
>>    */
>> @@ -217,124 +329,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
>>           file_set_hot(inode);
>>   }
>> -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
>> -                            bool hot, bool set)
>> -{
>> -    __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> -    int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> -    int hot_count = sbi->raw_super->hot_ext_count;
>> -    int total_count = cold_count + hot_count;
>> -    int start, count;
>> -    int i;
>> -
>> -    if (set) {
>> -        if (total_count == F2FS_MAX_EXTENSION)
>> -            return -EINVAL;
>> -    } else {
>> -        if (!hot && !cold_count)
>> -            return -EINVAL;
>> -        if (hot && !hot_count)
>> -            return -EINVAL;
>> -    }
>> -
>> -    if (hot) {
>> -        start = cold_count;
>> -        count = total_count;
>> -    } else {
>> -        start = 0;
>> -        count = cold_count;
>> -    }
>> -
>> -    for (i = start; i < count; i++) {
>> -        if (strcmp(name, extlist[i]))
>> -            continue;
>> -
>> -        if (set)
>> -            return -EINVAL;
>> -
>> -        memcpy(extlist[i], extlist[i + 1],
>> -                F2FS_EXTENSION_LEN * (total_count - i - 1));
>> -        memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
>> -        if (hot)
>> -            sbi->raw_super->hot_ext_count = hot_count - 1;
>> -        else
>> -            sbi->raw_super->extension_count =
>> -                        cpu_to_le32(cold_count - 1);
>> -        return 0;
>> -    }
>> -
>> -    if (!set)
>> -        return -EINVAL;
>> -
>> -    if (hot) {
>> -        memcpy(extlist[count], name, strlen(name));
>> -        sbi->raw_super->hot_ext_count = hot_count + 1;
>> -    } else {
>> -        char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
>> -
>> -        memcpy(buf, &extlist[cold_count],
>> -                F2FS_EXTENSION_LEN * hot_count);
>> -        memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
>> -        memcpy(extlist[cold_count], name, strlen(name));
>> -        memcpy(&extlist[cold_count + 1], buf,
>> -                F2FS_EXTENSION_LEN * hot_count);
>> -        sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
>> -    }
>> -    return 0;
>> -}
>> -
>> -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>> -                        const unsigned char *name)
>> -{
>> -    __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
>> -    unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
>> -    unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
>> -    unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
>> -    unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
>> -    int i, cold_count, hot_count;
>> -
>> -    if (!f2fs_sb_has_compression(sbi) ||
>> -            F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
>> -            !f2fs_may_compress(inode) ||
>> -            (!ext_cnt && !noext_cnt))
>> -        return;
>> -
>> -    f2fs_down_read(&sbi->sb_lock);
>> -
>> -    cold_count = le32_to_cpu(sbi->raw_super->extension_count);
>> -    hot_count = sbi->raw_super->hot_ext_count;
>> -
>> -    for (i = cold_count; i < cold_count + hot_count; i++) {
>> -        if (is_extension_exist(name, extlist[i], false)) {
>> -            f2fs_up_read(&sbi->sb_lock);
>> -            return;
>> -        }
>> -    }
>> -
>> -    f2fs_up_read(&sbi->sb_lock);
>> -
>> -    for (i = 0; i < noext_cnt; i++) {
>> -        if (is_extension_exist(name, noext[i], false)) {
>> -            f2fs_disable_compressed_file(inode);
>> -            return;
>> -        }
>> -    }
>> -
>> -    if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
>> -        return;
>> -
>> -    for (i = 0; i < ext_cnt; i++) {
>> -        if (!is_extension_exist(name, ext[i], false))
>> -            continue;
>> -
>> -        /* Do not use inline_data with compression */
>> -        stat_dec_inline_inode(inode);
>> -        clear_inode_flag(inode, FI_INLINE_DATA);
>> -        set_compress_context(inode);
>> -        return;
>> -    }
>> -}
>> -
>>   static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>>                  struct dentry *dentry, umode_t mode, bool excl)
>>   {
>> @@ -352,15 +346,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
>>       if (err)
>>           return err;
>> -    inode = f2fs_new_inode(mnt_userns, dir, mode);
>> +    inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
>>       if (IS_ERR(inode))
>>           return PTR_ERR(inode);
>>       if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
>>           set_file_temperature(sbi, inode, dentry->d_name.name);
>> -    set_compress_inode(sbi, inode, dentry->d_name.name);
>> -
>>       inode->i_op = &f2fs_file_inode_operations;
>>       inode->i_fop = &f2fs_file_operations;
>>       inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> @@ -689,7 +681,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>       if (err)
>>           return err;
>> -    inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
>> +    inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
>>       if (IS_ERR(inode))
>>           return PTR_ERR(inode);
>> @@ -760,7 +752,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>       if (err)
>>           return err;
>> -    inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
>> +    inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
>> +                        dentry->d_name.name);
>
> Why we need to pass directory's name to set_compress_new_inode()?
>
> Could we just check S_IFDIR in child inode?
>
> Thanks,
>
>>       if (IS_ERR(inode))
>>           return PTR_ERR(inode);
>> @@ -817,7 +810,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>>       if (err)
>>           return err;
>> -    inode = f2fs_new_inode(mnt_userns, dir, mode);
>> +    inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>>       if (IS_ERR(inode))
>>           return PTR_ERR(inode);
>> @@ -856,7 +849,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
>>       if (err)
>>           return err;
>> -    inode = f2fs_new_inode(mnt_userns, dir, mode);
>> +    inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
>>       if (IS_ERR(inode))
>>           return PTR_ERR(inode);

2022-11-23 22:32:05

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

On 11/23, Chao Yu wrote:
> On 2022/11/17 9:12, Jaegeuk Kim wrote:
> > If compress_extension is set, and a newly created file matches the
> > extension, the file could be marked as compression file. However,
> > if inline_data is also enabled, there is no chance to check its
> > extension since f2fs_should_compress() always returns false.
> >
> > This patch moves set_compress_inode(), which do extension check, in
> > f2fs_should_compress() to check extensions before setting inline
> > data flag.
> >
> > Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> > Signed-off-by: Sheng Yong <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > fs/f2fs/f2fs.h | 2 +-
> > fs/f2fs/namei.c | 325 +++++++++++++++++++++++-------------------------
> > 2 files changed, 160 insertions(+), 167 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index b89b5d755ce0..dedac413bf64 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
> > /* Flags that should be inherited by new inodes from their parent. */
> > #define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
> > F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> > - F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
> > + F2FS_CASEFOLD_FL)
> > /* Flags that are appropriate for regular files (all but dir-specific ones). */
> > #define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
> > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > index e104409c3a0e..c25009bb72f2 100644
> > --- a/fs/f2fs/namei.c
> > +++ b/fs/f2fs/namei.c
> > @@ -22,8 +22,158 @@
> > #include "acl.h"
> > #include <trace/events/f2fs.h>
> > +static inline int is_extension_exist(const unsigned char *s, const char *sub,
> > + bool tmp_ext)
> > +{
> > + size_t slen = strlen(s);
> > + size_t sublen = strlen(sub);
> > + int i;
> > +
> > + if (sublen == 1 && *sub == '*')
> > + return 1;
> > +
> > + /*
> > + * filename format of multimedia file should be defined as:
> > + * "filename + '.' + extension + (optional: '.' + temp extension)".
> > + */
> > + if (slen < sublen + 2)
> > + return 0;
> > +
> > + if (!tmp_ext) {
> > + /* file has no temp extension */
> > + if (s[slen - sublen - 1] != '.')
> > + return 0;
> > + return !strncasecmp(s + slen - sublen, sub, sublen);
> > + }
> > +
> > + for (i = 1; i < slen - sublen; i++) {
> > + if (s[i] != '.')
> > + continue;
> > + if (!strncasecmp(s + i + 1, sub, sublen))
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> > + bool hot, bool set)
> > +{
> > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > + int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > + int hot_count = sbi->raw_super->hot_ext_count;
> > + int total_count = cold_count + hot_count;
> > + int start, count;
> > + int i;
> > +
> > + if (set) {
> > + if (total_count == F2FS_MAX_EXTENSION)
> > + return -EINVAL;
> > + } else {
> > + if (!hot && !cold_count)
> > + return -EINVAL;
> > + if (hot && !hot_count)
> > + return -EINVAL;
> > + }
> > +
> > + if (hot) {
> > + start = cold_count;
> > + count = total_count;
> > + } else {
> > + start = 0;
> > + count = cold_count;
> > + }
> > +
> > + for (i = start; i < count; i++) {
> > + if (strcmp(name, extlist[i]))
> > + continue;
> > +
> > + if (set)
> > + return -EINVAL;
> > +
> > + memcpy(extlist[i], extlist[i + 1],
> > + F2FS_EXTENSION_LEN * (total_count - i - 1));
> > + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> > + if (hot)
> > + sbi->raw_super->hot_ext_count = hot_count - 1;
> > + else
> > + sbi->raw_super->extension_count =
> > + cpu_to_le32(cold_count - 1);
> > + return 0;
> > + }
> > +
> > + if (!set)
> > + return -EINVAL;
> > +
> > + if (hot) {
> > + memcpy(extlist[count], name, strlen(name));
> > + sbi->raw_super->hot_ext_count = hot_count + 1;
> > + } else {
> > + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> > +
> > + memcpy(buf, &extlist[cold_count],
> > + F2FS_EXTENSION_LEN * hot_count);
> > + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> > + memcpy(extlist[cold_count], name, strlen(name));
> > + memcpy(&extlist[cold_count + 1], buf,
> > + F2FS_EXTENSION_LEN * hot_count);
> > + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> > + }
> > + return 0;
> > +}
> > +
> > +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
> > + struct inode *inode, const unsigned char *name)
> > +{
> > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > + unsigned char (*noext)[F2FS_EXTENSION_LEN] =
> > + F2FS_OPTION(sbi).noextensions;
> > + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> > + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > + int i, cold_count, hot_count;
> > +
> > + /* Caller should give the name of regular file or directory. */
> > + if (!f2fs_sb_has_compression(sbi) || !name)
> > + return;
> > +
> > + if (S_ISDIR(inode->i_mode))
> > + goto inherit_comp;
>
> Documentation/filesystems/f2fs.rst
>
> - Priority in between FS_COMPR_FL, FS_NOCOMP_FS, extensions:
>
> * compress_extension=so; nocompress_extension=zip; chattr +c dir; touch
> dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so and baz.txt
> should be compresse, bar.zip should be non-compressed. chattr +c dir/bar.zip
> can enable compress on bar.zip.
>
> It looks nocompress_extension has higher priority than flag inheriting?

I think so.

>
> > +
> > + /* Don't compress hot files. */
> > + f2fs_down_read(&sbi->sb_lock);
> > + cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > + hot_count = sbi->raw_super->hot_ext_count;
> > + for (i = cold_count; i < cold_count + hot_count; i++)
> > + if (is_extension_exist(name, extlist[i], false))
> > + break;
> > + f2fs_up_read(&sbi->sb_lock);
> > + if (i < (cold_count + hot_count))
> > + return;
> > +
> > + /* Don't compress unallowed extension. */
> > + for (i = 0; i < noext_cnt; i++)
> > + if (is_extension_exist(name, noext[i], false))
> > + return;
> > +
> > + /* Compress wanting extension. */
> > + for (i = 0; i < ext_cnt; i++) {
> > + if (is_extension_exist(name, ext[i], false)) {
> > + set_compress_context(inode);
> > + return;
> > + }
> > + }
> > +inherit_comp:
> > + /* Inherit the {no-}compression flag in directory */
> > + if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL)
> > + F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
>
> f2fs_mark_inode_dirty_sync(, true)?

Done.

>
> > + else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL)
> > + set_compress_context(inode);
> > +}
> > +
> > static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > - struct inode *dir, umode_t mode)
> > + struct inode *dir, umode_t mode,
> > + const char *name)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> > nid_t ino;
> > @@ -114,12 +264,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
> > set_inode_flag(inode, FI_PROJ_INHERIT);
> > - if (f2fs_sb_has_compression(sbi)) {
> > - /* Inherit the compression flag in directory */
> > - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
> > - f2fs_may_compress(inode))
> > - set_compress_context(inode);
> > - }
> > + /* Check compression first. */
> > + set_compress_new_inode(sbi, dir, inode, name);
> > /* Should enable inline_data after compression set */
> > if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> > @@ -153,40 +299,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > return ERR_PTR(err);
> > }
> > -static inline int is_extension_exist(const unsigned char *s, const char *sub,
> > - bool tmp_ext)
> > -{
> > - size_t slen = strlen(s);
> > - size_t sublen = strlen(sub);
> > - int i;
> > -
> > - if (sublen == 1 && *sub == '*')
> > - return 1;
> > -
> > - /*
> > - * filename format of multimedia file should be defined as:
> > - * "filename + '.' + extension + (optional: '.' + temp extension)".
> > - */
> > - if (slen < sublen + 2)
> > - return 0;
> > -
> > - if (!tmp_ext) {
> > - /* file has no temp extension */
> > - if (s[slen - sublen - 1] != '.')
> > - return 0;
> > - return !strncasecmp(s + slen - sublen, sub, sublen);
> > - }
> > -
> > - for (i = 1; i < slen - sublen; i++) {
> > - if (s[i] != '.')
> > - continue;
> > - if (!strncasecmp(s + i + 1, sub, sublen))
> > - return 1;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * Set file's temperature for hot/cold data separation
> > */
> > @@ -217,124 +329,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
> > file_set_hot(inode);
> > }
> > -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
> > - bool hot, bool set)
> > -{
> > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > - int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > - int hot_count = sbi->raw_super->hot_ext_count;
> > - int total_count = cold_count + hot_count;
> > - int start, count;
> > - int i;
> > -
> > - if (set) {
> > - if (total_count == F2FS_MAX_EXTENSION)
> > - return -EINVAL;
> > - } else {
> > - if (!hot && !cold_count)
> > - return -EINVAL;
> > - if (hot && !hot_count)
> > - return -EINVAL;
> > - }
> > -
> > - if (hot) {
> > - start = cold_count;
> > - count = total_count;
> > - } else {
> > - start = 0;
> > - count = cold_count;
> > - }
> > -
> > - for (i = start; i < count; i++) {
> > - if (strcmp(name, extlist[i]))
> > - continue;
> > -
> > - if (set)
> > - return -EINVAL;
> > -
> > - memcpy(extlist[i], extlist[i + 1],
> > - F2FS_EXTENSION_LEN * (total_count - i - 1));
> > - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
> > - if (hot)
> > - sbi->raw_super->hot_ext_count = hot_count - 1;
> > - else
> > - sbi->raw_super->extension_count =
> > - cpu_to_le32(cold_count - 1);
> > - return 0;
> > - }
> > -
> > - if (!set)
> > - return -EINVAL;
> > -
> > - if (hot) {
> > - memcpy(extlist[count], name, strlen(name));
> > - sbi->raw_super->hot_ext_count = hot_count + 1;
> > - } else {
> > - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
> > -
> > - memcpy(buf, &extlist[cold_count],
> > - F2FS_EXTENSION_LEN * hot_count);
> > - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
> > - memcpy(extlist[cold_count], name, strlen(name));
> > - memcpy(&extlist[cold_count + 1], buf,
> > - F2FS_EXTENSION_LEN * hot_count);
> > - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
> > - }
> > - return 0;
> > -}
> > -
> > -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> > - const unsigned char *name)
> > -{
> > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
> > - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
> > - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
> > - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > - int i, cold_count, hot_count;
> > -
> > - if (!f2fs_sb_has_compression(sbi) ||
> > - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> > - !f2fs_may_compress(inode) ||
> > - (!ext_cnt && !noext_cnt))
> > - return;
> > -
> > - f2fs_down_read(&sbi->sb_lock);
> > -
> > - cold_count = le32_to_cpu(sbi->raw_super->extension_count);
> > - hot_count = sbi->raw_super->hot_ext_count;
> > -
> > - for (i = cold_count; i < cold_count + hot_count; i++) {
> > - if (is_extension_exist(name, extlist[i], false)) {
> > - f2fs_up_read(&sbi->sb_lock);
> > - return;
> > - }
> > - }
> > -
> > - f2fs_up_read(&sbi->sb_lock);
> > -
> > - for (i = 0; i < noext_cnt; i++) {
> > - if (is_extension_exist(name, noext[i], false)) {
> > - f2fs_disable_compressed_file(inode);
> > - return;
> > - }
> > - }
> > -
> > - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> > - return;
> > -
> > - for (i = 0; i < ext_cnt; i++) {
> > - if (!is_extension_exist(name, ext[i], false))
> > - continue;
> > -
> > - /* Do not use inline_data with compression */
> > - stat_dec_inline_inode(inode);
> > - clear_inode_flag(inode, FI_INLINE_DATA);
> > - set_compress_context(inode);
> > - return;
> > - }
> > -}
> > -
> > static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> > struct dentry *dentry, umode_t mode, bool excl)
> > {
> > @@ -352,15 +346,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
> > set_file_temperature(sbi, inode, dentry->d_name.name);
> > - set_compress_inode(sbi, inode, dentry->d_name.name);
> > -
> > inode->i_op = &f2fs_file_inode_operations;
> > inode->i_fop = &f2fs_file_operations;
> > inode->i_mapping->a_ops = &f2fs_dblock_aops;
> > @@ -689,7 +681,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
> > + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -760,7 +752,8 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode,
> > + dentry->d_name.name);
>
> Why we need to pass directory's name to set_compress_new_inode()?
>
> Could we just check S_IFDIR in child inode?

Yup, good point. Let me send v5.

>
> Thanks,
>
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -817,7 +810,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);
> > @@ -856,7 +849,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> > if (err)
> > return err;
> > - inode = f2fs_new_inode(mnt_userns, dir, mode);
> > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
> > if (IS_ERR(inode))
> > return PTR_ERR(inode);

2022-11-23 22:38:50

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v5] f2fs: fix to enable compress for newly created file if extension matches

If compress_extension is set, and a newly created file matches the
extension, the file could be marked as compression file. However,
if inline_data is also enabled, there is no chance to check its
extension since f2fs_should_compress() always returns false.

This patch moves set_compress_inode(), which do extension check, in
f2fs_should_compress() to check extensions before setting inline
data flag.

Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
Signed-off-by: Sheng Yong <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---

Change log from v4:
- call f2fs_mark_inode_dirty_sync
- remove name for directory

fs/f2fs/f2fs.h | 2 +-
fs/f2fs/namei.c | 329 ++++++++++++++++++++++++------------------------
2 files changed, 164 insertions(+), 167 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 96bd3461c0bb..f0833638f59e 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2980,7 +2980,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
/* Flags that should be inherited by new inodes from their parent. */
#define F2FS_FL_INHERITED (F2FS_SYNC_FL | F2FS_NODUMP_FL | F2FS_NOATIME_FL | \
F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
- F2FS_CASEFOLD_FL | F2FS_COMPR_FL | F2FS_NOCOMP_FL)
+ F2FS_CASEFOLD_FL)

/* Flags that are appropriate for regular files (all but dir-specific ones). */
#define F2FS_REG_FLMASK (~(F2FS_DIRSYNC_FL | F2FS_PROJINHERIT_FL | \
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e104409c3a0e..54448dccbb6a 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -22,8 +22,163 @@
#include "acl.h"
#include <trace/events/f2fs.h>

+static inline int is_extension_exist(const unsigned char *s, const char *sub,
+ bool tmp_ext)
+{
+ size_t slen = strlen(s);
+ size_t sublen = strlen(sub);
+ int i;
+
+ if (sublen == 1 && *sub == '*')
+ return 1;
+
+ /*
+ * filename format of multimedia file should be defined as:
+ * "filename + '.' + extension + (optional: '.' + temp extension)".
+ */
+ if (slen < sublen + 2)
+ return 0;
+
+ if (!tmp_ext) {
+ /* file has no temp extension */
+ if (s[slen - sublen - 1] != '.')
+ return 0;
+ return !strncasecmp(s + slen - sublen, sub, sublen);
+ }
+
+ for (i = 1; i < slen - sublen; i++) {
+ if (s[i] != '.')
+ continue;
+ if (!strncasecmp(s + i + 1, sub, sublen))
+ return 1;
+ }
+
+ return 0;
+}
+
+int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
+ bool hot, bool set)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ int hot_count = sbi->raw_super->hot_ext_count;
+ int total_count = cold_count + hot_count;
+ int start, count;
+ int i;
+
+ if (set) {
+ if (total_count == F2FS_MAX_EXTENSION)
+ return -EINVAL;
+ } else {
+ if (!hot && !cold_count)
+ return -EINVAL;
+ if (hot && !hot_count)
+ return -EINVAL;
+ }
+
+ if (hot) {
+ start = cold_count;
+ count = total_count;
+ } else {
+ start = 0;
+ count = cold_count;
+ }
+
+ for (i = start; i < count; i++) {
+ if (strcmp(name, extlist[i]))
+ continue;
+
+ if (set)
+ return -EINVAL;
+
+ memcpy(extlist[i], extlist[i + 1],
+ F2FS_EXTENSION_LEN * (total_count - i - 1));
+ memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
+ if (hot)
+ sbi->raw_super->hot_ext_count = hot_count - 1;
+ else
+ sbi->raw_super->extension_count =
+ cpu_to_le32(cold_count - 1);
+ return 0;
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ if (hot) {
+ memcpy(extlist[count], name, strlen(name));
+ sbi->raw_super->hot_ext_count = hot_count + 1;
+ } else {
+ char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
+
+ memcpy(buf, &extlist[cold_count],
+ F2FS_EXTENSION_LEN * hot_count);
+ memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
+ memcpy(extlist[cold_count], name, strlen(name));
+ memcpy(&extlist[cold_count + 1], buf,
+ F2FS_EXTENSION_LEN * hot_count);
+ sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
+ }
+ return 0;
+}
+
+static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir,
+ struct inode *inode, const unsigned char *name)
+{
+ __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
+ unsigned char (*noext)[F2FS_EXTENSION_LEN] =
+ F2FS_OPTION(sbi).noextensions;
+ unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
+ unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
+ unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
+ int i, cold_count, hot_count;
+
+ if (!f2fs_sb_has_compression(sbi))
+ return;
+
+ if (S_ISDIR(inode->i_mode))
+ goto inherit_comp;
+
+ /* This name comes only from normal files. */
+ if (!name)
+ return;
+
+ /* Don't compress hot files. */
+ f2fs_down_read(&sbi->sb_lock);
+ cold_count = le32_to_cpu(sbi->raw_super->extension_count);
+ hot_count = sbi->raw_super->hot_ext_count;
+ for (i = cold_count; i < cold_count + hot_count; i++)
+ if (is_extension_exist(name, extlist[i], false))
+ break;
+ f2fs_up_read(&sbi->sb_lock);
+ if (i < (cold_count + hot_count))
+ return;
+
+ /* Don't compress unallowed extension. */
+ for (i = 0; i < noext_cnt; i++)
+ if (is_extension_exist(name, noext[i], false))
+ return;
+
+ /* Compress wanting extension. */
+ for (i = 0; i < ext_cnt; i++) {
+ if (is_extension_exist(name, ext[i], false)) {
+ set_compress_context(inode);
+ return;
+ }
+ }
+inherit_comp:
+ /* Inherit the {no-}compression flag in directory */
+ if (F2FS_I(dir)->i_flags & F2FS_NOCOMP_FL) {
+ F2FS_I(inode)->i_flags |= F2FS_NOCOMP_FL;
+ f2fs_mark_inode_dirty_sync(inode, true);
+ } else if (F2FS_I(dir)->i_flags & F2FS_COMPR_FL) {
+ set_compress_context(inode);
+ }
+}
+
static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
- struct inode *dir, umode_t mode)
+ struct inode *dir, umode_t mode,
+ const char *name)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
nid_t ino;
@@ -114,12 +269,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL)
set_inode_flag(inode, FI_PROJ_INHERIT);

- if (f2fs_sb_has_compression(sbi)) {
- /* Inherit the compression flag in directory */
- if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) &&
- f2fs_may_compress(inode))
- set_compress_context(inode);
- }
+ /* Check compression first. */
+ set_compress_new_inode(sbi, dir, inode, name);

/* Should enable inline_data after compression set */
if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
@@ -153,40 +304,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
return ERR_PTR(err);
}

-static inline int is_extension_exist(const unsigned char *s, const char *sub,
- bool tmp_ext)
-{
- size_t slen = strlen(s);
- size_t sublen = strlen(sub);
- int i;
-
- if (sublen == 1 && *sub == '*')
- return 1;
-
- /*
- * filename format of multimedia file should be defined as:
- * "filename + '.' + extension + (optional: '.' + temp extension)".
- */
- if (slen < sublen + 2)
- return 0;
-
- if (!tmp_ext) {
- /* file has no temp extension */
- if (s[slen - sublen - 1] != '.')
- return 0;
- return !strncasecmp(s + slen - sublen, sub, sublen);
- }
-
- for (i = 1; i < slen - sublen; i++) {
- if (s[i] != '.')
- continue;
- if (!strncasecmp(s + i + 1, sub, sublen))
- return 1;
- }
-
- return 0;
-}
-
/*
* Set file's temperature for hot/cold data separation
*/
@@ -217,124 +334,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode *
file_set_hot(inode);
}

-int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name,
- bool hot, bool set)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- int cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- int hot_count = sbi->raw_super->hot_ext_count;
- int total_count = cold_count + hot_count;
- int start, count;
- int i;
-
- if (set) {
- if (total_count == F2FS_MAX_EXTENSION)
- return -EINVAL;
- } else {
- if (!hot && !cold_count)
- return -EINVAL;
- if (hot && !hot_count)
- return -EINVAL;
- }
-
- if (hot) {
- start = cold_count;
- count = total_count;
- } else {
- start = 0;
- count = cold_count;
- }
-
- for (i = start; i < count; i++) {
- if (strcmp(name, extlist[i]))
- continue;
-
- if (set)
- return -EINVAL;
-
- memcpy(extlist[i], extlist[i + 1],
- F2FS_EXTENSION_LEN * (total_count - i - 1));
- memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN);
- if (hot)
- sbi->raw_super->hot_ext_count = hot_count - 1;
- else
- sbi->raw_super->extension_count =
- cpu_to_le32(cold_count - 1);
- return 0;
- }
-
- if (!set)
- return -EINVAL;
-
- if (hot) {
- memcpy(extlist[count], name, strlen(name));
- sbi->raw_super->hot_ext_count = hot_count + 1;
- } else {
- char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN];
-
- memcpy(buf, &extlist[cold_count],
- F2FS_EXTENSION_LEN * hot_count);
- memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN);
- memcpy(extlist[cold_count], name, strlen(name));
- memcpy(&extlist[cold_count + 1], buf,
- F2FS_EXTENSION_LEN * hot_count);
- sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1);
- }
- return 0;
-}
-
-static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
- const unsigned char *name)
-{
- __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list;
- unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions;
- unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions;
- unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
- unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
- int i, cold_count, hot_count;
-
- if (!f2fs_sb_has_compression(sbi) ||
- F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
- !f2fs_may_compress(inode) ||
- (!ext_cnt && !noext_cnt))
- return;
-
- f2fs_down_read(&sbi->sb_lock);
-
- cold_count = le32_to_cpu(sbi->raw_super->extension_count);
- hot_count = sbi->raw_super->hot_ext_count;
-
- for (i = cold_count; i < cold_count + hot_count; i++) {
- if (is_extension_exist(name, extlist[i], false)) {
- f2fs_up_read(&sbi->sb_lock);
- return;
- }
- }
-
- f2fs_up_read(&sbi->sb_lock);
-
- for (i = 0; i < noext_cnt; i++) {
- if (is_extension_exist(name, noext[i], false)) {
- f2fs_disable_compressed_file(inode);
- return;
- }
- }
-
- if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
- return;
-
- for (i = 0; i < ext_cnt; i++) {
- if (!is_extension_exist(name, ext[i], false))
- continue;
-
- /* Do not use inline_data with compression */
- stat_dec_inline_inode(inode);
- clear_inode_flag(inode, FI_INLINE_DATA);
- set_compress_context(inode);
- return;
- }
-}
-
static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
@@ -352,15 +351,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name);
if (IS_ERR(inode))
return PTR_ERR(inode);

if (!test_opt(sbi, DISABLE_EXT_IDENTIFY))
set_file_temperature(sbi, inode, dentry->d_name.name);

- set_compress_inode(sbi, inode, dentry->d_name.name);
-
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
@@ -689,7 +686,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -760,7 +757,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode);
+ inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -817,7 +814,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

@@ -856,7 +853,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
if (err)
return err;

- inode = f2fs_new_inode(mnt_userns, dir, mode);
+ inode = f2fs_new_inode(mnt_userns, dir, mode, NULL);
if (IS_ERR(inode))
return PTR_ERR(inode);

--
2.38.1.584.g0f3c55d4c2-goog

2022-11-24 03:55:13

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v4] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/24 5:29, Jaegeuk Kim wrote:
>>> + if (S_ISDIR(inode->i_mode))
>>> + goto inherit_comp;
>>
>> Documentation/filesystems/f2fs.rst
>>
>> - Priority in between FS_COMPR_FL, FS_NOCOMP_FS, extensions:
>>
>> * compress_extension=so; nocompress_extension=zip; chattr +c dir; touch
>> dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so and baz.txt
>> should be compresse, bar.zip should be non-compressed. chattr +c dir/bar.zip
>> can enable compress on bar.zip.
>>
>> It looks nocompress_extension has higher priority than flag inheriting?
>
> I think so.

Hi Sheng, Jaegeuk,

Yup, I guess I misunderstand the code. :)

Thanks,

2022-11-24 14:49:03

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v5] f2fs: fix to enable compress for newly created file if extension matches

On 2022/11/24 5:46, Jaegeuk Kim wrote:
> If compress_extension is set, and a newly created file matches the
> extension, the file could be marked as compression file. However,
> if inline_data is also enabled, there is no chance to check its
> extension since f2fs_should_compress() always returns false.
>
> This patch moves set_compress_inode(), which do extension check, in
> f2fs_should_compress() to check extensions before setting inline
> data flag.
>
> Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion")
> Signed-off-by: Sheng Yong <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,