truncate vs add_to_page_cache race

diff -puN mm/filemap.c~truncate-vs-add_to_page_cache mm/filemap.c
@@ -78,6 +78,9 @@
  *  ->i_sem
  *    ->i_alloc_sem             (various)
  *
+ *  ->i_lock			(do_generic_mapping_read)
+ *    ->mapping->tree_lock	(add_to_page_cache)
+ *
  *  ->inode_lock
  *    ->sb_lock			(fs/fs-writeback.c)
  *    ->mapping->tree_lock	(__sync_single_inode)

@@ -649,8 +652,9 @@ void do_generic_mapping_read(struct addr
 			     read_actor_t actor)
 {
 	struct inode *inode = mapping->host;
-	unsigned long index, offset;
+	unsigned long index, end_index, offset;
 	struct page *cached_page;
+	loff_t isize;
 	int error;
 	struct file_ra_state ra = *_ra;
 

@@ -658,26 +662,28 @@ void do_generic_mapping_read(struct addr
 	index = *ppos >> PAGE_CACHE_SHIFT;
 	offset = *ppos & ~PAGE_CACHE_MASK;
 
+	isize = i_size_read(inode);
+	end_index = isize >> PAGE_CACHE_SHIFT;
+	if (index > end_index)
+		goto out;
+
 	for (;;) {
 		struct page *page;
-		unsigned long end_index, nr, ret;
-		loff_t isize = i_size_read(inode);
+		unsigned long nr, ret;
 
-		end_index = isize >> PAGE_CACHE_SHIFT;
-			
-		if (index > end_index)
-			break;
+		cond_resched();
+
+		/* Establish the number of bytes to read from this page */
 		nr = PAGE_CACHE_SIZE;
 		if (index == end_index) {
 			nr = isize & ~PAGE_CACHE_MASK;
 			if (nr <= offset)
 				break;
 		}
+		nr = nr - offset;
 
-		cond_resched();
 		page_cache_readahead(mapping, &ra, filp, index);
 
-		nr = nr - offset;
 find_page:
 		page = find_get_page(mapping, index);
 		if (unlikely(page == NULL)) {

@@ -721,9 +727,6 @@ page_ok:
 		break;
 
 page_not_up_to_date:
-		if (PageUptodate(page))
-			goto page_ok;
-
 		/* Get exclusive access to the page ... */
 		lock_page(page);
 

@@ -770,8 +773,19 @@ no_cached_page:
 				break;
 			}
 		}
+
+		/* Take the i_lock to protect against a concurrent truncate */
+		spin_lock(&inode->i_lock);
+		isize = i_size_read(inode);
+		end_index = isize >> PAGE_CACHE_SHIFT;
+		if (index > end_index) {
+			spin_unlock(&inode->i_lock);
+			goto out;
+		}
 		error = add_to_page_cache_lru(cached_page, mapping,
 						index, GFP_KERNEL);
+		spin_unlock(&inode->i_lock);
+
 		if (error) {
 			if (error == -EEXIST)
 				goto find_page;

@@ -783,6 +797,7 @@ no_cached_page:
 		goto readpage;
 	}
 
+out:
 	*_ra = ra;
 
 	*ppos = ((loff_t) index << PAGE_CACHE_SHIFT) + offset;

diff -puN mm/memory.c~truncate-vs-add_to_page_cache mm/memory.c
@@ -1222,7 +1222,16 @@ int vmtruncate(struct inode * inode, lof
 
 	if (inode->i_size < offset)
 		goto do_expand;
+	
+	/*
+	 * Need i_lock to serialise against a concurrent reader adding a new
+	 * page to the pagecache. See mm/filemap.c.
+	 *
+	 * This isn't needed if i_size is being expanded.
+	 */
+	spin_lock(&inode->i_lock);
 	i_size_write(inode, offset);
+	spin_unlock(&inode->i_lock);
 	unmap_mapping_range(mapping, offset + PAGE_SIZE - 1, 0, 1);
 	truncate_inode_pages(mapping, offset);
 	goto out_truncate;

diff -puN mm/nommu.c~truncate-vs-add_to_page_cache mm/nommu.c
@@ -48,7 +48,15 @@ int vmtruncate(struct inode *inode, loff
 
 	if (inode->i_size < offset)
 		goto do_expand;
+	/*
+	 * Need i_lock to serialise against a concurrent reader adding a new
+	 * page to the pagecache. See mm/filemap.c.
+	 *
+	 * This isn't needed if i_size is being expanded.
+	 */
+	spin_lock(&inode->i_lock);
 	i_size_write(inode, offset);
+	spin_unlock(&inode->i_lock);
 
 	truncate_inode_pages(mapping, offset);
 	goto out_truncate;

diff -puN mm/filemap.c~truncate-vs-add_to_page_cache2 mm/filemap.c
@@ -661,23 +661,11 @@ void do_generic_mapping_read(struct addr
 	for (;;) {
 		struct page *page;
 		unsigned long end_index, nr, ret;
-		loff_t isize = i_size_read(inode);
-
-		end_index = isize >> PAGE_CACHE_SHIFT;
-			
-		if (index > end_index)
-			break;
-		nr = PAGE_CACHE_SIZE;
-		if (index == end_index) {
-			nr = isize & ~PAGE_CACHE_MASK;
-			if (nr <= offset)
-				break;
-		}
+		loff_t isize;
 
 		cond_resched();
 		page_cache_readahead(mapping, &ra, filp, index);
 
-		nr = nr - offset;
 find_page:
 		page = find_get_page(mapping, index);
 		if (unlikely(page == NULL)) {

@@ -687,6 +675,30 @@ find_page:
 		if (!PageUptodate(page))
 			goto page_not_up_to_date;
 page_ok:
+		/*
+		 * i_size must be checked after we have a reference
+		 * to the uptodate page. Checking i_size before readpage
+		 * can cause a racing truncate to cause readpage to see
+		 * the truncated i_size and zero-fill the page.
+		 *
+		 * Checking i_size after the readpage allows us to calculate
+		 * the correct value for "nr", which means the zero-filled
+		 * part of the page is not copied back to userspace (unless
+		 * another truncate extends the file, this is not a problem).
+		 */
+		isize = i_size_read(inode);
+		end_index = isize >> PAGE_CACHE_SHIFT;
+		if (index > end_index)
+			break;
+
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = isize & ~PAGE_CACHE_MASK;
+			if (nr <= offset)
+				break;
+		}
+		nr = nr - offset;
+
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
 		 * before reading the page on the kernel side.

@@ -721,9 +733,6 @@ page_ok:
 		break;
 
 page_not_up_to_date:
-		if (PageUptodate(page))
-			goto page_ok;
-
 		/* Get exclusive access to the page ... */
 		lock_page(page);