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

On Mon 21-03-11 10:24:41, Chris Mason wrote:

Excerpts from Jan Kara's message of 2011-03-21 10:04:51 -0400:

quoted

On Fri 18-03-11 17:07:55, Darrick J. Wong wrote:

quoted

quoted

quoted

Ok, here's what I have so far.  I took everyone's suggestions of where to add
calls to wait_on_page_writeback, which seems to handle the multiple-write case
adequately.  Unfortunately, it is still possible to generate checksum errors by
scribbling furiously on a mmap'd region, even after adding the writeback wait
in the ext4 writepage function.  Oddly, I couldn't break btrfs with mmap by
removing its wait_for_page_writeback call, so I suspect there's a bit more
going on in btrfs than I've been able to figure out.

I wonder, is it possible for this to happen:

1. Thread A mmaps a page and tries to write to it.  ext4_page_mkwrite executes,
   but there's no ongoing writeback, so it returns without delay.
2. Thread A starts writing furiously to the page.
3. Thread B runs fsync() or something that results in the page being
   checksummed and scheduled for writeout.
4. Thread A continues to write furiously(!) on that same page before the
   controller finishes the DMA transfer.
5. Disk gets the page, which now doesn't match its checksum, and *boom*

  What happens on writepage (see mm/page-writeback.c:write_cache_pages())
is:
  lock_page(page)
  ...
  clear_page_dirty_for_io() - removes PageDirty, marks page as read-only in
    PTE
  ...
  set_page_writeback() (happens e.g. in __block_write_full_page() called
from filesystem's writepage implementation).
  unlock_page(page)

  So if you compute the checksum after set_page_writeback() is done in the
writepage() implementation (you cannot use __block_write_full_page() in
that case)

  I should add that if you are computing the checksum in the block layer
once the bio is submitted, you obviously are computing it after the page is
marked as writeback. So that should be fine...

quoted

and you call wait_on_page_writeback() in ext4_page_mkwrite()
under page lock, you should be safe. If you do all this and still see
errors, something is broken I'd say...

Looking at the ext4_page_mkwrite, it does this:

lock the page
check for holes
unlock the page
if (no_holes)
	return;

write_begin/write_end
return

So, to have page_mkwrite work, you need to wait for writeback with the
page locked in both the no holes case and after the
write_begin/write_end.  write_begin will dirty the page, so someone can
wander in and start the IO while we are still in page_mkwrite.

  Oh right, that's a good point.

This is untested and uncompiled, but it should
do the trick.

Jan, did you get rid of all the buffer head based writeback for
data=ordered in ext4?  That's my only other idea, that someone is doing
writeback directly without taking the page lock.

  Yes, ext4 shouldn't do any buffer based writeback.

  This looks good AFAICT.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR