Re: [f2fs-dev] [PATCH v6] f2fs: compress: support compress level

Chao Yu
2020-12-04T01:19:19Z

On 2020/12/4 3:32, Eric Biggers wrote:
> On Thu, Dec 03, 2020 at 02:17:15PM +0800, Chao Yu wrote:
>> +config F2FS_FS_LZ4HC
>> + bool "LZ4HC compression support"
>> + depends on F2FS_FS_COMPRESSION
>> + depends on F2FS_FS_LZ4
>> + select LZ4HC_COMPRESS
>> + default y
>> + help
>> + Support LZ4HC compress algorithm, if unsure, say Y.
>> +
>
> It would be helpful to mention that LZ4HC is on-disk compatible with LZ4.

Sure, let me update description.

>
>> static int lz4_compress_pages(struct compress_ctx *cc)
>> {
>> int len;
>>
>> +#ifdef CONFIG_F2FS_FS_LZ4HC
>> + return lz4hc_compress_pages(cc);
>> +#endif
>
> This looks wrong; it always calls lz4hc compression even for regular lz4.

It's fine. lz4hc_compress_pages() will call LZ4_compress_HC() only when
compress level is set.

>
>> static int parse_options(struct super_block *sb, char *options, bool is_remount)
>> {
>> struct f2fs_sb_info *sbi = F2FS_SB(sb);
>> @@ -886,10 +939,22 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>> if (!strcmp(name, "lzo")) {
>> F2FS_OPTION(sbi).compress_algorithm =
>> COMPRESS_LZO;
>> - } else if (!strcmp(name, "lz4")) {
>> + } else if (!strncmp(name, "lz4", 3)) {
>
> strcmp() is fine, no need for strncmp().

Actually, I reuse "lz4" mount option parameter, to enable lz4hc algorithm, user
only need to specify compress level after "lz4" string, e.g.
compress_algorithm=lz4, f2fs use lz4 algorithm
compress_algorithm=lz4:xx, f2fs use lz4hc algorithm with compress level xx.

So, I use !strncmp(name, "lz4", 3) here.

>
>> @@ -1547,6 +1612,9 @@ static inline void f2fs_show_compress_options(struct seq_file *seq,
>> }
>> seq_printf(seq, ",compress_algorithm=%s", algtype);
>>
>> + if (!F2FS_OPTION(sbi).compress_level)
>> + seq_printf(seq, ":%d", F2FS_OPTION(sbi).compress_level);
>> +
>
> This looks wrong; it only prints compress_level if it is 0.

Yes, will fix.

>
>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
>> index 55be7afeee90..2dcc63fe8494 100644
>> --- a/include/linux/f2fs_fs.h
>> +++ b/include/linux/f2fs_fs.h
>> @@ -275,6 +275,9 @@ struct f2fs_inode {
>> __u8 i_compress_algorithm; /* compress algorithm */
>> __u8 i_log_cluster_size; /* log of cluster size */
>> __le16 i_compress_flag; /* compress flag */
>> + /* 0 bit: chksum flag
>> + * [10,15] bits: compress level
>> + */
>
> What is the use case for storing the compression level on-disk?

One case I can image is if user wants to specify different compress level for
different algorithm in separated files.

e.g.
mount -o comrpess_algorithm=zstd:10 /dev/sdc /f2fs
touch fileA;
write fileA;
mount -o remount,comrpess_algorithm=lz4:8 /f2fs
write fileA;
touch fileB;
write fileB;

Thanks,

>
> Keep in mind that compression levels are an implementation detail; the exact
> compressed data that is produced by a particular algorithm at a particular
> compression level is *not* a stable interface. It can change when the
> compressor is updated, as long as the output continues to be compatible with the
> decompressor.
>
> So does compression level really belong in the on-disk format?
>
> - Eric
> .
>