Kernel - Close VM/BIO races and document.o
authorMatthew Dillon <dillon@apollo.backplane.com>
Fri, 28 Aug 2009 03:34:50 +0000 (20:34 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Fri, 28 Aug 2009 05:41:10 +0000 (22:41 -0700)
* Remove vfs_setdirty(), it is no longer used.
  Remove vfs_page_set_valid(), it is no longer used.
  Remove vfs_bio_set_valid(), it is no longer used.

* When acquiring a buffer with getblk() whos size differs from the
  buffer already cached, no longer destroy the VM pages backing
  the buffer after completing the write.  Instead just release
  the buffer so a new, larger one can be constructed.

  NFS buffers which straddle file EOF can remain cached after the
  file has been extended via seek/write or ftruncate, and their
  underlying VM pages may become dirty via mmap.  If the buffer
  is acquired later the underlying VM pages beyond the buffer's
  original b_bcount size must be retained, not destroyed.

* No longer try to clear the pmap modified bit from misc vm_page_*()
  functions.  In cases where we desire the pmap modified bit to be
  clear, it should *already* have been cleared in the run-up to the
  I/O.  Clearing it later may cause the buffer cache to lose track
  of the fact that underlying VM pages may have been modified again.

  NFS buffers use b_dirtyoff/b_dirtyend to determine what to actually
  write.  If the VM page is modified again the current write operation
  will not cover all the dirty parts of the buffer and another write
  will have to be issued.  Clearing the pmap modified bit at later
  stages did not properly track changes in b_dirtyoff/b_dirtyend and
  resulted in dirty data being lost.

* Implement vfs_clean_one_page() to deal with nearly all buffer cache vs
  backing VM page dirty->clean handling at the appropriate time.

  In addition, this function now detects the case where a buffer has
  B_NEEDCOMMIT set but the underlying VM page is dirty.  This
  function necessarily only clears the dirty bits associated
  with the buffer because buffer sizes are not necessarily page aligned,
  which is different from clearing ALL the dirty bits as the putpages
  code is able to do.  So the B_NEEDCOMMIT test is only against those
  dirty bits associated with the buffer.  If this is found to be the
  case the B_NEEDCOMMIT flag is cleared.

  This fixes a race where VM pages backing a dirty buffer which has gone
  through the phase-1 commit are dirtied via a mmap, and NFS then goes
  through with the phase-2 commit and throws the data away when it really
  needed to go back and do another phase-1 commit.

* In vnode_generic_put_pages() no longer clear the VM page dirty bits
  associated with bits of a file which extend past file EOF in the
  page straddling the EOF.  We used to do this with the idea that
  we would only clear the dirty bits up to the file EOF later on
  in the I/O completion code.

  However, this was too fragile.  If a page ended up with any dirty
  bits left set it would remain endless dirty and be reflushed forever.

  We now clear the dirty bits for the entire page after a putpages
  operation completes without error, and don't bother doing it
  prior to I/O initiation.

* Call nfs_meta_setsize() for both seek+write extensions (holes) and for
  ftruncate extensions (holes).

  nfs_meta_setsize() now deterministically adjusts the size of the buffer
  that was straddling the PRIOR EOF point, fixing an issue where
  write-extending a file to near the end of a nfs buffer boundary (32K),
  then seek-write extending it further by creating a hole, then
  mmap()ing the end of the first chunk and modifying data past the
  original write-extend point... would lose the extra data because
  the original buffer was still intact and was still sized for the
  original EOF.  This was difficult to reproduce because it only occurred
  if all the dirty bits got cleared when the original buffer is flushed,
  meaning the original write-extend point had to be within 511 bytes of
  the end of a 32K boundary.

sys/kern/vfs_bio.c
sys/vfs/devfs/devfs_vnops.c
sys/vfs/nfs/nfs.h
sys/vfs/nfs/nfs_bio.c
sys/vfs/nfs/nfs_vnops.c
sys/vfs/nwfs/nwfs_io.c
sys/vfs/smbfs/smbfs_io.c
sys/vm/vm_page.c
sys/vm/vm_page2.h
sys/vm/vnode_pager.c

index 151f9f4..3c9d80c 100644 (file)
@@ -91,10 +91,8 @@ static MALLOC_DEFINE(M_BIOBUF, "BIO buffer", "BIO buffer");
 
 struct buf *buf;               /* buffer header pool */
 
-static void vfs_page_set_valid(struct buf *bp, vm_ooffset_t off,
-                              int pageno, vm_page_t m);
 static void vfs_clean_pages(struct buf *bp);
-static void vfs_setdirty(struct buf *bp);
+static void vfs_clean_one_page(struct buf *bp, int pageno, vm_page_t m);
 static void vfs_vmio_release(struct buf *bp);
 static int flushbufqueues(bufq_type_t q);
 static vm_page_t bio_page_alloc(vm_object_t obj, vm_pindex_t pg, int deficit);
@@ -335,9 +333,13 @@ buf_runningbufspace_severe(void)
 /*
  * vfs_buf_test_cache:
  *
- *     Called when a buffer is extended.  This function clears the B_CACHE
- *     bit if the newly extended portion of the buffer does not contain
- *     valid data.
+ * Called when a buffer is extended.  This function clears the B_CACHE
+ * bit if the newly extended portion of the buffer does not contain
+ * valid data.
+ *
+ * NOTE! Dirty VM pages are not processed into dirty (B_DELWRI) buffer
+ * cache buffers.  The VM pages remain dirty, as someone had mmap()'d
+ * them while a clean buffer was present.
  */
 static __inline__
 void
@@ -1058,16 +1060,19 @@ bdwrite(struct buf *bp)
        }
 
        /*
-        * Set the *dirty* buffer range based upon the VM system dirty pages.
-        */
-       vfs_setdirty(bp);
-
-       /*
-        * We need to do this here to satisfy the vnode_pager and the
-        * pageout daemon, so that it thinks that the pages have been
-        * "cleaned".  Note that since the pages are in a delayed write
-        * buffer -- the VFS layer "will" see that the pages get written
-        * out on the next sync, or perhaps the cluster will be completed.
+        * Because the underlying pages may still be mapped and
+        * writable trying to set the dirty buffer (b_dirtyoff/end)
+        * range here will be inaccurate.
+        *
+        * However, we must still clean the pages to satisfy the
+        * vnode_pager and pageout daemon, so theythink the pages
+        * have been "cleaned".  What has really occured is that
+        * they've been earmarked for later writing by the buffer
+        * cache.
+        *
+        * So we get the b_dirtyoff/end update but will not actually
+        * depend on it (NFS that is) until the pages are busied for
+        * writing later on.
         */
        vfs_clean_pages(bp);
        bqrelse(bp);
@@ -1377,6 +1382,8 @@ brelse(struct buf *bp)
                         * granular mess that exists to support odd block 
                         * sizes and UFS meta-data block sizes (e.g. 6144).
                         * A complete rewrite is required.
+                        *
+                        * XXX
                         */
                        if (bp->b_flags & (B_NOCACHE|B_ERROR)) {
                                int poffset = foff & PAGE_MASK;
@@ -2517,98 +2524,6 @@ inmem(struct vnode *vp, off_t loffset)
 }
 
 /*
- * vfs_setdirty:
- *
- *     Sets the dirty range for a buffer based on the status of the dirty
- *     bits in the pages comprising the buffer.
- *
- *     The range is limited to the size of the buffer.
- *
- *     This routine is primarily used by NFS, but is generalized for the
- *     B_VMIO case.
- */
-static void
-vfs_setdirty(struct buf *bp) 
-{
-       int i;
-       vm_object_t object;
-
-       /*
-        * Degenerate case - empty buffer
-        */
-
-       if (bp->b_bufsize == 0)
-               return;
-
-       /*
-        * We qualify the scan for modified pages on whether the
-        * object has been flushed yet.  The OBJ_WRITEABLE flag
-        * is not cleared simply by protecting pages off.
-        */
-
-       if ((bp->b_flags & B_VMIO) == 0)
-               return;
-
-       object = bp->b_xio.xio_pages[0]->object;
-
-       if ((object->flags & OBJ_WRITEABLE) && !(object->flags & OBJ_MIGHTBEDIRTY))
-               kprintf("Warning: object %p writeable but not mightbedirty\n", object);
-       if (!(object->flags & OBJ_WRITEABLE) && (object->flags & OBJ_MIGHTBEDIRTY))
-               kprintf("Warning: object %p mightbedirty but not writeable\n", object);
-
-       if (object->flags & (OBJ_MIGHTBEDIRTY|OBJ_CLEANING)) {
-               vm_offset_t boffset;
-               vm_offset_t eoffset;
-
-               /*
-                * test the pages to see if they have been modified directly
-                * by users through the VM system.
-                */
-               for (i = 0; i < bp->b_xio.xio_npages; i++) {
-                       vm_page_flag_clear(bp->b_xio.xio_pages[i], PG_ZERO);
-                       vm_page_test_dirty(bp->b_xio.xio_pages[i]);
-               }
-
-               /*
-                * Calculate the encompassing dirty range, boffset and eoffset,
-                * (eoffset - boffset) bytes.
-                */
-
-               for (i = 0; i < bp->b_xio.xio_npages; i++) {
-                       if (bp->b_xio.xio_pages[i]->dirty)
-                               break;
-               }
-               boffset = (i << PAGE_SHIFT) - (bp->b_loffset & PAGE_MASK);
-
-               for (i = bp->b_xio.xio_npages - 1; i >= 0; --i) {
-                       if (bp->b_xio.xio_pages[i]->dirty) {
-                               break;
-                       }
-               }
-               eoffset = ((i + 1) << PAGE_SHIFT) - (bp->b_loffset & PAGE_MASK);
-
-               /*
-                * Fit it to the buffer.
-                */
-
-               if (eoffset > bp->b_bcount)
-                       eoffset = bp->b_bcount;
-
-               /*
-                * If we have a good dirty range, merge with the existing
-                * dirty range.
-                */
-
-               if (boffset < eoffset) {
-                       if (bp->b_dirtyoff > boffset)
-                               bp->b_dirtyoff = boffset;
-                       if (bp->b_dirtyend < eoffset)
-                               bp->b_dirtyend = eoffset;
-               }
-       }
-}
-
-/*
  * findblk:
  *
  *     Locate and return the specified buffer.  Unless flagged otherwise,
@@ -2825,14 +2740,24 @@ loop:
                 * Any size inconsistancy with a dirty buffer or a buffer
                 * with a softupdates dependancy must be resolved.  Resizing
                 * the buffer in such circumstances can lead to problems.
+                *
+                * Dirty or dependant buffers are written synchronously.
+                * Other types of buffers are simply released and
+                * reconstituted as they may be backed by valid, dirty VM
+                * pages (but not marked B_DELWRI).
+                *
+                * NFS NOTE: NFS buffers which straddle EOF are oddly-sized
+                * and may be left over from a prior truncation (and thus
+                * no longer represent the actual EOF point), so we
+                * definitely do not want to B_NOCACHE the backing store.
                 */
                if (size != bp->b_bcount) {
                        get_mplock();
                        if (bp->b_flags & B_DELWRI) {
-                               bp->b_flags |= B_NOCACHE;
+                               bp->b_flags |= B_RELBUF;
                                bwrite(bp);
                        } else if (LIST_FIRST(&bp->b_dep)) {
-                               bp->b_flags |= B_NOCACHE;
+                               bp->b_flags |= B_RELBUF;
                                bwrite(bp);
                        } else {
                                bp->b_flags |= B_RELBUF;
@@ -2867,6 +2792,11 @@ loop:
                 * deal with this we set B_NOCACHE to scrap the buffer
                 * after the write.
                 *
+                * XXX Should this be B_RELBUF instead of B_NOCACHE?
+                *     I'm not even sure this state is still possible
+                *     now that getblk() writes out any dirty buffers
+                *     on size changes.
+                *
                 * We might be able to do something fancy, like setting
                 * B_CACHE in bwrite() except if B_DELWRI is already set,
                 * so the below call doesn't set B_CACHE, but that gets real
@@ -2875,6 +2805,9 @@ loop:
 
                if ((bp->b_flags & (B_CACHE|B_DELWRI)) == B_DELWRI) {
                        get_mplock();
+                       kprintf("getblk: Warning, bp %p loff=%jx DELWRI set "
+                               "and CACHE clear, b_flags %08x\n",
+                               bp, (intmax_t)bp->b_loffset, bp->b_flags);
                        bp->b_flags |= B_NOCACHE;
                        bwrite(bp);
                        rel_mplock();
@@ -3548,18 +3481,19 @@ bpdone(struct buf *bp, int elseit)
 
                        /*
                         * In the write case, the valid and clean bits are
-                        * already changed correctly ( see bdwrite() ), so we 
+                        * already changed correctly (see bdwrite()), so we
                         * only need to do this here in the read case.
                         */
                        if (cmd == BUF_CMD_READ && !bogusflag && resid > 0) {
-                               vfs_page_set_valid(bp, foff, i, m);
+                               vfs_clean_one_page(bp, i, m);
                        }
                        vm_page_flag_clear(m, PG_ZERO);
 
                        /*
-                        * when debugging new filesystems or buffer I/O methods, this
-                        * is the most common error that pops up.  if you see this, you
-                        * have not set the page busy flag correctly!!!
+                        * when debugging new filesystems or buffer I/O
+                        * methods, this is the most common error that pops
+                        * up.  if you see this, you have not set the page
+                        * busy flag correctly!!!
                         */
                        if (m->busy == 0) {
                                kprintf("biodone: page busy < 0, "
@@ -3726,43 +3660,6 @@ vfs_unbusy_pages(struct buf *bp)
 }
 
 /*
- * vfs_page_set_valid:
- *
- *     Set the valid bits in a page based on the supplied offset.   The
- *     range is restricted to the buffer's size.
- *
- *     This routine is typically called after a read completes.
- */
-static void
-vfs_page_set_valid(struct buf *bp, vm_ooffset_t off, int pageno, vm_page_t m)
-{
-       vm_ooffset_t soff, eoff;
-
-       /*
-        * Start and end offsets in buffer.  eoff - soff may not cross a
-        * page boundry or cross the end of the buffer.  The end of the
-        * buffer, in this case, is our file EOF, not the allocation size
-        * of the buffer.
-        */
-       soff = off;
-       eoff = (off + PAGE_SIZE) & ~(off_t)PAGE_MASK;
-       if (eoff > bp->b_loffset + bp->b_bcount)
-               eoff = bp->b_loffset + bp->b_bcount;
-
-       /*
-        * Set valid range.  This is typically the entire buffer and thus the
-        * entire page.
-        */
-       if (eoff > soff) {
-               vm_page_set_validclean(
-                   m,
-                  (vm_offset_t) (soff & PAGE_MASK),
-                  (vm_offset_t) (eoff - soff)
-               );
-       }
-}
-
-/*
  * vfs_busy_pages:
  *
  *     This routine is called before a device strategy routine.
@@ -3792,13 +3689,10 @@ vfs_busy_pages(struct vnode *vp, struct buf *bp)
 
        if (bp->b_flags & B_VMIO) {
                vm_object_t obj;
-               vm_ooffset_t foff;
 
                obj = vp->v_object;
-               foff = bp->b_loffset;
                KASSERT(bp->b_loffset != NOOFFSET,
                        ("vfs_busy_pages: no buffer offset"));
-               vfs_setdirty(bp);
 
                /*
                 * Loop until none of the pages are busy.
@@ -3829,40 +3723,83 @@ retry:
                 * Adjust protections for I/O and do bogus-page mapping.
                 * Assume that vm_page_protect() can block (it can block
                 * if VM_PROT_NONE, don't take any chances regardless).
+                *
+                * In particularly note that for writes we must incorporate
+                * page dirtyness from the VM system into the buffer's
+                * dirty range.
+                *
+                * For reads we theoretically must incorporate page dirtyness
+                * from the VM system to determine if the page needs bogus
+                * replacement, but we shortcut the test by simply checking
+                * that all m->valid bits are set, indicating that the page
+                * is fully valid and does not need to be re-read.  For any
+                * VM system dirtyness the page will also be fully valid
+                * since it was mapped at one point.
                 */
                bogus = 0;
                for (i = 0; i < bp->b_xio.xio_npages; i++) {
                        vm_page_t m = bp->b_xio.xio_pages[i];
 
-                       /*
-                        * When readying a vnode-backed buffer for a write
-                        * we must zero-fill any invalid portions of the
-                        * backing VM pages.
-                        *
-                        * When readying a vnode-backed buffer for a read
-                        * we must replace any dirty pages with a bogus
-                        * page so we do not destroy dirty data when
-                        * filling in gaps.  Dirty pages might not
-                        * necessarily be marked dirty yet, so use m->valid
-                        * as a reasonable test.
-                        *
-                        * Bogus page replacement is, uh, bogus.  We need
-                        * to find a better way.
-                        */
+                       vm_page_flag_clear(m, PG_ZERO); /* XXX */
                        if (bp->b_cmd == BUF_CMD_WRITE) {
+                               /*
+                                * When readying a vnode-backed buffer for
+                                * a write we must zero-fill any invalid
+                                * portions of the backing VM pages, mark
+                                * it valid and clear related dirty bits.
+                                *
+                                * vfs_clean_one_page() incorporates any
+                                * VM dirtyness and updates the b_dirtyoff
+                                * range (after we've made the page RO).
+                                *
+                                * It is also expected that the pmap modified
+                                * bit has already been cleared by the
+                                * vm_page_protect().  We may not be able
+                                * to clear all dirty bits for a page if it
+                                * was also memory mapped (NFS).
+                                */
                                vm_page_protect(m, VM_PROT_READ);
-                               vfs_page_set_valid(bp, foff, i, m);
+                               vfs_clean_one_page(bp, i, m);
                        } else if (m->valid == VM_PAGE_BITS_ALL) {
+                               /*
+                                * When readying a vnode-backed buffer for
+                                * read we must replace any dirty pages with
+                                * a bogus page so dirty data is not destroyed
+                                * when filling gaps.
+                                *
+                                * To avoid testing whether the page is
+                                * dirty we instead test that the page was
+                                * at some point mapped (m->valid fully
+                                * valid) with the understanding that
+                                * this also covers the dirty case.
+                                */
                                bp->b_xio.xio_pages[i] = bogus_page;
                                bogus++;
+                       } else if (m->valid & m->dirty) {
+                               /*
+                                * This case should not occur as partial
+                                * dirtyment can only happen if the buffer
+                                * is B_CACHE, and this code is not entered
+                                * if the buffer is B_CACHE.
+                                */
+                               kprintf("Warning: vfs_busy_pages - page not "
+                                       "fully valid! loff=%jx bpf=%08x "
+                                       "idx=%d val=%02x dir=%02x\n",
+                                       (intmax_t)bp->b_loffset, bp->b_flags,
+                                       i, m->valid, m->dirty);
+                               vm_page_protect(m, VM_PROT_NONE);
                        } else {
+                               /*
+                                * The page is not valid and can be made
+                                * part of the read.
+                                */
                                vm_page_protect(m, VM_PROT_NONE);
                        }
-                       foff = (foff + PAGE_SIZE) & ~(off_t)PAGE_MASK;
                }
-               if (bogus)
+               if (bogus) {
                        pmap_qenter(trunc_page((vm_offset_t)bp->b_data),
                                bp->b_xio.xio_pages, bp->b_xio.xio_npages);
+               }
        }
 
        /*
@@ -3890,61 +3827,113 @@ retry:
 static void
 vfs_clean_pages(struct buf *bp)
 {
+       vm_page_t m;
        int i;
 
-       if (bp->b_flags & B_VMIO) {
-               vm_ooffset_t foff;
+       if ((bp->b_flags & B_VMIO) == 0)
+               return;
 
-               foff = bp->b_loffset;
-               KASSERT(foff != NOOFFSET, ("vfs_clean_pages: no buffer offset"));
-               for (i = 0; i < bp->b_xio.xio_npages; i++) {
-                       vm_page_t m = bp->b_xio.xio_pages[i];
-                       vm_ooffset_t noff = (foff + PAGE_SIZE) & ~(off_t)PAGE_MASK;
+       KASSERT(bp->b_loffset != NOOFFSET,
+               ("vfs_clean_pages: no buffer offset"));
 
-                       vfs_page_set_valid(bp, foff, i, m);
-                       foff = noff;
-               }
+       for (i = 0; i < bp->b_xio.xio_npages; i++) {
+               m = bp->b_xio.xio_pages[i];
+               vfs_clean_one_page(bp, i, m);
        }
 }
 
-#if 0
 /*
- * vfs_bio_set_valid:
+ * vfs_clean_one_page:
+ *
+ *     Set the valid bits and clear the dirty bits in a page within a
+ *     buffer.  The range is restricted to the buffer's size and the
+ *     buffer's logical offset might index into the first page.
+ *
+ *     The caller has busied or soft-busied the page and it is not mapped,
+ *     test and incorporate the dirty bits into b_dirtyoff/end before
+ *     clearing them.  Note that we need to clear the pmap modified bits
+ *     after determining the the page was dirty, vm_page_set_validclean()
+ *     does not do it for us.
  *
- * Set the range within the buffer to valid.  The range is relative
- * to the beginning of the buffer, b_loffset.  Note that b_loffset
- * itself may be offset from the beginning of the first page.
+ *     This routine is typically called after a read completes (dirty should
+ *     be zero in that case as we are not called on bogus-replace pages),
+ *     or before a write is initiated.
  */
-void   
-vfs_bio_set_valid(struct buf *bp, int base, int size)
+static void
+vfs_clean_one_page(struct buf *bp, int pageno, vm_page_t m)
 {
-       if (bp->b_flags & B_VMIO) {
-               int i;
-               int n;
-
-               /*
-                * Fixup base to be relative to beginning of first page.
-                * Set initial n to be the maximum number of bytes in the
-                * first page that can be validated.
-                */
+       int bcount;
+       int xoff;
+       int soff;
+       int eoff;
 
-               base += (bp->b_loffset & PAGE_MASK);
-               n = PAGE_SIZE - (base & PAGE_MASK);
-
-               for (i = base / PAGE_SIZE; size > 0 && i < bp->b_xio.xio_npages; ++i) {
-                       vm_page_t m = bp->b_xio.xio_pages[i];
+       /*
+        * Calculate offset range within the page but relative to buffer's
+        * loffset.  loffset might be offset into the first page.
+        */
+       xoff = (int)bp->b_loffset & PAGE_MASK;  /* loffset offset into pg 0 */
+       bcount = bp->b_bcount + xoff;           /* offset adjusted */
 
-                       if (n > size)
-                               n = size;
+       if (pageno == 0) {
+               soff = xoff;
+               eoff = PAGE_SIZE;
+       } else {
+               soff = (pageno << PAGE_SHIFT);
+               eoff = soff + PAGE_SIZE;
+       }
+       if (eoff > bcount)
+               eoff = bcount;
+       if (soff >= eoff)
+               return;
 
-                       vm_page_set_valid(m, base & PAGE_MASK, n);
-                       base += n;
-                       size -= n;
-                       n = PAGE_SIZE;
+       /*
+        * Test dirty bits and adjust b_dirtyoff/end.
+        *
+        * If dirty pages are incorporated into the bp any prior
+        * B_NEEDCOMMIT state (NFS) must be cleared because the
+        * caller has not taken into account the new dirty data.
+        *
+        * If the page was memory mapped the dirty bits might go beyond the
+        * end of the buffer, but we can't really make the assumption that
+        * a file EOF straddles the buffer (even though this is the case for
+        * NFS if B_NEEDCOMMIT is also set).  So for the purposes of clearing
+        * B_NEEDCOMMIT we only test the dirty bits covered by the buffer.
+        * This also saves some console spam.
+        */
+       vm_page_test_dirty(m);
+       if (m->dirty) {
+               pmap_clear_modify(m);
+               if ((bp->b_flags & B_NEEDCOMMIT) &&
+                   (m->dirty & vm_page_bits(soff & PAGE_MASK, eoff - soff))) {
+                       kprintf("Warning: vfs_clean_one_page: bp %p "
+                               "loff=%jx,%d flgs=%08x clr B_NEEDCOMMIT\n",
+                               bp, (intmax_t)bp->b_loffset, bp->b_bcount,
+                               bp->b_flags);
+                       bp->b_flags &= ~B_NEEDCOMMIT;
                }
+               if (bp->b_dirtyoff > soff - xoff)
+                       bp->b_dirtyoff = soff - xoff;
+               if (bp->b_dirtyend < eoff - xoff)
+                       bp->b_dirtyend = eoff - xoff;
        }
+
+       /*
+        * Set related valid bits, clear related dirty bits.
+        * Does not mess with the pmap modified bit.
+        *
+        * WARNING!  We cannot just clear all of m->dirty here as the
+        *           buffer cache buffers may use a DEV_BSIZE'd aligned
+        *           block size, or have an odd size (e.g. NFS at file EOF).
+        *           The putpages code can clear m->dirty to 0.
+        *
+        *           If a VOP_WRITE generates a buffer cache buffer which
+        *           covers the same space as mapped writable pages the
+        *           buffer flush might not be able to clear all the dirty
+        *           bits and still require a putpages from the VM system
+        *           to finish it off.
+        */
+       vm_page_set_validclean(m, soff & PAGE_MASK, eoff - soff);
 }
-#endif
 
 /*
  * vfs_bio_clrbuf:
index 26ba070..7e41e8c 100644 (file)
@@ -1930,13 +1930,18 @@ devfs_spec_getpages(struct vop_getpages_args *ap)
 
                m->flags &= ~PG_ZERO;
 
+               /*
+                * NOTE: vm_page_undirty/clear_dirty etc do not clear the
+                *       pmap modified bit.  pmap modified bit should have
+                *       already been cleared.
+                */
                if (nextoff <= nread) {
                        m->valid = VM_PAGE_BITS_ALL;
                        vm_page_undirty(m);
                } else if (toff < nread) {
                        /*
                         * Since this is a VM request, we have to supply the
-                        * unaligned offset to allow vm_page_set_validclean()
+                        * unaligned offset to allow vm_page_set_valid()
                         * to zero sub-DEV_BSIZE'd portions of the page.
                         */
                        vm_page_set_valid(m, 0, nread - toff);
index 40c1795..94c7972 100644 (file)
@@ -783,7 +783,8 @@ int nfsrv_write (struct nfsrv_descript *nfsd, struct nfssvc_sock *slp,
                         struct thread *td, struct mbuf **mrq);
 void   nfsrv_rcv (struct socket *so, void *arg, int waitflag);
 void   nfsrv_slpderef (struct nfssvc_sock *slp);
-int    nfs_meta_setsize (struct vnode *vp, struct thread *td, u_quad_t nsize);
+int    nfs_meta_setsize (struct vnode *vp, struct thread *td, u_quad_t nsize,
+                       struct buf *obp);
 int    nfs_clientd(struct nfsmount *nmp, struct ucred *cred,
                        struct nfsd_cargs *ncd, int flag, caddr_t argp,
                        struct thread *td);
index 573b90d..037e9e8 100644 (file)
@@ -195,6 +195,10 @@ nfs_getpages(struct vop_getpages_args *ap)
 
                m->flags &= ~PG_ZERO;
 
+               /*
+                * NOTE: vm_page_undirty/clear_dirty etc do not clear the
+                *       pmap modified bit.
+                */
                if (nextoff <= size) {
                        /*
                         * Read operation filled an entire page
@@ -248,6 +252,11 @@ nfs_getpages(struct vop_getpages_args *ap)
 /*
  * Vnode op for VM putpages.
  *
+ * The pmap modified bit was cleared prior to the putpages and probably
+ * couldn't get set again until after our I/O completed, since the page
+ * should not be mapped.  But don't count on it.  The m->dirty bits must
+ * be completely cleared when we finish even if the count is truncated.
+ *
  * nfs_putpages(struct vnode *a_vp, vm_page_t *a_m, int a_count, int a_sync,
  *             int *a_rtvals, vm_ooffset_t a_offset)
  */
@@ -320,8 +329,10 @@ nfs_putpages(struct vop_putpages_args *ap)
 
        msf_buf_free(msf);
 
-       if (!error) {
-               int nwritten = round_page(count - (int)uio.uio_resid) / PAGE_SIZE;
+       if (error == 0) {
+               int nwritten;
+
+               nwritten = round_page(count - (int)uio.uio_resid) / PAGE_SIZE;
                for (i = 0; i < nwritten; i++) {
                        rtvals[i] = VM_PAGER_OK;
                        vm_page_undirty(pages[i]);
@@ -887,7 +898,7 @@ again:
                        bp = nfs_getcacheblk(vp, loffset, bcount, td);
 
                        if (bp != NULL) {
-                               long save;
+                               u_int32_t save;
 
                                np->n_size = uio->uio_offset + n;
                                np->n_flag |= NLMODIFIED;
@@ -912,9 +923,9 @@ again:
                        }
                        bp = nfs_getcacheblk(vp, loffset, bcount, td);
                        if (uio->uio_offset + n > np->n_size) {
-                               np->n_size = uio->uio_offset + n;
                                np->n_flag |= NLMODIFIED;
-                               vnode_pager_setsize(vp, np->n_size);
+                               nfs_meta_setsize(vp, td, uio->uio_offset + n,
+                                                bp);
                        }
                }
 
@@ -1233,6 +1244,8 @@ nfs_asyncio(struct vnode *vp, struct bio *bio)
  *      actually initiates the I/O AFTER it has gotten to the txthread.
  *
  * NOTE: td might be NULL.
+ *
+ * NOTE: Caller has already busied the I/O.
  */
 void
 nfs_startio(struct vnode *vp, struct bio *bio, struct thread *td)
@@ -1404,7 +1417,10 @@ nfs_doio(struct vnode *vp, struct bio *bio, struct thread *td)
            bp->b_resid = uiop->uio_resid;
        } else {
            /* 
-            * If we only need to commit, try to commit
+            * If we only need to commit, try to commit.
+            *
+            * NOTE: The I/O has already been staged for the write and
+            *       its pages busied, so b_dirtyoff/end is valid.
             */
            KKASSERT(bp->b_cmd == BUF_CMD_WRITE);
            if (bp->b_flags & B_NEEDCOMMIT) {
@@ -1528,33 +1544,52 @@ nfs_doio(struct vnode *vp, struct bio *bio, struct thread *td)
 }
 
 /*
- * Used to aid in handling ftruncate() operations on the NFS client side.
+ * Used to aid in handling ftruncate() and non-trivial write-extend
+ * operations on the NFS client side.  Note that trivial write-extend
+ * operations (appending with no write hole) are handled by nfs_write()
+ * directly to avoid silly flushes.
+ *
  * Truncation creates a number of special problems for NFS.  We have to
  * throw away VM pages and buffer cache buffers that are beyond EOF, and
  * we have to properly handle VM pages or (potentially dirty) buffers
  * that straddle the truncation point.
+ *
+ * File extension creates an issue with oddly sized buffers left in the
+ * buffer cache for prior EOF points.  If the buffer cache decides to
+ * flush such a buffer and its backing store has also been modified via
+ * a mmap()ed write, the data from the old EOF point to the next DEV_BSIZE
+ * boundary would get lost because the buffer cache clears that dirty bit
+ * and yet only wrote through b_bcount.  (If clearing that dirty bits
+ * results in b_dirty becoming 0, we lose track of the dirty data).
+ * To deal with this case we release such buffers.
  */
-
 int
-nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize)
+nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize,
+                struct buf *obp)
 {
        struct nfsnode *np = VTONFS(vp);
        u_quad_t tsize = np->n_size;
+       u_quad_t tbase;
        int biosize = vp->v_mount->mnt_stat.f_iosize;
+       int bufsize;
+       int obufsize;
        int error = 0;
+       u_int32_t save;
+       struct buf *bp;
+       off_t loffset;
 
        np->n_size = nsize;
 
        if (nsize < tsize) {
-               struct buf *bp;
-               off_t loffset;
-               int bufsize;
-
                /*
                 * vtruncbuf() doesn't get the buffer overlapping the 
                 * truncation point.  We may have a B_DELWRI and/or B_CACHE
                 * buffer that now needs to be truncated.
+                *
+                * obp has better be NULL since a write can't make the
+                * file any smaller!
                 */
+               KKASSERT(obp == NULL);
                error = vtruncbuf(vp, nsize, biosize);
                bufsize = nsize & (biosize - 1);
                loffset = nsize - bufsize;
@@ -1566,6 +1601,37 @@ nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize)
                bp->b_flags |= B_RELBUF;    /* don't leave garbage around */
                brelse(bp);
        } else {
+               /*
+                * The size of the buffer straddling the old EOF must
+                * be adjusted.  I tried to avoid this for the longest
+                * time but there are simply too many interactions with
+                * the buffer cache and dirty backing store via mmap().
+                *
+                * This is a non-trivial hole case.  The expanded space
+                * in the buffer is zerod.
+                */
+               if (tsize & (biosize - 1)) {
+                       tbase = tsize & ~(u_quad_t)(biosize - 1);
+                       if ((bp = obp) == NULL) {
+                               bp = findblk(vp, tbase, 0);
+                               if (bp)
+                                       bremfree(bp);
+                       }
+                       if (bp) {
+                               obufsize = bp->b_bcount;
+                               save = bp->b_flags & B_CACHE;
+                               if ((tsize ^ nsize) & ~(u_quad_t)(biosize - 1))
+                                       bufsize = biosize;
+                               else
+                                       bufsize = nsize & (biosize - 1);
+                               allocbuf(bp, bufsize);
+                               bp->b_flags |= save;
+                               bzero(bp->b_data + obufsize,
+                                     bufsize - obufsize);
+                               if (obp == NULL)
+                                       brelse(bp);
+                       }
+               }
                vnode_pager_setsize(vp, nsize);
        }
        return(error);
@@ -1693,6 +1759,8 @@ nfsmout:
 
 /*
  * nfs write call - BIO version
+ *
+ * NOTE: Caller has already busied the I/O.
  */
 void
 nfs_writerpc_bio(struct vnode *vp, struct bio *bio)
@@ -1709,7 +1777,8 @@ nfs_writerpc_bio(struct vnode *vp, struct bio *bio)
 
        /*
         * Setup for actual write.  Just clean up the bio if there
-        * is nothing to do.
+        * is nothing to do.  b_dirtyoff/end have already been staged
+        * by the bp's pages getting busied.
         */
        if (bio->bio_offset + bp->b_dirtyend > np->n_size)
                bp->b_dirtyend = np->n_size - bio->bio_offset;
index edac0c6..9d7afd3 100644 (file)
@@ -726,7 +726,7 @@ nfs_setattr(struct vop_setattr_args *ap)
                         */
                        tsize = np->n_size;
 again:
-                       error = nfs_meta_setsize(vp, td, vap->va_size);
+                       error = nfs_meta_setsize(vp, td, vap->va_size, NULL);
 
                        if (np->n_flag & NLMODIFIED) {
                            if (vap->va_size == 0)
@@ -3218,7 +3218,8 @@ nfs_flush_bp(struct buf *bp, void *data)
                 *
                 * We must call vfs_busy_pages() now so the commit operation
                 * is interlocked with user modifications to memory mapped
-                * pages.
+                * pages.  The b_dirtyoff/b_dirtyend range is not correct
+                * until after the pages have been busied.
                 *
                 * Note: to avoid loopback deadlocks, we do not
                 * assign b_runningbufspace.
index f56efab..d153baa 100644 (file)
@@ -446,11 +446,16 @@ nwfs_getpages(struct vop_getpages_args *ap)
 
                m->flags &= ~PG_ZERO;
 
+               /*
+                * NOTE: pmap dirty bit should have already been cleared.
+                *       We do not clear it here.
+                */
                if (nextoff <= size) {
                        m->valid = VM_PAGE_BITS_ALL;
                        m->dirty = 0;
                } else {
-                       int nvalid = ((size + DEV_BSIZE - 1) - toff) & ~(DEV_BSIZE - 1);
+                       int nvalid = ((size + DEV_BSIZE - 1) - toff) &
+                                     ~(DEV_BSIZE - 1);
                        vm_page_set_validclean(m, 0, nvalid);
                }
                
@@ -559,7 +564,7 @@ nwfs_putpages(struct vop_putpages_args *ap)
                int nwritten = round_page(count - uio.uio_resid) / PAGE_SIZE;
                for (i = 0; i < nwritten; i++) {
                        rtvals[i] = VM_PAGER_OK;
-                       pages[i]->dirty = 0;
+                       vm_page_undirty(pages[i]);
                }
        }
        return rtvals[0];
index d6b7340..71ccf1d 100644 (file)
@@ -484,11 +484,16 @@ smbfs_getpages(struct vop_getpages_args *ap)
 
                m->flags &= ~PG_ZERO;
 
+               /*
+                * NOTE: pmap dirty bit should have already been cleared.
+                *       We do not clear it here.
+                */
                if (nextoff <= size) {
                        m->valid = VM_PAGE_BITS_ALL;
                        m->dirty = 0;
                } else {
-                       int nvalid = ((size + DEV_BSIZE - 1) - toff) & ~(DEV_BSIZE - 1);
+                       int nvalid = ((size + DEV_BSIZE - 1) - toff) &
+                                     ~(DEV_BSIZE - 1);
                        vm_page_set_validclean(m, 0, nvalid);
                }
                
@@ -616,7 +621,7 @@ smbfs_putpages(struct vop_putpages_args *ap)
                int nwritten = round_page(count - uio.uio_resid) / PAGE_SIZE;
                for (i = 0; i < nwritten; i++) {
                        rtvals[i] = VM_PAGER_OK;
-                       pages[i]->dirty = 0;
+                       vm_page_undirty(pages[i]);
                }
        }
        return rtvals[0];
index 0a0b907..717522b 100644 (file)
@@ -1500,6 +1500,14 @@ vm_page_set_valid(vm_page_t m, int base, int size)
        m->valid |= vm_page_bits(base, size);
 }
 
+
+/*
+ * Set valid bits and clear dirty bits.
+ *
+ * NOTE: This function does not clear the pmap modified bit.
+ *      Also note that e.g. NFS may use a byte-granular base
+ *      and size.
+ */
 void
 vm_page_set_validclean(vm_page_t m, int base, int size)
 {
@@ -1510,17 +1518,24 @@ vm_page_set_validclean(vm_page_t m, int base, int size)
        m->valid |= pagebits;
        m->dirty &= ~pagebits;
        if (base == 0 && size == PAGE_SIZE) {
-               pmap_clear_modify(m);
+               /*pmap_clear_modify(m);*/
                vm_page_flag_clear(m, PG_NOSYNC);
        }
 }
 
+/*
+ * Clear dirty bits.
+ *
+ * NOTE: This function does not clear the pmap modified bit.
+ *      Also note that e.g. NFS may use a byte-granular base
+ *      and size.
+ */
 void
 vm_page_clear_dirty(vm_page_t m, int base, int size)
 {
        m->dirty &= ~vm_page_bits(base, size);
        if (base == 0 && size == PAGE_SIZE) {
-               pmap_clear_modify(m);
+               /*pmap_clear_modify(m);*/
                vm_page_flag_clear(m, PG_NOSYNC);
        }
 }
index 6f54edb..ff3010c 100644 (file)
@@ -179,6 +179,8 @@ vm_page_unregister_action(vm_page_t m, vm_page_action_t action)
  * Used when reading data in, typically via getpages.
  * The partial device block at the end of the truncation
  * range should not lose its dirty bit.
+ *
+ * NOTE: This function does not clear the pmap modified bit.
  */
 static __inline
 void
@@ -196,6 +198,8 @@ vm_page_clear_dirty_end_nonincl(vm_page_t m, int base, int size)
  * Used when truncating a buffer.  The partial device
  * block at the beginning of the truncation range
  * should not lose its dirty bit.
+ *
+ * NOTE: This function does not clear the pmap modified bit.
  */
 static __inline
 void
index f487e3c..0528caa 100644 (file)
@@ -595,12 +595,14 @@ vnode_pager_generic_getpages(struct vnode *vp, vm_page_t *m, int bytecount,
  * implement their own VOP_PUTPAGES, their VOP_PUTPAGES should call to
  * vnode_pager_generic_putpages() to implement the previous behaviour.
  *
+ * Caller has already cleared the pmap modified bits, if any.
+ *
  * All other FS's should use the bypass to get to the local media
  * backing vp's VOP_PUTPAGES.
  */
 static void
 vnode_pager_putpages(vm_object_t object, vm_page_t *m, int count,
-    boolean_t sync, int *rtvals)
+                    boolean_t sync, int *rtvals)
 {
        int rtval;
        struct vnode *vp;
@@ -624,7 +626,6 @@ vnode_pager_putpages(vm_object_t object, vm_page_t *m, int count,
        /*
         * Call device-specific putpages function
         */
-
        vp = object->handle;
        rtval = VOP_PUTPAGES(vp, m, bytes, sync, rtvals, 0);
        if (rtval == EOPNOTSUPP) {
@@ -649,9 +650,7 @@ vnode_pager_generic_putpages(struct vnode *vp, vm_page_t *m, int bytecount,
 {
        int i;
        vm_object_t object;
-       int count;
-
-       int maxsize, ncount;
+       int maxsize, ncount, count;
        vm_ooffset_t poffset;
        struct uio auio;
        struct iovec aiov;
@@ -678,12 +677,11 @@ vnode_pager_generic_putpages(struct vnode *vp, vm_page_t *m, int bytecount,
 
        /*
         * If the page-aligned write is larger then the actual file we
-        * have to invalidate pages occuring beyond the file EOF.  However,
-        * there is an edge case where a file may not be page-aligned where
-        * the last page is partially invalid.  In this case the filesystem
-        * may not properly clear the dirty bits for the entire page (which
-        * could be VM_PAGE_BITS_ALL due to the page having been mmap()d).
-        * With the page locked we are free to fix-up the dirty bits here.
+        * have to invalidate pages occuring beyond the file EOF.
+        *
+        * If the file EOF resides in the middle of a page we still clear
+        * all of that page's dirty bits later on.  If we didn't it would
+        * endlessly re-write.
         *
         * We do not under any circumstances truncate the valid bits, as
         * this will screw up bogus page replacement.
@@ -696,14 +694,8 @@ vnode_pager_generic_putpages(struct vnode *vp, vm_page_t *m, int bytecount,
         */
        if (poffset + maxsize > vp->v_filesize) {
                if (poffset < vp->v_filesize) {
-                       int pgoff;
-
                        maxsize = vp->v_filesize - poffset;
                        ncount = btoc(maxsize);
-                       if ((pgoff = (int)maxsize & PAGE_MASK) != 0) {
-                               vm_page_clear_dirty(m[ncount - 1], pgoff,
-                                       PAGE_SIZE - pgoff);
-                       }
                } else {
                        maxsize = 0;
                        ncount = 0;
@@ -752,8 +744,12 @@ vnode_pager_generic_putpages(struct vnode *vp, vm_page_t *m, int bytecount,
                            "vnode_pager_putpages: residual I/O %zd at %lu\n",
                            auio.uio_resid, (u_long)m[0]->pindex);
        }
-       for (i = 0; i < ncount; i++)
-               rtvals[i] = VM_PAGER_OK;
+       if (error == 0) {
+               for (i = 0; i < ncount; i++) {
+                       rtvals[i] = VM_PAGER_OK;
+                       vm_page_undirty(m[i]);
+               }
+       }
        return rtvals[0];
 }