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@xxxxxxxxxxxxxxx #v5.19+
>> Reviewed-by: Sungjong Seo <sj1557.seo@xxxxxxxxxxx>
>> Reviewed-by: Yeongjin Gil <youngjin.gil@xxxxxxxxxxx>
>> Signed-off-by: Sunmin Jeong <s_min.jeong@xxxxxxxxxxx>
>> ---
>> 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,
