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

["Sunmin Jeong" <s_min.jeong@samsung.com>]

Hi Chao Yu,

>> 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>
>> ---
>>   fs/f2fs/data.c   |  2 +-
>>   fs/f2fs/f2fs.h   |  7 ++++++-
>>   fs/f2fs/file.c   |  3 +++
>>   fs/f2fs/gc.c     | 12 ++++++++++--
>>   fs/f2fs/inline.c |  2 +-
>>   fs/f2fs/inode.c  |  3 ++-
>>   6 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>> 05158f89ef32..90ff0f6f7f7f 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2651,7 +2651,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
>> 59c5117e54b1..4f9fd1c1d024 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4267,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
>> 25b119cf3499..c9f0ba658cfd 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -2116,6 +2116,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 cow_inode to the atomic inode */
>> +		F2FS_I(fi->cow_inode)->cow_inode = inode;
>
>How about adding a union fields as below for readability?
>
>struct f2fs_inode_info {
>...
>	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 */
>	};
>...
>};
>
>Thanks,
>

Thanks for you opinion. I'll send patch v2.

>>   	} 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 136b9e8180a3..76854e732b35 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -1188,7 +1188,11 @@ static int ra_data_block(struct inode *inode,
>pgoff_t index)
>>   	};
>>   	int err;
>>
>> -	page = f2fs_grab_cache_page(mapping, index, true);
>> +	if (f2fs_is_cow_file(inode))
>> +		page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode-
>>i_mapping,
>> +						index, true);
>> +	else
>> +		page = f2fs_grab_cache_page(mapping, index, true);
>>   	if (!page)
>>   		return -ENOMEM;
>>
>> @@ -1287,7 +1291,11 @@ static int move_data_block(struct inode *inode,
>block_t bidx,
>>   				CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA;
>>
>>   	/* do not read out */
>> -	page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
>> +	if (f2fs_is_cow_file(inode))
>> +		page = f2fs_grab_cache_page(F2FS_I(inode)->cow_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
>> ac00423f117b..0186ec049db6 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
>> c26effdce9aa..c810304e2681 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -807,8 +807,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)->cow_inode = NULL;
>>   		iput(fi->cow_inode);
>>   		fi->cow_inode = NULL;
>>   	}



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