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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20061220232401.GB44411608@melbourne.sgi.com>
Date:	Thu, 21 Dec 2006 10:24:01 +1100
From:	David Chinner <dgc@....com>
To:	Linus Torvalds <torvalds@...l.org>
Cc:	Martin Michlmayr <tbm@...ius.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Hugh Dickins <hugh@...itas.com>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Arjan van de Ven <arjan@...radead.org>,
	Andrei Popa <andrei.popa@...eo.ro>,
	Andrew Morton <akpm@...l.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Florian Weimer <fw@...eb.enyo.de>,
	Marc Haber <mh+linux-kernel@...schlus.de>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Arnd Bergmann <arnd.bergmann@...ibm.com>,
	gordonfarquharson@...il.com
Subject: Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content corruption on ext3)

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/