Re: [PATCH 2.6.3-rc2-mm1] __block_write_full patch

[Andrew Morton <akpm@osdl.org>]

Daniel McNeil <daniel@osdl.org> wrote:
>
> Here is my original __block_write_full_page patch which adds
> a wait_on_buffer() to catch the case where i/o might be in flight
> from ll_rw_block().

We don't want to be doing this.

Also, I don't buy the original rationale for the patch.  Sure,
__block_write_full_page() clears PG_writeback.  But that's OK because that
function was also responsible for setting it, and everything is under the
page lock anyway.

My suspicion is that the real problem is that mpage_writepages() moved the
page onto mapping->locked_pages while there is buffer-level I/O in flight
(that's OK).  But PG_writeback is not set because writepage never started
any I/O.  So filemap_fdatawait() never waits for the ext3-initiated
buffer-level I/O.

If so, there are several ways to fix this:

a) Change ext3 so that it appropriately sets and clears page_writeback
   when any of the page's buffers are under writeout (messy).  Or

b) Change filemap_fdatawait() so that it also waits on buffer-level I/O.

   This is tricky because filemap_fdatawait() isn't allowed to assume
   that page->private points at buffer_heads.  Only the address_space
   implementation knows what is at page->private.  So it will need to be
   something like:

	lock_page(page);
	wait_on_page_writeback(page);
	mapping = page->mapping;
	if (mapping) {
		if (mapping->aops->wait_on_private_writeback)
			mapping->aops->wait_on_private_writeback(page);
	}
	unlock_page(page);


	ext3_wait_on_private_writeback(struct page *page)
	{
		for (the buffers)
			wait_on_buffer()
	}

  or

c) Change __block_write_full_page() to move the page back onto
   mapping->dirty_pages if it was WB_SYNC_NONE and we discovered that the
   page had a locked buffer.  This way, a subsequent WB_SYNC_ALL will
   correctly wait on that buffer.

Try c), please?