[RFC PATCH 1/2] erofs-utils: support tail-packing inline compressed data
Yue Hu
zbestahu at gmail.com
Wed Oct 27 12:58:07 AEDT 2021
Hi Xiang,
On Tue, 26 Oct 2021 20:00:37 +0800
Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
> Hi Yue,
>
> On Mon, Oct 25, 2021 at 08:30:43PM +0800, Yue Hu wrote:
> > Currently, we only support tail-end inline data for uncompressed
> > files, let's support it as well for compressed files.
> >
> > The idea is from Xiang.
> >
> > Signed-off-by: Yue Hu <huyue2 at yulong.com>
>
> The patch roughly looks good to me. I'll play with it this week later.
>
> Some comments inline:
>
> > ---
> > include/erofs/internal.h | 2 ++
> > include/erofs_fs.h | 6 +++-
> > lib/compress.c | 74 ++++++++++++++++++++++++++++++++--------
> > lib/compressor.c | 9 ++---
> > lib/decompress.c | 4 +++
> > lib/inode.c | 50 +++++++++++++++------------
> > mkfs/main.c | 6 ++++
> > 7 files changed, 109 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > index da7be56..42ae1ed 100644
> > --- a/include/erofs/internal.h
> > +++ b/include/erofs/internal.h
> > @@ -109,6 +109,7 @@ static inline void erofs_sb_clear_##name(void) \
> > EROFS_FEATURE_FUNCS(lz4_0padding, incompat, INCOMPAT_LZ4_0PADDING)
> > EROFS_FEATURE_FUNCS(compr_cfgs, incompat, INCOMPAT_COMPR_CFGS)
> > EROFS_FEATURE_FUNCS(big_pcluster, incompat, INCOMPAT_BIG_PCLUSTER)
> > +EROFS_FEATURE_FUNCS(tailpacking, incompat, INCOMPAT_TAILPACKING)
> > EROFS_FEATURE_FUNCS(sb_chksum, compat, COMPAT_SB_CHKSUM)
> >
> > #define EROFS_I_EA_INITED (1 << 0)
> > @@ -148,6 +149,7 @@ struct erofs_inode {
> > unsigned char inode_isize;
> > /* inline tail-end packing size */
> > unsigned short idata_size;
> > + bool idata_raw;
> >
> > unsigned int xattr_isize;
> > unsigned int extent_isize;
> > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > index 18fc182..7700b27 100644
> > --- a/include/erofs_fs.h
> > +++ b/include/erofs_fs.h
> > @@ -22,10 +22,12 @@
> > #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING 0x00000001
> > #define EROFS_FEATURE_INCOMPAT_COMPR_CFGS 0x00000002
> > #define EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER 0x00000002
> > +#define EROFS_FEATURE_INCOMPAT_TAILPACKING 0x00000004
> > #define EROFS_ALL_FEATURE_INCOMPAT \
> > (EROFS_FEATURE_INCOMPAT_LZ4_0PADDING | \
> > EROFS_FEATURE_INCOMPAT_COMPR_CFGS | \
> > - EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER)
> > + EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER | \
> > + EROFS_FEATURE_INCOMPAT_TAILPACKING)
> >
> > #define EROFS_SB_EXTSLOT_SIZE 16
> >
> > @@ -230,10 +232,12 @@ struct z_erofs_lz4_cfgs {
> > * (4B) + 2B + (4B) if compacted 2B is on.
> > * bit 1 : HEAD1 big pcluster (0 - off; 1 - on)
> > * bit 2 : HEAD2 big pcluster (0 - off; 1 - on)
> > + * bit 3 : inline (un)compressed data
> > */
> > #define Z_EROFS_ADVISE_COMPACTED_2B 0x0001
> > #define Z_EROFS_ADVISE_BIG_PCLUSTER_1 0x0002
> > #define Z_EROFS_ADVISE_BIG_PCLUSTER_2 0x0004
> > +#define Z_EROFS_ADVISE_TAILPACKING 0x0008
> >
> > struct z_erofs_map_header {
> > __le32 h_reserved1;
> > diff --git a/lib/compress.c b/lib/compress.c
> > index 2093bfd..c19f554 100644
> > --- a/lib/compress.c
> > +++ b/lib/compress.c
> > @@ -159,6 +159,18 @@ static unsigned int z_erofs_get_max_pclusterblks(struct erofs_inode *inode)
> > return cfg.c_physical_clusterblks;
> > }
> >
> > +static int z_erofs_fill_inline_data(struct erofs_inode *inode, char *data,
> > + unsigned int len, bool raw)
> > +{
> > + inode->idata_raw = raw;
> > + inode->idata_size = len;
> > + inode->idata = malloc(inode->idata_size);
> > + if (!inode->idata)
> > + return -ENOMEM;
> > + memcpy(inode->idata, data, inode->idata_size);
> > + return 0;
> > +}
> > +
> > static int vle_compress_one(struct erofs_inode *inode,
> > struct z_erofs_vle_compress_ctx *ctx,
> > bool final)
> > @@ -169,15 +181,19 @@ static int vle_compress_one(struct erofs_inode *inode,
> > int ret;
> > static char dstbuf[EROFS_CONFIG_COMPR_MAX_SZ + EROFS_BLKSIZ];
> > char *const dst = dstbuf + EROFS_BLKSIZ;
> > + bool tail_pcluster = false;
> >
> > while (len) {
> > - const unsigned int pclustersize =
> > + unsigned int pclustersize =
> > z_erofs_get_max_pclusterblks(inode) * EROFS_BLKSIZ;
> > bool raw;
> >
> > - if (len <= pclustersize) {
> > + if (!tail_pcluster && len <= pclustersize) {
> > if (final) {
> > - if (len <= EROFS_BLKSIZ)
> > + if (erofs_sb_has_tailpacking()) {
> > + tail_pcluster = true;
> > + pclustersize = EROFS_BLKSIZ;
>
> Not quite sure if such condition can be trigged for many times...
>
> Think about it. If the original pclustersize == 16 * EROFS_BLKSIZ, so we
> could have at least 16 new pclustersize == EROFS_BLKSIZ then?
>
> But only the last pclustersize == EROFS_BLKSIZ can be inlined...
Let me think about it more.
>
> > + } else if (len <= EROFS_BLKSIZ)
> > goto nocompression;
> > } else {
> > break;
> > @@ -194,6 +210,17 @@ static int vle_compress_one(struct erofs_inode *inode,
> > inode->i_srcpath,
> > erofs_strerror(ret));
> > }
> > + if (tail_pcluster && len < EROFS_BLKSIZ) {
> > + ret = z_erofs_fill_inline_data(inode,
> > + (char *)(ctx->queue + ctx->head), len,
> > + true);
> > + if (ret)
> > + return ret;
> > + count = len;
> > + raw = true;
> > + ctx->compressedblks = 1;
> > + goto add_head;
> > + }
> > nocompression:
> > ret = write_uncompressed_extent(ctx, &len, dst);
> > if (ret < 0)
> > @@ -202,6 +229,16 @@ nocompression:
> > ctx->compressedblks = 1;
> > raw = true;
> > } else {
> > + if (tail_pcluster && ret < EROFS_BLKSIZ &&
> > + !(len - count)) {
> > + ret = z_erofs_fill_inline_data(inode, dst, ret,
> > + false);
> > + if (ret)
> > + return ret;
> > + raw = false;
> > + ctx->compressedblks = 1;
> > + goto add_head;
> > + }
> > const unsigned int tailused = ret & (EROFS_BLKSIZ - 1);
> > const unsigned int padding =
> > erofs_sb_has_lz4_0padding() && tailused ?
> > @@ -226,11 +263,13 @@ nocompression:
> > raw = false;
> > }
> >
> > +add_head:
> > ctx->head += count;
> > /* write compression indexes for this pcluster */
> > vle_write_indexes(ctx, count, raw);
> >
> > - ctx->blkaddr += ctx->compressedblks;
> > + if (!inode->idata_size)
> > + ctx->blkaddr += ctx->compressedblks;
> > len -= count;
> >
> > if (!final && ctx->head >= EROFS_CONFIG_COMPR_MAX_SZ) {
> > @@ -475,7 +514,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode)
> > fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);
> > if (fd < 0) {
> > ret = -errno;
> > - goto err_free;
> > + goto err_free_meta;
> > }
> >
> > /* allocate main data buffer */
> > @@ -530,16 +569,19 @@ int erofs_write_compressed_file(struct erofs_inode *inode)
> > goto err_bdrop;
> > }
> >
> > + inode->idata_size = 0;
> > +
> > /* do the final round */
> > ret = vle_compress_one(inode, &ctx, true);
> > if (ret)
> > - goto err_bdrop;
> > + goto err_free_id;
> >
> > /* fall back to no compression mode */
> > compressed_blocks = ctx.blkaddr - blkaddr;
> > - if (compressed_blocks >= BLK_ROUND_UP(inode->i_size)) {
> > + if (compressed_blocks >= BLK_ROUND_UP(inode->i_size) -
> > + (inode->idata_size ? 1 : 0) ) {
> > ret = -ENOSPC;
> > - goto err_bdrop;
> > + goto err_free_id;
> > }
> >
> > vle_write_indexes_final(&ctx);
> > @@ -553,12 +595,11 @@ int erofs_write_compressed_file(struct erofs_inode *inode)
> > inode->i_srcpath, (unsigned long long)inode->i_size,
> > compressed_blocks);
> >
> > - /*
> > - * TODO: need to move erofs_bdrop to erofs_write_tail_end
> > - * when both mkfs & kernel support compression inline.
> > - */
> > - erofs_bdrop(bh, false);
> > - inode->idata_size = 0;
> > + if (inode->idata_size)
> > + inode->bh_data = bh;
> > + else
> > + erofs_bdrop(bh, false);
> > +
> > inode->u.i_blocks = compressed_blocks;
> >
> > legacymetasize = ctx.metacur - compressmeta;
> > @@ -573,11 +614,14 @@ int erofs_write_compressed_file(struct erofs_inode *inode)
> > inode->compressmeta = compressmeta;
> > return 0;
> >
> > +err_free_id:
> > + if (inode->idata)
> > + free(inode->idata);
> > err_bdrop:
> > erofs_bdrop(bh, true); /* revoke buffer */
> > err_close:
> > close(fd);
> > -err_free:
> > +err_free_meta:
> > free(compressmeta);
> > return ret;
> > }
> > diff --git a/lib/compressor.c b/lib/compressor.c
> > index 8836e0c..26189b6 100644
> > --- a/lib/compressor.c
> > +++ b/lib/compressor.c
> > @@ -28,7 +28,7 @@ int erofs_compress_destsize(struct erofs_compress *c,
> > void *dst,
> > unsigned int dstsize)
> > {
> > - unsigned uncompressed_size;
> > + unsigned compressed_size;
> > int ret;
> >
> > DBG_BUGON(!c->alg);
> > @@ -41,9 +41,10 @@ int erofs_compress_destsize(struct erofs_compress *c,
> > return ret;
> >
> > /* check if there is enough gains to compress */
> > - uncompressed_size = *srcsize;
> > - if (roundup(ret, EROFS_BLKSIZ) >= uncompressed_size *
> > - c->compress_threshold / 100)
> > + compressed_size = *srcsize <= EROFS_BLKSIZ ? ret :
> > + roundup(ret, EROFS_BLKSIZ);
> > +
> > + if (*srcsize <= compressed_size * c->compress_threshold / 100)
> > return -EAGAIN;
> > return ret;
> > }
> > diff --git a/lib/decompress.c b/lib/decompress.c
> > index 490c4bc..0b6678d 100644
> > --- a/lib/decompress.c
> > +++ b/lib/decompress.c
> > @@ -9,6 +9,7 @@
> >
> > #include "erofs/decompress.h"
> > #include "erofs/err.h"
> > +#include "erofs/print.h"
> >
> > #ifdef LZ4_ENABLED
> > #include <lz4.h>
> > @@ -50,6 +51,9 @@ static int z_erofs_decompress_lz4(struct z_erofs_decompress_req *rq)
> > rq->decodedlength);
> >
> > if (ret != (int)rq->decodedlength) {
> > + erofs_err("failed to %s decompress %d in[%u, %u] out[%u]",
> > + rq->partial_decoding ? "partial" : "full",
> > + ret, rq->inputsize, inputmargin, rq->decodedlength);
> > ret = -EIO;
> > goto out;
> > }
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 787e5b4..1c08608 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -521,9 +521,6 @@ int erofs_prepare_tail_block(struct erofs_inode *inode)
> > struct erofs_buffer_head *bh;
> > int ret;
> >
> > - if (!inode->idata_size)
> > - return 0;
> > -
> > bh = inode->bh_data;
> > if (!bh) {
> > bh = erofs_balloc(DATA, EROFS_BLKSIZ, 0, 0);
> > @@ -557,26 +554,21 @@ int erofs_prepare_inode_buffer(struct erofs_inode *inode)
> > inodesize = Z_EROFS_VLE_EXTENT_ALIGN(inodesize) +
> > inode->extent_isize;
> >
> > - if (is_inode_layout_compression(inode))
> > - goto noinline;
> > -
> > - /*
> > - * if the file size is block-aligned for uncompressed files,
> > - * should use EROFS_INODE_FLAT_PLAIN data mapping mode.
> > - */
> > if (!inode->idata_size)
> > - inode->datalayout = EROFS_INODE_FLAT_PLAIN;
> > + goto noinline;
> >
> > bh = erofs_balloc(INODE, inodesize, 0, inode->idata_size);
> > if (bh == ERR_PTR(-ENOSPC)) {
> > int ret;
> >
> > - inode->datalayout = EROFS_INODE_FLAT_PLAIN;
> > -noinline:
> > /* expend an extra block for tail-end data */
> > ret = erofs_prepare_tail_block(inode);
> > if (ret)
> > return ret;
> > +noinline:
> > + if (!is_inode_layout_compression(inode))
> > + inode->datalayout = EROFS_INODE_FLAT_PLAIN;
> > +
> > bh = erofs_balloc(INODE, inodesize, 0, 0);
> > if (IS_ERR(bh))
> > return PTR_ERR(bh);
> > @@ -584,7 +576,16 @@ noinline:
> > } else if (IS_ERR(bh)) {
> > return PTR_ERR(bh);
> > } else if (inode->idata_size) {
> > - inode->datalayout = EROFS_INODE_FLAT_INLINE;
> > + if (is_inode_layout_compression(inode)) {
> > + struct z_erofs_map_header *h = inode->compressmeta;
> > + h->h_advise |= Z_EROFS_ADVISE_TAILPACKING;
> > + erofs_dbg("%s: inline %scompressed data (%u bytes)",
> > + inode->i_srcpath,
> > + inode->idata_raw ? "un" : "",
> > + inode->idata_size);
> > + } else {
> > + inode->datalayout = EROFS_INODE_FLAT_INLINE;
> > + }
> >
> > /* allocate inline buffer */
> > ibh = erofs_battach(bh, META, inode->idata_size);
> > @@ -640,20 +641,25 @@ int erofs_write_tail_end(struct erofs_inode *inode)
> > ibh->op = &erofs_write_inline_bhops;
> > } else {
> > int ret;
> > - erofs_off_t pos;
> > + erofs_off_t pos, pos0;
> > + const unsigned short padding = EROFS_BLKSIZ - inode->idata_size;
> >
> > erofs_mapbh(bh->block);
> > pos = erofs_btell(bh, true) - EROFS_BLKSIZ;
> > + pos0 = pos + inode->idata_size;
> > +
> > + if (is_inode_layout_compression(inode) &&
> > + erofs_sb_has_lz4_0padding() && !inode->idata_raw) {
> > + pos0 = pos;
> > + pos += padding;
> > + }
>
> I'd suggest don't use 0padding but record compressed_sized for
> tail-packing pcluster in the map header.... (need 2 bytes to record)
It's right if we can handle it when full decompression.
Thanks.
>
> Thanks,
> Gao Xiang
>
> > ret = dev_write(inode->idata, pos, inode->idata_size);
> > if (ret)
> > return ret;
> > - if (inode->idata_size < EROFS_BLKSIZ) {
> > - ret = dev_fillzero(pos + inode->idata_size,
> > - EROFS_BLKSIZ - inode->idata_size,
> > - false);
> > - if (ret)
> > - return ret;
> > - }
> > + ret = dev_fillzero(pos0, padding, false);
> > + if (ret)
> > + return ret;
> > +
> > inode->idata_size = 0;
> > free(inode->idata);
> > inode->idata = NULL;
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index e476189..7dd29df 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -48,6 +48,7 @@ static struct option long_options[] = {
> > {"product-out", required_argument, NULL, 11},
> > {"fs-config-file", required_argument, NULL, 12},
> > #endif
> > + {"inline", no_argument, NULL, 13},
> > {0, 0, 0, 0},
> > };
> >
> > @@ -87,6 +88,7 @@ static void usage(void)
> > " --all-root make all files owned by root\n"
> > " --help display this help and exit\n"
> > " --max-extent-bytes=# set maximum decompressed extent size # in bytes\n"
> > + " --inline tail-packing inline compressed data\n"
> > #ifndef NDEBUG
> > " --random-pclusterblks randomize pclusterblks for big pcluster (debugging only)\n"
> > #endif
> > @@ -304,6 +306,10 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> > }
> > cfg.c_physical_clusterblks = i / EROFS_BLKSIZ;
> > break;
> > + case 13:
> > + erofs_sb_set_tailpacking();
> > + erofs_warn("EXPERIMENTAL compression inline feature in use. Use at your own risk!");
> > + break;
> >
> > case 1:
> > usage();
> > --
> > 2.29.0
> >
> >
More information about the Linux-erofs
mailing list