On Tue 24-11-20 12:19:12, Matthew Wilcox wrote: > On Mon, Nov 23, 2020 at 08:07:24PM -0800, Hugh Dickins wrote: > > Twice now, when exercising ext4 looped on shmem huge pages, I have crashed > > on the PF_ONLY_HEAD check inside PageWaiters(): ext4_finish_bio() calling > > end_page_writeback() calling wake_up_page() on tail of a shmem huge page, > > no longer an ext4 page at all. > > > > The problem is that PageWriteback is not accompanied by a page reference > > (as the NOTE at the end of test_clear_page_writeback() acknowledges): as > > soon as TestClearPageWriteback has been done, that page could be removed > > from page cache, freed, and reused for something else by the time that > > wake_up_page() is reached. > > > > https://lore.kernel.org/linux-mm/20200827122019.GC14765@xxxxxxxxxxxxxxxxxxxx/ > > Matthew Wilcox suggested avoiding or weakening the PageWaiters() tail > > check; but I'm paranoid about even looking at an unreferenced struct page, > > lest its memory might itself have already been reused or hotremoved (and > > wake_up_page_bit() may modify that memory with its ClearPageWaiters()). > > > > Then on crashing a second time, realized there's a stronger reason against > > that approach. If my testing just occasionally crashes on that check, > > when the page is reused for part of a compound page, wouldn't it be much > > more common for the page to get reused as an order-0 page before reaching > > wake_up_page()? And on rare occasions, might that reused page already be > > marked PageWriteback by its new user, and already be waited upon? What > > would that look like? > > > > It would look like BUG_ON(PageWriteback) after wait_on_page_writeback() > > in write_cache_pages() (though I have never seen that crash myself). > > I don't think this is it. write_cache_pages() holds a reference to the > page -- indeed, it holds the page lock! So this particular race cannot > cause the page to get recycled. I still have no good ideas what this > is :-( But does it really matter what write_cache_pages() does? I mean we start page writeback. I mean struct bio holds no reference to the page it writes. The only thing that prevents the page from being freed under bio's hands is PageWriteback bit. So when the bio is completing we do (e.g. in ext4_end_bio()), we usually walk all pages in a bio bio_for_each_segment_all() and for each page call end_page_writeback(), now once end_page_writeback() calls test_clear_page_writeback() which clears PageWriteback(), the page can get freed. And that can happen before the wake_up_page() call in end_page_writeback(). So a race will be like: CPU1 CPU2 ext4_end_bio() ... end_page_writeback(page) test_clear_page_writeback(page) free page reallocate page for something else we can even dirty & start to writeback 'page' wake_up_page(page) and we have a "spurious" wake up on 'page'. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
