Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 23 Feb 2021 13:19:26 +0800
From:   Gao Xiang <hsiangkao@redhat.com>
To:     Huang Jianan <huangjianan@oppo.com>
Cc:     linux-erofs@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        guoweichao@oppo.com, zhangshiming@oppo.com
Subject: Re: [PATCH v2] erofs: support adjust lz4 history window size
Message-ID: <20210223051926.GB1225203@xiangao.remote.csb>
References: <20210223043634.36807-1-huangjianan@oppo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210223043634.36807-1-huangjianan@oppo.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Tue, Feb 23, 2021 at 12:36:34PM +0800, Huang Jianan via Linux-erofs wrote:
> From: huangjianan <huangjianan@oppo.com>
> 
> lz4 uses LZ4_DISTANCE_MAX to record history preservation. When
> using rolling decompression, a block with a higher compression
> ratio will cause a larger memory allocation (up to 64k). It may
> cause a large resource burden in extreme cases on devices with
> small memory and a large number of concurrent IOs. So appropriately
> reducing this value can improve performance.
> 
> Decreasing this value will reduce the compression ratio (except
> when input_size <LZ4_DISTANCE_MAX). But considering that erofs
> currently only supports 4k output, reducing this value will not
> significantly reduce the compression benefits.

It'd be better to add some description words here to explain why
old kernels could use such new adjusted images (because we only
decrease the sliding window size, and LZ4_MAX_DISTANCE in principle
is 64kb due to the length limitation of "offset" field for each
lz4 sequence.)

> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
> 
> changes since previous version
> - change compr_alg to lz4_max_distance
> - calculate lz4_max_distance_pages when reading super_block
> 
>  fs/erofs/decompressor.c | 12 ++++++++----
>  fs/erofs/erofs_fs.h     |  3 ++-
>  fs/erofs/internal.h     |  3 +++
>  fs/erofs/super.c        |  5 +++++
>  4 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 1cb1ffd10569..fb2b4f1b8806 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -36,22 +36,26 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>  	struct page *availables[LZ4_MAX_DISTANCE_PAGES] = { NULL };
>  	unsigned long bounced[DIV_ROUND_UP(LZ4_MAX_DISTANCE_PAGES,
>  					   BITS_PER_LONG)] = { 0 };
> +	unsigned int lz4_max_distance_pages = LZ4_MAX_DISTANCE_PAGES;

How about directly
unsigned int lz4_max_distance_pages = EROFS_SB(rq->sb)->lz4_max_distance_pages
here... (and see below.)

>  	void *kaddr = NULL;
>  	unsigned int i, j, top;
>  
> +	if (EROFS_SB(rq->sb)->lz4_max_distance_pages)
> +		lz4_max_distance_pages = EROFS_SB(rq->sb)->lz4_max_distance_pages;
> +
>  	top = 0;
>  	for (i = j = 0; i < nr; ++i, ++j) {
>  		struct page *const page = rq->out[i];
>  		struct page *victim;
>  
> -		if (j >= LZ4_MAX_DISTANCE_PAGES)
> +		if (j >= lz4_max_distance_pages)
>  			j = 0;
>  
>  		/* 'valid' bounced can only be tested after a complete round */
>  		if (test_bit(j, bounced)) {
> -			DBG_BUGON(i < LZ4_MAX_DISTANCE_PAGES);
> -			DBG_BUGON(top >= LZ4_MAX_DISTANCE_PAGES);
> -			availables[top++] = rq->out[i - LZ4_MAX_DISTANCE_PAGES];
> +			DBG_BUGON(i < lz4_max_distance_pages);
> +			DBG_BUGON(top >= lz4_max_distance_pages);
> +			availables[top++] = rq->out[i - lz4_max_distance_pages];
>  		}
>  
>  		if (page) {
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> index 9ad1615f4474..5eb37002b1a3 100644
> --- a/fs/erofs/erofs_fs.h
> +++ b/fs/erofs/erofs_fs.h
> @@ -39,7 +39,8 @@ struct erofs_super_block {
>  	__u8 uuid[16];          /* 128-bit uuid for volume */
>  	__u8 volume_name[16];   /* volume name */
>  	__le32 feature_incompat;
> -	__u8 reserved2[44];
> +	__le16 lz4_max_distance;	/* lz4 max distance */
> +	__u8 reserved2[42];
>  };
>  
>  /*
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 67a7ec945686..7457710a763a 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -70,6 +70,9 @@ struct erofs_sb_info {
>  
>  	/* pseudo inode to manage cached pages */
>  	struct inode *managed_cache;
> +
> +	/* lz4 max distance pages */
> +	u16 lz4_max_distance_pages;
>  #endif	/* CONFIG_EROFS_FS_ZIP */
>  	u32 blocks;
>  	u32 meta_blkaddr;
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index d5a6b9b888a5..3a3d235de7cc 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -174,6 +174,11 @@ static int erofs_read_superblock(struct super_block *sb)
>  	sbi->islotbits = ilog2(sizeof(struct erofs_inode_compact));
>  	sbi->root_nid = le16_to_cpu(dsb->root_nid);
>  	sbi->inos = le64_to_cpu(dsb->inos);
> +#ifdef CONFIG_EROFS_FS_ZIP
> +	if (dsb->lz4_max_distance)
> +		sbi->lz4_max_distance_pages =
> +			DIV_ROUND_UP(le16_to_cpu(dsb->lz4_max_distance), PAGE_SIZE) + 1;
> +#endif

How about adding a new helper e.g. z_erofs_load_lz4_config(sb, dsb)
in decompressor.c, and

int z_erofs_load_lz4_config(sb, dsb)
{
	if (dsb->lz4_max_distance)
		sbi->lz4_max_distance_pages = DIV_ROUND_UP ...
	else
		sbi->lz4_max_distance_pages = LZ4_MAX_DISTANCE_PAGES;
	return 0;
}

Also add a declaration in internal.h:

#ifdef CONFIG_EROFS_FS_ZIP
int z_erofs_load_lz4_config.....
#else
static inline int z_erofs_load_lz4_config() { return 0; }
#endif

Thanks,
Gao Xiang

>  
>  	sbi->build_time = le64_to_cpu(dsb->build_time);
>  	sbi->build_time_nsec = le32_to_cpu(dsb->build_time_nsec);
> -- 
> 2.25.1
>