Peter Zijlstra wrote:
On Tue, 2006-12-19 at 15:36 +1100, Nick Piggin wrote:
plain text document attachment (fs-fix.patch)
Index: linux-2.6/fs/buffer.c
===================================================================
--- linux-2.6.orig/fs/buffer.c 2006-12-19 15:15:46.000000000 +1100
+++ linux-2.6/fs/buffer.c 2006-12-19 15:36:01.000000000 +1100
@@ -2852,7 +2852,17 @@ int try_to_free_buffers(struct page *pag
* This only applies in the rare case where try_to_free_buffers
* succeeds but the page is not freed.
*/
- clear_page_dirty(page);
+
+ /*
+ * If the page has been dirtied via the user mappings, then
+ * clean buffers does not indicate the page data is actually
+ * clean! Only clear the page dirty bit if there are no dirty
+ * ptes either.
+ *
+ * If there are dirty ptes, then the page must be uptodate, so
+ * the above concern does not apply.
+ */
+ clear_page_dirty_sync_ptes(page);
}
out:
if (buffers_to_free) {
Index: linux-2.6/include/linux/page-flags.h
===================================================================
--- linux-2.6.orig/include/linux/page-flags.h 2006-12-19 15:17:18.000000000
+1100
+++ linux-2.6/include/linux/page-flags.h 2006-12-19 15:34:24.000000000
+1100
@@ -254,6 +254,7 @@ static inline void SetPageUptodate(struc
struct page; /* forward declaration */
int test_clear_page_dirty(struct page *page);
+int test_clear_page_dirty_sync_ptes(struct page *page);
int test_clear_page_writeback(struct page *page);
int test_set_page_writeback(struct page *page);
@@ -262,6 +263,11 @@ static inline void clear_page_dirty(stru
test_clear_page_dirty(page);
}
+static inline void clear_page_dirty_sync_ptes(struct page *page)
+{
+ test_clear_page_dirty_sync_ptes(page);
+}
+
static inline void set_page_writeback(struct page *page)
{
test_set_page_writeback(page);
Index: linux-2.6/mm/page-writeback.c
===================================================================
--- linux-2.6.orig/mm/page-writeback.c 2006-12-19 15:17:53.000000000 +1100
+++ linux-2.6/mm/page-writeback.c 2006-12-19 15:33:29.000000000 +1100
@@ -844,9 +844,10 @@ EXPORT_SYMBOL(set_page_dirty_lock);
/*
* Clear a page's dirty flag, while caring for dirty memory accounting.
+ * Does not clear pte dirty bits.
* Returns true if the page was previously dirty.
*/
-int test_clear_page_dirty(struct page *page)
+static int test_clear_page_dirty_leave_ptes(struct page *page)
{
struct address_space *mapping = page_mapping(page);
unsigned long flags;
@@ -862,10 +863,8 @@ int test_clear_page_dirty(struct page *p
* We can continue to use `mapping' here because the
* page is locked, which pins the address_space
*/
- if (mapping_cap_account_dirty(mapping)) {
- page_mkclean(page);
+ if (mapping_cap_account_dirty(mapping))
dec_zone_page_state(page, NR_FILE_DIRTY);
- }
return 1;
}
write_unlock_irqrestore(&mapping->tree_lock, flags);
@@ -873,9 +872,43 @@ int test_clear_page_dirty(struct page *p
}
return TestClearPageDirty(page);
}
+
+/*
+ * As above, but does clear dirty bits from ptes
+ */
+int test_clear_page_dirty(struct page *page)
+{
+ struct address_space *mapping = page_mapping(page);
+
+ if (test_clear_page_dirty_leave_ptes(page)) {
+ if (mapping_cap_account_dirty(mapping))
+ page_mkclean(page);
+ return 1;
+ }
+ return 0;
+}
EXPORT_SYMBOL(test_clear_page_dirty);
/*
+ * As above, but redirties page if any dirty ptes are found (and then only
+ * if the mapping accounts dirty pages, otherwise dirty ptes are left dirty
+ * but the page is cleaned).
+ */
+int test_clear_page_dirty_sync_ptes(struct page *page)
+{
+ struct address_space *mapping = page_mapping(page);
+
+ if (test_clear_page_dirty_leave_ptes(page)) {
+ if (mapping_cap_account_dirty(mapping)) {
+ if (page_mkclean(page))
+ set_page_dirty(page);
+ }
+ return 1;
+ }
+ return 0;
+}
+
+/*
* Clear a page's dirty flag, while caring for dirty memory accounting.
* Returns true if the page was previously dirty.
*
Hmm, not quite; It certainly look better than the extra ,[01] tagged to
test_clear_page_dirty() though. Although I would have expected it the
other way around - test_clear_pages_dirty_sync_ptes to be the default
case and test_clear_pages_dirty_clean_ptes to be used in
clear_page_dirty_for_io().
Anyway it has the same issues as the others. See what happens when you
run two test_clear_page_dirty_sync_ptes() consecutively, you still loose
PG_dirty even though the page might actually be dirty.
How can this happen? We'll only test_clear_page_dirty_sync_ptes again
after buffers have been reattached, and subsequently cleaned. And in
that case if the ptes are still clean at this point then the page really
is clean.
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
-
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/
