Re: [v3 PATCH 2/5] mm: filemap: check if THP has hwpoisoned subpage for PMD page fault

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 12, 2021 at 11:02:09AM -0700, Yang Shi wrote:
> On Mon, Oct 11, 2021 at 6:44 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 08:55:26PM -0400, Peter Xu wrote:
> > > Another thing is I noticed soft_offline_in_use_page() will still ignore file
> > > backed split.  I'm not sure whether it means we'd better also handle that case
> > > as well, so shmem thp can be split there too?
> >
> > Please ignore this paragraph - I somehow read "!PageHuge(page)" as
> > "PageAnon(page)"...  So I think patch 5 handles soft offline too.
> 
> Yes, exactly. And even though the split is failed (or file THP didn't
> get split before patch 5/5), soft offline would just return -EBUSY
> instead of calling __soft_offline_page->page_handle_poison(). So
> page_handle_poison() should not see THP at all.

I see, so I'm trying to summarize myself on what I see now with the new logic..

I think the offline code handles hwpoison differently as it sets PageHWPoison
at the end of the process, IOW if anything failed during the offline process
the hwpoison bit is not set.

That's different from how the memory failure path is handling this, as in that
case the hwpoison bit on the subpage is set firstly, e.g. before split thp.  I
believe that's also why memory failure requires the extra sub-page-hwpoison bit
while offline code shouldn't need to: because for soft offline split happens
before setting hwpoison so we just won't ever see a "poisoned file thp", while
for memory failure it could happen, and the sub-page-hwpoison will be a temp
bit anyway only exist for a very short period right after we set hwpoison on
the small page but before we split the thp.

Am I right above?

I feel like __soft_offline_page() still has some code that assumes "thp can be
there", e.g. iiuc after your change to allow file thp split, "hpage" will
always be the same as "page" then in that function, and isolate_page() does not
need to pass in a pagelist pointer too as it'll always be handling a small page
anyway.  But maybe they're fine to be there for now as they'll just work as
before, I think, so just raise it up.

-- 
Peter Xu