Re: [PATCH v2 2/3] erofs: introduce the secondary compression head

Gao Xiang
2021-10-09T04:47:53Z

Hi Yue,

On Sat, Oct 09, 2021 at 11:50:32AM +0800, Yue Hu wrote:
> On Sat, 9 Oct 2021 04:08:38 +0800
> Gao Xiang <xiang@kernel.org> wrote:
>
> > From: Gao Xiang <hsiangkao@linux.alibaba.com>
> >
> > Previously, for each HEAD lcluster, it can be either HEAD or PLAIN
> > lcluster to indicate whether the whole pcluster is compressed or not.
> >
> > In this patch, a new HEAD2 head type is introduced to specify another
> > compression algorithm other than the primary algorithm for each
> > compressed file, which can be used for upcoming LZMA compression and
> > LZ4 range dictionary compression for various data patterns.
> >
> > It has been stayed in the EROFS roadmap for years. Complete it now!
> >
> > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > ---
> > fs/erofs/erofs_fs.h | 8 +++++---
> > fs/erofs/zmap.c | 36 +++++++++++++++++++++++++++---------
> > 2 files changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> > index b0b23f41abc3..f579c8c78fff 100644
> > --- a/fs/erofs/erofs_fs.h
> > +++ b/fs/erofs/erofs_fs.h
> > @@ -21,11 +21,13 @@
> > #define EROFS_FEATURE_INCOMPAT_COMPR_CFGS 0x00000002
> > #define EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER 0x00000002
> > #define EROFS_FEATURE_INCOMPAT_CHUNKED_FILE 0x00000004
> > +#define EROFS_FEATURE_INCOMPAT_COMPR_HEAD2 0x00000008
> > #define EROFS_ALL_FEATURE_INCOMPAT \
> > (EROFS_FEATURE_INCOMPAT_LZ4_0PADDING | \
> > EROFS_FEATURE_INCOMPAT_COMPR_CFGS | \
> > EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER | \
> > - EROFS_FEATURE_INCOMPAT_CHUNKED_FILE)
> > + EROFS_FEATURE_INCOMPAT_CHUNKED_FILE | \
> > + EROFS_FEATURE_INCOMPAT_COMPR_HEAD2)
> >
> > #define EROFS_SB_EXTSLOT_SIZE 16
> >
> > @@ -314,9 +316,9 @@ struct z_erofs_map_header {
> > */
> > enum {
> > Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0,
> > - Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1,
> > + Z_EROFS_VLE_CLUSTER_TYPE_HEAD1 = 1,
> > Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD = 2,
> > - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3,
> > + Z_EROFS_VLE_CLUSTER_TYPE_HEAD2 = 3,
> > Z_EROFS_VLE_CLUSTER_TYPE_MAX
> > };
> >
> > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > index 9d9c26343dab..03945f15ceae 100644
> > --- a/fs/erofs/zmap.c
> > +++ b/fs/erofs/zmap.c
> > @@ -69,11 +69,17 @@ static int z_erofs_fill_inode_lazy(struct inode *inode)
> > vi->z_algorithmtype[1] = h->h_algorithmtype >> 4;
> >
> > if (vi->z_algorithmtype[0] >= Z_EROFS_COMPRESSION_MAX) {
> > - erofs_err(sb, "unknown compression format %u for nid %llu, please upgrade kernel",
> > + erofs_err(sb, "unknown HEAD1 format %u for nid %llu, please upgrade kernel",
> > vi->z_algorithmtype[0], vi->nid);
> > err = -EOPNOTSUPP;
> > goto unmap_done;
> > }
> > + if (vi->z_algorithmtype[1] >= Z_EROFS_COMPRESSION_MAX) {
> > + erofs_err(sb, "unknown HEAD2 format %u for nid %llu, please upgrade kernel",
> > + vi->z_algorithmtype[1], vi->nid);
> > + err = -EOPNOTSUPP;
> > + goto unmap_done;
> > + }
>
> Seems duplicated a little, how about below code?
>
> if (vi->z_algorithmtype[i] >= Z_EROFS_COMPRESSION_MAX ||
> vi->z_algorithmtype[++i] >= Z_EROFS_COMPRESSION_MAX) {
> erofs_err(sb, "unknown HEAD%u format %u for nid %llu, please upgrade kernel",
> i, vi->z_algorithmtype[i], vi->nid);
> err = -EOPNOTSUPP;
> goto unmap_done;
> }

Yeah, good simplification. I will update it and rename `i' to `headnr'
here.

Thanks,
Gao Xiang