Messages in this thread | | | Subject | Re: [f2fs-dev] [PATCH v6] f2fs: compress: support compress level | From | Chao Yu <> | Date | Fri, 4 Dec 2020 09:18:31 +0800 |
| |
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 > . >
| |