Re: [PATCH] mm/memory-failure: make sure wait for page writeback in memory_failure

[yangerkun <yangerkun@huawei.com>]

在 2021/5/11 17:34, Oscar Salvador 写道:
> On Tue, May 11, 2021 at 10:46:00AM +0200, Jan Kara wrote:
>> We definitely need to wait for writeback of these pages and the change you
>> suggest makes sense to me. I'm just not sure whether the only problem with
>> these "pages in the process of being munlocked()" cannot confuse the state
>> machinery in memory_failure() also in some other way. Also I'm not sure if
>> are really allowed to call wait_on_page_writeback() on just any page that
>> hits memory_failure() - there can be slab pages, anon pages, completely
>> unknown pages given out by page allocator to device drivers etc. That needs
>> someone more familiar with these MM details than me.
> 
> I am not really into mm/writeback stuff, but:
> 
> shake_page() a few lines before tries to identifiy the page, and
> make those sitting in lruvec real PageLRU, and then we take page's lock.
> 
> I thought that such pages (pages on writeback) are stored in the file
> LRU, and maybe the code was written with that in mind? And given that
> we are under the PageLock, such state could not have changed.

Hi,


Crash of this bug show we can clear page LRU without lock_page by follow 
stack. So this page_lock in memory_failure seems useless to prevent this 
BUG.

do_mmap->mmap_region->do_munmap->munlock_vma_pages_range->__munlock_pagevec

static void __munlock_pagevec(struct pagevec *pvec, struct zone *zone)
{
     ...
     for (i = 0; i < nr; i++) {
         struct page *page = pvec->pages[i];

         if (TestClearPageMlocked(page)) {
             if (TestClearPageLRU(page)) { <=== clear LRU flag
                 ...
             }
             ...
         }
         ...
     }
     ...
}


> 
> But if such pages are allowed to not be in the LRU (maybe they are taken
> off before initiating the writeback?), I guess the change is correct.
> Checking wait_on_page_writeback(), it seems it first checks for
> Writeback bit, and since that bit is not "shared" and only being set
> in mm/writeback code, it should be fine to call that.
> 
> But alternatively, we could also modify the check and go with:
> 
> if (!PageTransTail(p) && !PageLRU(p) && !PageWriteBack(p))
> 		goto identify_page_state;

I have no idea should we process this page with such state. But it seems 
reasonable to add some comments to clarify our change.

Thanks,
Kun.

> 
> and stating why a page under writeback might not be in the LRU, as I
> think the code assumes.
> 
> AFAUI, mm/writeback locks the page before setting the bit, and since we
> hold the lock, we could not race here.
>