Re: brelse() panic

["Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>]

Sascha Wildner wrote:
> After discussing things on IRC with corecode, he came up with a fix that 
> works for me (see the followup to this mail).

I noticed two unrelated bugs, which are both fixed in the following patch:

1. rbp is geteblk()'ed, but in case of a VOP_BMAP() error returned without B_INVAL set.

2. I moved the read-ahead after starting the real read for bp, so that I can skip this if an error occured in the reading of bp.  Before rbp might have been a CLUSTER and didn't get special treatment for freeing (hence the panic).

cheers
 simon


diff -r 44daa8fb3603 sys/kern/vfs_cluster.c
--- a/sys/kern/vfs_cluster.c	Sun Jul 09 03:36:13 2006 +0200
+++ b/sys/kern/vfs_cluster.c	Sun Jul 09 21:34:01 2006 +0200
@@ -200,46 +200,6 @@ single_block_read:
	}

	/*
-	 * If we have been doing sequential I/O, then do some read-ahead.
-	 */
-	rbp = NULL;
-	if (seqcount &&
-	    loffset < origoffset + seqcount * size &&
-	    loffset + size <= filesize
-	) {
-		rbp = getblk(vp, loffset, size, 0, 0);
-		if ((rbp->b_flags & B_CACHE) == 0) {
-			int nblksread;
-			int ntoread;
-			int burstbytes;
-
-			error = VOP_BMAP(vp, loffset, NULL,
-					 &doffset, &burstbytes, NULL);
-			if (error || doffset == NOOFFSET) {
-				brelse(rbp);
-				rbp = NULL;
-				goto no_read_ahead;
-			}
-			ntoread = burstbytes / size;
-			nblksread = (totread + size - 1) / size;
-			if (seqcount < nblksread)
-				seqcount = nblksread;
-			if (seqcount < ntoread)
-				ntoread = seqcount;
-
-			rbp->b_flags |= B_RAM;
-			if (burstbytes) {
-				rbp = cluster_rbuild(vp, filesize, loffset,
-						     doffset, size, 
-						     ntoread, rbp, 1);
-			} else {
-				rbp->b_bio2.bio_offset = doffset;
-			}
-		}
-	}
-no_read_ahead:
-
-	/*
	 * Handle the synchronous read.  This only occurs if B_CACHE was
	 * not set.  bp (and rbp) could be either a cluster bp or a normal
	 * bp depending on the what cluster_rbuild() decided to do.  If
@@ -262,39 +222,72 @@ no_read_ahead:
	}

	/*
-	 * And if we have read-aheads, do them too. 
-	 */
-	if (rbp) {
-		if (error) {
+	 * If we have been doing sequential I/O, then do some read-ahead.
+	 */
+	rbp = NULL;
+	if (!error &&
+	    seqcount &&
+	    loffset < origoffset + seqcount * size &&
+	    loffset + size <= filesize
+	) {
+		int nblksread;
+		int ntoread;
+		int burstbytes;
+
+		rbp = getblk(vp, loffset, size, 0, 0);
+		if ((rbp->b_flags & B_CACHE)) {
+			bqrelse(rbp);
+			goto no_read_ahead;
+		}
+
+		error = VOP_BMAP(vp, loffset, NULL,
+				 &doffset, &burstbytes, NULL);
+		if (error || doffset == NOOFFSET) {
+			rbp->b_flags |= B_INVAL;
			brelse(rbp);
-		} else if (rbp->b_flags & B_CACHE) {
-			bqrelse(rbp);
+			rbp = NULL;
+			goto no_read_ahead;
+		}
+		ntoread = burstbytes / size;
+		nblksread = (totread + size - 1) / size;
+		if (seqcount < nblksread)
+			seqcount = nblksread;
+		if (seqcount < ntoread)
+			ntoread = seqcount;
+
+		rbp->b_flags |= B_RAM;
+		if (burstbytes) {
+			rbp = cluster_rbuild(vp, filesize, loffset,
+					     doffset, size, 
+					     ntoread, rbp, 1);
		} else {
+			rbp->b_bio2.bio_offset = doffset;
+		}
#if defined(CLUSTERDEBUG)
-			if (rcluster) {
-				if (bp)
-					printf("A+(%lld,%d,%lld,%d) ",
-					    rbp->b_loffset, rbp->b_bcount,
-					    rbp->b_loffset - origoffset,
-					    seqcount);
-				else
-					printf("A(%lld,%d,%lld,%d) ",
-					    rbp->b_loffset, rbp->b_bcount,
-					    rbp->b_loffset - origoffset,
-					    seqcount);
-			}
+		if (rcluster) {
+			if (bp)
+				printf("A+(%lld,%d,%lld,%d) ",
+				    rbp->b_loffset, rbp->b_bcount,
+				    rbp->b_loffset - origoffset,
+				    seqcount);
+			else
+				printf("A(%lld,%d,%lld,%d) ",
+				    rbp->b_loffset, rbp->b_bcount,
+				    rbp->b_loffset - origoffset,
+				    seqcount);
+		}
#endif
-			rbp->b_flags &= ~(B_ERROR|B_INVAL);
-			rbp->b_flags |= B_ASYNC;
-			rbp->b_cmd = BUF_CMD_READ;
-
-			if ((rbp->b_flags & B_CLUSTER) == 0)
-				vfs_busy_pages(vp, rbp);
-			if ((rbp->b_flags & B_ASYNC) || rbp->b_bio1.bio_done != NULL)
-				BUF_KERNPROC(rbp);
-			vn_strategy(vp, &rbp->b_bio1);
-		}
-	}
+		rbp->b_flags &= ~(B_ERROR|B_INVAL);
+		rbp->b_flags |= B_ASYNC;
+		rbp->b_cmd = BUF_CMD_READ;
+
+		if ((rbp->b_flags & B_CLUSTER) == 0)
+			vfs_busy_pages(vp, rbp);
+		BUF_KERNPROC(rbp);			/* B_ASYNC */
+		vn_strategy(vp, &rbp->b_bio1);
+	}
+no_read_ahead:
+
	if (reqbp)
		return (biowait(reqbp));
	else


--
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \

Attachment: signature.asc
Description: OpenPGP digital signature