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

[Chao Yu <chao@kernel.org>]

On 2024/7/5 11:25, Sunmin Jeong wrote:
> 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"?

Sunmin, yes, I think so.

Thanks,

> struct f2fs_io_info {
>      unsigned int post_read:1;   /* require post read */
> 
> Thanks,
>