fs: introduce some page/buffer invariants

It is a bug to set a page dirty if it is not uptodate unless it has buffers. If the page has buffers, then the page may be dirty (some buffers dirty) but not uptodate (some buffers not uptodate). The exception to this rule is if the set_page_dirty caller is racing with truncate or invalidate. A buffer can not be set dirty if it is not uptodate. If either of these situations occurs, it indicates there could be some data loss problem. Some of these warnings could be a harmless one where the page or buffer is set uptodate immediately after it is dirtied, however we should fix those up, and enforce this ordering. Bring the order of operations for truncate into line with those of invalidate. This will prevent a page from being able to go !uptodate while we're holding the tree_lock, which is probably a good thing anyway.

Nick Piggin committed 19 years ago

Commit 787d2211 parent a1ed3dd3 files · +39 −18Browse files

Unified · Split

3 files

fs/buffer.c +37 −17
mm/page-writeback.c +1 −0
mm/truncate.c +1 −1

fs/buffer.c+37−17

 }
 EXPORT_SYMBOL(mark_buffer_dirty_inode);
+/*
+ * Mark the page dirty, and set it dirty in the radix tree, and mark the inode
+ * dirty.
+ *
+ * If warn is true, then emit a warning if the page is not uptodate and has
+ * not been truncated.
+ */
+static int __set_page_dirty(struct page *page,
+		struct address_space *mapping, int warn)
+{
+	if (unlikely(!mapping))
+		return !TestSetPageDirty(page);
+	if (TestSetPageDirty(page))
+		return 0;
+	write_lock_irq(&mapping->tree_lock);
+	if (page->mapping) {	/* Race with truncate? */
+		WARN_ON_ONCE(warn && !PageUptodate(page));
+		if (mapping_cap_account_dirty(mapping)) {
+			__inc_zone_page_state(page, NR_FILE_DIRTY);
+			task_io_account_write(PAGE_CACHE_SIZE);
+		}
+		radix_tree_tag_set(&mapping->page_tree,
+				page_index(page), PAGECACHE_TAG_DIRTY);
+	}
+	write_unlock_irq(&mapping->tree_lock);
+	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+	return 1;
+}
 /*
  * Add a page to the dirty page list.
  *
  */
 int __set_page_dirty_buffers(struct page *page)
 {
-	struct address_space * const mapping = page_mapping(page);
+	struct address_space *mapping = page_mapping(page);
 	if (unlikely(!mapping))
 		return !TestSetPageDirty(page);
 	}
 	spin_unlock(&mapping->private_lock);
-	if (TestSetPageDirty(page))
+	return __set_page_dirty(page, mapping, 1);
-		return 0;
-	write_lock_irq(&mapping->tree_lock);
-	if (page->mapping) {	/* Race with truncate? */
-		if (mapping_cap_account_dirty(mapping)) {
-			__inc_zone_page_state(page, NR_FILE_DIRTY);
-			task_io_account_write(PAGE_CACHE_SIZE);
-		}
-		radix_tree_tag_set(&mapping->page_tree,
-				page_index(page), PAGECACHE_TAG_DIRTY);
-	}
-	write_unlock_irq(&mapping->tree_lock);
-	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
-	return 1;
 }
 EXPORT_SYMBOL(__set_page_dirty_buffers);
  */
 void fastcall mark_buffer_dirty(struct buffer_head *bh)
 {
+	WARN_ON_ONCE(!buffer_uptodate(bh));
 	if (!buffer_dirty(bh) && !test_set_buffer_dirty(bh))
-		__set_page_dirty_nobuffers(bh->b_page);
+		__set_page_dirty(bh->b_page, page_mapping(bh->b_page), 0);
 }
 /*

mm/page-writeback.c+1−0

@@ -824,6 +824,7 @@ int __set_page_dirty_nobuffers(struct page *page) · 823 unmodified lines
824	mapping2 = page_mapping(page);	824	mapping2 = page_mapping(page);
825	if (mapping2) { /* Race with truncate? */	825	if (mapping2) { /* Race with truncate? */
826	BUG_ON(mapping2 != mapping);	826	BUG_ON(mapping2 != mapping);
		827	WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
827	if (mapping_cap_account_dirty(mapping)) {	828	if (mapping_cap_account_dirty(mapping)) {
828	__inc_zone_page_state(page, NR_FILE_DIRTY);	829	__inc_zone_page_state(page, NR_FILE_DIRTY);
829	task_io_account_write(PAGE_CACHE_SIZE);	830	task_io_account_write(PAGE_CACHE_SIZE);

mm/truncate.c+1−1

 	if (PagePrivate(page))
 		do_invalidatepage(page, 0);
+	remove_from_page_cache(page);
 	ClearPageUptodate(page);
 	ClearPageMappedToDisk(page);
-	remove_from_page_cache(page);
 	page_cache_release(page);	/* pagecache ref */
 }