Re: [RFC PATCH] mm/memory-failure: release private data before split THP

[Yin Fengwei <fengwei.yin@intel.com>]

On 2022/8/4 09:19, Miaohe Lin wrote:
> On 2022/8/3 21:01, Matthew Wilcox wrote:
>> On Wed, Aug 03, 2022 at 10:52:43AM +0800, Yin Fengwei wrote:
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index da39ec8afca8..08e21973b120 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -1484,7 +1484,16 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>>>  
>>>  static int try_to_split_thp_page(struct page *page, const char *msg)
>>>  {
>>> +	struct page *head = compound_head(page);
>>  > +
>>>  	lock_page(page);
>>> +	/*
>>> +	 * If thp page has private data attached, thp split will fail.
>>> +	 * Release private data before split thp.
>>> +	 */
>>> +	if (page_has_private(head))
>>> +		try_to_release_page(head, GFP_KERNEL);
>>> +
>>>  	if (unlikely(split_huge_page(page))) {
>>>  		unsigned long pfn = page_to_pfn(page);
>>
>> It seems a shame to use the old page approach instead of the
>> shiny new folio approach.  We're quite close to being able to remove
>> try_to_release_page() in 6.1 or 6.2 so adding a new caller is a bad idea.
>> How about this:
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index da39ec8afca8..765b383288b1 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1484,16 +1484,21 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>>  
>>  static int try_to_split_thp_page(struct page *page, const char *msg)
>>  {
>> -	lock_page(page);
>> +	struct folio *folio = page_folio(page);
>> +
>> +	folio_lock(folio);
>> +	if (folio_test_private(folio))
>> +		filemap_release_folio(folio, GFP_KERNEL);
> 
> If filemap_release_folio fails, we could avoid trying split_huge_page here?
Maybe we could stick to this regarding this is error recovery path
instead of hot path?

> 
>>  	if (unlikely(split_huge_page(page))) {
>>  		unsigned long pfn = page_to_pfn(page);
>>  
>> -		unlock_page(page);
>> +		folio_unlock(folio);
>>  		pr_info("%s: %#lx: thp split failed\n", msg, pfn);
>> -		put_page(page);
>> +		folio_put(folio);
>>  		return -EBUSY;
>>  	}
>> -	unlock_page(page);
>> +	folio = page_folio(page);
> 
> Above line (re-fetching folio) might need a comment to avoid future confusing ?
I will add one line comment about why page_folio need here.

> 
> Anyway, this patch looks good to me. Thanks for fixing.
Thanks.

Regards
Yin, Fengwei

> 
>> +	folio_unlock(folio);
>>  
>>  	return 0;
>>  }
>>
>> .
>>
>