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

[HORIGUCHI NAOYA(堀口 直也)]

On Mon, Apr 11, 2022 at 09:19:26PM +0800, Miaohe Lin wrote:
> On 2022/4/11 14:35, HORIGUCHI NAOYA(堀口 直也) wrote:
> > On Thu, Apr 07, 2022 at 09:03:50PM +0800, 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.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> >> ---
> >>  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.
> >> -- 
> > 
> > I foudn that with this change the following VM_BUG_ON_FOLIO() is triggered
> > when calling soft-offline for a swapcache.  Maybe we need check both of
> > PageAnon and PageSwapCache instead of either?
> > 
> 
> Many thanks for your test! This is my overlook. Sorry about it! :( The root cause is that the page is
> added into swapcache and lru( so that it can pass the HWPoisonHandlable check) but page anon is not
> set yet due to page lock is held by __soft_offline_page. So we have the below core dump:
> 
> [   41.232172] page:0000000033d8a20c refcount:0 mapcount:0 mapping:00000000bc103d88 index:0x36d pfn:0x14359b
> 										^^^ page is not anon
> 
> [   41.236576] flags: 0x57ffffc0080415(locked|uptodate|lru|owner_priv_1|swapbacked|node=1|zone=2|lastcpupid=0x1fffff)
> 								^^^^^^^^^^^^^^^^^^ page is in swapcache
> 
> It seems we can check !PageAnon(page) && !PageSwapCache(page), as you suggested, to fix this issue. But maybe I
> should drop this patch because invalidate_inode_page will always return 0 for PageAnon due to folio_mapping == NULL.
> So nothing is really done for anonymous page here. And the origin !PageSwapCache(page) check should do the right work.

Thanks for clarification.

> Or we shouldn't even try to call invalidate_inode_page with anonymous page in principle?

I think just keeping the current behavior is fine (because as you stated
above invalidate_inode_page() simple ignores anonymous pages).

Thanks,
Naoya Horiguchi

> BTW: PageSwapCache should be reliable here as folio_test_swapbacked is checked implicitly inside it. In such case, PG_swapcache
> can't serve as PG_owner_priv_1 as pagecache page shouldn't set PG_swapbacked (shmem will set PG_swapbacked but PG_owner_priv_1
> is not used anyway). Or am I miss something again?