Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content corruption on ext3)

[David Chinner]

On Wed, Dec 20, 2006 at 11:50:50AM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 20 Dec 2006, Linus Torvalds wrote:
> > 
> > So that's why I've been harping on the fact that I think we simply do 
> > really wrong things with PG_dirty at times [ ... ]
> 
> Ok, I'll just put my money where my mouth is, and suggest a patch like 
> THIS instead.
> 
> This one clears up all the issues I find irritating:
> 
>  - "test_clear_page_dirty()" is insane, both conceptually and as an 
>    implementation. "Give me a 'C', give me an 'R', give me an 'A', give me 
>    a 'P'".
> 
>    So rip out that mindfart entirely.
> 
>  - "clear_page_dirty()" is badly named, and should be about CANCELLING the 
>    dirty bit, and must never be called with pages mapped anyway. So throw 
>    that out too, and replace it with a new function:
> 
> 	void cancel_dirty_page(struct page *page, unsigned int accounting_size);
> 
>  - "clear_page_dirty_for_io()" is fine.
> 
> And with that, I then either rip out any old users of 
> "test_clear_page_dirty()" or "clear_page_dirty()", and if appropriate (and 
> it's realy lonly appropriate for "truncate()", I replace them with the new 
> "cancel_dirty_page()". Most of the time, they should just be deleted 
> entirely.
> 
> NOTE NOTE NOTE! I _only_ did enough to make things compile for my 
> particular configuration. That means that right now the following 
> filesystems are broken with this patch (because they use the totally 
> broken old crap):
> 
> 	CIFS, FUSE, JFS, ReiserFS, XFS


XFS appears to call clear_page_dirty to get the mapping tree dirty
tag set correctly at the same time the page dirty flag is cleared. I
note that this can be done by set_page_writeback() if we clear the
dirty flag on the page first when we are writing back the entire page.

Hence it seems to me that the XFS call to clear_page_dirty() could
easily be substituted by clear_page_dirty_for_io() followed by a
call to set_page_writeback() to get the mapping tree tags set
correctly after the page has been marked clean.

Does this make sense (even without the posted patch)?

---
 fs/xfs/linux-2.6/xfs_aops.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c	2006-12-19 12:22:47.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c	2006-12-21 10:15:04.545375877 +1100
@@ -340,9 +340,9 @@ xfs_start_page_writeback(
 {
 	ASSERT(PageLocked(page));
 	ASSERT(!PageWriteback(page));
-	set_page_writeback(page);
 	if (clear_dirty)
-		clear_page_dirty(page);
+		clear_page_dirty_for_io(page);
+	set_page_writeback(page);
 	unlock_page(page);
 	if (!buffers) {
 		end_page_writeback(page);

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/