Re: page cache: Store only head pages in i_pages

["Kirill A. Shutemov" <kirill@shutemov.name>]

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