Hi Yue, On Fri, Oct 08, 2021 at 11:00:29AM +0800, Yue Hu wrote: > On Fri, 8 Oct 2021 01:06:03 +0800 > Gao Xiang <[email protected]> wrote: > > > From: Gao Xiang <[email protected]> > > > > Currently, z_erofs_map_blocks returns whether extents are compressed > > Mapping function is z_erofs_map_blocks_iter()?
I just meant the compression mapping... I'm fine with the update.. > > > or not, and the decompression frontend gets the specific algorithms > > then. > > > > It works but not quite well in many aspests, for example: > > - The decompression frontend has to deal with whether extents are > > compressed or not again and lookup the algorithms if compressed. > > It's duplicated and too detailed about the on-disk mapping. > > > > - A new secondary compression head will be introduced later so that > > each file can have 2 compression algorithms at most for different > > type of data. It could increase the complexity of the decompression > > frontend if still handled in this way; > > > > - A new readmore decompression strategy will be introduced to get > > better performance for much bigger pcluster and lzma, which needs > > the specific algorithm in advance as well. > > > > Let's look up compression algorithms directly in z_erofs_map_blocks() > > Same mapping function name mistake? Ok, will update as well. > > > instead. > > > > Signed-off-by: Gao Xiang <[email protected]> > > --- > > fs/erofs/compress.h | 5 ----- > > fs/erofs/internal.h | 12 +++++++++--- > > fs/erofs/zdata.c | 12 ++++++------ > > fs/erofs/zmap.c | 19 ++++++++++--------- > > include/trace/events/erofs.h | 2 +- > > 5 files changed, 26 insertions(+), 24 deletions(-) > > > > diff --git a/fs/erofs/compress.h b/fs/erofs/compress.h > > index 3701c72bacb2..ad62d1b4d371 100644 > > --- a/fs/erofs/compress.h > > +++ b/fs/erofs/compress.h > > @@ -8,11 +8,6 @@ > > > > #include "internal.h" > > > > -enum { > > - Z_EROFS_COMPRESSION_SHIFTED = Z_EROFS_COMPRESSION_MAX, > > - Z_EROFS_COMPRESSION_RUNTIME_MAX > > -}; > > - > > struct z_erofs_decompress_req { > > struct super_block *sb; > > struct page **in, **out; > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h > > index 9524e155b38f..48bfc6eb2b02 100644 > > --- a/fs/erofs/internal.h > > +++ b/fs/erofs/internal.h > > @@ -338,7 +338,7 @@ extern const struct address_space_operations > > z_erofs_aops; > > * of the corresponding uncompressed data in the file. > > */ > > enum { > > - BH_Zipped = BH_PrivateStart, > > + BH_Encoded = BH_PrivateStart, > > BH_FullMapped, > > }; > > > > @@ -346,8 +346,8 @@ enum { > > #define EROFS_MAP_MAPPED (1 << BH_Mapped) > > /* Located in metadata (could be copied from bd_inode) */ > > #define EROFS_MAP_META (1 << BH_Meta) > > -/* The extent has been compressed */ > > -#define EROFS_MAP_ZIPPED (1 << BH_Zipped) > > +/* The extent is encoded */ > > +#define EROFS_MAP_ENCODED (1 << BH_Encoded) > > /* The length of extent is full */ > > #define EROFS_MAP_FULL_MAPPED (1 << BH_FullMapped) > > > > @@ -355,6 +355,7 @@ struct erofs_map_blocks { > > erofs_off_t m_pa, m_la; > > u64 m_plen, m_llen; > > > > + char m_algorithmformat; > > unsigned int m_flags; > > > > struct page *mpage; > > @@ -368,6 +369,11 @@ struct erofs_map_blocks { > > */ > > #define EROFS_GET_BLOCKS_FIEMAP 0x0002 > > > > +enum { > > + Z_EROFS_COMPRESSION_SHIFTED = Z_EROFS_COMPRESSION_MAX, > > + Z_EROFS_COMPRESSION_RUNTIME_MAX > > +}; > > + > > /* zmap.c */ > > extern const struct iomap_ops z_erofs_iomap_report_ops; > > > > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c > > index 11c7a1aaebad..5c34ef66677f 100644 > > --- a/fs/erofs/zdata.c > > +++ b/fs/erofs/zdata.c > > @@ -476,6 +476,11 @@ static int z_erofs_register_collection(struct > > z_erofs_collector *clt, > > struct erofs_workgroup *grp; > > int err; > > > > + if (!(map->m_flags & EROFS_MAP_ENCODED)) { > > + DBG_BUGON(1); > > + return -EFSCORRUPTED; > > + } > > + > > /* no available pcluster, let's allocate one */ > > pcl = z_erofs_alloc_pcluster(map->m_plen >> PAGE_SHIFT); > > if (IS_ERR(pcl)) > > @@ -483,16 +488,11 @@ static int z_erofs_register_collection(struct > > z_erofs_collector *clt, > > > > atomic_set(&pcl->obj.refcount, 1); > > pcl->obj.index = map->m_pa >> PAGE_SHIFT; > > - > > + pcl->algorithmformat = map->m_algorithmformat; > > pcl->length = (map->m_llen << Z_EROFS_PCLUSTER_LENGTH_BIT) | > > (map->m_flags & EROFS_MAP_FULL_MAPPED ? > > Z_EROFS_PCLUSTER_FULL_LENGTH : 0); > > > > - if (map->m_flags & EROFS_MAP_ZIPPED) > > - pcl->algorithmformat = Z_EROFS_COMPRESSION_LZ4; > > - else > > - pcl->algorithmformat = Z_EROFS_COMPRESSION_SHIFTED; > > - > > /* new pclusters should be claimed as type 1, primary and followed */ > > pcl->next = clt->owned_head; > > clt->mode = COLLECT_PRIMARY_FOLLOWED; > > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c > > index 7a6df35fdc91..9d9c26343dab 100644 > > --- a/fs/erofs/zmap.c > > +++ b/fs/erofs/zmap.c > > @@ -111,7 +111,7 @@ struct z_erofs_maprecorder { > > > > unsigned long lcn; > > /* compression extent information gathered */ > > - u8 type; > > + u8 type, headtype; > > u16 clusterofs; > > u16 delta[2]; > > erofs_blk_t pblk, compressedlcs; > > @@ -446,9 +446,8 @@ static int z_erofs_extent_lookback(struct > > z_erofs_maprecorder *m, > > } > > return z_erofs_extent_lookback(m, m->delta[0]); > > case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN: > > - map->m_flags &= ~EROFS_MAP_ZIPPED; > > - fallthrough; > > case Z_EROFS_VLE_CLUSTER_TYPE_HEAD: > > + m->headtype = m->type; > > map->m_la = (lcn << lclusterbits) | m->clusterofs; > > break; > > default: > > @@ -472,7 +471,7 @@ static int z_erofs_get_extent_compressedlen(struct > > z_erofs_maprecorder *m, > > > > DBG_BUGON(m->type != Z_EROFS_VLE_CLUSTER_TYPE_PLAIN && > > m->type != Z_EROFS_VLE_CLUSTER_TYPE_HEAD); > > - if (!(map->m_flags & EROFS_MAP_ZIPPED) || > > + if (m->headtype == Z_EROFS_VLE_CLUSTER_TYPE_PLAIN || > > !(vi->z_advise & Z_EROFS_ADVISE_BIG_PCLUSTER_1)) { > > map->m_plen = 1 << lclusterbits; > > return 0; > > @@ -609,16 +608,13 @@ int z_erofs_map_blocks_iter(struct inode *inode, > > if (err) > > goto unmap_out; > > > > - map->m_flags = EROFS_MAP_ZIPPED; /* by default, compressed */ > > end = (m.lcn + 1ULL) << lclusterbits; > > > > switch (m.type) { > > case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN: > > - if (endoff >= m.clusterofs) > > - map->m_flags &= ~EROFS_MAP_ZIPPED; > > - fallthrough; > > case Z_EROFS_VLE_CLUSTER_TYPE_HEAD: > > if (endoff >= m.clusterofs) { > > + m.headtype = m.type; > > map->m_la = (m.lcn << lclusterbits) | m.clusterofs; > > break; > > } > > @@ -650,12 +646,17 @@ int z_erofs_map_blocks_iter(struct inode *inode, > > > > map->m_llen = end - map->m_la; > > map->m_pa = blknr_to_addr(m.pblk); > > - map->m_flags |= EROFS_MAP_MAPPED; > > + map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED; > > Seems the new flag EROFS_MAP_ENCODED may be a little redundant? > > It is used only for checking whether the file system is corrupted or not when > entering z_erofs_register_collection(). > > And before collection begins, we will check whether the EROFS_MAP_MAPPED is > set > or not. We will do the collection only if EROFS_MAP_MAPPED is set. That's to > say, > the above checking to EROFS_MAP_ENCODED should be not possible? I think it's useful to mark such data isn't a plain data (cannot read directly, plus m_llen != m_plen). For example, t's useful for fiemap (actually there is an improvement / a fix for iomap fiemap report code to identify/mark the encode flag.) BTW, I found that this patchset was still buggy, I'd suggest to ignore this patchset. I need to revise them later. Thanks, Gao Xiang > > Thanks.
