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

[Chao Yu <chao@xxxxxxxxxx>]

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@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"?

Sunmin, yes, I think so.

Thanks,

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