2022-04-08 09:07:53

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH 1/3] mm/memory-failure.c: avoid false-postive PageSwapCache test

On 07.04.22 15:03, Miaohe Lin wrote:
> PageSwapCache is only reliable when PageAnon is true because PG_swapcache
> serves as PG_owner_priv_1 which can be used by fs if it's pagecache page.
> So we should test PageAnon to distinguish pagecache page from swapcache
> page to avoid false-postive PageSwapCache test.

Well, that's not quite correct. Just because a page is PageAnon()
doesn't mean that it's in the swapache. It means that it might be in the
swapcache but cannot be in the pagecache.

Maybe you wanted to say

"So we should test PageAnon() to distinguish pagecache pages from
anonymous pages."

>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/memory-failure.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ef402b490663..2e97302d62e4 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2262,7 +2262,7 @@ static int __soft_offline_page(struct page *page)
> return 0;
> }
>
> - if (!PageHuge(page) && PageLRU(page) && !PageSwapCache(page))
> + if (!PageHuge(page) && PageLRU(page) && !PageAnon(page))
> /*
> * Try to invalidate first. This should work for
> * non dirty unmapped page cache pages.


--
Thanks,

David / dhildenb


2022-04-09 04:52:58

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH 1/3] mm/memory-failure.c: avoid false-postive PageSwapCache test

On Fri, Apr 8, 2022 at 1:52 AM David Hildenbrand <[email protected]> wrote:
>
> On 07.04.22 15:03, Miaohe Lin wrote:
> > PageSwapCache is only reliable when PageAnon is true because PG_swapcache
> > serves as PG_owner_priv_1 which can be used by fs if it's pagecache page.
> > So we should test PageAnon to distinguish pagecache page from swapcache
> > page to avoid false-postive PageSwapCache test.
>
> Well, that's not quite correct. Just because a page is PageAnon()
> doesn't mean that it's in the swapache. It means that it might be in the
> swapcache but cannot be in the pagecache.
>
> Maybe you wanted to say
>
> "So we should test PageAnon() to distinguish pagecache pages from
> anonymous pages."

Yeah, I agree. The patch looks fine to me with David's comment addressed.

>
> >
> > Signed-off-by: Miaohe Lin <[email protected]>
> > ---
> > mm/memory-failure.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index ef402b490663..2e97302d62e4 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -2262,7 +2262,7 @@ static int __soft_offline_page(struct page *page)
> > return 0;
> > }
> >
> > - if (!PageHuge(page) && PageLRU(page) && !PageSwapCache(page))
> > + if (!PageHuge(page) && PageLRU(page) && !PageAnon(page))
> > /*
> > * Try to invalidate first. This should work for
> > * non dirty unmapped page cache pages.
>
>
> --
> Thanks,
>
> David / dhildenb
>
>