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

[yangerkun <yangerkun@xxxxxxxxxx>]

在 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.