Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
From: Gao Xiang <hsiangkao@gmx.com>
To: Li Guifu <bluce.lee@aliyun.com>
Cc: linux-erofs@lists.ozlabs.org
Subject: Re: [PATCH v5] erofs-utils: introduce segment compression
Date: Sat, 20 Jun 2020 07:03:36 +0800
Message-ID: <20200619230335.GA22685@hsiangkao-HP-ZHAN-66-Pro-G1> (raw)
In-Reply-To: <20200619175133.6919-1-bluce.lee@aliyun.com>

On Sat, Jun 20, 2020 at 01:51:33AM +0800, Li Guifu via Linux-erofs wrote:
> Support segment compression which seperates files in several logic
> units (segments) and each segment is compressed independently.
>
> Advantages:
>  - more friendly for data differencing;
>  - it can also be used for parallel compression in the same file later.
>
> Signed-off-by: Li Guifu <bluce.lee@aliyun.com>
> ---
>  include/erofs/config.h |  1 +
>  lib/compress.c         | 16 ++++++++++++++--
>  lib/config.c           |  1 +
>  man/mkfs.erofs.1       |  4 ++++
>  mkfs/main.c            | 16 +++++++++++++++-
>  5 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/include/erofs/config.h b/include/erofs/config.h
> index 2f09749..995664d 100644
> --- a/include/erofs/config.h
> +++ b/include/erofs/config.h
> @@ -36,6 +36,7 @@ struct erofs_configure {
>  	char *c_src_path;
>  	char *c_compr_alg_master;
>  	int c_compr_level_master;
> +	u64 c_compr_seg_size;

Could you please move this variable up a bit? Thanks.

char *c_compr_alg_master;
u64 c_compr_seg_size;
int c_compr_level_master;

>  	int c_force_inodeversion;
>  	/* < 0, xattr disabled and INT_MAX, always use inline xattrs */
>  	int c_inline_xattr_tolerance;
> diff --git a/lib/compress.c b/lib/compress.c
> index 6cc68ed..383ee00 100644
> --- a/lib/compress.c
> +++ b/lib/compress.c
> @@ -32,6 +32,7 @@ struct z_erofs_vle_compress_ctx {
>
>  	erofs_blk_t blkaddr;	/* pointing to the next blkaddr */
>  	u16 clusterofs;
> +	u64 segavail;
>  };
>
>  #define Z_EROFS_LEGACY_MAP_HEADER_SIZE	\
> @@ -158,13 +159,19 @@ static int vle_compress_one(struct erofs_inode *inode,
>  	while (len) {
>  		bool raw;

unsigned int limit;

>
> +		count = min_t(u64, len, ctx->segavail);

kill this line.

> +		if (ctx->segavail <= EROFS_BLKSIZ) {
> +			if (len < ctx->segavail && !final)
> +				break;

limit = ctx->segavail;

> +			goto nocompression;
> +		}
> +
>  		if (len <= EROFS_BLKSIZ) {
>  			if (final)
>  				goto nocompression;
>  			break;
>  		}
>
> -		count = len;

count = min_t(u64, len, ctx->segavail);

>  		ret = erofs_compress_destsize(h, compressionlevel,
>  					      ctx->queue + ctx->head,
>  					      &count, dst, EROFS_BLKSIZ);
> @@ -174,8 +181,9 @@ static int vle_compress_one(struct erofs_inode *inode,
>  					  inode->i_srcpath,
>  					  erofs_strerror(ret));
>  			}
> +			count = len;

kill this line and add limit = EROFS_BLKSIZ;

>  nocompression:
> -			ret = write_uncompressed_block(ctx, &len, dst);
> +			ret = write_uncompressed_block(ctx, &count, dst);

ret = write_uncompressed_block(ctx, &count, limit, dst);

and update write_uncompressed_block as well.

>  			if (ret < 0)
>  				return ret;
>  			count = ret;
> @@ -202,6 +210,9 @@ nocompression:
>
>  		++ctx->blkaddr;
>  		len -= count;
> +		ctx->segavail -= count;
> +		if (!ctx->segavail)
> +			ctx->segavail = cfg.c_compr_seg_size;
>
>  		if (!final && ctx->head >= EROFS_CONFIG_COMPR_MAX_SZ) {
>  			const unsigned int qh_aligned =
> @@ -422,6 +433,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode)
>  	ctx.head = ctx.tail = 0;
>  	ctx.clusterofs = 0;
>  	remaining = inode->i_size;
> +	ctx.segavail = cfg.c_compr_seg_size;
>
>  	while (remaining) {
>  		const u64 readcount = min_t(u64, remaining,
> diff --git a/lib/config.c b/lib/config.c
> index da0c260..de982e1 100644
> --- a/lib/config.c
> +++ b/lib/config.c
> @@ -23,6 +23,7 @@ void erofs_init_configure(void)
>  	cfg.c_force_inodeversion = 0;
>  	cfg.c_inline_xattr_tolerance = 2;
>  	cfg.c_unix_timestamp = -1;
> +	cfg.c_compr_seg_size = UINT64_MAX;

cfg.c_compr_seg_size = -1;

since it is a very simple way to assign UINT_MAX by implicit sign
extension without taking care for the specific data type.

>  }
>
>  void erofs_show_config(void)
> diff --git a/man/mkfs.erofs.1 b/man/mkfs.erofs.1
> index 891c5a8..b12cb22 100644
> --- a/man/mkfs.erofs.1
> +++ b/man/mkfs.erofs.1
> @@ -52,6 +52,10 @@ Forcely generate extended inodes (64-byte inodes) to output.
>  Set all files to the given UNIX timestamp. Reproducible builds requires setting
>  all to a specific one.
>  .TP
> +.BI "\-S " #
> +Set the max input stream size at one compression. The default is unsigned 64bit MAX.
> +It must be algin to EROFS block size(4096).

it's hard for end users to type "max 64-bit unsigned value"...

I'd suggest "Set max input stream size for each individual segment (disabled if 0).
The default value is 0. It should be aligned with blocksize."

> +.TP
>  .BI "\-\-exclude-path=" path
>  Ignore file that matches the exact literal path.
>  You may give multiple `--exclude-path' options.
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 94bf1e6..96cc053 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -61,6 +61,7 @@ static void usage(void)
>  	      " -x#               set xattr tolerance to # (< 0, disable xattrs; default 2)\n"
>  	      " -EX[,...]         X=extended options\n"
>  	      " -T#               set a fixed UNIX timestamp # to all files\n"
> +	      " -S#               set the max input stream size # at one compression\n"

-S#               Set max input stream size # for each individual segment\n

>  	      " --exclude-path=X  avoid including file X (X = exact literal path)\n"
>  	      " --exclude-regex=X avoid including files that match X (X = regular expression)\n"
>  #ifdef HAVE_LIBSELINUX
> @@ -138,7 +139,7 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  	char *endptr;
>  	int opt, i;
>
> -	while((opt = getopt_long(argc, argv, "d:x:z:E:T:",
> +	while((opt = getopt_long(argc, argv, "d:x:z:E:T:S:",
>  				 long_options, NULL)) != -1) {
>  		switch (opt) {
>  		case 'z':
> @@ -188,6 +189,19 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  				return -EINVAL;
>  			}
>  			break;
> +		case 'S':
> +			cfg.c_compr_seg_size = strtoll(optarg, &endptr, 0);
> +			if (*endptr != '\0' || !cfg.c_compr_seg_size) {

Disable this if cfg.c_compr_seg_size == 0

> +				erofs_err("invalid compress segment size %s",
> +					  optarg);
> +				return -EINVAL;
> +			}
> +			if (cfg.c_compr_seg_size % EROFS_BLKSIZ) {
> +				erofs_err("segment size:%"PRIu64" should be align to %u",


Could you follow my advice in the previous reply?
Although I'm not good at English, but I don't think the above message is _reasonable_.

Thanks,
Gao Xiang

> +					  cfg.c_compr_seg_size, EROFS_BLKSIZ);
> +				return -EINVAL;
> +			}
> +			break;
>  		case 2:
>  			opt = erofs_parse_exclude_path(optarg, false);
>  			if (opt) {
> --
> 2.17.1
>

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18 16:26 [PATCH v4] " Li Guifu via Linux-erofs
2020-06-18 23:05 ` Gao Xiang
2020-06-19 17:51 ` [PATCH v5] " Li Guifu via Linux-erofs
2020-06-19 23:03   ` Gao Xiang [this message]
2020-06-21 10:51   ` [PATCH v6] erofs-utils: introduce segment limits compression Li Guifu via Linux-erofs
2020-06-21 12:27     ` [PATCH v8] erofs-utils: introduce segment compression Li Guifu via Linux-erofs
2020-06-21 14:37       ` Gao Xiang via Linux-erofs
2020-06-30 17:27         ` [PATCH v10] " Li Guifu via Linux-erofs
2020-07-05  8:32           ` [PATCH v11] " Li Guifu via Linux-erofs
2020-07-05 18:20             ` Gao Xiang via Linux-erofs

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200619230335.GA22685@hsiangkao-HP-ZHAN-66-Pro-G1 \
    --to=hsiangkao@gmx.com \
    --cc=bluce.lee@aliyun.com \
    --cc=linux-erofs@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EROFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-erofs/0 linux-erofs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-erofs linux-erofs/ https://lore.kernel.org/linux-erofs \
		linux-erofs@lists.ozlabs.org linux-erofs@ozlabs.org
	public-inbox-index linux-erofs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linux-erofs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git