diff mbox series

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

Message ID 20220407130352.15618-2-linmiaohe@huawei.com (mailing list archive)
State New
Headers show
Series A few fixup and cleanup patches for memory failure | expand

Commit Message

Miaohe Lin April 7, 2022, 1:03 p.m. UTC
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@huawei.com>
---
 mm/memory-failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Hildenbrand April 8, 2022, 8:52 a.m. UTC | #1
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 <linmiaohe@huawei.com>
> ---
>  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.
Yang Shi April 8, 2022, 5:32 p.m. UTC | #2
On Fri, Apr 8, 2022 at 1:52 AM David Hildenbrand <david@redhat.com> 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 <linmiaohe@huawei.com>
> > ---
> >  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
>
>
Miaohe Lin April 9, 2022, 2:36 a.m. UTC | #3
On 2022/4/9 1:32, Yang Shi wrote:
> On Fri, Apr 8, 2022 at 1:52 AM David Hildenbrand <david@redhat.com> 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."

That's indeed what I want to say.

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

Many thanks for both of you! Will do it in v3.

> 
>>
>>>
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> ---
>>>  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
>>
>>
> .
>
HORIGUCHI NAOYA(堀口 直也) April 11, 2022, 6:35 a.m. UTC | #4
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@huawei.com>
> ---
>  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?

Thanks,
Naoya Horiguchi

[   41.232172] page:0000000033d8a20c refcount:0 mapcount:0 mapping:00000000bc103d88 index:0x36d pfn:0x14359b
[   41.234931] memcg:ffff8c2f84d72000
[   41.235850] aops:swap_aops
[   41.236576] flags: 0x57ffffc0080415(locked|uptodate|lru|owner_priv_1|swapbacked|node=1|zone=2|lastcpupid=0x1fffff)
[   41.239221] raw: 0057ffffc0080415 ffffef2c050eda48 ffffef2c050dbe08 0000000000000000
[   41.241216] raw: 000000000000036d 000000000000036e 00000000ffffffff ffff8c2f84d72000
[   41.243184] page dumped because: VM_BUG_ON_FOLIO(folio_test_lru(folio))
[   41.244872] ------------[ cut here ]------------
[   41.246074] kernel BUG at mm/memcontrol.c:7062!
[   41.247248] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[   41.248539] CPU: 5 PID: 1036 Comm: bash Tainted: G            E     5.18.0-rc1-v5.18-rc1-220408-2310-012-gf501f+ #11
[   41.251844] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
[   41.254087] RIP: 0010:mem_cgroup_swapout+0x181/0x2c0
[   41.255399] Code: 03 0f 85 37 01 00 00 65 48 ff 08 48 83 c4 08 5b 5d 41 5c 41 5d 41 5e 41 5f e9 8b a3 e0 ff 48 c7 c6 88 b7 5a aa e8 3f 26 f7 ff <0f> 0b 48 c7 c6 d0 ec 5a aa e8 31 26 f7 ff 0f 0b 66 90 8b 45 5c 48
[   41.260408] RSP: 0018:ffffa9340218fce0 EFLAGS: 00010082
[   41.261780] RAX: 000000000000003b RBX: ffff8c2fc180a000 RCX: 0000000000000000
[   41.263604] RDX: 0000000000000002 RSI: ffffffffaa599561 RDI: 00000000ffffffff
[   41.265435] RBP: ffffef2c050d66c0 R08: 0000000000000000 R09: 00000000ffffdfff
[   41.267266] R10: ffffa9340218fad0 R11: ffffffffaa940d08 R12: 000000000000036e
[   41.269094] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8c2fc180a008
[   41.270911] FS:  00007f00259d3740(0000) GS:ffff8c30bbc80000(0000) knlGS:0000000000000000
[   41.272975] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   41.274422] CR2: 0000561d1325a973 CR3: 00000001452c2001 CR4: 0000000000170ee0
[   41.276253] Call Trace:
[   41.277013]  <TASK>
[   41.277561]  __remove_mapping+0xce/0x300
[   41.278604]  remove_mapping+0x12/0xe0
[   41.279571]  soft_offline_page+0x834/0x8b0
[   41.280972]  soft_offline_page_store+0x43/0x70
[   41.282171]  kernfs_fop_write_iter+0x11c/0x1b0
[   41.283292]  new_sync_write+0xf9/0x160
[   41.284310]  vfs_write+0x209/0x290
[   41.285174]  ksys_write+0x4f/0xc0
[   41.286049]  do_syscall_64+0x3b/0x90
[   41.286991]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   41.288328] RIP: 0033:0x7f00257018b7
[   41.289232] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[   41.294001] RSP: 002b:00007fff3dc50748 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   41.295937] RAX: ffffffffffffffda RBX: 000000000000000c RCX: 00007f00257018b7
[   41.297767] RDX: 000000000000000c RSI: 0000561d1325a970 RDI: 0000000000000001
[   41.299600] RBP: 0000561d1325a970 R08: 0000000000000000 R09: 00007f00257b64e0
[   41.301418] R10: 00007f00257b63e0 R11: 0000000000000246 R12: 000000000000000c
[   41.303305] R13: 00007f00257fb5a0 R14: 000000000000000c R15: 00007f00257fb7a0
[   41.305179]  </TASK>
Miaohe Lin April 11, 2022, 1:19 p.m. UTC | #5
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@huawei.com>
>> ---
>>  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.
Or we shouldn't even try to call invalidate_inode_page with anonymous page in principle?

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?

> Thanks,
> Naoya Horiguchi

Thanks a lot!

> 
> [   41.232172] page:0000000033d8a20c refcount:0 mapcount:0 mapping:00000000bc103d88 index:0x36d pfn:0x14359b
> [   41.234931] memcg:ffff8c2f84d72000
> [   41.235850] aops:swap_aops
> [   41.236576] flags: 0x57ffffc0080415(locked|uptodate|lru|owner_priv_1|swapbacked|node=1|zone=2|lastcpupid=0x1fffff)
> [   41.239221] raw: 0057ffffc0080415 ffffef2c050eda48 ffffef2c050dbe08 0000000000000000
> [   41.241216] raw: 000000000000036d 000000000000036e 00000000ffffffff ffff8c2f84d72000
> [   41.243184] page dumped because: VM_BUG_ON_FOLIO(folio_test_lru(folio))
> [   41.244872] ------------[ cut here ]------------
> [   41.246074] kernel BUG at mm/memcontrol.c:7062!
> [   41.247248] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> [   41.248539] CPU: 5 PID: 1036 Comm: bash Tainted: G            E     5.18.0-rc1-v5.18-rc1-220408-2310-012-gf501f+ #11
> [   41.251844] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
> [   41.254087] RIP: 0010:mem_cgroup_swapout+0x181/0x2c0
> [   41.255399] Code: 03 0f 85 37 01 00 00 65 48 ff 08 48 83 c4 08 5b 5d 41 5c 41 5d 41 5e 41 5f e9 8b a3 e0 ff 48 c7 c6 88 b7 5a aa e8 3f 26 f7 ff <0f> 0b 48 c7 c6 d0 ec 5a aa e8 31 26 f7 ff 0f 0b 66 90 8b 45 5c 48
> [   41.260408] RSP: 0018:ffffa9340218fce0 EFLAGS: 00010082
> [   41.261780] RAX: 000000000000003b RBX: ffff8c2fc180a000 RCX: 0000000000000000
> [   41.263604] RDX: 0000000000000002 RSI: ffffffffaa599561 RDI: 00000000ffffffff
> [   41.265435] RBP: ffffef2c050d66c0 R08: 0000000000000000 R09: 00000000ffffdfff
> [   41.267266] R10: ffffa9340218fad0 R11: ffffffffaa940d08 R12: 000000000000036e
> [   41.269094] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8c2fc180a008
> [   41.270911] FS:  00007f00259d3740(0000) GS:ffff8c30bbc80000(0000) knlGS:0000000000000000
> [   41.272975] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   41.274422] CR2: 0000561d1325a973 CR3: 00000001452c2001 CR4: 0000000000170ee0
> [   41.276253] Call Trace:
> [   41.277013]  <TASK>
> [   41.277561]  __remove_mapping+0xce/0x300
> [   41.278604]  remove_mapping+0x12/0xe0
> [   41.279571]  soft_offline_page+0x834/0x8b0
> [   41.280972]  soft_offline_page_store+0x43/0x70
> [   41.282171]  kernfs_fop_write_iter+0x11c/0x1b0
> [   41.283292]  new_sync_write+0xf9/0x160
> [   41.284310]  vfs_write+0x209/0x290
> [   41.285174]  ksys_write+0x4f/0xc0
> [   41.286049]  do_syscall_64+0x3b/0x90
> [   41.286991]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   41.288328] RIP: 0033:0x7f00257018b7
> [   41.289232] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> [   41.294001] RSP: 002b:00007fff3dc50748 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [   41.295937] RAX: ffffffffffffffda RBX: 000000000000000c RCX: 00007f00257018b7
> [   41.297767] RDX: 000000000000000c RSI: 0000561d1325a970 RDI: 0000000000000001
> [   41.299600] RBP: 0000561d1325a970 R08: 0000000000000000 R09: 00007f00257b64e0
> [   41.301418] R10: 00007f00257b63e0 R11: 0000000000000246 R12: 000000000000000c
> [   41.303305] R13: 00007f00257fb5a0 R14: 000000000000000c R15: 00007f00257fb7a0
> [   41.305179]  </TASK>
>
HORIGUCHI NAOYA(堀口 直也) April 12, 2022, 6:37 a.m. UTC | #6
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@huawei.com>
> >> ---
> >>  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?
Miaohe Lin April 12, 2022, 8:57 a.m. UTC | #7
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@huawei.com>
>>>> ---
>>>>  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?
diff mbox series

Patch

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.