NFS - Properly handle NFSv3 EOF short-reads
authorMatthew Dillon <dillon@apollo.backplane.com>
Mon, 24 Aug 2009 22:41:42 +0000 (15:41 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Mon, 24 Aug 2009 22:41:42 +0000 (15:41 -0700)
* Short reads were not causing the remainder of the buffer to be
  zero'd out in all cases, causing the fsx filesystem test to fail.

* An EOF condition was sometimes improperly reporting a non-zero
  bp->b_resid.  Race against the server where the client's idea of
  the file size differs from the server could result in a non-zero
  bp->b_resid after a BIO.

  Zero out the remaining space and and set bp->b_resid to 0.  The
  NFS client code always specifies buffers within the bounds of
  the file.

Reported-by: Antonio Huete Jimenez <tuxillo@quantumachine.net>
sys/vfs/nfs/nfs_bio.c
sys/vfs/nfs/nfs_vnops.c

index b23d6a1..705811c 100644 (file)
@@ -492,17 +492,16 @@ again:
                 * If B_CACHE is not set, we must issue the read.  If this
                 * fails, we return an error.
                 */
-
                if ((bp->b_flags & B_CACHE) == 0) {
-                   bp->b_cmd = BUF_CMD_READ;
-                   bp->b_bio2.bio_done = nfsiodone_sync;
-                   bp->b_bio2.bio_flags |= BIO_SYNC;
-                   vfs_busy_pages(vp, bp);
-                   error = nfs_doio(vp, &bp->b_bio2, td);
-                   if (error) {
-                       brelse(bp);
-                       return (error);
-                   }
+                       bp->b_cmd = BUF_CMD_READ;
+                       bp->b_bio2.bio_done = nfsiodone_sync;
+                       bp->b_bio2.bio_flags |= BIO_SYNC;
+                       vfs_busy_pages(vp, bp);
+                       error = nfs_doio(vp, &bp->b_bio2, td);
+                       if (error) {
+                               brelse(bp);
+                               return (error);
+                       }
                }
 
                /*
@@ -512,7 +511,6 @@ again:
                 *
                 * Then figure out how many bytes we can copy into the uio.
                 */
-
                n = 0;
                if (on < bcount)
                        n = (int)szmin((unsigned)(bcount - on), uio->uio_resid);
@@ -524,16 +522,16 @@ again:
                if (bp == NULL)
                        return (EINTR);
                if ((bp->b_flags & B_CACHE) == 0) {
-                   bp->b_cmd = BUF_CMD_READ;
-                   bp->b_bio2.bio_done = nfsiodone_sync;
-                   bp->b_bio2.bio_flags |= BIO_SYNC;
-                   vfs_busy_pages(vp, bp);
-                   error = nfs_doio(vp, &bp->b_bio2, td);
-                   if (error) {
-                       bp->b_flags |= B_ERROR | B_INVAL;
-                       brelse(bp);
-                       return (error);
-                   }
+                       bp->b_cmd = BUF_CMD_READ;
+                       bp->b_bio2.bio_done = nfsiodone_sync;
+                       bp->b_bio2.bio_flags |= BIO_SYNC;
+                       vfs_busy_pages(vp, bp);
+                       error = nfs_doio(vp, &bp->b_bio2, td);
+                       if (error) {
+                               bp->b_flags |= B_ERROR | B_INVAL;
+                               brelse(bp);
+                               return (error);
+                       }
                }
                n = (int)szmin(uio->uio_resid, bp->b_bcount - bp->b_resid);
                on = 0;
@@ -924,33 +922,28 @@ again:
                }
 
                /*
-                * Issue a READ if B_CACHE is not set.  In special-append
-                * mode, B_CACHE is based on the buffer prior to the write
-                * op and is typically set, avoiding the read.  If a read
-                * is required in special append mode, the server will
-                * probably send us a short-read since we extended the file
-                * on our end, resulting in b_resid == 0 and, thusly, 
-                * B_CACHE getting set.
+                * Avoid a read by setting B_CACHE where the data we
+                * intend to write covers the entire buffer.  This also
+                * handles the normal append case as bcount will have
+                * byte resolution.  The buffer state must also be adjusted.
                 *
-                * We can also avoid issuing the read if the write covers
-                * the entire buffer.  We have to make sure the buffer state
-                * is reasonable in this case since we will not be initiating
-                * I/O.  See the comments in kern/vfs_bio.c's getblk() for
+                * See the comments in kern/vfs_bio.c's getblk() for
                 * more information.
                 *
-                * B_CACHE may also be set due to the buffer being cached
-                * normally.
-                *
                 * When doing a UIO_NOCOPY write the buffer is not
                 * overwritten and we cannot just set B_CACHE unconditionally
                 * for full-block writes.
                 */
-
                if (on == 0 && n == bcount && uio->uio_segflg != UIO_NOCOPY) {
                        bp->b_flags |= B_CACHE;
                        bp->b_flags &= ~(B_ERROR | B_INVAL);
                }
 
+               /*
+                * b_resid may be set due to file EOF if we extended out.
+                * The NFS bio code will zero the difference anyway so
+                * just acknowledged the fact and set b_resid to 0.
+                */
                if ((bp->b_flags & B_CACHE) == 0) {
                        bp->b_cmd = BUF_CMD_READ;
                        bp->b_bio2.bio_done = nfsiodone_sync;
@@ -961,6 +954,7 @@ again:
                                brelse(bp);
                                break;
                        }
+                       bp->b_resid = 0;
                }
                if (!bp) {
                        error = EINTR;
@@ -1317,6 +1311,7 @@ nfs_doio(struct vnode *vp, struct bio *bio, struct thread *td)
        struct nfsmount *nmp;
        int error = 0;
        int iomode, must_commit;
+       size_t n;
        struct uio uio;
        struct iovec io;
 
@@ -1346,26 +1341,23 @@ nfs_doio(struct vnode *vp, struct bio *bio, struct thread *td)
 
            switch (vp->v_type) {
            case VREG:
+               /*
+                * When reading from a regular file zero-fill any residual.
+                * Note that this residual has nothing to do with NFS short
+                * reads, which nfs_readrpc_uio() will handle for us.
+                *
+                * We have to do this because when we are write extending
+                * a file the server may not have the same notion of
+                * filesize as we do.  Our BIOs should already be sized
+                * (b_bcount) to account for the file EOF.
+                */
                nfsstats.read_bios++;
                uiop->uio_offset = bio->bio_offset;
                error = nfs_readrpc_uio(vp, uiop);
-               if (error == 0) {
-                   if (uiop->uio_resid) {
-                       /*
-                        * If we had a short read with no error, we must have
-                        * hit a file hole.  We should zero-fill the remainder.
-                        * This can also occur if the server hits the file EOF.
-                        *
-                        * Holes used to be able to occur due to pending 
-                        * writes, but that is not possible any longer.
-                        */
-                       int nread = bp->b_bcount - bp->b_resid;
-                       int left  = bp->b_resid;
-
-                       if (left > 0)
-                               bzero((char *)bp->b_data + nread, left);
-                       bp->b_resid = 0;
-                   }
+               if (error == 0 && uiop->uio_resid) {
+                       n = (size_t)bp->b_bcount - uiop->uio_resid;
+                       bzero(bp->b_data + n, bp->b_bcount - n);
+                       uiop->uio_resid = 0;
                }
                if (td && td->td_proc && (vp->v_flag & VTEXT) &&
                    np->n_mtime != np->n_vattr.va_mtime.tv_sec) {
@@ -1547,9 +1539,8 @@ nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize)
 
        np->n_size = nsize;
 
-       if (np->n_size < tsize) {
+       if (nsize < tsize) {
                struct buf *bp;
-               daddr_t lbn;
                off_t loffset;
                int bufsize;
 
@@ -1559,7 +1550,6 @@ nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize)
                 * buffer that now needs to be truncated.
                 */
                error = vtruncbuf(vp, nsize, biosize);
-               lbn = nsize / biosize;
                bufsize = nsize & (biosize - 1);
                loffset = nsize - bufsize;
                bp = nfs_getcacheblk(vp, loffset, bufsize, td);
@@ -1567,7 +1557,7 @@ nfs_meta_setsize(struct vnode *vp, struct thread *td, u_quad_t nsize)
                        bp->b_dirtyoff = bp->b_bcount;
                if (bp->b_dirtyend > bp->b_bcount)
                        bp->b_dirtyend = bp->b_bcount;
-               bp->b_flags |= B_RELBUF;  /* don't leave garbage around */
+               bp->b_flags |= B_RELBUF;    /* don't leave garbage around */
                brelse(bp);
        } else {
                vnode_pager_setsize(vp, nsize);
@@ -1664,23 +1654,28 @@ nfs_readrpc_bio_done(nfsm_info_t info)
        info->mrep = NULL;
 
        /*
-        * No error occured, fill the hole if any
+        * No error occured, if retlen is less then bcount and no EOF
+        * and NFSv3 a zero-fill short read occured.
+        *
+        * For NFSv2 a short-read indicates EOF.
         */
-       if (retlen < bp->b_bcount) {
+       if (retlen < bp->b_bcount && info->v3 && eof == 0) {
                bzero(bp->b_data + retlen, bp->b_bcount - retlen);
+               retlen = bp->b_bcount;
        }
-       bp->b_resid = bp->b_bcount - retlen;
-#if 0
-       /* retlen */
-       tsiz -= retlen;
-       if (info.v3) {
-               if (eof || retlen == 0) {
-                       tsiz = 0;
-               }
-       } else if (retlen < len) {
-               tsiz = 0;
+
+       /*
+        * If we hit an EOF we still zero-fill, but return the expected
+        * b_resid anyway.  This should normally not occur since async
+        * BIOs are not used for read-before-write case.  Races against
+        * the server can cause it though and we don't want to leave
+        * garbage in the buffer.
+        */
+       if (retlen < bp->b_bcount) {
+               bzero(bp->b_data + retlen, bp->b_bcount - retlen);
        }
-#endif
+       bp->b_resid = 0;
+       /* bp->b_resid = bp->b_bcount - retlen; */
 nfsmout:
        kfree(info, M_NFSREQ);
        if (error) {
@@ -1999,7 +1994,6 @@ nfsmout:
                bp->b_resid = 0;
                biodone(bio);
        } else {
-               kprintf("commitrpc_bioC %lld -> CHAIN WRITE\n", bio->bio_offset);
                nfs_writerpc_bio(info->vp, bio);
        }
 }
index 5cad213..edac0c6 100644 (file)
@@ -1229,9 +1229,7 @@ nfsmout:
 }
 
 /*
- * nfs read rpc.
- *
- * If bio is non-NULL and asynchronous
+ * nfs synchronous read rpc using UIO
  */
 int
 nfs_readrpc_uio(struct vnode *vp, struct uio *uiop)
@@ -1240,6 +1238,7 @@ nfs_readrpc_uio(struct vnode *vp, struct uio *uiop)
        struct nfsmount *nmp;
        int error = 0, len, retlen, tsiz, eof, attrflag;
        struct nfsm_info info;
+       off_t tmp_off;
 
        info.mrep = NULL;
        info.v3 = NFS_ISV3(vp);
@@ -1249,8 +1248,10 @@ nfs_readrpc_uio(struct vnode *vp, struct uio *uiop)
 #endif
        nmp = VFSTONFS(vp->v_mount);
        tsiz = uiop->uio_resid;
-       if (uiop->uio_offset + tsiz > nmp->nm_maxfilesize)
+       tmp_off = uiop->uio_offset + tsiz;
+       if (tmp_off > nmp->nm_maxfilesize || tmp_off < uiop->uio_offset)
                return (EFBIG);
+       tmp_off = uiop->uio_offset;
        while (tsiz > 0) {
                nfsstats.rpccnt[NFSPROC_READ]++;
                len = (tsiz > nmp->nm_rsize) ? nmp->nm_rsize : tsiz;
@@ -1276,15 +1277,31 @@ nfs_readrpc_uio(struct vnode *vp, struct uio *uiop)
                } else {
                        ERROROUT(nfsm_loadattr(&info, vp, NULL));
                }
-               NEGATIVEOUT(retlen = nfsm_strsiz(&info, nmp->nm_rsize));
+               NEGATIVEOUT(retlen = nfsm_strsiz(&info, len));
                ERROROUT(nfsm_mtouio(&info, uiop, retlen));
                m_freem(info.mrep);
                info.mrep = NULL;
+
+               /*
+                * Handle short-read from server (NFSv3).  If EOF is not
+                * flagged (and no error occurred), but retlen is less
+                * then the request size, we must zero-fill the remainder.
+                */
+               if (retlen < len && info.v3 && eof == 0) {
+                       ERROROUT(uiomovez(len - retlen, uiop));
+                       retlen = len;
+               }
                tsiz -= retlen;
+
+               /*
+                * Terminate loop on EOF or zero-length read.
+                *
+                * For NFSv2 a short-read indicates EOF, not zero-fill,
+                * and also terminates the loop.
+                */
                if (info.v3) {
-                       if (eof || retlen == 0) {
+                       if (eof || retlen == 0)
                                tsiz = 0;
-                       }
                } else if (retlen < len) {
                        tsiz = 0;
                }