svn commit: r294954 - in head/sys: kern ufs/ffs

[Kirk McKusick]

Author: mckusick
Date: Wed Jan 27 21:23:01 2016
New Revision: 294954
URL: https://svnweb.freebsd.org/changeset/base/294954

Log:
  The bread() function was inconsistent about whether it would return
  a buffer pointer in the event of an error (for some errors it would
  return a buffer pointer and for other errors it would not return a
  buffer pointer). The cluster_read() function was similarly inconsistent.
  
  Clients of these functions were inconsistent in handling errors.
  Some would assume that no buffer was returned after an error and
  would thus lose buffers under certain error conditions. Others would
  assume that brelse() should always be called after an error and
  would thus panic the system under certain error conditions.
  
  To correct both of these problems with minimal code churn, bread()
  and cluster_write() now always free the buffer when returning an
  error thus ensuring that buffers will never be lost. The brelse()
  routine checks for being passed a NULL buffer pointer and silently
  returns to avoid panics. Thus both approaches to handling error
  returns from bread() and cluster_read() will work correctly.
  
  Future code should be written assuming that bread() and cluster_read()
  will never return a buffer with an error, so should not attempt to
  brelse() the buffer when an error is returned.
  
  Reviewed by: kib

Modified:
  head/sys/kern/vfs_bio.c
  head/sys/kern/vfs_cluster.c
  head/sys/ufs/ffs/ffs_inode.c

Modified: head/sys/kern/vfs_bio.c
==============================================================================
--- head/sys/kern/vfs_bio.c	Wed Jan 27 21:17:43 2016	(r294953)
+++ head/sys/kern/vfs_bio.c	Wed Jan 27 21:23:01 2016	(r294954)
@@ -1809,6 +1809,8 @@ breada(struct vnode * vp, daddr_t * rabl
  * must clear BIO_ERROR and B_INVAL prior to initiating I/O.  If B_CACHE
  * is set, the buffer is valid and we do not have to do anything, see
  * getblk(). Also starts asynchronous I/O on read-ahead blocks.
+ *
+ * Always return a NULL buffer pointer (in bpp) when returning an error.
  */
 int
 breadn_flags(struct vnode *vp, daddr_t blkno, int size, daddr_t *rablkno,
@@ -1844,6 +1846,10 @@ breadn_flags(struct vnode *vp, daddr_t b
 
 	if (readwait) {
 		rv = bufwait(bp);
+		if (rv != 0) {
+			brelse(bp);
+			*bpp = NULL;
+		}
 	}
 	return (rv);
 }
@@ -2238,6 +2244,12 @@ brelse(struct buf *bp)
 {
 	int qindex;
 
+	/*
+	 * Many function erroneously call brelse with a NULL bp under rare
+	 * error conditions. Simply return when called with a NULL bp.
+	 */
+	if (bp == NULL)
+		return;
 	CTR3(KTR_BUF, "brelse(%p) vp %p flags %X",
 	    bp, bp->b_vp, bp->b_flags);
 	KASSERT(!(bp->b_flags & (B_CLUSTER|B_PAGING)),

Modified: head/sys/kern/vfs_cluster.c
==============================================================================
--- head/sys/kern/vfs_cluster.c	Wed Jan 27 21:17:43 2016	(r294953)
+++ head/sys/kern/vfs_cluster.c	Wed Jan 27 21:23:01 2016	(r294954)
@@ -119,6 +119,8 @@ cluster_read(struct vnode *vp, u_quad_t 
 	 * get the requested block
 	 */
 	*bpp = reqbp = bp = getblk(vp, lblkno, size, 0, 0, gbflags);
+	if (bp == NULL)
+		return (EBUSY);
 	origblkno = lblkno;
 
 	/*
@@ -295,10 +297,18 @@ cluster_read(struct vnode *vp, u_quad_t 
 		curthread->td_ru.ru_inblock++;
 	}
 
-	if (reqbp)
-		return (bufwait(reqbp));
-	else
-		return (error);
+	if (reqbp) {
+		/*
+		 * Like bread, always brelse() the buffer when
+		 * returning an error.
+		 */
+		error = bufwait(reqbp);
+		if (error != 0) {
+			brelse(reqbp);
+			*bpp = NULL;
+		}
+	}
+	return (error);
 }
 
 /*

Modified: head/sys/ufs/ffs/ffs_inode.c
==============================================================================
--- head/sys/ufs/ffs/ffs_inode.c	Wed Jan 27 21:17:43 2016	(r294953)
+++ head/sys/ufs/ffs/ffs_inode.c	Wed Jan 27 21:23:01 2016	(r294954)
@@ -113,10 +113,8 @@ loop:
 	     fsbtodb(fs, ino_to_fsba(fs, ip->i_number)),
 	     (int) fs->fs_bsize, 0, 0, 0, NOCRED, flags, &bp);
 	if (error != 0) {
-		if (error != EBUSY) {
-			brelse(bp);
+		if (error != EBUSY)
 			return (error);
-		}
 		KASSERT((IS_SNAPSHOT(ip)), ("EBUSY from non-snapshot"));
 		/*
 		 * Wait for our inode block to become available.