Re: kjournald() with DIO

[Andrew Morton <akpm@osdl.org>]

Badari Pulavarty <pbadari@us.ibm.com> wrote:
>
> The buffers were attached to the journal (earlier). kjournald()
>  submitted them for IO and waiting for IO to finish. (So it has a ref.
>  count on it).
> 
> 
>  journal_commit_transaction()
>          ...
>          submited buffers for IO
>          /* Waiting for IO to complete */
>          while (commit_transaction->t_locked_list) {
>                  ...
>                  get_bh(bh);
>                  if (buffer_locked(bh)) {
>                          spin_unlock(&journal->j_list_lock);
>                          wait_on_buffer(bh);  <<<<<<
> 
> 
>  In the mean while, DIO process through releasepage invalidated the page,
>  released the journal head attached to the buffer and trying to drop the
>  buffer. But the drop buffer fails due to the kjournald ref. count.
> 
>  invalidate_complete_page():
>          ..
>          ext3_releasepage()
>                  journal_try_to_free_buffers()
>                          journal_put_journal_head()
>                                  __journal_try_to_free_buffer()
>                                          <--- freed jh
> 
>                          try_to_free_buffers()
>                                  drop_buffers()
>                                          if (buffer_busy(bh))
>                                                  goto failed;
>                                          		<<--

OK.

<checks email>

The problem is that

	__generic_file_aio_write_nolock
	->invalidate_inode_pages2_range
	  ->invalidate_complete_page

was unable to invalidate the page and so -EIO was propagated back.


Well, that was wrong of us.  invalidate_*_pages() is best-effort.  It does
not guarantee to remove the pages from pagecache or anything else.  The
truncate_*_pages() functions do have such guarantees.  They will block on
that locked buffer, for example.

 * invalidate_mapping_pages - Invalidate all the unlocked pages of one inode
 * @mapping: the address_space which holds the pages to invalidate
 * @start: the offset 'from' which to invalidate
 * @end: the offset 'to' which to invalidate (inclusive)
 *
 * This function only removes the unlocked pages, if you want to
 * remove all the pages of one inode, you must call truncate_inode_pages.
 *
 * invalidate_mapping_pages() will not block on IO activity. It will not
 * invalidate pages which are dirty, locked, under writeback or mapped into
 * pagetables.

So if we really want to take down that pagecache we'll need to call
->invalidatepage() against each page, not ->releasepage().  Yes, the naming
is whacky.  truncate_inode_pages uses ->invalidatepage and
invalidate_inode_pages uses ->releasepage().

Or simply ignore the invalidate_inode_pages2_range() return value in
generic_file_direct_IO().