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  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]
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

Powered by blists - more mailing lists