[PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[Chao Yu]

SQLite App			GC Thread	Shrinker
- f2fs_ioc_start_atomic_write

- f2fs_ioc_commit_atomic_write
 - f2fs_commit_atomic_write
  - filemap_write_and_wait_range
  : write atomic_file's data to cow_inode
						echo 3 > drop_caches
				- f2fs_gc
				 - gc_data_segment
				  - move_data_page
				   - set_page_dirty
				   : it may load data of previous
				     transaction into pagecache.
  - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
  - __f2fs_commit_atomic_write
  - f2fs_up_write(&fi->i_gc_rwsem[WRITE])

During committing atomic_file, GC may be triggered to migrate
atomic_file's block, so it may contain data of previous transaction
in page cache, we should drop atomic_file's cache once it was
migrated by GC.

And also, we should writeback atomic_file and cow_file's data
w/ i_gc_rwsem lock held, in order to avoid block address change
during __f2fs_commit_atomic_write().

Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
to wait completion of block migration.

Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Chao Yu <chao@kernel.org>
---
v2:
- fix error path handling.
 fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 3aee71c9f3c6..a43054ab0cf1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode *inode, pgoff_t index,
 		return err;
 	}
 
+	if (__is_valid_data_blkaddr(dn.data_blkaddr))
+		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr, 1);
+
 	if (recover) {
 		/* dn.data_blkaddr is always valid */
 		if (!__is_valid_data_blkaddr(new_addr)) {
@@ -339,6 +342,9 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
 				goto out;
 			}
 
+			f2fs_wait_on_block_writeback_range(cow_inode,
+								blkaddr, 1);
+
 			new = f2fs_kmem_cache_alloc(revoke_entry_slab, GFP_NOFS,
 							true, NULL);
 
@@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	int err;
 
+	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
+
 	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
 	if (err)
-		return err;
+		goto out;
 
-	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
-	f2fs_lock_op(sbi);
+	/* writeback GCing page of cow_inode */
+	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
+							0, LLONG_MAX);
+	if (err)
+		goto out;
 
-	err = __f2fs_commit_atomic_write(inode);
+	filemap_invalidate_lock(inode->i_mapping);
+
+	/* don't allow clean page loaded by GC to pollute atomic_file */
+	truncate_pagecache(inode, 0);
 
+	f2fs_lock_op(sbi);
+	err = __f2fs_commit_atomic_write(inode);
 	f2fs_unlock_op(sbi);
+
+	filemap_invalidate_unlock(inode->i_mapping);
+out:
 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
 
 	return err;
-- 
2.40.1

RE: [f2fs-dev] [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[Sunmin Jeong]

Hello Chao Yu,

>SQLite App			GC Thread	Shrinker
>- f2fs_ioc_start_atomic_write
>
>- f2fs_ioc_commit_atomic_write
> - f2fs_commit_atomic_write
>  - filemap_write_and_wait_range
>  : write atomic_file's data to cow_inode
>						echo 3 > drop_caches
>				- f2fs_gc
>				 - gc_data_segment
>				  - move_data_page

We modified the code to make GC of the atomic file go to move_data_block
in commit b40a2b003709 (f2fs: use meta inode for GC of atomic file).
Could you please check if this patch is still necessary?

Thanks,

>				   - set_page_dirty
>				   : it may load data of previous
>				     transaction into pagecache.
>  - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
>  - __f2fs_commit_atomic_write
>  - f2fs_up_write(&fi->i_gc_rwsem[WRITE])
>
>During committing atomic_file, GC may be triggered to migrate atomic_file's
>block, so it may contain data of previous transaction in page cache, we
>should drop atomic_file's cache once it was migrated by GC.
>
>And also, we should writeback atomic_file and cow_file's data w/ i_gc_rwsem
>lock held, in order to avoid block address change during
>__f2fs_commit_atomic_write().
>
>Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
>to wait completion of block migration.
>
>Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>Signed-off-by: Chao Yu <chao@kernel.org>
>---
>v2:
>- fix error path handling.
> fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
>diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
>3aee71c9f3c6..a43054ab0cf1 100644
>--- a/fs/f2fs/segment.c
>+++ b/fs/f2fs/segment.c
>@@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode
>*inode, pgoff_t index,
> 		return err;
> 	}
>
>+	if (__is_valid_data_blkaddr(dn.data_blkaddr))
>+		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr,
1);
>+
> 	if (recover) {
> 		/* dn.data_blkaddr is always valid */
> 		if (!__is_valid_data_blkaddr(new_addr)) { @@ -339,6 +342,9
@@
>static int __f2fs_commit_atomic_write(struct inode *inode)
> 				goto out;
> 			}
>
>+			f2fs_wait_on_block_writeback_range(cow_inode,
>+								blkaddr, 1);
>+
> 			new = f2fs_kmem_cache_alloc(revoke_entry_slab,
GFP_NOFS,
> 							true, NULL);
>
>@@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
> 	struct f2fs_inode_info *fi = F2FS_I(inode);
> 	int err;
>
>+	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>+
> 	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> 	if (err)
>-		return err;
>+		goto out;
>
>-	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>-	f2fs_lock_op(sbi);
>+	/* writeback GCing page of cow_inode */
>+	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
>+							0, LLONG_MAX);
>+	if (err)
>+		goto out;
>
>-	err = __f2fs_commit_atomic_write(inode);
>+	filemap_invalidate_lock(inode->i_mapping);
>+
>+	/* don't allow clean page loaded by GC to pollute atomic_file */
>+	truncate_pagecache(inode, 0);
>
>+	f2fs_lock_op(sbi);
>+	err = __f2fs_commit_atomic_write(inode);
> 	f2fs_unlock_op(sbi);
>+
>+	filemap_invalidate_unlock(inode->i_mapping);
>+out:
> 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>
> 	return err;
>--
>2.40.1
>
>
>
>_______________________________________________
>Linux-f2fs-devel mailing list
>Linux-f2fs-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[Chao Yu]

On 2024/8/20 10:44, Sunmin Jeong wrote:
> Hello Chao Yu,
> 
>> SQLite App			GC Thread	Shrinker
>> - f2fs_ioc_start_atomic_write
>>
>> - f2fs_ioc_commit_atomic_write
>> - f2fs_commit_atomic_write
>>   - filemap_write_and_wait_range
>>   : write atomic_file's data to cow_inode
>> 						echo 3 > drop_caches
>> 				- f2fs_gc
>> 				 - gc_data_segment
>> 				  - move_data_page
> 
> We modified the code to make GC of the atomic file go to move_data_block
> in commit b40a2b003709 (f2fs: use meta inode for GC of atomic file).
> Could you please check if this patch is still necessary?

Sunmin,

Yes, you're right.

Also, I've checked f2fs/003 of xfstests w/ commit b40a2b003709, it can fix the
regression issue as well, so let's drop it.

Thanks for pointing it out. :)

Thanks,

> 
> Thanks,
> 
>> 				   - set_page_dirty
>> 				   : it may load data of previous
>> 				     transaction into pagecache.
>>   - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
>>   - __f2fs_commit_atomic_write
>>   - f2fs_up_write(&fi->i_gc_rwsem[WRITE])
>>
>> During committing atomic_file, GC may be triggered to migrate atomic_file's
>> block, so it may contain data of previous transaction in page cache, we
>> should drop atomic_file's cache once it was migrated by GC.
>>
>> And also, we should writeback atomic_file and cow_file's data w/ i_gc_rwsem
>> lock held, in order to avoid block address change during
>> __f2fs_commit_atomic_write().
>>
>> Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
>> to wait completion of block migration.
>>
>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>> v2:
>> - fix error path handling.
>> fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
>> 1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
>> 3aee71c9f3c6..a43054ab0cf1 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode
>> *inode, pgoff_t index,
>> 		return err;
>> 	}
>>
>> +	if (__is_valid_data_blkaddr(dn.data_blkaddr))
>> +		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr,
> 1);
>> +
>> 	if (recover) {
>> 		/* dn.data_blkaddr is always valid */
>> 		if (!__is_valid_data_blkaddr(new_addr)) { @@ -339,6 +342,9
> @@
>> static int __f2fs_commit_atomic_write(struct inode *inode)
>> 				goto out;
>> 			}
>>
>> +			f2fs_wait_on_block_writeback_range(cow_inode,
>> +								blkaddr, 1);
>> +
>> 			new = f2fs_kmem_cache_alloc(revoke_entry_slab,
> GFP_NOFS,
>> 							true, NULL);
>>
>> @@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
>> 	struct f2fs_inode_info *fi = F2FS_I(inode);
>> 	int err;
>>
>> +	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>> +
>> 	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>> 	if (err)
>> -		return err;
>> +		goto out;
>>
>> -	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>> -	f2fs_lock_op(sbi);
>> +	/* writeback GCing page of cow_inode */
>> +	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
>> +							0, LLONG_MAX);
>> +	if (err)
>> +		goto out;
>>
>> -	err = __f2fs_commit_atomic_write(inode);
>> +	filemap_invalidate_lock(inode->i_mapping);
>> +
>> +	/* don't allow clean page loaded by GC to pollute atomic_file */
>> +	truncate_pagecache(inode, 0);
>>
>> +	f2fs_lock_op(sbi);
>> +	err = __f2fs_commit_atomic_write(inode);
>> 	f2fs_unlock_op(sbi);
>> +
>> +	filemap_invalidate_unlock(inode->i_mapping);
>> +out:
>> 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>
>> 	return err;
>> --
>> 2.40.1
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

Re: [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[Jaegeuk Kim]

On 08/14, Chao Yu wrote:
> SQLite App			GC Thread	Shrinker
> - f2fs_ioc_start_atomic_write
> 
> - f2fs_ioc_commit_atomic_write
>  - f2fs_commit_atomic_write
>   - filemap_write_and_wait_range
>   : write atomic_file's data to cow_inode
> 						echo 3 > drop_caches
> 				- f2fs_gc
> 				 - gc_data_segment
> 				  - move_data_page
> 				   - set_page_dirty
> 				   : it may load data of previous
> 				     transaction into pagecache.
>   - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
>   - __f2fs_commit_atomic_write
>   - f2fs_up_write(&fi->i_gc_rwsem[WRITE])
> 
> During committing atomic_file, GC may be triggered to migrate
> atomic_file's block, so it may contain data of previous transaction
> in page cache, we should drop atomic_file's cache once it was
> migrated by GC.
> 
> And also, we should writeback atomic_file and cow_file's data
> w/ i_gc_rwsem lock held, in order to avoid block address change
> during __f2fs_commit_atomic_write().
> 
> Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
> to wait completion of block migration.
> 
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> v2:
> - fix error path handling.
>  fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 3aee71c9f3c6..a43054ab0cf1 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode *inode, pgoff_t index,
>  		return err;
>  	}
>  
> +	if (__is_valid_data_blkaddr(dn.data_blkaddr))
> +		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr, 1);
> +
>  	if (recover) {
>  		/* dn.data_blkaddr is always valid */
>  		if (!__is_valid_data_blkaddr(new_addr)) {
> @@ -339,6 +342,9 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>  				goto out;
>  			}
>  
> +			f2fs_wait_on_block_writeback_range(cow_inode,
> +								blkaddr, 1);
> +
>  			new = f2fs_kmem_cache_alloc(revoke_entry_slab, GFP_NOFS,
>  							true, NULL);
>  
> @@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	int err;
>  
> +	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
> +
>  	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>  	if (err)
> -		return err;
> +		goto out;
>  
> -	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
> -	f2fs_lock_op(sbi);
> +	/* writeback GCing page of cow_inode */
> +	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
> +							0, LLONG_MAX);
> +	if (err)
> +		goto out;
>  
> -	err = __f2fs_commit_atomic_write(inode);
> +	filemap_invalidate_lock(inode->i_mapping);
> +
> +	/* don't allow clean page loaded by GC to pollute atomic_file */
> +	truncate_pagecache(inode, 0);

Performance impact?

>  
> +	f2fs_lock_op(sbi);
> +	err = __f2fs_commit_atomic_write(inode);
>  	f2fs_unlock_op(sbi);
> +
> +	filemap_invalidate_unlock(inode->i_mapping);
> +out:
>  	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>  
>  	return err;
> -- 
> 2.40.1

Re: [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

[Chao Yu]

On 2024/8/28 23:27, Jaegeuk Kim wrote:
> On 08/14, Chao Yu wrote:
>> SQLite App			GC Thread	Shrinker
>> - f2fs_ioc_start_atomic_write
>>
>> - f2fs_ioc_commit_atomic_write
>>   - f2fs_commit_atomic_write
>>    - filemap_write_and_wait_range
>>    : write atomic_file's data to cow_inode
>> 						echo 3 > drop_caches
>> 				- f2fs_gc
>> 				 - gc_data_segment
>> 				  - move_data_page
>> 				   - set_page_dirty
>> 				   : it may load data of previous
>> 				     transaction into pagecache.
>>    - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
>>    - __f2fs_commit_atomic_write
>>    - f2fs_up_write(&fi->i_gc_rwsem[WRITE])
>>
>> During committing atomic_file, GC may be triggered to migrate
>> atomic_file's block, so it may contain data of previous transaction
>> in page cache, we should drop atomic_file's cache once it was
>> migrated by GC.
>>
>> And also, we should writeback atomic_file and cow_file's data
>> w/ i_gc_rwsem lock held, in order to avoid block address change
>> during __f2fs_commit_atomic_write().
>>
>> Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
>> to wait completion of block migration.
>>
>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>> v2:
>> - fix error path handling.
>>   fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 3aee71c9f3c6..a43054ab0cf1 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode *inode, pgoff_t index,
>>   		return err;
>>   	}
>>   
>> +	if (__is_valid_data_blkaddr(dn.data_blkaddr))
>> +		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr, 1);
>> +
>>   	if (recover) {
>>   		/* dn.data_blkaddr is always valid */
>>   		if (!__is_valid_data_blkaddr(new_addr)) {
>> @@ -339,6 +342,9 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>   				goto out;
>>   			}
>>   
>> +			f2fs_wait_on_block_writeback_range(cow_inode,
>> +								blkaddr, 1);
>> +
>>   			new = f2fs_kmem_cache_alloc(revoke_entry_slab, GFP_NOFS,
>>   							true, NULL);
>>   
>> @@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
>>   	struct f2fs_inode_info *fi = F2FS_I(inode);
>>   	int err;
>>   
>> +	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>> +
>>   	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>>   	if (err)
>> -		return err;
>> +		goto out;
>>   
>> -	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>> -	f2fs_lock_op(sbi);
>> +	/* writeback GCing page of cow_inode */
>> +	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
>> +							0, LLONG_MAX);
>> +	if (err)
>> +		goto out;
>>   
>> -	err = __f2fs_commit_atomic_write(inode);
>> +	filemap_invalidate_lock(inode->i_mapping);
>> +
>> +	/* don't allow clean page loaded by GC to pollute atomic_file */
>> +	truncate_pagecache(inode, 0);
> 
> Performance impact?

Please drop this patch due to commit b40a2b003709 (f2fs: use meta inode for
GC of atomic file) has fixed this issue. Thanks.

Thanks,

> 
>>   
>> +	f2fs_lock_op(sbi);
>> +	err = __f2fs_commit_atomic_write(inode);
>>   	f2fs_unlock_op(sbi);
>> +
>> +	filemap_invalidate_unlock(inode->i_mapping);
>> +out:
>>   	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>   
>>   	return err;
>> -- 
>> 2.40.1