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. 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. -- Oscar Salvador SUSE L3
