This patch will not bring significant performance improvements, I test this on
mobile phone, use androbench, the sequential write test case was open file with
O_TRUNC, set write size to 4KB, performance improved about 2%-3%. If write
size set to 32MB, performance improved about 0.5% .
-----邮件原件-----
发件人: Chao Yu <yuch...@huawei.com>
发送时间: 2021年5月6日 18:38
收件人: Gao Xiang <hsiang...@aol.com>
抄送: Gao Xiang <hsiang...@redhat.com>; changfeng...@vivo.com;
jaeg...@kernel.org; linux-f2fs-devel@lists.sourceforge.net
主题: Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in
f2fs_prepare_compress_overwrite
Hi Xiang,
On 2021/5/6 17:58, Gao Xiang wrote:
> Hi Chao,
>
> On Thu, May 06, 2021 at 05:15:04PM +0800, Chao Yu wrote:
>> On 2021/4/26 17:00, Gao Xiang wrote:
>>> On Mon, Apr 26, 2021 at 04:42:20PM +0800, changfeng...@vivo.com wrote:
>>>> Thank you for the reminder, I hadn't thought about fallocate before.
>>>> I have done some tests and the results are as expected.
>>>> Here is my test method, create a compressed file, and use fallocate
>>>> with keep size, when write data to expand area,
>>>> f2fs_prepare_compress_overwrite always return 0, the behavior is same as
>>>> my patch , apply my patch can avoid those check.
>>>> Is there anything else I haven't thought of?
>>>
>>> Nope, I didn't look into the implementation. Just a wild guess.
>>>
>>> (I just wondered if the cluster size is somewhat large (e.g. 64k),
>>> but with a partial fallocate (e.g. 16k), and does it behave ok?
>>> or some other corner cases/conditions are needed.)
>>
>> Xiang, sorry for late reply.
>>
>> Now, f2fs triggers compression only if one cluster is fully written,
>> e.g. cluster size is 16kb, isize is 8kb, then the first cluster is
>> non-compressed one, so we don't need to prepare for compressed
>> cluster overwrite during write_begin(). Also, blocks fallocated
>> beyond isize should never be compressed, so we don't need to worry
>> about that.
>>
>
> Yeah, that could make it unnoticable. but my main concern is actually
> not what the current runtime compression logic is, but what the
> on-disk compresion format actually is, or there could cause
> compatibility issue if some later kernel makes full use of this and
> use old kernels
That's related, if there is layout v2 or we updated runtime compression policy
later, it needs to reconsider newly introduced logic of this patch, I guess we
need to add comments here to indicate why we can skip the preparation function.
> instead (also considering some corrupted compression indexes, which is
> not generated by the normal runtime compression logic.)
Yes, that's good concern, but that was not done by
f2fs_prepare_compress_overwrite(), another sanity check logic needs to be
designed and implemented in separated patch.
>
> My own suggestion about this is still verifying compress indexes first
> rather than relying much on runtime logic constraint. (Except that
> this patch can show signifiant benefit performance numbers to prove it
> can improve performance a lot.) Just my own premature thoughts.
Fengnan, could you please give some numbers to show how that check can impact
performance?
Thanks,
>
> Thanks,
> Gao Xiang
>
>> Thanks,
>>
>>>
>>> If that is fine, I have no problem about this, yet i_size here is
>>> generally somewhat risky since after post-EOF behavior changes (e.g.
>>> supporting FALLOC_FL_ZERO_RANGE with keep size later), it may cause
>>> some potential regression.
>>>
>>>>
>>>> -----邮件原件-----
>>>> 发件人: Gao Xiang <hsiang...@redhat.com>
>>>> 发送时间: 2021年4月26日 11:19
>>>> 收件人: Fengnan Chang <changfeng...@vivo.com>
>>>> 抄送: c...@kernel.org; jaeg...@kernel.org;
>>>> linux-f2fs-devel@lists.sourceforge.net
>>>> 主题: Re: [f2fs-dev] [PATCH] f2fs: compress: avoid unnecessary check
>>>> in f2fs_prepare_compress_overwrite
>>>>
>>>> On Mon, Apr 26, 2021 at 10:11:53AM +0800, Fengnan Chang wrote:
>>>>> when write compressed file with O_TRUNC, there will be a lot of
>>>>> unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
>>>>> especially when written in page size, remove it.
>>>>>
>>>>> Signed-off-by: Fengnan Chang <changfeng...@vivo.com>
>>>>
>>>> Even though I didn't look into the whole thing, my reaction here is
>>>> roughly how to handle fallocate with keep size? Does it work as expected?
>>>>
>>>>> ---
>>>>> fs/f2fs/data.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>>>> cf935474ffba..9c3b0849f35e 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -3270,6 +3270,7 @@ static int f2fs_write_begin(struct file
>>>>> *file, struct address_space *mapping,
>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>> struct page *page = NULL;
>>>>> pgoff_t index = ((unsigned long long) pos) >> PAGE_SHIFT;
>>>>> + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
>>>>> +PAGE_SHIFT;
>>>>> bool need_balance = false, drop_atomic = false;
>>>>> block_t blkaddr = NULL_ADDR;
>>>>> int err = 0;
>>>>> @@ -3306,6 +3307,9 @@ static int f2fs_write_begin(struct file
>>>>> *file, struct address_space *mapping,
>>>>>
>>>>> *fsdata = NULL;
>>>>>
>>>>> + if (index >= end)
>>>>> + goto repeat;
>>>>> +
>>>>> ret = f2fs_prepare_compress_overwrite(inode, pagep,
>>>>> index, fsdata);
>>>>> if (ret < 0) {
>>>>> --
>>>>> 2.29.0
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
