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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Aug 03, 2022 at 09:32:41PM +0800, Yin, Fengwei wrote:
> On 8/3/2022 9:01 PM, 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:
> I am not aware try_to_release_page() was on remove plan. Yes. New API
> is good.

Generally, anything in folio-compat.c is on the remove schedule.
Depending on the callers, that schedule might be a few years away (eg
unlock_page() has around 700 callers).

> >  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 (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);
> Already got page folio. I suppose don't need above line.

Ah, no.  If the thp split succeeded, we need to get the new folio for
this page.

> I will re-run the test with the new folio API based patch.

Thanks!