On Tue 12-08-08 13:06:39, Mingming Cao wrote:

在 2008-08-12二的 09:28 -0400，Chris Mason写道： 

quoted

On Mon, 2008-08-11 at 15:25 +0900, Hisashi Hifumi wrote:

quoted

quoted

quoted

quoted

quoted

quoted

I am wondering why we need stronger invalidate hurantees for DIO->
invalidate_inode_pages_range(),which force the page being removed from
page cache? In case of bh is busy due to ext3 writeout,
journal_try_to_free_buffers() could return different error number(EBUSY)
to try_to_releasepage() (instead of EIO).  In that case,  could we just
leave the page in the cache, clean pageuptodate() (to force later buffer
read to read from disk) and then invalidate_complete_page2() return
successfully? Any issue with this way?

My idea is that journal_try_to_free_buffers returns EBUSY if it fails due to
bh busy, and dio write falls back to buffered write. This is easy to fix.

What about the invalidates done after the DIO has already run
non-buffered?

Dio write falls back to buffered IO when writing to a hole on ext3, I 

think. I want to 

quoted

apply this mechanism to fix this issue. When try_to_release_page fails on 

a page 

quoted

due to bh busy, dio write does buffered write, sync_page_range, and 
wait_on_page_writeback, imvalidates page cache to preserve dio semantics. 
Even if page invalidation that is carried out after 

wait_on_page_writeback fails, 

quoted

there is no inconsistency between HDD and page cache.

Sorry, I'm sure I wasn't very clear, I was referencing this code from
mm/filemap.c:

       written = mapping->a_ops->direct_IO(WRITE, iocb, iov, pos, *nr_segs);

       /*
        * Finally, try again to invalidate clean pages which might have been
        * cached by non-direct readahead, or faulted in by get_user_pages()
        * if the source of the write was an mmap'ed region of the file
        * we're writing.  Either one is a pretty crazy thing to do,
        * so we don't support it 100%.  If this invalidation
        * fails, tough, the write still worked...
        */
       if (mapping->nrpages) {
               invalidate_inode_pages2_range(mapping,
                                             pos >> PAGE_CACHE_SHIFT, end);
       }

If this second invalidate fails during a DIO write, we'll have up to
date pages in cache that don't match the data on disk.  It is unlikely
to fail because the conditions that make jbd unable to free a buffer are
rare, but it can still happen with the write combination of mmap usage.

The good news is the second invalidate doesn't make O_DIRECT return
-EIO.  But, it sounds like fixing do_launder_page to always call into
the FS can fix all of these problems.  Am I missing something?

My approach is not implementing do_launder_page for ext3.
It is needed to modify VFS.

My patch is as follows:

Sorry, I'm still not sure why the do_launder_page implementation is a
bad idea.  Clearly Mingming spent quite some time on it in the past, but
given that it could provide a hook for the FS to do expensive operations
to make the page really go away, why not do it?

quoted

As far as I can tell, the only current users afs, nfs and fuse.  Pushing
down the PageDirty check to those filesystems should be trivial.

I thought about your suggestion before, there should be no problem to
push down the pagedirty check to underlying fs. 

 My concern is  even if we wait for page writeback cleared  (from
ext3_ordered_writepage() ) in the launder_page(),  (which the wait
actually already done in previous DIO ->filemap_write_wait()),
ext3_ordered_writepage()  still possibly hold the ref to the bh and
later journal_try_to_free_buffers() could still fail due to that.

  Yes, how to properly wait for writepage() to finish is a different matter
and doing it launder_page() does not help. The only thing is that in
launder_page() we can do more expensive things because it is going to be
called only before DIO, not for ordinary page freeing on memory pressure.

quoted

       ->ext3_ordered_writepage()
         walk_page_buffers() <- take a bh ref
         block_write_full_page() <- unlock_page
              : <- end_page_writeback
                : <- race! (dio write->try_to_release_page fails)

 here is the  window.

quoted

                 walk_page_buffers() <-release a bh ref

And we need someway to notify DIO code from ext3_ordered_writepage() to
indicating they are done with those buffers. That's the hard way, as Jan
mentioned.

  Well, we can always introduce something like a per-sb waitqueue where
processes waiting for references to some buffer to be released would dwell.
We would wakeup processes in this queue after writepage drops all it's
references, we could even use the same mechanism for waiting till commit
code releases those references... But returning EBUSY and falling back to
buffered writes is definitely easier to do (modulo what I wrote to Chris
about hiding possible problems).

quoted

With that said, I don't have strong feelings against falling back to
buffered IO when the invalidate fails.  

 
It seems a little odd that we have to back to buffered IO in this case.
The pages are all flushed,  DIO just want to make sure the
journaltransactions who still keep those buffers are removed from their
list. It did that, the only reason to keep DIO fail is someone else
hasn't release the bh.

Current code enforce all the buffers have to be freed and pages are
removed from page cache, in order to force later read are from disk.  I
am not sure why can't we just leave the page in the cache, just clear it
uptodate flag, without reduce the page ref count?   I think DIO should
proceed it's IO in this case...

  The problem with clearing page uptodate is described in commit
84209e02de48d72289650cc5a7ae8dd18223620f. The page may be currently in the
pipe and clearing the uptodate bit under it makes them unhappy (returning
errors or so). So either one has to change the pipe handling or we have to
cope without clearing page uptodate bit.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR
--
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