| Message ID | 20240702120624.476067-1-s_min.jeong@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev,1/2] f2fs: use meta inode for GC of atomic file | expand |
On 2024/7/2 20:06, Sunmin Jeong wrote: > 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,
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,
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, >
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++)
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(-)