[f2fs-dev] [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[f2fs-dev] [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Fengnan Chang]

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 <changfengnan@vivo.com>
---
 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

Re: [f2fs-dev] [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Gao Xiang]

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 <changfengnan@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

[f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[changfengnan]

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?

-----邮件原件-----
发件人: Gao Xiang <hsiangkao@redhat.com>
发送时间: 2021年4月26日 11:19
收件人: Fengnan Chang <changfengnan@vivo.com>
抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Gao Xiang]

On Mon, Apr 26, 2021 at 04:42:20PM +0800, changfengnan@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.)

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 <hsiangkao@redhat.com>
> 发送时间: 2021年4月26日 11:19
> 收件人: Fengnan Chang <changfengnan@vivo.com>
> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

[f2fs-dev] 答复: 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[changfengnan]

Maybe I should have been clearer. This modification has nothing to do with cluster size, and test is ok.
For now, f2fs_prepare_compress_overwrite will calculate number of valid blocks in compressed cluster, then decide whether need to read out the old data, when write pos is bigger than inode size, it's obvious unnecessary to calculate, because it always return 0, so check pos first to avoid this condition.
So in my opinion, as long as the node size matches the valid data there will be no problem. 
I don't quite understand what you say post-EOF behavior changes, but I found compressed file not support FALLOC_FL_ZERO_RANGE flag in fallocate, is this ok ?

-----邮件原件-----
发件人: Gao Xiang <hsiangkao@redhat.com> 
发送时间: 2021年4月26日 17:00
收件人: changfengnan@vivo.com
抄送: chao@kernel.org; jaegeuk@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 04:42:20PM +0800, changfengnan@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.)

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 <hsiangkao@redhat.com>
> 发送时间: 2021年4月26日 11:19
> 收件人: Fengnan Chang <changfengnan@vivo.com>
> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Chao Yu]

On 2021/4/26 10:11, 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 <changfengnan@vivo.com>
> ---
>   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;

Should define this under compression macro, otherwise it may cause compile
warning.

>   	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

Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Chao Yu]

On 2021/4/26 17:00, Gao Xiang wrote:
> On Mon, Apr 26, 2021 at 04:42:20PM +0800, changfengnan@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.

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 <hsiangkao@redhat.com>
>> 发送时间: 2021年4月26日 11:19
>> 收件人: Fengnan Chang <changfengnan@vivo.com>
>> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Gao Xiang via Linux-f2fs-devel]

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, changfengnan@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
instead (also considering some corrupted compression indexes, which
is not generated by the normal runtime compression logic.)

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.

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 <hsiangkao@redhat.com>
> > > 发送时间: 2021年4月26日 11:19
> > > 收件人: Fengnan Chang <changfengnan@vivo.com>
> > > 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Chao Yu]

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, changfengnan@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 <hsiangkao@redhat.com>
>>>> 发送时间: 2021年4月26日 11:19
>>>> 收件人: Fengnan Chang <changfengnan@vivo.com>
>>>> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Gao Xiang via Linux-f2fs-devel]

On Thu, May 06, 2021 at 06:37:45PM +0800, Chao Yu wrote:
> 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, changfengnan@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.

Anyway, my thoughts is mainly to distinguish the current runtime
compression logic and the proposal on-disk format by design. If it's
easy to support reading partial written clusters and post-EOF cases
in practice with a few lines, so the later compression logic could
use compat feature (or at least ro_compat feature) to update, which
is much better than an incompat feature for older kernels.

But if it's somewhat hard to add simply, that makes no difference so
v2 may need to be introduced instead.

> 
> > 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?

IMO, it'd be better to show some real numbers to add more constraint
like this, if it can be measureable, that is another story indeed.

Thanks,
Gao Xiang

> 
> 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 <hsiangkao@redhat.com>
> > > > > 发送时间: 2021年4月26日 11:19
> > > > > 收件人: Fengnan Chang <changfengnan@vivo.com>
> > > > > 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

[f2fs-dev] 答复: 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[changfengnan]

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 <yuchao0@huawei.com> 
发送时间: 2021年5月6日 18:38
收件人: Gao Xiang <hsiangkao@aol.com>
抄送: Gao Xiang <hsiangkao@redhat.com>; changfengnan@vivo.com; jaegeuk@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, changfengnan@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 <hsiangkao@redhat.com>
>>>> 发送时间: 2021年4月26日 11:19
>>>> 收件人: Fengnan Chang <changfengnan@vivo.com>
>>>> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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

Re: [f2fs-dev] 答复: 答复: [PATCH] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite

[Gao Xiang via Linux-f2fs-devel]

On Thu, May 06, 2021 at 08:15:40PM +0800, changfengnan@vivo.com wrote:
> 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%.

Ok, so considering this, my suggestion is that it'd be better to add
your own configuration and raw test results to the commit message to
show the reason why we need this constraint here.

Also, adding some inline comments about this sounds better.

Thanks,
Gao Xiang

> 
> 
> -----邮件原件-----
> 发件人: Chao Yu <yuchao0@huawei.com> 
> 发送时间: 2021年5月6日 18:38
> 收件人: Gao Xiang <hsiangkao@aol.com>
> 抄送: Gao Xiang <hsiangkao@redhat.com>; changfengnan@vivo.com; jaegeuk@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, changfengnan@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 <hsiangkao@redhat.com>
> >>>> 发送时间: 2021年4月26日 11:19
> >>>> 收件人: Fengnan Chang <changfengnan@vivo.com>
> >>>> 抄送: chao@kernel.org; jaegeuk@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 <changfengnan@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