Re: [PATCH] mm,hwpoison: unmap poisoned page before invalidation

[Rik van Riel <riel@surriel.com>]

[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]

On Sat, 2022-03-26 at 15:48 +0800, Miaohe Lin wrote:
> On 2022/3/26 4:14, Rik van Riel wrote:
> > 
> > +++ b/mm/memory.c
> > @@ -3918,14 +3918,18 @@ static vm_fault_t __do_fault(struct
> > vm_fault *vmf)
> >                 return ret;
> >  
> >         if (unlikely(PageHWPoison(vmf->page))) {
> > +               struct page *page = vmf->page;
> >                 vm_fault_t poisonret = VM_FAULT_HWPOISON;
> >                 if (ret & VM_FAULT_LOCKED) {
> > +                       if (page_mapped(page))
> > +                               unmap_mapping_pages(page_mapping(pa
> > ge),
> > +                                                   page->index, 1,
> > false);
> 
> It seems this unmap_mapping_pages also helps the success rate of the
> below invalidate_inode_page.
> 

That is indeed what it is supposed to do.

It isn't fool proof, since you can still end up
with dirty pages that don't get cleaned immediately,
but it seems to turn infinite loops of a program
being killed every time it's started into a more
manageable situation where the task succeeds again
pretty quickly.

> >                         /* Retry if a clean page was removed from
> > the cache. */
> > -                       if (invalidate_inode_page(vmf->page))
> > -                               poisonret = 0;
> > -                       unlock_page(vmf->page);
> > +                       if (invalidate_inode_page(page))
> > +                               poisonret = VM_FAULT_NOPAGE;
> > +                       unlock_page(page);
> >                 }
> > -               put_page(vmf->page);
> > +               put_page(page);
> 
> Do we use page instead of vmf->page just for simplicity? Or there is
> some other concern?
> 

Just a simplification, and not dereferencing the same thing
6 times.

> >                 vmf->page = NULL;
> 
> We return either VM_FAULT_NOPAGE or VM_FAULT_HWPOISON with vmf->page
> = NULL. If any case,
> finish_fault won't be called later. So I think your fix is right.

Want to send in a Reviewed-by or Acked-by? :)

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]