On Sat, Mar 30, 2019 at 08:23:26PM -0700, Matthew Wilcox wrote: > On Sat, Mar 30, 2019 at 07:10:52AM -0700, Matthew Wilcox wrote: > > On Fri, Mar 29, 2019 at 08:04:32PM -0700, Matthew Wilcox wrote: > > > Excellent! I'm not comfortable with the rule that you have to be holding > > > the i_pages lock in order to call find_get_page() on a swap address_space. > > > How does this look to the various smart people who know far more about the > > > MM than I do? > > > > > > The idea is to ensure that if this race does happen, the page will be > > > handled the same way as a pagecache page. If __delete_from_swap_cache() > > > can be called while the page is still part of a VMA, then this patch > > > will break page_to_pgoff(). But I don't think that can happen ... ? > > > > Oh, blah, that can totally happen. reuse_swap_page() calls > > delete_from_swap_cache(). Need a new plan. > > I don't see a good solution here that doesn't involve withdrawing this > patch and starting over. Bad solutions: > > - Take the i_pages lock around each page lookup call in the swap code > (not just the one you found; there are others like mc_handle_swap_pte() > in memcontrol.c) > - Call synchronize_rcu() in __delete_from_swap_cache() > - Swap the roles of ->index and ->private for swap pages, and then don't > clear ->index when deleting a page from the swap cache > > The first two would be slow and non-scalable. The third is still prone > to a race where the page is looked up on one CPU, while another CPU > removes it from one swap file then moves it to a different location, > potentially in a different swap file. Hard to hit, but not a race we > want to introduce. > > I believe that the swap code actually never wants to see subpages. So if > we start again, introducing APIs (eg find_get_head()) which return the > head page, then convert the swap code over to use those APIs, we don't > need to solve the problem of finding the subpage of a swap page while > not holding the page lock. > > I'm obviously reluctant to withdraw the patch, but I don't see a better > option. Your testing has revealed a problem that needs a deeper solution > than just adding a fix patch. Hm. Isn't the problem with VM_BUGs themself? I mean find_subpage() produces right result (or am I wrong here?), but VM_BUGs flags it as wrong. Maybe we should relax the VM_BUGs? -- Kirill A. Shutemov
