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 <[email protected]>
---
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));
}
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/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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]/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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel