On Tue, 24 Nov 2020, Linus Torvalds wrote: > On Tue, Nov 24, 2020 at 12:16 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > So my s/if/while/ suggestion is wrong and we need to do something to > > prevent spurious wakeups. Unless we bury the spurious wakeup logic > > inside wait_on_page_writeback() ... > > We can certainly make the "if()" in that loop be a "while()'. > > That's basically what the old code did - simply by virtue of the > wakeup not happening if the writeback bit was set in > wake_page_function(): > > if (test_bit(key->bit_nr, &key->page->flags)) > return -1; > > of course, the race was still there - because the writeback bit might > be clear at that point, but another CPU would reallocate and dirty it, > and then autoremove_wake_function() would happen anyway. > > But back in the bad old days, the wait_on_page_bit_common() code would > then double-check in a loop, so it would catch that case, re-insert > itself on the wait queue, and try again. Except for the DROP case, > which isn't used by writeback. > > Anyway, making that "if()" be a "while()" in wait_on_page_writeback() > would basically re-introduce that old behavior. I don't really care, > because it was the lock bit that really mattered, the writeback bit is > not really all that interesting (except from a "let's fix this bug" > angle) > > I'm not 100% sure I like the fragility of this writeback thing. > > Anyway, I'm certainly happy with either model, whether it be an added > while() in wait_on_page_writeback(), or it be the page reference count > in end_page_writeback(). > > Strong opinions? Responding to "Strong opinions?" before having digested Matthew's DMA sequence (no, not his DNA sequence). I think it comes down to whether my paranoia (about accessing an unreferenced struct page) is realistic or not: since I do hold that paranoia, I do prefer (whatever variant of) my patch. I'm not a memory hotremove guy. I did search mm/memory_hotplug.c for references to rcu or stop_machine(), but found none. I can imagine that the memory containing the struct pages would be located elsewhere than the memory itself, with some strong barrier in between removals; but think there were patches posted just a few days ago, with intent to allocate struct pages from the same memory block. It would be easy to forget this writeback issue when hotremove advances, if we don't fix it properly now. Another problem with the s/if/while/ solution: I think Matthew pointed to another patch needed, to prevent wake_up_page_bit() from doing an inappropriate ClearPageWaiters (I've not studied that patch); and would also need a further patch to deal with my PF_ONLY_HEAD VM_BUG_ON(PageTail). More? I think the unreferenced struct page asks for trouble. Hugh
