Re: [PATCH v2] erofs-utils: support per-inode compress pcluster

[Huang Jianan via Linux-erofs <linux-erofs@lists.ozlabs.org>]

在 2021/8/25 9:27, Gao Xiang 写道:
> On Wed, Aug 25, 2021 at 09:17:38AM +0800, Gao Xiang wrote:
>> On Wed, Aug 18, 2021 at 12:27:15PM +0800, Huang Jianan via Linux-erofs wrote:
>>> Add an option to configure per-inode compression strategy. Each line
>>> of the file should be in the following form:
>>>
>>> <Regular-expression> <pcluster-in-bytes>
>>>
>>> When pcluster is 0, it means that the file shouldn't be compressed.
>>>
>>> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
>> Sorry for the delay. Due to busy work, I will look into the details
>> this weekend. Some comments in advance.
>>
>>> ---
>>> changes since v1:
>>>   - rename c_pclusterblks to c_physical_clusterblks and place it in union
>>>   - change cfg.c_physical_clusterblks > 1 to erofs_sb_has_big_pcluster()
>>>     since it's per-inode compression strategy
>>>
>>>   include/erofs/compress_rule.h |  25 ++++++++
>> How about calling "compress_hints"? Does it sound better?
Sounds good, naming things is quite hard. 🙁
>>
>>>   include/erofs/config.h        |   1 +
>> ...
>>
>>> index 0000000..497d662
>>> --- /dev/null
>>> +++ b/lib/compress_rule.c
>>> @@ -0,0 +1,106 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * erofs-utils/lib/compress_rule.c
>>> + *
>>> + * Copyright (C), 2008-2021, OPPO Mobile Comm Corp., Ltd.
>>> + * Created by Huang Jianan <huangjianan@oppo.com>
>>> + */
>>> +#include <string.h>
>>> +#include <stdlib.h>
>>> +#include "erofs/err.h"
>>> +#include "erofs/list.h"
>>> +#include "erofs/print.h"
>>> +#include "erofs/compress_rule.h"
>>> +
>>> +static LIST_HEAD(compress_rule_head);
>>> +
>>> +static void dump_regerror(int errcode, const char *s, const regex_t *preg)
>>> +{
>>> +	char str[512];
>>> +
>>> +	regerror(errcode, preg, str, sizeof(str));
>>> +	erofs_err("invalid regex %s (%s)\n", s, str);
>>> +}
>>> +
>>> +static int erofs_insert_compress_rule(const char *s, unsigned int blks)
>>> +{
>>> +	struct erofs_compress_rule *r;
>>> +	int ret = 0;
>>> +
>>> +	r = malloc(sizeof(struct erofs_compress_rule));
>>> +	if (!r)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = regcomp(&r->reg, s, REG_EXTENDED|REG_NOSUB);
>>> +	if (ret) {
>>> +		dump_regerror(ret, s, &r->reg);
>>> +		goto err;
>>> +	}
>>> +	r->c_physical_clusterblks = blks;
>>> +
>>> +	list_add_tail(&r->list, &compress_rule_head);
>>> +	erofs_info("insert compress rule %s: %u", s, blks);
>>> +	return ret;
>>> +
>>> +err:
>>> +	free(r);
>>> +	return ret;
>>> +}
>>> +
>>> +unsigned int erofs_parse_pclusterblks(struct erofs_inode *inode)
>>> +{
>>> +	const char *s;
>>> +	struct erofs_compress_rule *r;
>>> +
>>> +	if (inode->c_physical_clusterblks)
>>> +		return inode->c_physical_clusterblks;
>>> +
>>> +	s = erofs_fspath(inode->i_srcpath);
>>> +
>>> +	list_for_each_entry(r, &compress_rule_head, list) {
>>> +		int ret = regexec(&r->reg, s, (size_t)0, NULL, 0);
>>> +
>>> +		if (!ret) {
>>> +			inode->c_physical_clusterblks = r->c_physical_clusterblks;
>>> +			return r->c_physical_clusterblks;
>>> +		}
>>> +		if (ret > REG_NOMATCH)
>>> +			dump_regerror(ret, s, &r->reg);
>>> +	}
>>> +
>>> +	inode->c_physical_clusterblks = cfg.c_physical_clusterblks;
>>> +	return cfg.c_physical_clusterblks;
>>> +}
>>> +
>>> +int erofs_load_compress_rule()
>>> +{
>>> +	char buf[PATH_MAX + 100];
>>> +	FILE* f;
>>> +	int ret = 0;
>>> +
>>> +	if (!cfg.compress_rule_file)
>>> +		return 0;
>>> +
>>> +	f = fopen(cfg.compress_rule_file, "r");
>>> +	if (f == NULL)
>>> +		return -errno;
>>> +
>>> +	while (fgets(buf, sizeof(buf), f)) {
>>> +		char* line = buf;
>>> +		char* s;
>>> +		unsigned int blks;
>>> +
>>> +		s = strtok(line, " ");
>>> +		blks = atoi(strtok(NULL, " "));
>>> +		if (blks % EROFS_BLKSIZ) {
>> We might need to guarantee these are power of 2.
> Oh, never mind. It's not necessary to leave pcluster power of 2.
> (I need some wake-up coffee...)
>
>> Also, how about just printing out warning message but using default "-C"
>> value instead?
ok
>>> +			erofs_err("invalid physical clustersize %u", blks);
>>> +			ret = -EINVAL;
>>> +			goto out;
>>> +		}
>>> +		erofs_insert_compress_rule(s, blks / EROFS_BLKSIZ);
>>> +	}
>>> +
>>> +out:
>>> +	fclose(f);
>>> +	return ret;
>>> +}
>> ...
>>
>>> diff --git a/mkfs/main.c b/mkfs/main.c
>>> index 10fe14d..467e875 100644
>>> --- a/mkfs/main.c
>>> +++ b/mkfs/main.c
>>> @@ -23,6 +23,7 @@
>>>   #include "erofs/xattr.h"
>>>   #include "erofs/exclude.h"
>>>   #include "erofs/block_list.h"
>>> +#include "erofs/compress_rule.h"
>>>   
>>>   #ifdef HAVE_LIBUUID
>>>   #include <uuid.h>
>>> @@ -44,11 +45,12 @@ static struct option long_options[] = {
>>>   	{"random-pclusterblks", no_argument, NULL, 8},
>>>   #endif
>>>   	{"max-extent-bytes", required_argument, NULL, 9},
>>> +	{"compress-rule", required_argument, NULL, 10},
>>>   #ifdef WITH_ANDROID
>>> -	{"mount-point", required_argument, NULL, 10},
>>> -	{"product-out", required_argument, NULL, 11},
>>> -	{"fs-config-file", required_argument, NULL, 12},
>>> -	{"block-list-file", required_argument, NULL, 13},
>>> +	{"mount-point", required_argument, NULL, 20},
>>> +	{"product-out", required_argument, NULL, 21},
>>> +	{"fs-config-file", required_argument, NULL, 22},
>>> +	{"block-list-file", required_argument, NULL, 23},
>> I think we might clean up these first with a separated patch.
>> Use >= 256 for all of them instead.
ok, I will send a separated patch first.

Thanks,
Jianan
>> Thanks,
>> Gao Xiang