Re: [f2fs-dev] [PATCH v6] f2fs: compress: add nocompress extensions support

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 07/02, Chao Yu wrote:
> On 2021/7/2 0:57, Jaegeuk Kim wrote:
> > On 07/01, Chao Yu wrote:
> > > Hi all,
> > > 
> > > I think code in this patch is clear, not sure about the comments,
> > > Jaegeuk, any comments on this patch, Fengnan has pinged many times....
> > > 
> > > Let me know, if you are too busy to review this patch these days.
> > 
> > I reviewed it, and looks good to me. Merged in -dev.
> 
> Cool,
> 
> Reviewed-by: Chao Yu <chao@kernel.org>

Chao, please check f2fs-dev branch, as I had to modify in set_compress_inode()
to address build failure and neat code.

Thanks,

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks
> > > 
> > > On 2021/7/1 11:30, Fengnan Chang wrote:
> > > > Hi chao & jaegeuk:
> > > >       Any comments about this version? It's have been a while，are you not
> > > > agree with this patch?  Pelase give me some feedback.
> > > > 
> > > > Thanks
> > > > 
> > > > On 2021/6/21 11:37, Fengnan Chang wrote:
> > > > > Hi chao & jaegeuk:
> > > > >      Any comments about this version?
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > On 2021/6/8 19:15, Fengnan Chang wrote:
> > > > > > When we create a directory with enable compression, all file write into
> > > > > > directory will try to compress.But sometimes we may know, new file
> > > > > > cannot meet compression ratio requirements.
> > > > > > We need a nocompress extension to skip those files to avoid unnecessary
> > > > > > compress page test.
> > > > > > 
> > > > > > After add nocompress_extension, the priority should be:
> > > > > > dir_flag < comp_extention,nocompress_extension < comp_file_flag,
> > > > > > no_comp_file_flag.
> > > > > > 
> > > > > > 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.
> > > > > >       * compress_extension=so; nocompress_extension=zip; chattr -c dir;
> > > > > >         touch dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so
> > > > > >         should be compresse, bar.zip and baz.txt should be non-compressed.
> > > > > >         chattr+c dir/bar.zip; chattr+c dir/baz.txt; can enable compress on
> > > > > >         bar.zip and baz.txt.
> > > > > > 
> > > > > > Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> > > > > > ---
> > > > > >     Documentation/filesystems/f2fs.rst | 31 +++++++++++-
> > > > > >     fs/f2fs/f2fs.h                     |  2 +
> > > > > >     fs/f2fs/namei.c                    | 18 +++++--
> > > > > >     fs/f2fs/super.c                    | 79 +++++++++++++++++++++++++++++-
> > > > > >     4 files changed, 125 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/Documentation/filesystems/f2fs.rst
> > > > > > b/Documentation/filesystems/f2fs.rst
> > > > > > index 992bf91eeec8..c68f1c822665 100644
> > > > > > --- a/Documentation/filesystems/f2fs.rst
> > > > > > +++ b/Documentation/filesystems/f2fs.rst
> > > > > > @@ -281,6 +281,18 @@ compress_extension=%s     Support adding
> > > > > > specified extension, so that f2fs can enab
> > > > > >                  For other files, we can still enable compression via
> > > > > > ioctl.
> > > > > >                  Note that, there is one reserved special extension '*', it
> > > > > >                  can be set to enable compression for all files.
> > > > > > +nocompress_extension=%s       Support adding specified extension, so
> > > > > > that f2fs can disable
> > > > > > +             compression on those corresponding files, just contrary
> > > > > > to compression extension.
> > > > > > +             If you know exactly which files cannot be compressed,
> > > > > > you can use this.
> > > > > > +             The same extension name can't appear in both compress
> > > > > > and nocompress
> > > > > > +             extension at the same time.
> > > > > > +             If the compress extension specifies all files, the types
> > > > > > specified by the
> > > > > > +             nocompress extension will be treated as special cases
> > > > > > and will not be compressed.
> > > > > > +             Don't allow use '*' to specifie all file in nocompress
> > > > > > extension.
> > > > > > +             After add nocompress_extension, the priority should be:
> > > > > > +             dir_flag < comp_extention,nocompress_extension <
> > > > > > comp_file_flag,no_comp_file_flag.
> > > > > > +             See more in compression sections.
> > > > > > +
> > > > > >     compress_chksum         Support verifying chksum of raw data in
> > > > > > compressed cluster.
> > > > > >     compress_mode=%s     Control file compression mode. This supports
> > > > > > "fs" and "user"
> > > > > >                  modes. In "fs" mode (default), f2fs does automatic
> > > > > > compression
> > > > > > @@ -814,13 +826,30 @@ Compression implementation
> > > > > >       all logical blocks in cluster contain valid data and compress
> > > > > > ratio of
> > > > > >       cluster data is lower than specified threshold.
> > > > > > -- To enable compression on regular inode, there are three ways:
> > > > > > +- To enable compression on regular inode, there are four ways:
> > > > > >       * chattr +c file
> > > > > >       * chattr +c dir; touch dir/file
> > > > > >       * mount w/ -o compress_extension=ext; touch file.ext
> > > > > >       * mount w/ -o compress_extension=*; touch any_file
> > > > > > +- To disable compression on regular inode, there are two ways:
> > > > > > +
> > > > > > +  * chattr -c file
> > > > > > +  * mount w/ -o nocompress_extension=ext; touch file.ext
> > > > > > +
> > > > > > +- 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.
> > > > > > +  * compress_extension=so; nocompress_extension=zip; chattr -c dir;
> > > > > > touch
> > > > > > +    dir/foo.so; touch dir/bar.zip; touch dir/baz.txt; then foo.so
> > > > > > should be
> > > > > > +    compresse, bar.zip and baz.txt should be non-compressed.
> > > > > > +    chattr+c dir/bar.zip; chattr+c dir/baz.txt; can enable compress
> > > > > > on bar.zip
> > > > > > +    and baz.txt.
> > > > > > +
> > > > > >     - At this point, compression feature doesn't expose compressed space
> > > > > > to user
> > > > > >       directly in order to guarantee potential data updates later to the
> > > > > > space.
> > > > > >       Instead, the main goal is to reduce data writes to flash disk as
> > > > > > much as
> > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > index 9ad502f92529..7d13d4d64d59 100644
> > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > @@ -150,8 +150,10 @@ struct f2fs_mount_info {
> > > > > >         unsigned char compress_level;        /* compress level */
> > > > > >         bool compress_chksum;            /* compressed data chksum */
> > > > > >         unsigned char compress_ext_cnt;        /* extension count */
> > > > > > +    unsigned char nocompress_ext_cnt;        /* nocompress extension
> > > > > > count */
> > > > > >         int compress_mode;            /* compression mode */
> > > > > >         unsigned char
> > > > > > extensions[COMPRESS_EXT_NUM][F2FS_EXTENSION_LEN];    /* extensions */
> > > > > > +    unsigned char noextensions[COMPRESS_EXT_NUM][F2FS_EXTENSION_LEN];
> > > > > > /* extensions */
> > > > > >     };
> > > > > >     #define F2FS_FEATURE_ENCRYPT        0x0001
> > > > > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > > > > > index d4139e166b95..f343842da4f3 100644
> > > > > > --- a/fs/f2fs/namei.c
> > > > > > +++ b/fs/f2fs/namei.c
> > > > > > @@ -287,14 +287,16 @@ 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];
> > > > > > -    unsigned int ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > > > > > +    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) ||
> > > > > > -            is_inode_flag_set(inode, FI_COMPRESSED_FILE) ||
> > > > > >                 F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL ||
> > > > > > -            !f2fs_may_compress(inode))
> > > > > > +            !f2fs_may_compress(inode) ||
> > > > > > +            (!ext_cnt && !noext_cnt))
> > > > > >             return;
> > > > > >         down_read(&sbi->sb_lock);
> > > > > > @@ -311,6 +313,16 @@ static void set_compress_inode(struct
> > > > > > f2fs_sb_info *sbi, struct inode *inode,
> > > > > >         up_read(&sbi->sb_lock);
> > > > > > +    for (i = 0; i < noext_cnt; i++) {
> > > > > > +        if (is_extension_exist(name, noext[i])) {
> > > > > > +            f2fs_disable_compressed_file(inode);
> > > > > > +            return;
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    if (is_inode_flag_set(inode, FI_COMPRESSED_FILE))
> > > > > > +        return;
> > > > > > +
> > > > > >         ext = F2FS_OPTION(sbi).extensions;
> > > > > >         for (i = 0; i < ext_cnt; i++) {
> > > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > > > index 6788e7b71e27..e720323b900c 100644
> > > > > > --- a/fs/f2fs/super.c
> > > > > > +++ b/fs/f2fs/super.c
> > > > > > @@ -148,6 +148,7 @@ enum {
> > > > > >         Opt_compress_algorithm,
> > > > > >         Opt_compress_log_size,
> > > > > >         Opt_compress_extension,
> > > > > > +    Opt_nocompress_extension,
> > > > > >         Opt_compress_chksum,
> > > > > >         Opt_compress_mode,
> > > > > >         Opt_atgc,
> > > > > > @@ -222,6 +223,7 @@ static match_table_t f2fs_tokens = {
> > > > > >         {Opt_compress_algorithm, "compress_algorithm=%s"},
> > > > > >         {Opt_compress_log_size, "compress_log_size=%u"},
> > > > > >         {Opt_compress_extension, "compress_extension=%s"},
> > > > > > +    {Opt_nocompress_extension, "nocompress_extension=%s"},
> > > > > >         {Opt_compress_chksum, "compress_chksum"},
> > > > > >         {Opt_compress_mode, "compress_mode=%s"},
> > > > > >         {Opt_atgc, "atgc"},
> > > > > > @@ -473,6 +475,43 @@ static int f2fs_set_test_dummy_encryption(struct
> > > > > > super_block *sb,
> > > > > >     }
> > > > > >     #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > > > +/*
> > > > > > + * 1. The same extension name cannot not appear in both compress and
> > > > > > non-compress extension
> > > > > > + * at the same time.
> > > > > > + * 2. If the compress extension specifies all files, the types
> > > > > > specified by the non-compress
> > > > > > + * extension will be treated as special cases and will not be
> > > > > > compressed.
> > > > > > + * 3. Don't allow the non-compress extension specifies all files.
> > > > > > + */
> > > > > > +static int f2fs_test_compress_extension(struct f2fs_sb_info *sbi)
> > > > > > +{
> > > > > > +    unsigned char (*ext)[F2FS_EXTENSION_LEN];
> > > > > > +    unsigned char (*noext)[F2FS_EXTENSION_LEN];
> > > > > > +    int ext_cnt, noext_cnt, index = 0, no_index = 0;
> > > > > > +
> > > > > > +    ext = F2FS_OPTION(sbi).extensions;
> > > > > > +    ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt;
> > > > > > +    noext = F2FS_OPTION(sbi).noextensions;
> > > > > > +    noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > > > > > +
> > > > > > +    if (!noext_cnt)
> > > > > > +        return 0;
> > > > > > +
> > > > > > +    for (no_index = 0; no_index < noext_cnt; no_index++) {
> > > > > > +        if (!strcasecmp("*", noext[no_index])) {
> > > > > > +            f2fs_info(sbi, "Don't allow the nocompress extension
> > > > > > specifies all files");
> > > > > > +            return -EINVAL;
> > > > > > +        }
> > > > > > +        for (index = 0; index < ext_cnt; index++) {
> > > > > > +            if (!strcasecmp(ext[index], noext[no_index])) {
> > > > > > +                f2fs_info(sbi, "Don't allow the same extension %s
> > > > > > appear in both compress and nocompress extension",
> > > > > > +                        ext[index]);
> > > > > > +                return -EINVAL;
> > > > > > +            }
> > > > > > +        }
> > > > > > +    }
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > >     #ifdef CONFIG_F2FS_FS_LZ4
> > > > > >     static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char
> > > > > > *str)
> > > > > >     {
> > > > > > @@ -546,7 +585,8 @@ static int parse_options(struct super_block *sb,
> > > > > > char *options, bool is_remount)
> > > > > >         substring_t args[MAX_OPT_ARGS];
> > > > > >     #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > > >         unsigned char (*ext)[F2FS_EXTENSION_LEN];
> > > > > > -    int ext_cnt;
> > > > > > +    unsigned char (*noext)[F2FS_EXTENSION_LEN];
> > > > > > +    int ext_cnt, noext_cnt;
> > > > > >     #endif
> > > > > >         char *p, *name;
> > > > > >         int arg = 0;
> > > > > > @@ -1049,6 +1089,30 @@ static int parse_options(struct super_block
> > > > > > *sb, char *options, bool is_remount)
> > > > > >                 F2FS_OPTION(sbi).compress_ext_cnt++;
> > > > > >                 kfree(name);
> > > > > >                 break;
> > > > > > +        case Opt_nocompress_extension:
> > > > > > +            if (!f2fs_sb_has_compression(sbi)) {
> > > > > > +                f2fs_info(sbi, "Image doesn't support compression");
> > > > > > +                break;
> > > > > > +            }
> > > > > > +            name = match_strdup(&args[0]);
> > > > > > +            if (!name)
> > > > > > +                return -ENOMEM;
> > > > > > +
> > > > > > +            noext = F2FS_OPTION(sbi).noextensions;
> > > > > > +            noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt;
> > > > > > +
> > > > > > +            if (strlen(name) >= F2FS_EXTENSION_LEN ||
> > > > > > +                noext_cnt >= COMPRESS_EXT_NUM) {
> > > > > > +                f2fs_err(sbi,
> > > > > > +                    "invalid extension length/number");
> > > > > > +                kfree(name);
> > > > > > +                return -EINVAL;
> > > > > > +            }
> > > > > > +
> > > > > > +            strcpy(noext[noext_cnt], name);
> > > > > > +            F2FS_OPTION(sbi).nocompress_ext_cnt++;
> > > > > > +            kfree(name);
> > > > > > +            break;
> > > > > >             case Opt_compress_chksum:
> > > > > >                 F2FS_OPTION(sbi).compress_chksum = true;
> > > > > >                 break;
> > > > > > @@ -1070,6 +1134,7 @@ static int parse_options(struct super_block *sb,
> > > > > > char *options, bool is_remount)
> > > > > >             case Opt_compress_algorithm:
> > > > > >             case Opt_compress_log_size:
> > > > > >             case Opt_compress_extension:
> > > > > > +        case Opt_nocompress_extension:
> > > > > >             case Opt_compress_chksum:
> > > > > >             case Opt_compress_mode:
> > > > > >                 f2fs_info(sbi, "compression options not supported");
> > > > > > @@ -1123,6 +1188,13 @@ static int parse_options(struct super_block
> > > > > > *sb, char *options, bool is_remount)
> > > > > >         }
> > > > > >     #endif
> > > > > > +#ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > > > +    if (f2fs_test_compress_extension(sbi)) {
> > > > > > +        f2fs_err(sbi, "invalid compress or nocompress extension");
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +#endif
> > > > > > +
> > > > > >         if (F2FS_IO_SIZE_BITS(sbi) && !f2fs_lfs_mode(sbi)) {
> > > > > >             f2fs_err(sbi, "Should set mode=lfs with %uKB-sized IO",
> > > > > >                  F2FS_IO_SIZE_KB(sbi));
> > > > > > @@ -1671,6 +1743,11 @@ static inline void
> > > > > > f2fs_show_compress_options(struct seq_file *seq,
> > > > > >                 F2FS_OPTION(sbi).extensions[i]);
> > > > > >         }
> > > > > > +    for (i = 0; i < F2FS_OPTION(sbi).nocompress_ext_cnt; i++) {
> > > > > > +        seq_printf(seq, ",nocompress_extension=%s",
> > > > > > +            F2FS_OPTION(sbi).noextensions[i]);
> > > > > > +    }
> > > > > > +
> > > > > >         if (F2FS_OPTION(sbi).compress_chksum)
> > > > > >             seq_puts(seq, ",compress_chksum");
> > > > > > 


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