On Tue, Mar 06, 2007 at 10:36:41PM -0800, Andrew Morton wrote: > On Wed, 21 Feb 2007 05:50:05 +0100 (CET) Nick Piggin <[EMAIL PROTECTED]> > wrote: > > > Fix the race between invalidate_inode_pages and do_no_page. > > > > Andrea Arcangeli identified a subtle race between invalidation of > > pages from pagecache with userspace mappings, and do_no_page. > > > > The issue is that invalidation has to shoot down all mappings to the > > page, before it can be discarded from the pagecache. Between shooting > > down ptes to a particular page, and actually dropping the struct page > > from the pagecache, do_no_page from any process might fault on that > > page and establish a new mapping to the page just before it gets > > discarded from the pagecache. > > > > The most common case where such invalidation is used is in file > > truncation. This case was catered for by doing a sort of open-coded > > seqlock between the file's i_size, and its truncate_count. > > > > Truncation will decrease i_size, then increment truncate_count before > > unmapping userspace pages; do_no_page will read truncate_count, then > > find the page if it is within i_size, and then check truncate_count > > under the page table lock and back out and retry if it had > > subsequently been changed (ptl will serialise against unmapping, and > > ensure a potentially updated truncate_count is actually visible). > > > > Complexity and documentation issues aside, the locking protocol fails > > in the case where we would like to invalidate pagecache inside i_size. > > do_no_page can come in anytime and filemap_nopage is not aware of the > > invalidation in progress (as it is when it is outside i_size). The > > end result is that dangling (->mapping == NULL) pages that appear to > > be from a particular file may be mapped into userspace with nonsense > > data. Valid mappings to the same place will see a different page. > > > > Andrea implemented two working fixes, one using a real seqlock, > > another using a page->flags bit. He also proposed using the page lock > > in do_no_page, but that was initially considered too heavyweight. > > However, it is not a global or per-file lock, and the page cacheline > > is modified in do_no_page to increment _count and _mapcount anyway, so > > a further modification should not be a large performance hit. > > Scalability is not an issue. > > > > This patch implements this latter approach. ->nopage implementations > > return with the page locked if it is possible for their underlying > > file to be invalidated (in that case, they must set a special vm_flags > > bit to indicate so). do_no_page only unlocks the page after setting > > up the mapping completely. invalidation is excluded because it holds > > the page lock during invalidation of each page (and ensures that the > > page is not mapped while holding the lock). > > > > This also allows significant simplifications in do_no_page, because > > we have the page locked in the right place in the pagecache from the > > start. > > > > Why was truncate_inode_pages_range() altered to unmap the page if it got > mapped again? > > Oh. Because the unmap_mapping_range() call got removed from vmtruncate(). > Why? (Please send suitable updates to the changelog).
We have to ensure it is unmapped, and be prepared to unmap it while under the page lock. > I guess truncate of a mmapped area isn't sufficiently common to worry about > the inefficiency of this change. Yeah, and it should be more efficient for files that aren't mmapped, because we don't have to take i_mmap_lock for them. > Lots of memory barriers got removed in memory.c, unchangeloggedly. Yeah they were all for the lockless truncate_count checks. Now that we use the page lock, we don't need barriers. > Gratuitous renaming of locals in do_no_page() makes the change hard to > review. Should have been a separate patch. > > In fact, the patch would have been heaps clearer if that renaming had been > a separate patch. Shall I? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/