[RFC PATCH v3 2/2] erofs-utils: fuse: support tail-packing inline compressed data

Yue Hu zbestahu at gmail.com
Mon Dec 6 14:40:10 AEDT 2021


Hi Xiang,

On Mon, 6 Dec 2021 08:39:04 +0800
Gao Xiang <hsiangkao at linux.alibaba.com> wrote:

> Hi Yue,
> 
> On Wed, Nov 17, 2021 at 05:22:01PM +0800, Yue Hu via Linux-erofs wrote:
> > Add tail-packing inline compressed data support for erofsfuse.
> > 
> > Signed-off-by: Yue Hu <huyue2 at yulong.com>  
> 
> Sorry about long delay, I'm very busy in container use cases now.
> I've checked your patch, some inlined comments below.
> 
> > ---
> > v3:
> > - remove z_idata_addr, add z_idata_headlcn instead of m_taillcn.
> > - add bug_on for legacy if enable inline and disable big pcluster.
> > - extract z_erofs_do_map_blocks() instead of added
> >   z_erofs_map_tail_data_blocks() with similar logic.
> > 
> > v2:
> > - add tail-packing information to inode and get it on first read.
> > - update tail-packing checking logic.
> > 
> >  include/erofs/internal.h |   2 +
> >  lib/decompress.c         |   3 -
> >  lib/zmap.c               | 136 +++++++++++++++++++++++++++++++++------
> >  3 files changed, 120 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > index 54e5939..5d1a44c 100644
> > --- a/include/erofs/internal.h
> > +++ b/include/erofs/internal.h
> > @@ -172,6 +172,8 @@ struct erofs_inode {
> >  			uint8_t  z_algorithmtype[2];
> >  			uint8_t  z_logical_clusterbits;
> >  			uint8_t  z_physical_clusterblks;
> > +			uint16_t z_idata_size;
> > +			uint32_t z_idata_headlcn;
> >  		};
> >  	};
> >  #ifdef WITH_ANDROID
> > diff --git a/lib/decompress.c b/lib/decompress.c
> > index 6f4ecc2..806ac91 100644
> > --- a/lib/decompress.c
> > +++ b/lib/decompress.c
> > @@ -71,9 +71,6 @@ out:
> >  int z_erofs_decompress(struct z_erofs_decompress_req *rq)
> >  {
> >  	if (rq->alg == Z_EROFS_COMPRESSION_SHIFTED) {
> > -		if (rq->inputsize != EROFS_BLKSIZ)
> > -			return -EFSCORRUPTED;
> > -  
> 
> 		if (rq->inputsize > EROFS_BLKSIZ)
> 			return -EFSCORRUPTED;
> 
> >  		DBG_BUGON(rq->decodedlength > EROFS_BLKSIZ);
> >  		DBG_BUGON(rq->decodedlength < rq->decodedskip);
> >  
> > diff --git a/lib/zmap.c b/lib/zmap.c
> > index 458030b..42783e5 100644
> > --- a/lib/zmap.c
> > +++ b/lib/zmap.c
> > @@ -10,6 +10,9 @@
> >  #include "erofs/io.h"
> >  #include "erofs/print.h"
> >  
> > +static int z_erofs_do_map_blocks(struct erofs_inode *vi,
> > +				 struct erofs_map_blocks *map);
> > +
> >  int z_erofs_fill_inode(struct erofs_inode *vi)
> >  {
> >  	if (!erofs_sb_has_big_pcluster() &&
> > @@ -18,12 +21,69 @@ int z_erofs_fill_inode(struct erofs_inode *vi)
> >  		vi->z_algorithmtype[0] = 0;
> >  		vi->z_algorithmtype[1] = 0;
> >  		vi->z_logical_clusterbits = LOG_BLOCK_SIZE;
> > +		vi->z_idata_size = 0;
> >  
> >  		vi->flags |= EROFS_I_Z_INITED;
> > +		DBG_BUGON(erofs_sb_has_tail_packing());
> >  	}
> >  	return 0;
> >  }
> >  
> > +static erofs_off_t compacted_inline_data_addr(struct erofs_inode *vi,
> > +					      unsigned int totalidx)
> > +{
> > +	const erofs_off_t ebase = round_up(iloc(vi->nid) + vi->inode_isize +
> > +				  vi->xattr_isize, 8) +
> > +				  sizeof(struct z_erofs_map_header);
> > +	unsigned int compacted_4b_initial, compacted_4b_end;
> > +	unsigned int compacted_2b;
> > +	erofs_off_t addr;
> > +
> > +	compacted_4b_initial = (32 - ebase % 32) / 4;
> > +	if (compacted_4b_initial == 32 / 4)
> > +		compacted_4b_initial = 0;
> > +
> > +	if (compacted_4b_initial > totalidx) {
> > +		compacted_4b_initial = 0;
> > +		compacted_2b = 0;
> > +	} else if (vi->z_advise & Z_EROFS_ADVISE_COMPACTED_2B) {
> > +		compacted_2b = rounddown(totalidx - compacted_4b_initial, 16);
> > +	} else
> > +		compacted_2b = 0;
> > +
> > +	compacted_4b_end = totalidx - compacted_4b_initial - compacted_2b;
> > +
> > +	addr = ebase + compacted_4b_initial * 4 + compacted_2b * 2;
> > +	if (compacted_4b_end > 1)
> > +		addr += (compacted_4b_end/2) * 8;
> > +	if (compacted_4b_end % 2)
> > +		addr += 8;
> > +
> > +	return addr;
> > +}
> > +
> > +static erofs_off_t legacy_inline_data_addr(struct erofs_inode *vi,
> > +					   unsigned int totalidx)
> > +{
> > +	return Z_EROFS_VLE_LEGACY_INDEX_ALIGN(iloc(vi->nid) + vi->inode_isize +
> > +					      vi->xattr_isize) +
> > +		totalidx * sizeof(struct z_erofs_vle_decompressed_index);
> > +}
> > +
> > +static erofs_off_t z_erofs_inline_data_addr(struct erofs_inode *vi)
> > +{
> > +	const unsigned int datamode = vi->datalayout;
> > +	const unsigned int totalidx = DIV_ROUND_UP(vi->i_size, EROFS_BLKSIZ);
> > +
> > +	if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> > +		return compacted_inline_data_addr(vi, totalidx);
> > +
> > +	if (datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY)
> > +		return legacy_inline_data_addr(vi, totalidx);
> > +
> > +	return -EINVAL;
> > +}  
> 
> no need these three new functions if introducing
> EROFS_GET_BLOCKS_FINDTAIL, see below..

Let me optimize the logic.

> 
> > +
> >  static int z_erofs_fill_inode_lazy(struct erofs_inode *vi)
> >  {
> >  	int ret;
> > @@ -44,6 +104,7 @@ static int z_erofs_fill_inode_lazy(struct erofs_inode *vi)
> >  
> >  	h = (struct z_erofs_map_header *)buf;
> >  	vi->z_advise = le16_to_cpu(h->h_advise);
> > +	vi->z_idata_size = le16_to_cpu(h->h_idata_size);
> >  	vi->z_algorithmtype[0] = h->h_algorithmtype & 15;
> >  	vi->z_algorithmtype[1] = h->h_algorithmtype >> 4;
> >  
> > @@ -61,6 +122,16 @@ static int z_erofs_fill_inode_lazy(struct erofs_inode *vi)
> >  			  vi->nid * 1ULL);
> >  		return -EFSCORRUPTED;
> >  	}
> > +
> > +	if (vi->z_idata_size) {
> > +		struct erofs_map_blocks map = { .m_la = vi->i_size - 1 };
> > +
> > +		ret = z_erofs_do_map_blocks(vi, &map);
> > +		if (ret)
> > +			return ret;  
> 
> How about introducing another mode called
> EROFS_GET_BLOCKS_FINDTAIL

Aha, nice name. That flag have leaped into my mind :)

> 
> which implys .m_la = vi->i_size - 1 internally, so we won't need to
> handle .m_la here.
> 
> > +		vi->z_idata_headlcn = map.m_la >> vi->z_logical_clusterbits;  
> 
> Also updaing vi->z_idata_headlcn when EROFS_GET_BLOCKS_FINDTAIL.

Right.

> 
> > +	}
> > +
> >  	vi->flags |= EROFS_I_Z_INITED;
> >  	return 0;
> >  }
> > @@ -374,7 +445,8 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> >  }
> >  
> >  static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
> > -					    unsigned int initial_lcn)
> > +					    unsigned int initial_lcn,
> > +					    bool idatamap)
> >  {
> >  	struct erofs_inode *const vi = m->inode;
> >  	struct erofs_map_blocks *const map = m->map;
> > @@ -384,6 +456,12 @@ 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 (idatamap) {
> > +		map->m_plen = vi->z_idata_size;
> > +		return 0;
> > +	}
> > +
> >  	if (!(map->m_flags & EROFS_MAP_ZIPPED) ||
> >  	    !(vi->z_advise & Z_EROFS_ADVISE_BIG_PCLUSTER_1)) {
> >  		map->m_plen = 1 << lclusterbits;
> > @@ -440,8 +518,8 @@ err_bonus_cblkcnt:
> >  	return -EFSCORRUPTED;
> >  }
> >  
> > -int z_erofs_map_blocks_iter(struct erofs_inode *vi,
> > -			    struct erofs_map_blocks *map)
> > +static int z_erofs_do_map_blocks(struct erofs_inode *vi,
> > +				 struct erofs_map_blocks *map)
> >  {
> >  	struct z_erofs_maprecorder m = {
> >  		.inode = vi,
> > @@ -452,18 +530,7 @@ int z_erofs_map_blocks_iter(struct erofs_inode *vi,
> >  	unsigned int lclusterbits, endoff;
> >  	unsigned long initial_lcn;
> >  	unsigned long long ofs, end;
> > -
> > -	/* when trying to read beyond EOF, leave it unmapped */
> > -	if (map->m_la >= vi->i_size) {
> > -		map->m_llen = map->m_la + 1 - vi->i_size;
> > -		map->m_la = vi->i_size;
> > -		map->m_flags = 0;
> > -		goto out;
> > -	}
> > -
> > -	err = z_erofs_fill_inode_lazy(vi);
> > -	if (err)
> > -		goto out;
> > +	bool idatamap;
> >  
> >  	lclusterbits = vi->z_logical_clusterbits;
> >  	ofs = map->m_la;
> > @@ -510,19 +577,52 @@ int z_erofs_map_blocks_iter(struct erofs_inode *vi,
> >  		goto out;
> >  	}
> >  
> > +	/* check if mapping tail-packing data */
> > +	idatamap = vi->z_idata_size && (ofs == vi->i_size - 1 ||
> > +		   m.lcn == vi->z_idata_headlcn);  
> 
> better namin as `is_idata'...

Okay.

> 
> I think no need to handle ofs == vi->i_size - 1 specially here
> if EROFS_GET_BLOCKS_FINDTAIL is introduced...

Right.

> 
> > +
> > +	/* need to trim tail-packing data if beyond file size */
> >  	map->m_llen = end - map->m_la;
> > -	map->m_pa = blknr_to_addr(m.pblk);
> > +	if (idatamap && end > vi->i_size)
> > +		map->m_llen -= end - vi->i_size;  
> 
> No need as well?

Let me confirm.

> 
> >  
> > -	err = z_erofs_get_extent_compressedlen(&m, initial_lcn);
> > +	if (idatamap && (vi->z_advise & Z_EROFS_ADVISE_INLINE_DATA)) {
> > +		map->m_pa = z_erofs_inline_data_addr(vi);  
> 
> How about setting another u32 z_idataoff when EROFS_GET_BLOCKS_FINDTAIL
> so we won't need to introduce another calculate methods instead?

Agree, need to improve it.

> 
> Thanks,
> Gao Xiang



More information about the Linux-erofs mailing list