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

[Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

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


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