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

From: yangerkun <yangerkun@huawei.com>
To: Oscar Salvador <osalvador@suse.de>, Jan Kara <jack@suse.cz>,
	<naoya.horiguchi@nec.com>
Cc: <naoya.horiguchi@nec.com>, <akpm@linux-foundation.org>,
	<viro@zeniv.linux.org.uk>, <tytso@mit.edu>, <linux-mm@kvack.org>,
	<linux-fsdevel@vger.kernel.org>, <yi.zhang@huawei.com>,
	<yukuai3@huawei.com>, <houtao1@huawei.com>, <yebin10@huawei.com>
Subject: Re: [PATCH] mm/memory-failure: make sure wait for page writeback in memory_failure
Date: Mon, 17 May 2021 14:48:46 +0800	[thread overview]
Message-ID: <4803a723-666f-c710-3ad4-2579390e4a9d@huawei.com> (raw)
In-Reply-To: <YJpPj3dGxk4TFL4b@localhost.localdomain>

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