| Message ID | 20240702120636.476119-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: > In case of the COW file, new updates and GC writes are already > separated to page caches of the atomic file and COW file. As some cases > that use the meta inode for GC, there are some race issues between a > foreground thread and GC thread. > > To handle them, we need to take care when to invalidate and wait > writeback of GC pages in COW files as the case of using the meta inode. > Also, a pointer from the COW inode to the original inode is required to > check the state of original pages. > > For the former, we can solve the problem by using the meta inode for GC > of COW files. Then let's get a page from the original inode in > move_data_block when GCing the COW file to avoid race condition. > > 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/data.c | 2 +- > fs/f2fs/f2fs.h | 7 ++++++- > fs/f2fs/file.c | 3 +++ > fs/f2fs/gc.c | 12 ++++++++++-- > fs/f2fs/inline.c | 2 +- > fs/f2fs/inode.c | 3 ++- > 6 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 05158f89ef32..90ff0f6f7f7f 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2651,7 +2651,7 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio) > return true; > if (IS_NOQUOTA(inode)) > return true; > - if (f2fs_is_atomic_file(inode)) > + if (f2fs_used_in_atomic_write(inode)) > return true; > /* rewrite low ratio compress data w/ OPU mode to avoid fragmentation */ > if (f2fs_compressed_file(inode) && > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 59c5117e54b1..4f9fd1c1d024 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4267,9 +4267,14 @@ static inline bool f2fs_post_read_required(struct inode *inode) > f2fs_compressed_file(inode); > } > > +static inline bool f2fs_used_in_atomic_write(struct inode *inode) > +{ > + return f2fs_is_atomic_file(inode) || f2fs_is_cow_file(inode); > +} > + > static inline bool f2fs_meta_inode_gc_required(struct inode *inode) > { > - return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode); > + return f2fs_post_read_required(inode) || f2fs_used_in_atomic_write(inode); > } > > /* > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 25b119cf3499..c9f0ba658cfd 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -2116,6 +2116,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > set_inode_flag(fi->cow_inode, FI_COW_FILE); > clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > + > + /* Set the COW inode's cow_inode to the atomic inode */ > + F2FS_I(fi->cow_inode)->cow_inode = inode; How about adding a union fields as below for readability? struct f2fs_inode_info { ... union { struct inode *cow_inode; /* copy-on-write inode for atomic write */ struct inode *atomic_inode; /* point to atomic_inode, available only for cow_inode */ }; ... }; Thanks, > } else { > /* Reuse the already created COW inode */ > ret = f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 136b9e8180a3..76854e732b35 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -1188,7 +1188,11 @@ static int ra_data_block(struct inode *inode, pgoff_t index) > }; > int err; > > - page = f2fs_grab_cache_page(mapping, index, true); > + if (f2fs_is_cow_file(inode)) > + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode->i_mapping, > + index, true); > + else > + page = f2fs_grab_cache_page(mapping, index, true); > if (!page) > return -ENOMEM; > > @@ -1287,7 +1291,11 @@ static int move_data_block(struct inode *inode, block_t bidx, > CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA; > > /* do not read out */ > - page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); > + if (f2fs_is_cow_file(inode)) > + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode->i_mapping, > + bidx, false); > + else > + page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); > if (!page) > return -ENOMEM; > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index ac00423f117b..0186ec049db6 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -16,7 +16,7 @@ > > static bool support_inline_data(struct inode *inode) > { > - if (f2fs_is_atomic_file(inode)) > + if (f2fs_used_in_atomic_write(inode)) > return false; > if (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode)) > return false; > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > index c26effdce9aa..c810304e2681 100644 > --- a/fs/f2fs/inode.c > +++ b/fs/f2fs/inode.c > @@ -807,8 +807,9 @@ void f2fs_evict_inode(struct inode *inode) > > f2fs_abort_atomic_write(inode, true); > > - if (fi->cow_inode) { > + if (fi->cow_inode && f2fs_is_cow_file(fi->cow_inode)) { > clear_inode_flag(fi->cow_inode, FI_COW_FILE); > + F2FS_I(fi->cow_inode)->cow_inode = NULL; > iput(fi->cow_inode); > fi->cow_inode = NULL; > }
Hi Chao Yu, >> In case of the COW file, new updates and GC writes are already >> separated to page caches of the atomic file and COW file. As some >> cases that use the meta inode for GC, there are some race issues >> between a foreground thread and GC thread. >> >> To handle them, we need to take care when to invalidate and wait >> writeback of GC pages in COW files as the case of using the meta inode. >> Also, a pointer from the COW inode to the original inode is required >> to check the state of original pages. >> >> For the former, we can solve the problem by using the meta inode for >> GC of COW files. Then let's get a page from the original inode in >> move_data_block when GCing the COW file to avoid race condition. >> >> 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/data.c | 2 +- >> fs/f2fs/f2fs.h | 7 ++++++- >> fs/f2fs/file.c | 3 +++ >> fs/f2fs/gc.c | 12 ++++++++++-- >> fs/f2fs/inline.c | 2 +- >> fs/f2fs/inode.c | 3 ++- >> 6 files changed, 23 insertions(+), 6 deletions(-) >> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index >> 05158f89ef32..90ff0f6f7f7f 100644 >> --- a/fs/f2fs/data.c >> +++ b/fs/f2fs/data.c >> @@ -2651,7 +2651,7 @@ bool f2fs_should_update_outplace(struct inode >*inode, struct f2fs_io_info *fio) >> return true; >> if (IS_NOQUOTA(inode)) >> return true; >> - if (f2fs_is_atomic_file(inode)) >> + if (f2fs_used_in_atomic_write(inode)) >> return true; >> /* rewrite low ratio compress data w/ OPU mode to avoid >fragmentation */ >> if (f2fs_compressed_file(inode) && >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index >> 59c5117e54b1..4f9fd1c1d024 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -4267,9 +4267,14 @@ static inline bool f2fs_post_read_required(struct >inode *inode) >> f2fs_compressed_file(inode); >> } >> >> +static inline bool f2fs_used_in_atomic_write(struct inode *inode) { >> + return f2fs_is_atomic_file(inode) || f2fs_is_cow_file(inode); } >> + >> static inline bool f2fs_meta_inode_gc_required(struct inode *inode) >> { >> - return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode); >> + return f2fs_post_read_required(inode) || >> +f2fs_used_in_atomic_write(inode); >> } >> >> /* >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index >> 25b119cf3499..c9f0ba658cfd 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -2116,6 +2116,9 @@ static int f2fs_ioc_start_atomic_write(struct >> file *filp, bool truncate) >> >> set_inode_flag(fi->cow_inode, FI_COW_FILE); >> clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >> + >> + /* Set the COW inode's cow_inode to the atomic inode */ >> + F2FS_I(fi->cow_inode)->cow_inode = inode; > >How about adding a union fields as below for readability? > >struct f2fs_inode_info { >... > union { > struct inode *cow_inode; /* copy-on-write inode for atomic >write */ > struct inode *atomic_inode; /* point to atomic_inode, >available only for cow_inode */ > }; >... >}; > >Thanks, > Thanks for you opinion. I'll send patch v2. >> } else { >> /* Reuse the already created COW inode */ >> ret = f2fs_do_truncate_blocks(fi->cow_inode, 0, true); diff - >-git >> a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 136b9e8180a3..76854e732b35 100644 >> --- a/fs/f2fs/gc.c >> +++ b/fs/f2fs/gc.c >> @@ -1188,7 +1188,11 @@ static int ra_data_block(struct inode *inode, >pgoff_t index) >> }; >> int err; >> >> - page = f2fs_grab_cache_page(mapping, index, true); >> + if (f2fs_is_cow_file(inode)) >> + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode- >>i_mapping, >> + index, true); >> + else >> + page = f2fs_grab_cache_page(mapping, index, true); >> if (!page) >> return -ENOMEM; >> >> @@ -1287,7 +1291,11 @@ static int move_data_block(struct inode *inode, >block_t bidx, >> CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA; >> >> /* do not read out */ >> - page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); >> + if (f2fs_is_cow_file(inode)) >> + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode- >>i_mapping, >> + bidx, false); >> + else >> + page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); >> if (!page) >> return -ENOMEM; >> >> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index >> ac00423f117b..0186ec049db6 100644 >> --- a/fs/f2fs/inline.c >> +++ b/fs/f2fs/inline.c >> @@ -16,7 +16,7 @@ >> >> static bool support_inline_data(struct inode *inode) >> { >> - if (f2fs_is_atomic_file(inode)) >> + if (f2fs_used_in_atomic_write(inode)) >> return false; >> if (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode)) >> return false; >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index >> c26effdce9aa..c810304e2681 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -807,8 +807,9 @@ void f2fs_evict_inode(struct inode *inode) >> >> f2fs_abort_atomic_write(inode, true); >> >> - if (fi->cow_inode) { >> + if (fi->cow_inode && f2fs_is_cow_file(fi->cow_inode)) { >> clear_inode_flag(fi->cow_inode, FI_COW_FILE); >> + F2FS_I(fi->cow_inode)->cow_inode = NULL; >> iput(fi->cow_inode); >> fi->cow_inode = NULL; >> }
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 05158f89ef32..90ff0f6f7f7f 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2651,7 +2651,7 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio) return true; if (IS_NOQUOTA(inode)) return true; - if (f2fs_is_atomic_file(inode)) + if (f2fs_used_in_atomic_write(inode)) return true; /* rewrite low ratio compress data w/ OPU mode to avoid fragmentation */ if (f2fs_compressed_file(inode) && diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 59c5117e54b1..4f9fd1c1d024 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -4267,9 +4267,14 @@ static inline bool f2fs_post_read_required(struct inode *inode) f2fs_compressed_file(inode); } +static inline bool f2fs_used_in_atomic_write(struct inode *inode) +{ + return f2fs_is_atomic_file(inode) || f2fs_is_cow_file(inode); +} + static inline bool f2fs_meta_inode_gc_required(struct inode *inode) { - return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode); + return f2fs_post_read_required(inode) || f2fs_used_in_atomic_write(inode); } /* diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 25b119cf3499..c9f0ba658cfd 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -2116,6 +2116,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) set_inode_flag(fi->cow_inode, FI_COW_FILE); clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); + + /* Set the COW inode's cow_inode to the atomic inode */ + F2FS_I(fi->cow_inode)->cow_inode = inode; } else { /* Reuse the already created COW inode */ ret = f2fs_do_truncate_blocks(fi->cow_inode, 0, true); diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 136b9e8180a3..76854e732b35 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -1188,7 +1188,11 @@ static int ra_data_block(struct inode *inode, pgoff_t index) }; int err; - page = f2fs_grab_cache_page(mapping, index, true); + if (f2fs_is_cow_file(inode)) + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode->i_mapping, + index, true); + else + page = f2fs_grab_cache_page(mapping, index, true); if (!page) return -ENOMEM; @@ -1287,7 +1291,11 @@ static int move_data_block(struct inode *inode, block_t bidx, CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA; /* do not read out */ - page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); + if (f2fs_is_cow_file(inode)) + page = f2fs_grab_cache_page(F2FS_I(inode)->cow_inode->i_mapping, + bidx, false); + else + page = f2fs_grab_cache_page(inode->i_mapping, bidx, false); if (!page) return -ENOMEM; diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index ac00423f117b..0186ec049db6 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -16,7 +16,7 @@ static bool support_inline_data(struct inode *inode) { - if (f2fs_is_atomic_file(inode)) + if (f2fs_used_in_atomic_write(inode)) return false; if (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode)) return false; diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index c26effdce9aa..c810304e2681 100644 --- a/fs/f2fs/inode.c +++ b/fs/f2fs/inode.c @@ -807,8 +807,9 @@ void f2fs_evict_inode(struct inode *inode) f2fs_abort_atomic_write(inode, true); - if (fi->cow_inode) { + if (fi->cow_inode && f2fs_is_cow_file(fi->cow_inode)) { clear_inode_flag(fi->cow_inode, FI_COW_FILE); + F2FS_I(fi->cow_inode)->cow_inode = NULL; iput(fi->cow_inode); fi->cow_inode = NULL; }
In case of the COW file, new updates and GC writes are already separated to page caches of the atomic file and COW file. As some cases that use the meta inode for GC, there are some race issues between a foreground thread and GC thread. To handle them, we need to take care when to invalidate and wait writeback of GC pages in COW files as the case of using the meta inode. Also, a pointer from the COW inode to the original inode is required to check the state of original pages. For the former, we can solve the problem by using the meta inode for GC of COW files. Then let's get a page from the original inode in move_data_block when GCing the COW file to avoid race condition. 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/data.c | 2 +- fs/f2fs/f2fs.h | 7 ++++++- fs/f2fs/file.c | 3 +++ fs/f2fs/gc.c | 12 ++++++++++-- fs/f2fs/inline.c | 2 +- fs/f2fs/inode.c | 3 ++- 6 files changed, 23 insertions(+), 6 deletions(-)