lkml.org 
[lkml]   [2004]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] zap_pte_range should not mark non-uptodate pages dirty
    From
    Date
    Andrew & Andrea,
    What is the status of this patch? It would be nice to have it in the
    -mm4 kernel.

    Thanks,
    Shaggy

    On Fri, 2004-10-22 at 18:24, Andrew Morton wrote:
    > Andrea Arcangeli <andrea@novell.com> wrote:
    > >
    > > On Thu, Oct 21, 2004 at 07:03:20PM -0700, Andrew Morton wrote:
    > > > Yeah. I think the only 100% reliable way to do this is:
    > > >
    > > > lock_page()
    > > > unmap_mapping_range(page)
    > > > ClearPageUptodate(page);
    > > > invalidate(page); /* Try to free the thing */
    > > > unlock_page(page);
    > > >
    > > > (Can do it for a whole range of pages if we always agree to lock pages in
    > > > file-index-ascending order).
    > > >
    > > > but hrm, we don't even have the locking there to prevent do_no_page() from
    > > > reinstantiating the pte immediately after the unmap_mapping_range().
    > > >
    > > > So what to do?
    > > >
    > > > - The patch you originally sent has a race window which can be made
    > > > nonfatal by removing the BUGcheck in mpage_writepage().
    > > >
    > > > - NFS should probably be taught to use unmap_mapping_range() regardless
    > > > of what we do, so the existing-mmaps-hold-stale-data problem gets fixed
    > > > up.
    > > >
    > > > - invalidate_inode_pages2() isn't successfully invalidating the page if
    > > > it has buffers.
    > >
    > > I did it right in 2.4, somebody obviously broken the thing in 2.6. Now
    > > w.r.t. to ext2/ext3 I never heard a problem, and we nuke buffers
    > > regardless if the page is mapped or not (so this isn't a corner case).
    > >
    > > define block_invalidate_page(page) discard_bh_page(page, 0, 0)
    > >
    > > } else {
    > > if (page->buffers) {
    > > /* Restart after this page */
    > > list_del(head);
    > > list_add_tail(head, curr);
    > >
    > > page_cache_get(page);
    > > spin_unlock(&pagecache_lock);
    > > block_invalidate_page(page);
    > > } else
    > > unlocked = 0;
    > >
    > > ClearPageDirty(page);
    >
    > This is incorrect for ext3 - we have to go through the a_ops to invalidate
    > the buffer_heads. That's bad news for my (unmerged) 2.4 direct-io-for-ext3
    > patch.
    >
    > > The guarantee is total in 2.4 w.r.t. buffers, as worse it can generate
    > > an hearth attack to ext3 but it won't risk to lose coherency. Only 2.6
    > > broke the O_DIRECT vs buffered I/O coherency protocol.
    >
    > Only for mmapped pages. I believe that normal pagecache-vs-directIO is OK
    > in 2.6.
    >
    > > The semantics of not uptodate pages mapped in the address space are as
    > > well understood and working fine in 2.4.
    >
    > I'd prefer to continue to disallow the mapping of nonuptodate pages into
    > process address space. It's indistinguishable from a security hole and I
    > think we can indeed preserve this invariant while fixing the mmap coherency
    > problem.
    >
    > > > This is the biggest problem, because the pages can trivially have
    > > > buffers - just write()ing to them will attach buffers, if they're ext2-
    > > > or ext3-backed.
    > > >
    > > > It means that a direct-IO write to a section of a file which is mmapped
    > > > causes pagecache and disk to get out of sync. Question is: do we care,
    > > > or do we say "don't do that"? Nobody seems to have noticed thus far and
    > > > it's a pretty dopey thing to be doing.
    > >
    > > this is a supported feature. People is writing with O_DIRECT and reading
    > > with buffered read and we must support that (think a database writing
    > > and a tar.gz reading). The coherency is a must after the O_DIRECT has
    > > completed. If people runs the operations at the same time the result is
    > > undefined. But by serializing the I/O at the userspace level (like
    > > stop_db(); tar.gz) we must provide guarantees to them.
    >
    > If the page is not mapped into pagetables then we'll invalidate it OK even
    > if it's backed by ext3. The only hole I can see is if someone comes in and
    > dirties the page via write() _after_ generic_file_direct_IO() has done its
    > filemap_write_and_wait().
    >
    > And I agree that in that case we should propagate the
    > invalidate_complete_page() failure back to the diret-io write() caller. I wonder
    > how - via a short write, or via -EFOO?
    >
    > Probably we should sync all the ptes prior to a direct-io read as well.
    >
    > > > If we _do_ want to fix it, it seems the simplest approach would be to
    > > > nuke the pte's in invalidate_inode_pages2(). And we'd need some "oh we
    > > > raced - try again" loop in there to handle the "do_no_page()
    > > > reinstantiated a pte" race.
    > >
    > > the do_no_page don't need to be coherent. The pte is the last thing we
    > > can care about. The API only guarantees read/write syscalls to be
    > > coherent, mmap not.
    >
    > I want to fix the mmap case too. We currently have a half-fix for NFS
    > which doesn't work at all for (say) ext3.
    >
    > > the only bug is the bh and it has to be fixed as easily as I did in 2.4.
    >
    > No, we need to call into the filesystem to kill the buffer_heads.
    >
    > > > Something like this? Slow as a dog, but possibly correct ;)
    > > >
    > > >
    > > > void invalidate_inode_pages2(struct address_space *mapping)
    > > > {
    > > > struct pagevec pvec;
    > > > pgoff_t next = 0;
    > > > int i;
    > > >
    > > > pagevec_init(&pvec, 0);
    > > > while (pagevec_lookup(&pvec, mapping, next, PAGEVEC_SIZE)) {
    > > > for (i = 0; i < pagevec_count(&pvec); i++) {
    > > > struct page *page = pvec.pages[i];
    > > >
    > > > lock_page(page);
    > > > if (page->mapping == mapping) { /* truncate race? */
    > > > wait_on_page_writeback(page);
    > > > next = page->index + 1;
    > > > while (page_mapped(page)) {
    > > > unmap_mapping_range(mapping,
    > > > page->index << PAGE_CACHE_SHIFT,
    > > > page->index << PAGE_CACHE_SHIFT+1,
    > > > 0);
    > > > clear_page_dirty(page);
    > > > }
    > > > invalidate_complete_page(mapping, page);
    > > > }
    > > > unlock_page(page);
    > > > }
    > > > pagevec_release(&pvec);
    > > > cond_resched();
    > > > }
    > > > }
    > >
    > > You know the page_mapped path isn't really very important as far as we
    > > execute both clear_page_dirty and ClearPageUptodate.
    >
    > It's important for NFS - it will allow mmaps to see server-side updates.
    >
    > > We must provide the
    > > guarantee to the syscall. Leaving not updoate pages in the ptes is
    > > fine, the VM can deal with that. the semantics of that condition is that
    > > the page changed on disk due O_DIRECT from under us, so the page isn't
    > > uptodate anymore. If something we can argue about ext3 not being capable
    > > of dealing with a PG_update going away from under it, but the VM is
    > > capable of that, there is only one spot that must be aware about it
    > > (zap fix we posted to start the thread).
    > >
    > > BTW, we have these fixes in our tree to make it work:
    > >
    > > --- sles/mm/truncate.c.~1~ 2004-05-18 19:24:40.000000000 +0200
    > > +++ sles/mm/truncate.c 2004-05-19 02:09:28.311781864 +0200
    > > @@ -260,9 +260,10 @@ void invalidate_inode_pages2(struct addr
    > > if (page->mapping == mapping) { /* truncate race? */
    > > wait_on_page_writeback(page);
    > > next = page->index + 1;
    > > - if (page_mapped(page))
    > > + if (page_mapped(page)) {
    > > + ClearPageUptodate(page);
    > > clear_page_dirty(page);
    > > - else
    > > + } else
    > > invalidate_complete_page(mapping, page);
    > > }
    > > unlock_page(page);
    >
    > That's in 2.6.9.
    >
    > > and then this fix for BLKFLSBUF:
    > >
    > > diff -urNp linux-2.6.8/mm/truncate.c linux-2.6.8.SUSE/mm/truncate.c
    > > --- linux-2.6.8/mm/truncate.c 2004-09-21 15:39:37.850379202 +0200
    > > +++ linux-2.6.8.SUSE/mm/truncate.c 2004-09-21 15:39:54.733668309 +0200
    > > @@ -86,6 +86,12 @@ invalidate_complete_page(struct address_
    > > write_unlock_irq(&mapping->tree_lock);
    > > return 0;
    > > }
    > > +
    > > + BUG_ON(PagePrivate(page));
    > > + if (page_count(page) != 2) {
    > > + write_unlock_irq(&mapping->tree_lock);
    > > + return 0;
    > > + }
    > > __remove_from_page_cache(page);
    > > write_unlock_irq(&mapping->tree_lock);
    > > ClearPageUptodate(page);
    > > @@ -306,7 +312,11 @@ void invalidate_inode_pages2(struct addr
    > > clear_page_dirty(page);
    > > ClearPageUptodate(page);
    > > } else {
    > > - invalidate_complete_page(mapping, page);
    > > + if (!invalidate_complete_page(mapping,
    > > + page)) {
    > > + clear_page_dirty(page);
    > > + ClearPageUptodate(page);
    > > + }
    > > }
    > > }
    > > unlock_page(page);
    > >
    >
    > That's in 2.6.9 as well. I need to think about this a bit more - the first
    > hunk is for read() versus BLKFLSBUF (if someone else is playing with the
    > page, leave it alone) whereas the second hunk is kinda buggy, because it
    > will reintroduce the EIO-in-read() error which the first hunk tried to fix.
    >
    > (You're using an rwlock for tree_lock. It's still unclear to me whether we
    > should go that way - it helps big SMP and hurts small SMP. Did you perform
    > an evaluation of this?)
    >
    > >
    > > apparently those weren't submitted to mainline yet, that could have
    > > created some confusion on why we submitted the third patch that avoids
    > > the BUG in pdflush and it avoids to try writing to disk a page that is
    > > not uptodate anymore. Do you see why it's wrong to pass down to pdflush
    > > a page that isn't utpodate?
    >
    > Sure. That's why there's a BUG() there ;)
    >
    > > That page must be re-read, it sure must not
    > > be written out. The semantics of mapped pages not uptodate are very well
    > > defined and they're pose no risk to the VM (modulo the bug in pdflush
    > > and this clear improvement in zap).
    > >
    > > > The unmapping from pagetables means that invalidate_complete_page() will
    > > > generally successfully remove the page from pagecache. That mostly fixes
    > > > the direct-write-of-mmapped-data coherency problem: the pages simply aren't
    > > > in pagecache any more so we'll surely redo physical I/O.
    > >
    > > the direct-write-of-mmapped-data coherency is the last thing we can care
    > > about.
    >
    > But we can fix it.
    >
    > > Even before thinking about direct-write-of-mmapped-data we must
    > > fix direct-write-of-cached-data (assuming it's not mmapped cache). That
    > > is a showstopper bug,
    >
    > I don't think we have a bug here. After the sync(), ext3_releasepage()
    > will be able to release the buffers. But we should check for failures.
    >
    > The problem we're seeing is with mmapped pages. In that case, clearing
    > PG_uptodate is insufficient and directly invalidating the buffer_heads will
    > probably kill ext3 and we're not allowed to assume that page->private
    > contains bh's anyway...
    >
    > Probably it is sufficient to run try_to_release_page() in the page_mapped
    > leg of invalidate_inode_pages2(), and to check the return value.
    >
    > > the direct-write-of-mmapped-data is "undefined" by
    > > the linux API. Garbage can end up in the pagecache as far as the kernel
    > > is concerned (all we guarantee is that no random page contents are
    > > delivered to userspace).
    > >
    > > > But it's not 100% reliable because ->invalidatepage isn't 100% reliable and
    > > > it really sucks that we're offering behaviour which only works most of the
    > > > time :(
    > >
    > > That don't need to ever work, I don't think we even need to add a pte
    > > shootdown, the behviour has never been provided by design. All we need
    > > to do is to fix the bh layer, and fixup the spots in zap to stop bugging
    > > out on dirty not uptodate pages, plus the above patches in this email,
    > > then it'll work as well as 2.4 and it will avoid to break apps. Note
    > > that this is a very serious matter, what happens is that oracle writes
    > > with O_DIRECT and when you do a tar.gz of the database the last block
    > > may be corrupt if it's not 512byte aligned if it was a partial page and
    > > it had partial bh. mysql uses O_DIRECT too.
    >
    > The current buffered-vs-direct handling looks OK, as long as the pages
    > aren't mmapped, and as long as nobody dirties a page after
    > generic_file_direct_IO() has done filemap_write_and_wait().
    >
    > For the former I'd like to unmap the pages. For the latter we should check
    > the invalidate_complete_page() return value.
    >
    > > I'm not particularly
    > > concerned about ext3, that can be fixed too, or we can add a new bitflag
    > > as you suggested to avoid fixing the filesystems to deal with that. But
    > > for the VM I'm confortable clearing the uptodate page is much simpler
    > > and it poses no risk to the VM. For the fs you may be very well right
    > > that the BH_reread_from_disk might be needed instead.
    > >
    > > So let's forget the mmapped case and let's fix the bh screwup in 2.6.
    > > Then we can think at the mmapped case.
    >
    > The "bh screwup" _is_ the mmapped case. Maybe I said something different
    > yesterday - it's been a while since I looked in there.
    >
    > > And to fix the mmapped case IMHO all we have to do is:
    > >
    > > clear_page_dirty
    > > ClearPageUptodate
    > > unmap_page_range
    > >
    > > Not the other way around. The uptodate bit must be clared before zapping
    > > the pte to force a reload.
    >
    > Something like this...
    >
    >
    >
    >
    > - When invalidating pages, take care to shoot down any ptes which map them
    > as well.
    >
    > This ensures that the next mmap access to the page will generate a major
    > fault, so NFS's server-side modifications are picked up.
    >
    > This also allows us to call invalidate_complete_page() on all pages, so
    > filesytems such as ext3 get a chance to invalidate the buffer_heads.
    >
    > - Don't mark in-pagetable pages as non-uptodate any more. That broke a
    > previous guarantee that mapped-into-user-process pages are always uptodate.
    >
    > - Check the return value of invalidate_complete_page(). It can fail if
    > someone redirties a page after generic_file_direct_IO() write it back.
    >
    > But we still have a problem. If invalidate_inode_pages2() calls
    > unmap_mapping_range(), that can cause zap_pte_range() to dirty the pagecache
    > pages. That will redirty the page's buffers and will cause
    > invalidate_complete_page() to fail.
    >
    > So, in generic_file_direct_IO() we do a complete pte shootdown on the file
    > up-front, prior to writing back dirty pagecache. This is only done for
    > O_DIRECT writes. It _could_ be done for O_DIRECT reads too, providing full
    > mmap-vs-direct-IO coherency for both O_DIRECT reads and O_DIRECT writes, but
    > permitting the pte shootdown on O_DIRECT reads trivially allows people to nuke
    > other people's mapped pagecache.
    >
    > NFS also uses invalidate_inode_pages2() for handling server-side modification
    > notifications. But in the NFS case the clear_page_dirty() in
    > invalidate_inode_pages2() is sufficient, because NFS doesn't have to worry
    > about the "dirty buffers against a clean page" problem. (I think)
    >
    > Signed-off-by: Andrew Morton <akpm@osdl.org>
    > ---
    >
    > 25-akpm/include/linux/fs.h | 2 -
    > 25-akpm/mm/filemap.c | 19 ++++++++++---
    > 25-akpm/mm/truncate.c | 63 +++++++++++++++++++++++++++------------------
    > 3 files changed, 55 insertions(+), 29 deletions(-)
    >
    > diff -puN mm/truncate.c~invalidate_inode_pages-mmap-coherency-fix mm/truncate.c
    > --- 25/mm/truncate.c~invalidate_inode_pages-mmap-coherency-fix Fri Oct 22 15:40:26 2004
    > +++ 25-akpm/mm/truncate.c Fri Oct 22 16:05:26 2004
    > @@ -65,6 +65,8 @@ truncate_complete_page(struct address_sp
    > * be marked dirty at any time too. So we re-check the dirtiness inside
    > * ->tree_lock. That provides exclusion against the __set_page_dirty
    > * functions.
    > + *
    > + * Returns non-zero if the page was successfully invalidated.
    > */
    > static int
    > invalidate_complete_page(struct address_space *mapping, struct page *page)
    > @@ -281,50 +283,63 @@ unsigned long invalidate_inode_pages(str
    > EXPORT_SYMBOL(invalidate_inode_pages);
    >
    > /**
    > - * invalidate_inode_pages2 - remove all unmapped pages from an address_space
    > + * invalidate_inode_pages2 - remove all pages from an address_space
    > * @mapping - the address_space
    > *
    > - * invalidate_inode_pages2() is like truncate_inode_pages(), except for the case
    > - * where the page is seen to be mapped into process pagetables. In that case,
    > - * the page is marked clean but is left attached to its address_space.
    > - *
    > - * The page is also marked not uptodate so that a subsequent pagefault will
    > - * perform I/O to bringthe page's contents back into sync with its backing
    > - * store.
    > + * Any pages which are found to be mapped into pagetables are unmapped prior to
    > + * invalidation.
    > *
    > - * FIXME: invalidate_inode_pages2() is probably trivially livelockable.
    > + * Returns -EIO if any pages could not be invalidated.
    > */
    > -void invalidate_inode_pages2(struct address_space *mapping)
    > +int invalidate_inode_pages2(struct address_space *mapping)
    > {
    > struct pagevec pvec;
    > pgoff_t next = 0;
    > int i;
    > + int ret = 0;
    > + int did_full_unmap = 0;
    >
    > pagevec_init(&pvec, 0);
    > - while (pagevec_lookup(&pvec, mapping, next, PAGEVEC_SIZE)) {
    > - for (i = 0; i < pagevec_count(&pvec); i++) {
    > + while (!ret && pagevec_lookup(&pvec, mapping, next, PAGEVEC_SIZE)) {
    > + for (i = 0; !ret && i < pagevec_count(&pvec); i++) {
    > struct page *page = pvec.pages[i];
    >
    > lock_page(page);
    > - if (page->mapping == mapping) { /* truncate race? */
    > - wait_on_page_writeback(page);
    > - next = page->index + 1;
    > - if (page_mapped(page)) {
    > - clear_page_dirty(page);
    > - ClearPageUptodate(page);
    > + if (page->mapping != mapping) { /* truncate race? */
    > + unlock_page(page);
    > + continue;
    > + }
    > + wait_on_page_writeback(page);
    > + next = page->index + 1;
    > + while (page_mapped(page)) {
    > + if (!did_full_unmap) {
    > + /*
    > + * Zap the rest of the file in one hit.
    > + * FIXME: invalidate_inode_pages2()
    > + * should take start/end offsets.
    > + */
    > + unmap_mapping_range(mapping,
    > + page->index << PAGE_CACHE_SHIFT,
    > + -1, 0);
    > + did_full_unmap = 1;
    > } else {
    > - if (!invalidate_complete_page(mapping,
    > - page)) {
    > - clear_page_dirty(page);
    > - ClearPageUptodate(page);
    > - }
    > + /*
    > + * Just zap this page
    > + */
    > + unmap_mapping_range(mapping,
    > + page->index << PAGE_CACHE_SHIFT,
    > + (page->index << PAGE_CACHE_SHIFT)+1,
    > + 0);
    > }
    > }
    > + clear_page_dirty(page);
    > + if (!invalidate_complete_page(mapping, page))
    > + ret = -EIO;
    > unlock_page(page);
    > }
    > pagevec_release(&pvec);
    > cond_resched();
    > }
    > + return ret;
    > }
    > -
    > EXPORT_SYMBOL_GPL(invalidate_inode_pages2);
    > diff -puN include/linux/fs.h~invalidate_inode_pages-mmap-coherency-fix include/linux/fs.h
    > --- 25/include/linux/fs.h~invalidate_inode_pages-mmap-coherency-fix Fri Oct 22 15:54:50 2004
    > +++ 25-akpm/include/linux/fs.h Fri Oct 22 15:54:58 2004
    > @@ -1404,7 +1404,7 @@ static inline void invalidate_remote_ino
    > S_ISLNK(inode->i_mode))
    > invalidate_inode_pages(inode->i_mapping);
    > }
    > -extern void invalidate_inode_pages2(struct address_space *mapping);
    > +extern int invalidate_inode_pages2(struct address_space *mapping);
    > extern void write_inode_now(struct inode *, int);
    > extern int filemap_fdatawrite(struct address_space *);
    > extern int filemap_flush(struct address_space *);
    > diff -puN mm/filemap.c~invalidate_inode_pages-mmap-coherency-fix mm/filemap.c
    > --- 25/mm/filemap.c~invalidate_inode_pages-mmap-coherency-fix Fri Oct 22 15:55:06 2004
    > +++ 25-akpm/mm/filemap.c Fri Oct 22 16:17:19 2004
    > @@ -2214,7 +2214,8 @@ ssize_t generic_file_writev(struct file
    > EXPORT_SYMBOL(generic_file_writev);
    >
    > /*
    > - * Called under i_sem for writes to S_ISREG files
    > + * Called under i_sem for writes to S_ISREG files. Returns -EIO if something
    > + * went wrong during pagecache shootdown.
    > */
    > ssize_t
    > generic_file_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
    > @@ -2224,14 +2225,24 @@ generic_file_direct_IO(int rw, struct ki
    > struct address_space *mapping = file->f_mapping;
    > ssize_t retval;
    >
    > + /*
    > + * If it's a write, unmap all mmappings of the file up-front. This
    > + * will cause any pte dirty bits to be propagated into the pageframes
    > + * for the subsequent filemap_write_and_wait().
    > + */
    > + if (rw == WRITE && mapping_mapped(mapping))
    > + unmap_mapping_range(mapping, 0, -1, 0);
    > +
    > retval = filemap_write_and_wait(mapping);
    > if (retval == 0) {
    > retval = mapping->a_ops->direct_IO(rw, iocb, iov,
    > offset, nr_segs);
    > - if (rw == WRITE && mapping->nrpages)
    > - invalidate_inode_pages2(mapping);
    > + if (rw == WRITE && mapping->nrpages) {
    > + int err = invalidate_inode_pages2(mapping);
    > + if (err)
    > + retval = err;
    > + }
    > }
    > return retval;
    > }
    > -
    > EXPORT_SYMBOL_GPL(generic_file_direct_IO);
    > _

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 14:07    [W:0.073 / U:59.584 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site