Re: [RFC, PATCHv2 0/2] mm: map few pages around fault address if they are in page cache

[Matthew Wilcox <willy@linux.intel.com>]

On Tue, Feb 18, 2014 at 10:02:26AM -0800, Linus Torvalds wrote:
> On Tue, Feb 18, 2014 at 6:15 AM, Wilcox, Matthew R
> <matthew.r.wilcox@intel.com> wrote:
> > We don't really need to lock all the pages being returned to protect
> > against truncate.  We only need to lock the one at the highest index,
> > and check i_size while that lock is held since truncate_inode_pages_range()
> > will block on any page that is locked.
> >
> > We're still vulnerable to holepunches, but there's no locking currently
> > between holepunches and truncate, so we're no worse off now.
> 
> It's not "holepunches and truncate", it's "holepunches and page
> mapping", and I do think we currently serialize the two - the whole
> "check page->mapping still being non-NULL" before mapping it while
> having the page locked does that.

Yes, I did mean "holepunches and page faults".  But here's the race I see:

Process A			Process B
ext4_fallocate()
ext4_punch_hole()
filemap_write_and_wait_range()
mutex_lock(&inode->i_mutex);
truncate_pagecache_range()
unmap_mapping_range()
				__do_fault()
				filemap_fault()
				lock_page_or_retry()
				(page->mapping == mapping at this point)
				set_pte_at()
				unlock_page()
truncate_inode_pages_range()
(now the pte is pointing at a page that
 is no longer attached to this file)
mutex_unlock(&inode->i_mutex);

Would we solve the problem by putting in a second call to
unmap_mapping_range() after calling truncate_inode_pages_range() in
truncate_pagecache_range(), like truncate_pagecache() does?

> Besides, that per-page locking should serialize against truncate too.
> No, there is no "global" serialization, but there *is* exactly that
> page-level serialization where both truncation and hole punching end
> up making sure that the page no longer exists in the page cache and
> isn't mapped.

What I'm suggesting is going back to Kirill's earlier patch, but only
locking the page with the highest index instead of all of the pages.
truncate() will block on that page and then we'll notice that some or
all of the other pages are also now past i_size and give up.