Hi Xiang, On Tue, 26 Oct 2021 20:00:37 +0800 Gao Xiang <[email protected]> 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 <[email protected]> > > 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 > > > >
