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
