Re: [f2fs-dev] [PATCH v2] f2fs: compress: fix overwrite may reduce compress ratio unproperly

[Chao Yu <chao@kernel.org>]

On 2021/10/22 11:53, 常凤楠 wrote:
> 
> 
>> -----Original Message-----
>> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf Of
>> Chao Yu
>> Sent: Thursday, October 21, 2021 10:34 PM
>> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
>> Cc: linux-f2fs-devel@lists.sourceforge.net
>> Subject: Re: [PATCH v2] f2fs: compress: fix overwrite may reduce compress
>> ratio unproperly
>>
>> On 2021/10/14 21:12, Fengnan Chang wrote:
>>> when overwrite only first block of cluster, since cluster is not full,
>>> it will call f2fs_write_raw_pages when f2fs_write_multi_pages, and
>>> cause the whole cluster become uncompressed eventhough data can be
>> compressed.
>>> this may will make random write bench score reduce a lot.
>>>
>>> root# dd if=/dev/zero of=./fio-test bs=1M count=1
>>>
>>> root# sync
>>>
>>> root# echo 3 > /proc/sys/vm/drop_caches
>>>
>>> root# f2fs_io get_cblocks ./fio-test
>>>
>>> root# dd if=/dev/zero of=./fio-test bs=4K count=1 oflag=direct
>>> conv=notrunc
>>>
>>> w/o patch:
>>> root# f2fs_io get_cblocks ./fio-test
>>> 189
>>>
>>> w/ patch:
>>> root# f2fs_io get_cblocks ./fio-test
>>> 192
>>>
>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>> ---
>>>    fs/f2fs/compress.c | 12 ++++++++++++
>>>    fs/f2fs/data.c     |  7 +++++++
>>>    fs/f2fs/f2fs.h     |  1 +
>>>    3 files changed, 20 insertions(+)
>>>
>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index
>>> c1bf9ad4c220..c4f36ead6f17 100644
>>> --- a/fs/f2fs/compress.c
>>> +++ b/fs/f2fs/compress.c
>>> @@ -857,6 +857,18 @@ void f2fs_end_read_compressed_page(struct page
>> *page, bool failed,
>>>    		f2fs_decompress_cluster(dic);
>>>    }
>>>
>>> +bool is_page_same_cluster(struct compress_ctx *cc, struct pagevec
>>> +*pvec, int index)
>>
>> Fengnan,
>>
>> It needs add f2fs prefix for non-static symbol to avoid global namespace
>> pollution.
>>
>> Anyway, how about this?
> 
> It looks good for me, you can make this as V3.

Well, could you please send v3 with below updates?

Thanks,

>>
>> ---
>>    fs/f2fs/compress.c | 19 +++++++++++++++++++
>>    fs/f2fs/data.c     |  7 ++++---
>>    fs/f2fs/f2fs.h     |  2 ++
>>    3 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index
>> c1bf9ad4c220..15d9b89d4df0 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -881,6 +881,25 @@ bool f2fs_cluster_can_merge_page(struct
>> compress_ctx *cc, pgoff_t index)
>>    	return is_page_in_cluster(cc, index);
>>    }
>>
>> +bool f2fs_all_cluster_page_loaded(struct compress_ctx *cc, struct pagevec
>> *pvec,
>> +						int index, int nr_pages)
>> +{
>> +	unsigned long pgidx;
>> +	int i;
>> +
>> +	if (nr_pages - index < cc->cluster_size)
>> +		return false;
>> +
>> +	pgidx = pvec->pages[index]->index;
>> +
>> +	for (i = 1; i < cc->cluster_size; i++) {
>> +		if (pvec->pages[index + i]->index != pgidx + i)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>    static bool cluster_has_invalid_data(struct compress_ctx *cc)
>>    {
>>    	loff_t i_size = i_size_read(cc->inode); diff --git a/fs/f2fs/data.c
>> b/fs/f2fs/data.c index 14fe5c6216cc..b0665f69c093 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -3075,9 +3075,10 @@ static int f2fs_write_cache_pages(struct
>> address_space *mapping,
>>    						done = 1;
>>    						break;
>>    					} else if (ret2 &&
>> -						!f2fs_compress_write_end(inode,
>> -								fsdata, page->index,
>> -								1)) {
>> +						(!f2fs_compress_write_end(inode,
>> +							fsdata, page->index, 1) ||
>> +						!f2fs_all_cluster_page_loaded(&cc,
>> +							&pvec, i, nr_pages))) {
>>    						retry = 1;
>>    						break;
>>    					}
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 7a32c2127945..b8da34198ce0
>> 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4047,6 +4047,8 @@ void f2fs_end_read_compressed_page(struct page
>> *page, bool failed,
>>    							block_t blkaddr);
>>    bool f2fs_cluster_is_empty(struct compress_ctx *cc);
>>    bool f2fs_cluster_can_merge_page(struct compress_ctx *cc, pgoff_t index);
>> +bool f2fs_all_cluster_page_loaded(struct compress_ctx *cc, struct pagevec
>> *pvec,
>> +						int index, int nr_pages);
>>    bool f2fs_sanity_check_cluster(struct dnode_of_data *dn);
>>    void f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page
>> *page);
>>    int f2fs_write_multi_pages(struct compress_ctx *cc,
>> --
>> 2.32.0
>>
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel