Hi Chao, On Mon, Mar 29, 2021 at 02:26:05PM +0800, Chao Yu wrote: > On 2021/3/29 9:23, Gao Xiang wrote: > > From: Gao Xiang <hsiang...@redhat.com> > > > > Add a bitmap for available compression algorithms and a variable-sized > > on-disk table for compression options in preparation for upcoming big > > pcluster and LZMA algorithm, which follows the end of super block. > > > > To parse the compression options, the bitmap is scanned one by one. > > For each available algorithm, there is data followed by 2-byte `length' > > correspondingly (it's enough for most cases, or entire fs blocks should > > be used.) > > > > With such available algorithm bitmap, kernel itself can also refuse to > > mount such filesystem if any unsupported compression algorithm exists. > > > > Note that COMPR_CFGS feature will be enabled with BIG_PCLUSTER. > > > > Signed-off-by: Gao Xiang <hsiang...@redhat.com> > > --- > > fs/erofs/decompressor.c | 2 +- > > fs/erofs/erofs_fs.h | 14 ++-- > > fs/erofs/internal.h | 5 +- > > fs/erofs/super.c | 143 +++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 157 insertions(+), 7 deletions(-) > > > > diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c > > index 97538ff24a19..27aa6a99b371 100644 > > --- a/fs/erofs/decompressor.c > > +++ b/fs/erofs/decompressor.c > > @@ -41,7 +41,7 @@ int z_erofs_load_lz4_config(struct super_block *sb, > > } > > distance = le16_to_cpu(lz4->max_distance); > > } else { > > - distance = le16_to_cpu(dsb->lz4_max_distance); > > + distance = le16_to_cpu(dsb->u1.lz4_max_distance); > > } > > EROFS_SB(sb)->lz4.max_distance_pages = distance ? > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > > index e0f3c0db1f82..5a126493d4d9 100644 > > --- a/fs/erofs/erofs_fs.h > > +++ b/fs/erofs/erofs_fs.h > > @@ -18,15 +18,16 @@ > > * be incompatible with this kernel version. > > */ > > #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING 0x00000001 > > +#define EROFS_FEATURE_INCOMPAT_COMPR_CFGS 0x00000002 > > #define EROFS_ALL_FEATURE_INCOMPAT > > EROFS_FEATURE_INCOMPAT_LZ4_0PADDING > > -/* 128-byte erofs on-disk super block */ > > +/* erofs on-disk super block (currently 128 bytes) */ > > struct erofs_super_block { > > __le32 magic; /* file system magic number */ > > __le32 checksum; /* crc32c(super_block) */ > > __le32 feature_compat; > > __u8 blkszbits; /* support block_size == PAGE_SIZE only */ > > - __u8 reserved; > > + __u8 sb_extslots; /* superblock size = 128 + sb_extslots * 16 */ > > __le16 root_nid; /* nid of root directory */ > > __le64 inos; /* total valid ino # (== f_files - f_favail) */ > > @@ -39,8 +40,12 @@ struct erofs_super_block { > > __u8 uuid[16]; /* 128-bit uuid for volume */ > > __u8 volume_name[16]; /* volume name */ > > __le32 feature_incompat; > > - /* customized lz4 sliding window size instead of 64k by default */ > > - __le16 lz4_max_distance; > > + union { > > + /* bitmap for available compression algorithms */ > > + __le16 available_compr_algs; > > + /* customized sliding window size instead of 64k by default */ > > + __le16 lz4_max_distance; > > + } __packed u1; > > __u8 reserved2[42]; > > }; > > @@ -196,6 +201,7 @@ enum { > > Z_EROFS_COMPRESSION_LZ4 = 0, > > Z_EROFS_COMPRESSION_MAX > > }; > > +#define Z_EROFS_ALL_COMPR_ALGS (1 << (Z_EROFS_COMPRESSION_MAX > > - 1)) > > /* 14 bytes (+ length field = 16 bytes) */ > > struct z_erofs_lz4_cfgs { > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h > > index 46b977f348eb..f3fa895d809f 100644 > > --- a/fs/erofs/internal.h > > +++ b/fs/erofs/internal.h > > @@ -75,6 +75,7 @@ struct erofs_sb_info { > > struct xarray managed_pslots; > > unsigned int shrinker_run_no; > > + u16 available_compr_algs; > > /* pseudo inode to manage cached pages */ > > struct inode *managed_cache; > > @@ -90,6 +91,7 @@ struct erofs_sb_info { > > /* inode slot unit size in bit shift */ > > unsigned char islotbits; > > + u32 sb_size; /* total superblock size */ > > u32 build_time_nsec; > > u64 build_time; > > @@ -233,6 +235,7 @@ static inline bool erofs_sb_has_##name(struct > > erofs_sb_info *sbi) \ > > } > > EROFS_FEATURE_FUNCS(lz4_0padding, incompat, INCOMPAT_LZ4_0PADDING) > > +EROFS_FEATURE_FUNCS(compr_cfgs, incompat, INCOMPAT_COMPR_CFGS) > > EROFS_FEATURE_FUNCS(sb_chksum, compat, COMPAT_SB_CHKSUM) > > /* atomic flag definitions */ > > @@ -454,7 +457,7 @@ static inline int z_erofs_load_lz4_config(struct > > super_block *sb, > > struct erofs_super_block *dsb, > > struct z_erofs_lz4_cfgs *lz4, int len) > > { > > - if (lz4 || dsb->lz4_max_distance) { > > + if (lz4 || dsb->u1.lz4_max_distance) { > > erofs_err(sb, "lz4 algorithm isn't enabled"); > > return -EINVAL; > > } > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c > > index 1ca8da3f2125..628c751634fe 100644 > > --- a/fs/erofs/super.c > > +++ b/fs/erofs/super.c > > @@ -122,6 +122,138 @@ static bool check_layout_compatibility(struct > > super_block *sb, > > return true; > > } > > +#ifdef CONFIG_EROFS_FS_ZIP > > +/* read variable-sized metadata, offset will be aligned by 4-byte */ > > +static void *erofs_read_metadata(struct super_block *sb, struct page > > **pagep, > > + erofs_off_t *offset, int *lengthp) > > +{ > > + struct page *page = *pagep; > > + u8 *buffer, *ptr; > > + int len, i, cnt; > > + erofs_blk_t blk; > > + > > + *offset = round_up(*offset, 4); > > + blk = erofs_blknr(*offset); > > + > > + if (!page || page->index != blk) { > > + if (page) { > > + unlock_page(page); > > + put_page(page); > > + } > > + page = erofs_get_meta_page(sb, blk); > > + if (IS_ERR(page)) > > + goto err_nullpage; > > + } > > + > > + ptr = kmap(page); > > + len = le16_to_cpu(*(__le16 *)&ptr[erofs_blkoff(*offset)]); > > + if (!len) > > + len = U16_MAX + 1; > > + buffer = kmalloc(len, GFP_KERNEL); > > + if (!buffer) { > > + buffer = ERR_PTR(-ENOMEM); > > + goto out; > > + } > > + *offset += sizeof(__le16); > > + *lengthp = len; > > + > > + for (i = 0; i < len; i += cnt) { > > + cnt = min(EROFS_BLKSIZ - (int)erofs_blkoff(*offset), len - i); > > + blk = erofs_blknr(*offset); > > + > > + if (!page || page->index != blk) { > > + if (page) { > > + kunmap(page); > > + unlock_page(page); > > + put_page(page); > > + } > > + page = erofs_get_meta_page(sb, blk); > > + if (IS_ERR(page)) { > > + kfree(buffer); > > + goto err_nullpage; > > + } > > + ptr = kmap(page); > > + } > > + memcpy(buffer + i, ptr + erofs_blkoff(*offset), cnt); > > + *offset += cnt; > > + } > > +out: > > + kunmap(page); > > + *pagep = page; > > + return buffer; > > +err_nullpage: > > + *pagep = NULL; > > + return page; > > +} > > + > > +static int erofs_load_compr_cfgs(struct super_block *sb, > > + struct erofs_super_block *dsb) > > +{ > > + struct erofs_sb_info *sbi; > > + struct page *page; > > + unsigned int algs, alg; > > + erofs_off_t offset; > > + int size, ret; > > + > > + sbi = EROFS_SB(sb); > > + sbi->available_compr_algs = le16_to_cpu(dsb->u1.available_compr_algs); > > + > > + if (sbi->available_compr_algs & ~Z_EROFS_ALL_COMPR_ALGS) { > > + erofs_err(sb, > > +"try to load compressed image with unsupported algorithms %x", > > Minor style issue: > > "try to load compressed image with unsupported " > "algorithms %x", > sbi->available_compr_algs & ~Z_EROFS_ALL_COMPR_ALGS);
If I remembered correctly, kernel code style suggests "don't break the print message, it'd more easy to grep". Actually such style above is from XFS, and can pass checkpatch.pl check, see: (fs/xfs/xfs_log_recover.c) xlog_recover(): /* * Version 5 superblock log feature mask validation. We know the * log is dirty so check if there are any unknown log features * in what we need to recover. If there are unknown features * (e.g. unsupported transactions, then simply reject the * attempt at recovery before touching anything. */ if (XFS_SB_VERSION_NUM(&log->l_mp->m_sb) == XFS_SB_VERSION_5 && xfs_sb_has_incompat_log_feature(&log->l_mp->m_sb, XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN)) { xfs_warn(log->l_mp, "Superblock has unknown incompatible log features (0x%x) enabled.", (log->l_mp->m_sb.sb_features_log_incompat & XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN)); xfs_warn(log->l_mp, "The log can not be fully and/or safely recovered by this kernel."); xfs_warn(log->l_mp, "Please recover the log on a kernel that supports the unknown features."); return -EINVAL; } If that does't look ok for us, I could use > 80 line for this instead, but I tend to not break the message .. > > > + sbi->available_compr_algs & ~Z_EROFS_ALL_COMPR_ALGS); > > + return -EINVAL; > > + } > > + > > + offset = EROFS_SUPER_OFFSET + sbi->sb_size; > > + page = NULL; > > + alg = 0; > > + ret = 0; > > + > > + for (algs = sbi->available_compr_algs; algs; algs >>= 1, ++alg) { > > + void *data; > > + > > + if (!(algs & 1)) > > + continue; > > + > > + data = erofs_read_metadata(sb, &page, &offset, &size); > > + if (IS_ERR(data)) { > > + ret = PTR_ERR(data); > > + goto err; > > + } > > + > > + switch (alg) { > > + case Z_EROFS_COMPRESSION_LZ4: > > + ret = z_erofs_load_lz4_config(sb, dsb, data, size); > > + break; > > + default: > > + DBG_BUGON(1); > > + ret = -EFAULT; > > + } > > + kfree(data); > > + if (ret) > > + goto err; > > + } > > +err: > > + if (page) { > > + unlock_page(page); > > + put_page(page); > > + } > > + return ret; > > +} > > +#else > > +static int erofs_load_compr_cfgs(struct super_block *sb, > > + struct erofs_super_block *dsb) > > +{ > > + if (dsb->u1.available_compr_algs) { > > + erofs_err(sb, > > +"try to load compressed image when compression is disabled"); > > Ditto, > erofs_err(sb, "try to load compressed image when " > "compression is disabled"); > The same above.... > > > + return -EINVAL; > > + } > > + return 0; > > +} > > +#endif > > + > > static int erofs_read_superblock(struct super_block *sb) > > { > > struct erofs_sb_info *sbi; > > @@ -166,6 +298,12 @@ static int erofs_read_superblock(struct super_block > > *sb) > > if (!check_layout_compatibility(sb, dsb)) > > goto out; > > + sbi->sb_size = 128 + dsb->sb_extslots * 16; > > sbi->sb_size = sizeof(struct erofs_super_block) + > dsb->sb_extslots * EROFS_EXTSLOT_SIZE; > > Otherwise it looks good to me, > > Reviewed-by: Chao Yu <yuch...@huawei.com> Thanks for the review! Thanks, Gao Xiang > > Thanks, > > > + if (sbi->sb_size > EROFS_BLKSIZ) { > > + erofs_err(sb, "invalid sb_extslots %u (more than a fs block)", > > + sbi->sb_size); > > + goto out; > > + } > > sbi->blocks = le32_to_cpu(dsb->blocks); > > sbi->meta_blkaddr = le32_to_cpu(dsb->meta_blkaddr); > > #ifdef CONFIG_EROFS_FS_XATTR > > @@ -189,7 +327,10 @@ static int erofs_read_superblock(struct super_block > > *sb) > > } > > /* parse on-disk compression configurations */ > > - ret = z_erofs_load_lz4_config(sb, dsb, NULL, 0); > > + if (erofs_sb_has_compr_cfgs(sbi)) > > + ret = erofs_load_compr_cfgs(sb, dsb); > > + else > > + ret = z_erofs_load_lz4_config(sb, dsb, NULL, 0); > > out: > > kunmap(page); > > put_page(page); > > >