RE: [PATCH 1/2] f2fs: use meta inode for GC of atomic file

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

Hi Chao Yu,

>> The page cache of the atomic file keeps new data pages which will be
>> stored in the COW file. It can also keep old data pages when GCing the
>> atomic file. In this case, new data can be overwritten by old data if
>> a GC thread sets the old data page as dirty after new data page was
>> evicted.
>>
>> Also, since all writes to the atomic file are redirected to COW
>> inodes, GC for the atomic file is not working well as below.
>>
>> f2fs_gc(gc_type=FG_GC)
>>    - select A as a victim segment
>>    do_garbage_collect
>>      - iget atomic file's inode for block B
>>      move_data_page
>>        f2fs_do_write_data_page
>>          - use dn of cow inode
>>          - set fio->old_blkaddr from cow inode
>>      - seg_freed is 0 since block B is still valid
>>    - goto gc_more and A is selected as victim again
>>
>> To solve the problem, let's separate GC writes and updates in the
>> atomic file by using the meta inode for GC writes.
>>
>> 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/f2fs.h    | 5 +++++
>>   fs/f2fs/gc.c      | 6 +++---
>>   fs/f2fs/segment.c | 4 ++--
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index
>> a000cb024dbe..59c5117e54b1 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4267,6 +4267,11 @@ static inline bool f2fs_post_read_required(struct
>inode *inode)
>>   		f2fs_compressed_file(inode);
>>   }
>>
>> +static inline bool f2fs_meta_inode_gc_required(struct inode *inode) {
>> +	return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode);
>> +}
>> +
>>   /*
>>    * compress.c
>>    */
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index
>> a079eebfb080..136b9e8180a3 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -1580,7 +1580,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi,
>struct f2fs_summary *sum,
>>   			start_bidx = f2fs_start_bidx_of_node(nofs, inode) +
>>   								ofs_in_node;
>>
>> -			if (f2fs_post_read_required(inode)) {
>> +			if (f2fs_meta_inode_gc_required(inode)) {
>>   				int err = ra_data_block(inode, start_bidx);
>>
>>   				f2fs_up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>> @@ -1631,7 +1631,7 @@ static int gc_data_segment(struct f2fs_sb_info
>> *sbi, struct f2fs_summary *sum,
>>
>>   			start_bidx = f2fs_start_bidx_of_node(nofs, inode)
>>   								+ ofs_in_node;
>> -			if (f2fs_post_read_required(inode))
>> +			if (f2fs_meta_inode_gc_required(inode))
>>   				err = move_data_block(inode, start_bidx,
>>   							gc_type, segno, off);
>>   			else
>> @@ -1639,7 +1639,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi,
>struct f2fs_summary *sum,
>>   								segno, off);
>>
>>   			if (!err && (gc_type == FG_GC ||
>> -					f2fs_post_read_required(inode)))
>> +					f2fs_meta_inode_gc_required(inode)))
>>   				submitted++;
>>
>>   			if (locked) {
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
>> 7e47b8054413..b55fc4bd416a 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -3823,7 +3823,7 @@ void f2fs_wait_on_block_writeback(struct inode
>*inode, block_t blkaddr)
>>   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>   	struct page *cpage;
>>
>> -	if (!f2fs_post_read_required(inode))
>> +	if (!f2fs_meta_inode_gc_required(inode))
>>   		return;
>>
>>   	if (!__is_valid_data_blkaddr(blkaddr))
>> @@ -3842,7 +3842,7 @@ void f2fs_wait_on_block_writeback_range(struct
>inode *inode, block_t blkaddr,
>>   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>   	block_t i;
>>
>> -	if (!f2fs_post_read_required(inode))
>> +	if (!f2fs_meta_inode_gc_required(inode))
>>   		return;
>>
>>   	for (i = 0; i < len; i++)
>
>f2fs_write_single_data_page()
>...
>		.post_read = f2fs_post_read_required(inode) ? 1 : 0,
>
>Do we need to use f2fs_meta_inode_gc_required() here?
>
>Thanks,

As you said, we need to use f2fs_meta_inode_gc_required instead of
f2fs_post_read_required. Then what about changing the variable name
"post_read" to another one such as "meta_gc"? 
struct f2fs_io_info {
    unsigned int post_read:1;   /* require post read */

Thanks,