On 2022/8/4 9:54, Yin Fengwei wrote:
> 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?
That should be fine. Thanks.
