Re: [RFC] block integrity: Fix write after checksum calculation problem

[Mingming Cao <cmm@us.ibm.com>]

On Mon, 2011-04-11 at 20:57 -0400, Christoph Hellwig wrote:
> On Mon, Apr 11, 2011 at 05:46:52PM -0700, Mingming Cao wrote:
> > Oh, right. Currently ext4_page_mkwrite drops the page lock before
> > calling it's dirty the page (by write_begin() and write_end().  I
> > suspect regrab the lock() after write_end() (with your proposed change)
> > and returning with locked still leave the dirty by ext4_page_mkwrite
> > unlocked.  We probably should to keep the page locked the page during
> > the entire ext4_page_mkwrite() call. Any reason to drop the page lock()
> > before calling aops->write_begin()?
> 
> write_begin takes the page lock by itself.  That's one of the reasons why
> block_page_mkwrite doesn't use plain ->write_begin / write_end, the
> other beeing that we already get a page passed to use, so there's no
> need to do the pagecache lookup or allocation done by
> grab_cache_page_write_begin.
> 
> The best thing would be to completely drop ext4's current version
> of page_mkwrite and start out with a copy of block_page_mkwrite which
> has the journalling calls added back into it.

The problem is the locking order, we can't hold page lock then start the
journal lock. Kjournald will need to hold the journal lock first, then
commit, commit may need to callback writepages, which need to hold the
page lock.


I looked at ext3, in that case, ext3 even don't have ext3_page_mkwrite()
to do the stable page yet. It requires some block reservation/delayed
allocation for filling holes in mmaped IO case. Jan tried that before I
don't think the proposal get far.


Now looking back ext4_page_mkwrite(), it calls write_begin(), as long as
we do wait in write_begin() things would be fine, right? It seems
Darrick already did that wait per Dave Chinner's suggestion when grab
the page and lock that page. But still a puzzle to me why that's not
sufficient.


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