HWPOISON: shmem: call set_page_dirty() with locked page

The dirtying of page and set_page_dirty() can be moved into the page lock. - In shmem_write_end(), the page was dirtied while the page lock was held, but it's being marked dirty just after dropping the page lock. - In shmem_symlink(), both dirtying and marking can be moved into page lock. It's valuable for the hwpoison code to know whether one bad page can be dropped without losing data. It mainly judges by testing the PG_dirty bit after taking the page lock. So it becomes important that the dirtying of page and the marking of dirtiness are both done inside the page lock. Which is a common practice, but sadly not a rule. The noticeable exceptions are - mapped pages - pages with buffer_heads The above pages could go dirty at any time. Fortunately the hwpoison will unmap the page and release the buffer_heads beforehand anyway. Many other types of pages (eg. metadata pages) can also be dirtied at will by their owners, the hwpoison code cannot do meaningful things to them anyway. Only the dirtiness of pagecache pages owned by regular files are interested. v2: AK: Add comment about set_page_dirty rules (suggested by Peter Zijlstra) Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk> Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Signed-off-by: Andi Kleen <ak@linux.intel.com>

HWPOISON: shmem: call set_page_dirty() with locked page
The dirtying of page and set_page_dirty() can be moved into the page lock. - In shmem_write_end(), the page was dirtied while the page lock was held, but it's being marked dirty just after dropping the page lock. - In shmem_symlink(), both dirtying and marking can be moved into page lock. It's valuable for the hwpoison code to know whether one bad page can be dropped without losing data. It mainly judges by testing the PG_dirty bit after taking the page lock. So it becomes important that the dirtying of page and the marking of dirtiness are both done inside the page lock. Which is a common practice, but sadly not a rule. The noticeable exceptions are - mapped pages - pages with buffer_heads The above pages could go dirty at any time. Fortunately the hwpoison will unmap the page and release the buffer_heads beforehand anyway. Many other types of pages (eg. metadata pages) can also be dirtied at will by their owners, the hwpoison code cannot do meaningful things to them anyway. Only the dirtiness of pagecache pages owned by regular files are interested. v2: AK: Add comment about set_page_dirty rules (suggested by Peter Zijlstra) Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk> Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Signed-off-by: Andi Kleen <ak@linux.intel.com>
Wu Fengguang · Andi Kleen
1 parent 2571873621
Showing 2 changed files with 9 additions and 2 deletions Side-by-side Diff
mm/page-writeback.c
mm/shmem.c
@@ -1149,6 +1149,13 @@
 EXPORT_SYMBOL(redirty_page_for_writepage);
  
 /*
+ * Dirty a page.
+ *
+ * For pages with a mapping this should be done under the page lock
+ * for the benefit of asynchronous memory errors who prefer a consistent
+ * dirty state. This rule can be broken in some special cases,
+ * but should be better not to.
+ *
  * If the mapping doesn't provide a set_page_dirty a_op, then
  * just fall through and assume that it wants buffer_heads.
  */
@@ -1630,8 +1630,8 @@
 	if (pos + copied > inode->i_size)
 		i_size_write(inode, pos + copied);
  
-	unlock_page(page);
 	set_page_dirty(page);
+	unlock_page(page);
 	page_cache_release(page);
  
 	return copied;
  
@@ -1968,13 +1968,13 @@
 			iput(inode);
 			return error;
 		}
-		unlock_page(page);
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_op = &shmem_symlink_inode_operations;
 		kaddr = kmap_atomic(page, KM_USER0);
 		memcpy(kaddr, symname, len);
 		kunmap_atomic(kaddr, KM_USER0);
 		set_page_dirty(page);
+		unlock_page(page);
 		page_cache_release(page);
 	}
 	if (dir->i_mode & S_ISGID)
...	...	@@ -1149,6 +1149,13 @@
1149	1149	EXPORT_SYMBOL(redirty_page_for_writepage);
1150	1150
1151	1151	/*
	1152	+ * Dirty a page.
	1153	+ *
	1154	+ * For pages with a mapping this should be done under the page lock
	1155	+ * for the benefit of asynchronous memory errors who prefer a consistent
	1156	+ * dirty state. This rule can be broken in some special cases,
	1157	+ * but should be better not to.
	1158	+ *
1152	1159	* If the mapping doesn't provide a set_page_dirty a_op, then
1153	1160	* just fall through and assume that it wants buffer_heads.
1154	1161	*/
...	...	@@ -1630,8 +1630,8 @@
1630	1630	if (pos + copied > inode->i_size)
1631	1631	i_size_write(inode, pos + copied);
1632	1632
1633		- unlock_page(page);
1634	1633	set_page_dirty(page);
	1634	+ unlock_page(page);
1635	1635	page_cache_release(page);
1636	1636
1637	1637	return copied;
1638	1638
...	...	@@ -1968,13 +1968,13 @@
1968	1968	iput(inode);
1969	1969	return error;
1970	1970	}
1971		- unlock_page(page);
1972	1971	inode->i_mapping->a_ops = &shmem_aops;
1973	1972	inode->i_op = &shmem_symlink_inode_operations;
1974	1973	kaddr = kmap_atomic(page, KM_USER0);
1975	1974	memcpy(kaddr, symname, len);
1976	1975	kunmap_atomic(kaddr, KM_USER0);
1977	1976	set_page_dirty(page);
	1977	+ unlock_page(page);
1978	1978	page_cache_release(page);
1979	1979	}
1980	1980	if (dir->i_mode & S_ISGID)