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

[Miaohe Lin]

On 2022/4/12 14:37, HORIGUCHI NAOYA(堀口 直也) wrote:
> 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).
> 

Will drop this patch. Sorry for make noise. :(

> 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?