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

[<changfengnan@vivo.com>]

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