RE: [PATCH] Fix incorrect compound page flags store

["yaoaili [么爱利]" <yaoaili@kingsoft.com>]

Does the if (PageCompound(p) && compound_head(p) != orig_head) check cover the case you mentioned?
Thanks

-----Original Message-----
From: HORIGUCHI NAOYA(堀口　直也) [mailto:naoya.horiguchi@nec.com] 
Sent: Tuesday, September 8, 2020 4:37 PM
To: Oscar Salvador <osalvador@suse.de>
Cc: yaoaili126@163.com; linux-mm@kvack.org; YANGFENG1 [杨峰] <YANGFENG1@kingsoft.com>; yaoaili [么爱利] <yaoaili@kingsoft.com>; willy@infradead.org
Subject: Re: [PATCH] Fix incorrect compound page flags store

On Tue, Sep 08, 2020 at 09:26:13AM +0200, Oscar Salvador wrote:
> On Tue, Sep 08, 2020 at 07:02:12AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> > On Mon, Sep 07, 2020 at 08:44:42PM -0700, yaoaili126@163.com wrote:
> > > From: Aili Yao <yaoaili@kingsoft.com>
> > >
> > > PageHuge(p) branch will never be true,but for compound page we need to set page_flags to correct value.
> > >
> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > Signed-off-by: Yang Feng < yangfeng1@kingsoft.com>
> > > Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
> >
> > I found that this PageHuge() check is removed and no long exists in 
> > the latest mmotm, so we don't have worry about it.
>
> I might be missing something, so bear with me.
>
> It is true that the PageHuge check is gone, but we are storing the 
> page's flags in page_flags, even if the page is a tail (e.g: part of a 
> compound page ).
> Should not we store heads' flags instead?

I think that saving ->flags of the raw error page is OK, because even in thp case we focus on the raw page instead of its head page from memory error perspective.  It's not assumed that we can contain the error thp in the thp unit and instead we always rely on thp split. (For hugetlb, we now contain the errored hugetlb in hugetlb unit, so saving its head page might be OK.)

Theoretically, it could happen that a error could be collapsed into a new thp just after passing over the following block:

  1408          if (PageTransHuge(hpage)) {
  1409                  if (try_to_split_thp_page(p, "Memory Failure") < 0) {
  1410                          action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
  1411                          return -EBUSY;
  1412                  }
  1413                  VM_BUG_ON_PAGE(!page_count(p), p);
  1414          }

So I feel that some check might be added after holding page lock to avoid that case. Or acutally, it might better that moving the above block into page lock is more better for simpler code.

Thanks,
Naoya Horiguchi

> AFAICS, hpage contains either the head of the compound page, or the 
> page itself in case it is a normal page.
>
> --
> Oscar Salvador
> SUSE L3
>