Re: [f2fs-dev] [PATCH v2 2/2] f2fs: use meta inode for GC of COW file

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 07/05, Sunmin Jeong wrote:
> In case of the COW file, new updates and GC writes are already
> separated to page caches of the atomic file and COW file. As some cases
> that use the meta inode for GC, there are some race issues between a
> foreground thread and GC thread.
> 
> To handle them, we need to take care when to invalidate and wait
> writeback of GC pages in COW files as the case of using the meta inode.
> Also, a pointer from the COW inode to the original inode is required to
> check the state of original pages.
> 
> For the former, we can solve the problem by using the meta inode for GC
> of COW files. Then let's get a page from the original inode in
> move_data_block when GCing the COW file to avoid race condition.
> 
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@vger.kernel.org #v5.19+
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
> Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>
> ---
> v2:
> - use union for cow inode to point to atomic inode
>  fs/f2fs/data.c   |  2 +-
>  fs/f2fs/f2fs.h   | 13 +++++++++++--
>  fs/f2fs/file.c   |  3 +++
>  fs/f2fs/gc.c     | 12 ++++++++++--
>  fs/f2fs/inline.c |  2 +-
>  fs/f2fs/inode.c  |  3 ++-
>  6 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 9a213d03005d..f6b1782f965a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2606,7 +2606,7 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
>  		return true;
>  	if (IS_NOQUOTA(inode))
>  		return true;
> -	if (f2fs_is_atomic_file(inode))
> +	if (f2fs_used_in_atomic_write(inode))
>  		return true;
>  	/* rewrite low ratio compress data w/ OPU mode to avoid fragmentation */
>  	if (f2fs_compressed_file(inode) &&
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 796ae11c0fa3..4a8621e4a33a 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -843,7 +843,11 @@ struct f2fs_inode_info {
>  	struct task_struct *atomic_write_task;	/* store atomic write task */
>  	struct extent_tree *extent_tree[NR_EXTENT_CACHES];
>  					/* cached extent_tree entry */
> -	struct inode *cow_inode;	/* copy-on-write inode for atomic write */
> +	union {
> +		struct inode *cow_inode;	/* copy-on-write inode for atomic write */
> +		struct inode *atomic_inode;
> +					/* point to atomic_inode, available only for cow_inode */
> +	};
>  
>  	/* avoid racing between foreground op and gc */
>  	struct f2fs_rwsem i_gc_rwsem[2];
> @@ -4263,9 +4267,14 @@ static inline bool f2fs_post_read_required(struct inode *inode)
>  		f2fs_compressed_file(inode);
>  }
>  
> +static inline bool f2fs_used_in_atomic_write(struct inode *inode)
> +{
> +	return f2fs_is_atomic_file(inode) || f2fs_is_cow_file(inode);
> +}
> +
>  static inline bool f2fs_meta_inode_gc_required(struct inode *inode)
>  {
> -	return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode);
> +	return f2fs_post_read_required(inode) || f2fs_used_in_atomic_write(inode);
>  }
>  
>  /*
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index e4a7cff00796..547e7ec32b1f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2183,6 +2183,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>  
>  		set_inode_flag(fi->cow_inode, FI_COW_FILE);
>  		clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> +
> +		/* Set the COW inode's atomic_inode to the atomic inode */
> +		F2FS_I(fi->cow_inode)->atomic_inode = inode;
>  	} else {
>  		/* Reuse the already created COW inode */
>  		ret = f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index cb3006551ab5..61913fcefd9e 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1186,7 +1186,11 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>  	};
>  	int err;

How about giving the right mapping like this?

	mapping = f2fs_is_cow_file() ?
		F2FS_I(inode)->actomic_inode->i_mapping : inode->i_mapping;
	page = f2fs_grab_cache_page(mapping, index, true);

>  
> -	page = f2fs_grab_cache_page(mapping, index, true);
> +	if (f2fs_is_cow_file(inode))
> +		page = f2fs_grab_cache_page(F2FS_I(inode)->atomic_inode->i_mapping,
> +						index, true);
> +	else
> +		page = f2fs_grab_cache_page(mapping, index, true);
>  	if (!page)
>  		return -ENOMEM;
>  
> @@ -1282,7 +1286,11 @@ static int move_data_block(struct inode *inode, block_t bidx,
>  				CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA;
>  
>  	/* do not read out */

ditto?

> -	page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
> +	if (f2fs_is_cow_file(inode))
> +		page = f2fs_grab_cache_page(F2FS_I(inode)->atomic_inode->i_mapping,
> +						bidx, false);
> +	else
> +		page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
>  	if (!page)
>  		return -ENOMEM;
>  
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 1fba5728be70..cca7d448e55c 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -16,7 +16,7 @@
>  
>  static bool support_inline_data(struct inode *inode)
>  {
> -	if (f2fs_is_atomic_file(inode))
> +	if (f2fs_used_in_atomic_write(inode))
>  		return false;
>  	if (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))
>  		return false;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 7a3e2458b2d9..18dea43e694b 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -804,8 +804,9 @@ void f2fs_evict_inode(struct inode *inode)
>  
>  	f2fs_abort_atomic_write(inode, true);
>  
> -	if (fi->cow_inode) {
> +	if (fi->cow_inode && f2fs_is_cow_file(fi->cow_inode)) {
>  		clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> +		F2FS_I(fi->cow_inode)->atomic_inode = NULL;
>  		iput(fi->cow_inode);
>  		fi->cow_inode = NULL;
>  	}
> -- 
> 2.25.1


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