[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