| Message ID | 20230410022418.1843178-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,1/2] f2fs: remove folio_detach_private() in .invalidate_folio and .release_folio | expand |
On 04/10, Chao Yu wrote: > We have maintain PagePrivate and page_private and page reference > w/ {set,clear}_page_private_*, it doesn't need to call > folio_detach_private() in the end of .invalidate_folio and > .release_folio, remove it and use f2fs_bug_on instead. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 4946df6dd253..8b179b4bdc03 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3737,7 +3737,8 @@ void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length) > inode->i_ino == F2FS_COMPRESS_INO(sbi)) > clear_page_private_data(&folio->page); > > - folio_detach_private(folio); > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > + f2fs_bug_on(sbi, page_private(&folio->page)); I think we can just check page_private() only. > } > > bool f2fs_release_folio(struct folio *folio, gfp_t wait) > @@ -3759,7 +3760,9 @@ bool f2fs_release_folio(struct folio *folio, gfp_t wait) > clear_page_private_reference(&folio->page); > clear_page_private_gcing(&folio->page); > > - folio_detach_private(folio); > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > + f2fs_bug_on(sbi, page_private(&folio->page)); > + > return true; > } > > -- > 2.25.1
On 2023/4/11 2:33, Jaegeuk Kim wrote: > On 04/10, Chao Yu wrote: >> We have maintain PagePrivate and page_private and page reference >> w/ {set,clear}_page_private_*, it doesn't need to call >> folio_detach_private() in the end of .invalidate_folio and >> .release_folio, remove it and use f2fs_bug_on instead. >> >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/data.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >> index 4946df6dd253..8b179b4bdc03 100644 >> --- a/fs/f2fs/data.c >> +++ b/fs/f2fs/data.c >> @@ -3737,7 +3737,8 @@ void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length) >> inode->i_ino == F2FS_COMPRESS_INO(sbi)) >> clear_page_private_data(&folio->page); >> >> - folio_detach_private(folio); >> + f2fs_bug_on(sbi, PagePrivate(&folio->page)); >> + f2fs_bug_on(sbi, page_private(&folio->page)); > > I think we can just check page_private() only. Why? how about the case PagePrivate was set, but page_private was't? It must be a bug as well? Thanks, > >> } >> >> bool f2fs_release_folio(struct folio *folio, gfp_t wait) >> @@ -3759,7 +3760,9 @@ bool f2fs_release_folio(struct folio *folio, gfp_t wait) >> clear_page_private_reference(&folio->page); >> clear_page_private_gcing(&folio->page); >> >> - folio_detach_private(folio); >> + f2fs_bug_on(sbi, PagePrivate(&folio->page)); >> + f2fs_bug_on(sbi, page_private(&folio->page)); >> + >> return true; >> } >> >> -- >> 2.25.1
On 04/11, Chao Yu wrote: > On 2023/4/11 2:33, Jaegeuk Kim wrote: > > On 04/10, Chao Yu wrote: > > > We have maintain PagePrivate and page_private and page reference > > > w/ {set,clear}_page_private_*, it doesn't need to call > > > folio_detach_private() in the end of .invalidate_folio and > > > .release_folio, remove it and use f2fs_bug_on instead. > > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > --- > > > fs/f2fs/data.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > index 4946df6dd253..8b179b4bdc03 100644 > > > --- a/fs/f2fs/data.c > > > +++ b/fs/f2fs/data.c > > > @@ -3737,7 +3737,8 @@ void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length) > > > inode->i_ino == F2FS_COMPRESS_INO(sbi)) > > > clear_page_private_data(&folio->page); > > > - folio_detach_private(folio); > > > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > > > + f2fs_bug_on(sbi, page_private(&folio->page)); > > > > I think we can just check page_private() only. > > Why? how about the case PagePrivate was set, but page_private was't? It must > be a bug as well? Given the code, I think both are set all the time. My concern is someone is not doing set/get properly. Actually, I got a panic on page_private() when running fsstress overnight. I'm trying to reproduce it to find which bit was set. > > Thanks, > > > > > > } > > > bool f2fs_release_folio(struct folio *folio, gfp_t wait) > > > @@ -3759,7 +3760,9 @@ bool f2fs_release_folio(struct folio *folio, gfp_t wait) > > > clear_page_private_reference(&folio->page); > > > clear_page_private_gcing(&folio->page); > > > - folio_detach_private(folio); > > > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > > > + f2fs_bug_on(sbi, page_private(&folio->page)); > > > + > > > return true; > > > } > > > -- > > > 2.25.1
On 04/11, Jaegeuk Kim wrote: > On 04/11, Chao Yu wrote: > > On 2023/4/11 2:33, Jaegeuk Kim wrote: > > > On 04/10, Chao Yu wrote: > > > > We have maintain PagePrivate and page_private and page reference > > > > w/ {set,clear}_page_private_*, it doesn't need to call > > > > folio_detach_private() in the end of .invalidate_folio and > > > > .release_folio, remove it and use f2fs_bug_on instead. > > > > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > --- > > > > fs/f2fs/data.c | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > index 4946df6dd253..8b179b4bdc03 100644 > > > > --- a/fs/f2fs/data.c > > > > +++ b/fs/f2fs/data.c > > > > @@ -3737,7 +3737,8 @@ void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length) > > > > inode->i_ino == F2FS_COMPRESS_INO(sbi)) > > > > clear_page_private_data(&folio->page); > > > > - folio_detach_private(folio); > > > > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > > > > + f2fs_bug_on(sbi, page_private(&folio->page)); > > > > > > I think we can just check page_private() only. > > > > Why? how about the case PagePrivate was set, but page_private was't? It must > > be a bug as well? > > Given the code, I think both are set all the time. My concern is someone is > not doing set/get properly. Actually, I got a panic on page_private() when > running fsstress overnight. I'm trying to reproduce it to find which bit was > set. It turned out that inline bit is somehow set, guessing the bit was not cleared when the first dirty page was truncated or somewhere else. Anyway, tooking a look at the usecase of flushing inline_data to inode page aggressively, I feel it's kinda hack and may increase the checkpoint latency. Hence, I'd like to remove it simply. https://lore.kernel.org/linux-f2fs-devel/20230412160810.1534632-1-jaegeuk@kernel.org/T/#t > > > > > Thanks, > > > > > > > > > } > > > > bool f2fs_release_folio(struct folio *folio, gfp_t wait) > > > > @@ -3759,7 +3760,9 @@ bool f2fs_release_folio(struct folio *folio, gfp_t wait) > > > > clear_page_private_reference(&folio->page); > > > > clear_page_private_gcing(&folio->page); > > > > - folio_detach_private(folio); > > > > + f2fs_bug_on(sbi, PagePrivate(&folio->page)); > > > > + f2fs_bug_on(sbi, page_private(&folio->page)); > > > > + > > > > return true; > > > > } > > > > -- > > > > 2.25.1 > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 4946df6dd253..8b179b4bdc03 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -3737,7 +3737,8 @@ void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length) inode->i_ino == F2FS_COMPRESS_INO(sbi)) clear_page_private_data(&folio->page); - folio_detach_private(folio); + f2fs_bug_on(sbi, PagePrivate(&folio->page)); + f2fs_bug_on(sbi, page_private(&folio->page)); } bool f2fs_release_folio(struct folio *folio, gfp_t wait) @@ -3759,7 +3760,9 @@ bool f2fs_release_folio(struct folio *folio, gfp_t wait) clear_page_private_reference(&folio->page); clear_page_private_gcing(&folio->page); - folio_detach_private(folio); + f2fs_bug_on(sbi, PagePrivate(&folio->page)); + f2fs_bug_on(sbi, page_private(&folio->page)); + return true; }
We have maintain PagePrivate and page_private and page reference w/ {set,clear}_page_private_*, it doesn't need to call folio_detach_private() in the end of .invalidate_folio and .release_folio, remove it and use f2fs_bug_on instead. Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/data.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)