linux-ext4 - Re: [PATCH v3] ext4: Don't set PageUptodate in ext4_end

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: <20110426121938.GD9486@thunk.org>
Date:	Tue, 26 Apr 2011 08:19:38 -0400
From:	Ted Ts'o <tytso@....edu>
To:	Yongqiang Yang <xiaoqiangnk@...il.com>
Cc:	Andreas Dilger <adilger@...ger.ca>,
	Curt Wohlgemuth <curtw@...gle.com>, linux-ext4@...r.kernel.org,
	jim@...ering.net, cmm@...ibm.com, hughd@...gle.com
Subject: Re: [PATCH v3] ext4: Don't set PageUptodate in ext4_end_bio()

On Tue, Apr 26, 2011 at 03:41:47PM +0800, Yongqiang Yang wrote:
> This function is only called from write path which flushes pages to
> disk,  actually, pages' state have been set right at time when
> write_end() is called.  Why did we handle pages' state in this
> function?

This was a bug on my part.  A lot of page_io.c was copied from
fs/buffer.c, and then optimized for bulk page writes, instead of
sending down a separate 4k bio write request at a time.  I
(incorrectly) copied logic from block_commit_write() into the write
completion callback().  So I agree with Curt that the messing with the
page state should be removed from ext4_end_bio().

As far as messing with the buffer_dirty() logic, it's just wrong to be
clearing the buffer dirty bit at all in the write callback.  In the
original codepath, the buffer dirty bit was set by
block_commit_write(), only to be cleared immediately by
__block_write_full_page().  I'm pretty sure the setting and clearing
the buffer dirty bit can be optimized out of fs/ext4/page_io.c

What does bother me a little bit is that we have code that tests
buffer_dirty() in the delalloc logic in fs/ex4/inode.c --- note
ext4_bh_delay_on_unwritten(), and write_cache_pages_da(), where we are
testing the buffer_dirty bit for the pages that hang off of struct
page.  The only thing that might bear on this is the
ext4_block_truncate_page(), which does call mark_buffer_dirty() on the
buffer containing the truncation point (which by the way has made me
very worried about the punch code getting very well tested on 1k block
size file systems, since the edge cases when the region to be
truncated are not page-aligned are obviously going to be very hard to
get right).

I *think* we should be nuking the buffer_dirty manipulations in the
delalloc path, but we need to look very closely at that to make sure
there isn't anything going on here...

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html