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; >> } >> >> . >> >