Re: [PATCH 6/8] mm/memory-failure: Convert memory_failure() to use a folio

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/3/11 20:31, Matthew Wilcox wrote:
> On Fri, Mar 08, 2024 at 04:48:33PM +0800, Miaohe Lin wrote:
>> On 2024/3/1 5:20, Matthew Wilcox (Oracle) wrote:
>>> @@ -2277,8 +2277,8 @@ int memory_failure(unsigned long pfn, int flags)
>>>  		}
>>>  	}
>>>  
>>> -	hpage = compound_head(p);
>>> -	if (PageTransHuge(hpage)) {
>>> +	folio = page_folio(p);
>>> +	if (folio_test_large(folio)) {
>>>  		/*
>>>  		 * The flag must be set after the refcount is bumped
>>>  		 * otherwise it may race with THP split.
> [...]
>>> @@ -2318,11 +2319,11 @@ int memory_failure(unsigned long pfn, int flags)
>>>  	 * race window. If this happens, we could try again to hopefully
>>>  	 * handle the page next round.
>>>  	 */
>>> -	if (PageCompound(p)) {
>>> +	if (folio_test_large(folio)) {
>>
>> folio_test_large() only checks whether PG_head is set but PageCompound() also checks PageTail().
>> So folio_test_large() and PageCompound() are not equivalent?
> 
> Assuming we have a refcount on this page so it can't be simultaneously
> split/freed/whatever, these three sequences are equivalent:

If page is stable after page refcnt is held, I agree below three sequences are equivalent.

> 
> 1	if (PageCompound(p))
> 
> 2	struct page *head = compound_head(p);
> 2	if (PageHead(head))
> 
> 3	struct folio *folio = page_folio(p);
> 3	if (folio_test_large(folio))
> 
> .
> 

But please see below commit:

"""
commit f37d4298aa7f8b74395aa13c728677e2ed86fdaf
Author: Andi Kleen <ak@linux.intel.com>
Date:   Wed Aug 6 16:06:49 2014 -0700

    hwpoison: fix race with changing page during offlining

    When a hwpoison page is locked it could change state due to parallel
    modifications.  The original compound page can be torn down and then
    this 4k page becomes part of a differently-size compound page is is a
    standalone regular page.

    Check after the lock if the page is still the same compound page.

    We could go back, grab the new head page and try again but it should be
    quite rare, so I thought this was safest.  A retry loop would be more
    difficult to test and may have more side effects.

    The hwpoison code by design only tries to handle cases that are
    reasonably common in workloads, as visible in page-flags.

    I'm not really that concerned about handling this (likely rare case),
    just not crashing on it.

    Signed-off-by: Andi Kleen <ak@linux.intel.com>
    Acked-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a013bc94ebbe..44c6bd201d3a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1172,6 +1172,16 @@ int memory_failure(unsigned long pfn, int trapno, int flags)

 	lock_page(hpage);

+	/*
+	 * The page could have changed compound pages during the locking.
+	 * If this happens just bail out.
+	 */
+	if (compound_head(p) != hpage) {
+		action_result(pfn, "different compound page after locking", IGNORED);
+		res = -EBUSY;
+		goto out;
+	}
+
 	/*
 	 * We use page flags to determine what action should be taken, but
 	 * the flags can be modified by the error containment action.  One

"""

It says a page could still change to a differently-size compound page due to parallel
modifications even if extra page refcnt is held and page is locked. But this commit is
early (ten years ago) things might have changed. Any thoughts?

Thanks.