[f2fs-dev] [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[f2fs-dev] [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[Fengnan Chang]

For now,when overwrite compressed file, we need read old data to page
cache first and update pages.
But when we need overwrite whole cluster, we don't need old data
anymore.
So, I think we can remove read data  process in this case, I have made
some simple changes to test, tests have shown that this can lead to
significant performance improvements, the speed of sequential write
up to 2x.

I haven't figured out the whole process yet,so the following
modification is a simple and crude, any suggestions are welcome.
I want to know if this idea is workable,if so, I will continue to
improve this patch.
Thanks.

Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/f2fs/compress.c | 12 ++++++------
 fs/f2fs/data.c     |  4 ++--
 fs/f2fs/f2fs.h     |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 925a5ca3744a..7056c009ee9a 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -990,7 +990,7 @@ static void set_cluster_dirty(struct compress_ctx *cc)
 }
 
 static int prepare_compress_overwrite(struct compress_ctx *cc,
-		struct page **pagep, pgoff_t index, void **fsdata)
+		struct page **pagep, pgoff_t index, void **fsdata, int len)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
 	struct address_space *mapping = cc->inode->i_mapping;
@@ -1007,6 +1007,8 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
 	if (ret <= 0)
 		return ret;
 
+	if (len == PAGE_SIZE)
+		return 0;
 	/* compressed case */
 	prealloc = (ret < cc->cluster_size);
 
@@ -1045,7 +1047,6 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
 		if (ret)
 			goto out;
 	}
-
 	for (i = 0; i < cc->cluster_size; i++) {
 		f2fs_bug_on(sbi, cc->rpages[i]);
 
@@ -1101,7 +1102,7 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
 }
 
 int f2fs_prepare_compress_overwrite(struct inode *inode,
-		struct page **pagep, pgoff_t index, void **fsdata)
+		struct page **pagep, pgoff_t index, void **fsdata, int len)
 {
 	struct compress_ctx cc = {
 		.inode = inode,
@@ -1111,8 +1112,7 @@ int f2fs_prepare_compress_overwrite(struct inode *inode,
 		.rpages = NULL,
 		.nr_rpages = 0,
 	};
-
-	return prepare_compress_overwrite(&cc, pagep, index, fsdata);
+	return prepare_compress_overwrite(&cc, pagep, index, fsdata, len);
 }
 
 bool f2fs_compress_write_end(struct inode *inode, void *fsdata,
@@ -1155,7 +1155,7 @@ int f2fs_truncate_partial_cluster(struct inode *inode, u64 from, bool lock)
 
 	/* truncate compressed cluster */
 	err = f2fs_prepare_compress_overwrite(inode, &pagep,
-						start_idx, &fsdata);
+						start_idx, &fsdata, 0);
 
 	/* should not be a normal cluster */
 	f2fs_bug_on(F2FS_I_SB(inode), err == 0);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 009a09fb9d88..9a6b82d8f278 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2916,7 +2916,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 
 					ret2 = f2fs_prepare_compress_overwrite(
 							inode, &pagep,
-							page->index, &fsdata);
+							page->index, &fsdata, 0);
 					if (ret2 < 0) {
 						ret = ret2;
 						done = 1;
@@ -3307,7 +3307,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 		*fsdata = NULL;
 
 		ret = f2fs_prepare_compress_overwrite(inode, pagep,
-							index, fsdata);
+							index, fsdata, len);
 		if (ret < 0) {
 			err = ret;
 			goto fail;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c83d90125ebd..8d4fedf0291b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3932,7 +3932,7 @@ static inline bool f2fs_post_read_required(struct inode *inode)
 bool f2fs_is_compressed_page(struct page *page);
 struct page *f2fs_compress_control_page(struct page *page);
 int f2fs_prepare_compress_overwrite(struct inode *inode,
-			struct page **pagep, pgoff_t index, void **fsdata);
+			struct page **pagep, pgoff_t index, void **fsdata, int len);
 bool f2fs_compress_write_end(struct inode *inode, void *fsdata,
 					pgoff_t index, unsigned copied);
 int f2fs_truncate_partial_cluster(struct inode *inode, u64 from, bool lock);
-- 
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] 答复: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[changfengnan]

Hi:
 Any comments for this?
	
Thanks.

-----邮件原件-----
发件人: Fengnan Chang <changfengnan@vivo.com> 
发送时间: 2021年5月18日 20:51
收件人: jaegeuk@kernel.org; chao@kernel.org;
linux-f2fs-devel@lists.sourceforge.net
抄送: Fengnan Chang <changfengnan@vivo.com>
主题: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole
cluster

For now,when overwrite compressed file, we need read old data to page cache
first and update pages.
But when we need overwrite whole cluster, we don't need old data anymore.
So, I think we can remove read data  process in this case, I have made some
simple changes to test, tests have shown that this can lead to significant
performance improvements, the speed of sequential write up to 2x.

I haven't figured out the whole process yet,so the following modification is
a simple and crude, any suggestions are welcome.
I want to know if this idea is workable,if so, I will continue to improve
this patch.
Thanks.

Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/f2fs/compress.c | 12 ++++++------
 fs/f2fs/data.c     |  4 ++--
 fs/f2fs/f2fs.h     |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index
925a5ca3744a..7056c009ee9a 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -990,7 +990,7 @@ static void set_cluster_dirty(struct compress_ctx *cc)
}
 
 static int prepare_compress_overwrite(struct compress_ctx *cc,
-		struct page **pagep, pgoff_t index, void **fsdata)
+		struct page **pagep, pgoff_t index, void **fsdata, int len)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
 	struct address_space *mapping = cc->inode->i_mapping; @@ -1007,6
+1007,8 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
 	if (ret <= 0)
 		return ret;
 
+	if (len == PAGE_SIZE)
+		return 0;
 	/* compressed case */
 	prealloc = (ret < cc->cluster_size);
 
@@ -1045,7 +1047,6 @@ static int prepare_compress_overwrite(struct
compress_ctx *cc,
 		if (ret)
 			goto out;
 	}
-
 	for (i = 0; i < cc->cluster_size; i++) {
 		f2fs_bug_on(sbi, cc->rpages[i]);
 
@@ -1101,7 +1102,7 @@ static int prepare_compress_overwrite(struct
compress_ctx *cc,  }
 
 int f2fs_prepare_compress_overwrite(struct inode *inode,
-		struct page **pagep, pgoff_t index, void **fsdata)
+		struct page **pagep, pgoff_t index, void **fsdata, int len)
 {
 	struct compress_ctx cc = {
 		.inode = inode,
@@ -1111,8 +1112,7 @@ int f2fs_prepare_compress_overwrite(struct inode
*inode,
 		.rpages = NULL,
 		.nr_rpages = 0,
 	};
-
-	return prepare_compress_overwrite(&cc, pagep, index, fsdata);
+	return prepare_compress_overwrite(&cc, pagep, index, fsdata, len);
 }
 
 bool f2fs_compress_write_end(struct inode *inode, void *fsdata, @@ -1155,7
+1155,7 @@ int f2fs_truncate_partial_cluster(struct inode *inode, u64 from,
bool lock)
 
 	/* truncate compressed cluster */
 	err = f2fs_prepare_compress_overwrite(inode, &pagep,
-						start_idx, &fsdata);
+						start_idx, &fsdata, 0);
 
 	/* should not be a normal cluster */
 	f2fs_bug_on(F2FS_I_SB(inode), err == 0); diff --git a/fs/f2fs/data.c
b/fs/f2fs/data.c index 009a09fb9d88..9a6b82d8f278 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2916,7 +2916,7 @@ static int f2fs_write_cache_pages(struct address_space
*mapping,
 
 					ret2 =
f2fs_prepare_compress_overwrite(
 							inode, &pagep,
-							page->index,
&fsdata);
+							page->index,
&fsdata, 0);
 					if (ret2 < 0) {
 						ret = ret2;
 						done = 1;
@@ -3307,7 +3307,7 @@ static int f2fs_write_begin(struct file *file, struct
address_space *mapping,
 		*fsdata = NULL;
 
 		ret = f2fs_prepare_compress_overwrite(inode, pagep,
-							index, fsdata);
+							index, fsdata, len);
 		if (ret < 0) {
 			err = ret;
 			goto fail;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index
c83d90125ebd..8d4fedf0291b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3932,7 +3932,7 @@ static inline bool f2fs_post_read_required(struct
inode *inode)  bool f2fs_is_compressed_page(struct page *page);  struct page
*f2fs_compress_control_page(struct page *page);  int
f2fs_prepare_compress_overwrite(struct inode *inode,
-			struct page **pagep, pgoff_t index, void **fsdata);
+			struct page **pagep, pgoff_t index, void **fsdata,
int len);
 bool f2fs_compress_write_end(struct inode *inode, void *fsdata,
 					pgoff_t index, unsigned copied);
 int f2fs_truncate_partial_cluster(struct inode *inode, u64 from, bool
lock);
--
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] [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[Chao Yu]

On 2021/5/18 20:51, Fengnan Chang wrote:
> For now,when overwrite compressed file, we need read old data to page
> cache first and update pages.
> But when we need overwrite whole cluster, we don't need old data
> anymore.

I only see you just check the whole page was dirty as below rather than
the whole cluster is dirty during write().

Thanks,

> +	if (len == PAGE_SIZE)
> +		return 0;
>   	/* compressed case */
>   	prealloc = (ret < cc->cluster_size);
>   


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

[f2fs-dev] 答复: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[changfengnan]

yes, I just check wheather the whole page was dirty, because of when write
cache f2fs_prepare_compress_overwrite will be called again, 
when update whole cluster,  cc in prepare_compress_overwrite will be empty,
so will not submit bio.
when only update one page in cluster,  cc in prepare_compress_overwrite will
not be empty, so will submit bio.
This is my thinking, not sure if I've missed anything


-----邮件原件-----
发件人: Chao Yu <yuchao0@huawei.com> 
发送时间: 2021年5月24日 19:39
收件人: Fengnan Chang <changfengnan@vivo.com>; jaegeuk@kernel.org;
chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
主题: Re: [f2fs-dev] [RFC PATCH] f2fs: compress: remove unneeded read when
rewrite whole cluster

On 2021/5/18 20:51, Fengnan Chang wrote:
> For now,when overwrite compressed file, we need read old data to page 
> cache first and update pages.
> But when we need overwrite whole cluster, we don't need old data 
> anymore.

I only see you just check the whole page was dirty as below rather than the
whole cluster is dirty during write().

Thanks,

> +	if (len == PAGE_SIZE)
> +		return 0;
>   	/* compressed case */
>   	prealloc = (ret < cc->cluster_size);
>   




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

Re: [f2fs-dev] 答复: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[Chao Yu]

On 2021/5/25 20:05, changfengnan@vivo.com wrote:
> yes, I just check wheather the whole page was dirty, because of when write
> cache f2fs_prepare_compress_overwrite will be called again,
> when update whole cluster,  cc in prepare_compress_overwrite will be empty,
> so will not submit bio.
> when only update one page in cluster,  cc in prepare_compress_overwrite will
> not be empty, so will submit bio.
> This is my thinking, not sure if I've missed anything

Well, it looks more like we did for mmap() write case.

So I guess we can change as below:

To Jaegeuk, comments?

---
  fs/f2fs/data.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 2ea887a114c8..723c59df51b7 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3323,7 +3323,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
  	}

  #ifdef CONFIG_F2FS_FS_COMPRESSION
-	if (f2fs_compressed_file(inode)) {
+	if (f2fs_compressed_file(inode) && len != PAGE_SIZE) {
  		int ret;

  		*fsdata = NULL;
-- 
2.29.2

Thanks,

> 
> 
> -----邮件原件-----
> 发件人: Chao Yu <yuchao0@huawei.com>
> 发送时间: 2021年5月24日 19:39
> 收件人: Fengnan Chang <changfengnan@vivo.com>; jaegeuk@kernel.org;
> chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
> 主题: Re: [f2fs-dev] [RFC PATCH] f2fs: compress: remove unneeded read when
> rewrite whole cluster
> 
> On 2021/5/18 20:51, Fengnan Chang wrote:
>> For now,when overwrite compressed file, we need read old data to page
>> cache first and update pages.
>> But when we need overwrite whole cluster, we don't need old data
>> anymore.
> 
> I only see you just check the whole page was dirty as below rather than the
> whole cluster is dirty during write().
> 
> Thanks,
> 
>> +	if (len == PAGE_SIZE)
>> +		return 0;
>>    	/* compressed case */
>>    	prealloc = (ret < cc->cluster_size);
>>    
> 
> 
> .
> 


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

Re: [f2fs-dev] 答复: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[Chao Yu]

On 2021/5/28 10:49, Chao Yu wrote:
> On 2021/5/25 20:05, changfengnan@vivo.com wrote:
>> yes, I just check wheather the whole page was dirty, because of when write
>> cache f2fs_prepare_compress_overwrite will be called again,
>> when update whole cluster,  cc in prepare_compress_overwrite will be empty,
>> so will not submit bio.
>> when only update one page in cluster,  cc in prepare_compress_overwrite will
>> not be empty, so will submit bio.
>> This is my thinking, not sure if I've missed anything
> 
> Well, it looks more like we did for mmap() write case.
> 
> So I guess we can change as below:
> 
> To Jaegeuk, comments?
> 
> ---
>    fs/f2fs/data.c | 2 +-
>    1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 2ea887a114c8..723c59df51b7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3323,7 +3323,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>    	}
> 
>    #ifdef CONFIG_F2FS_FS_COMPRESSION
> -	if (f2fs_compressed_file(inode)) {
> +	if (f2fs_compressed_file(inode) && len != PAGE_SIZE) {

It will cause panic in f2fs_write_end() due to miss to assign fsdata with NULL,
updated as below:

---
  fs/f2fs/data.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 2ea887a114c8..4e214ac87925 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3328,6 +3328,9 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,

  		*fsdata = NULL;

+		if (len == PAGE_SIZE)
+			goto repeat;
+
  		ret = f2fs_prepare_compress_overwrite(inode, pagep,
  							index, fsdata);
  		if (ret < 0) {
-- 
2.29.2

Thanks,

>    		int ret;
> 
>    		*fsdata = NULL;
> 


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

[f2fs-dev] 答复: 答复: [RFC PATCH] f2fs: compress: remove unneeded read when rewrite whole cluster

[changfengnan--- via Linux-f2fs-devel]

Hi Jaegeuk:

	Any comments about this patch?
 
Thanks.

-----邮件原件-----
发件人: Chao Yu <yuchao0@huawei.com> 
发送时间: 2021年5月29日 9:13
收件人: changfengnan@vivo.com; jaegeuk@kernel.org
抄送: linux-f2fs-devel@lists.sourceforge.net
主题: Re: [f2fs-dev] 答复: [RFC PATCH] f2fs: compress: remove unneeded read
when rewrite whole cluster

On 2021/5/28 10:49, Chao Yu wrote:
> On 2021/5/25 20:05, changfengnan@vivo.com wrote:
>> yes, I just check wheather the whole page was dirty, because of when 
>> write cache f2fs_prepare_compress_overwrite will be called again, 
>> when update whole cluster,  cc in prepare_compress_overwrite will be 
>> empty, so will not submit bio.
>> when only update one page in cluster,  cc in 
>> prepare_compress_overwrite will not be empty, so will submit bio.
>> This is my thinking, not sure if I've missed anything
> 
> Well, it looks more like we did for mmap() write case.
> 
> So I guess we can change as below:
> 
> To Jaegeuk, comments?
> 
> ---
>    fs/f2fs/data.c | 2 +-
>    1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 
> 2ea887a114c8..723c59df51b7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3323,7 +3323,7 @@ static int f2fs_write_begin(struct file *file,
struct address_space *mapping,
>    	}
> 
>    #ifdef CONFIG_F2FS_FS_COMPRESSION
> -	if (f2fs_compressed_file(inode)) {
> +	if (f2fs_compressed_file(inode) && len != PAGE_SIZE) {

It will cause panic in f2fs_write_end() due to miss to assign fsdata with
NULL, updated as below:

---
  fs/f2fs/data.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
2ea887a114c8..4e214ac87925 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3328,6 +3328,9 @@ static int f2fs_write_begin(struct file *file, struct
address_space *mapping,

  		*fsdata = NULL;

+		if (len == PAGE_SIZE)
+			goto repeat;
+
  		ret = f2fs_prepare_compress_overwrite(inode, pagep,
  							index, fsdata);
  		if (ret < 0) {
--
2.29.2

Thanks,

>    		int ret;
> 
>    		*fsdata = NULL;
> 




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