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

["HORIGUCHI NAOYA(堀口 直也)" <naoya.horiguchi@nec.com>]

On Tue, May 11, 2021 at 11:34:07AM +0200, Oscar Salvador wrote:
> 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.

As long as I checked through all call sites of PG_writeback's setter APIs
(set_page_writeback() and its variant), PG_writeback is set only during
IO operation, and that's as stated in Documentation/filesystems/locking.rst.
So if we can assume that this flag is set only for a short time, calling
wait_on_page_writeback() even for non-LRU pages seems OK to 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.
> 
> 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;
> 
> 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.

Yes, this approach seems more possible to me, because of less assumption
on PG_writeback users.

Thanks,
Naoya Horiguchi