Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 13 Sep 2010 20:29:10 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r212583 - stable/8/sys/kern
Message-ID:  <201009132029.o8DKTAUa030348@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kib
Date: Mon Sep 13 20:29:09 2010
New Revision: 212583
URL: http://svn.freebsd.org/changeset/base/212583

Log:
  MFC r211213:
  The buffers b_vflags field is not always properly protected by
  bufobj lock. If b_bufobj is not NULL, then bufobj lock should be
  held when manipulating the flags. Not doing this sometimes leaves
  BV_BKGRDINPROG to be erronously set, causing softdep' getdirtybuf() to
  stuck indefinitely in "getbuf" sleep, waiting for background write to
  finish which is not actually performed.
  
  Add BO_LOCK() in the cases where it was missed.

Modified:
  stable/8/sys/kern/vfs_bio.c
  stable/8/sys/kern/vfs_subr.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)

Modified: stable/8/sys/kern/vfs_bio.c
==============================================================================
--- stable/8/sys/kern/vfs_bio.c	Mon Sep 13 20:11:18 2010	(r212582)
+++ stable/8/sys/kern/vfs_bio.c	Mon Sep 13 20:29:09 2010	(r212583)
@@ -391,6 +391,8 @@ bufcountwakeup(struct buf *bp) 
 
 	KASSERT((bp->b_vflags & BV_INFREECNT) == 0,
 	    ("buf %p already counted as free", bp));
+	if (bp->b_bufobj != NULL)
+		mtx_assert(BO_MTX(bp->b_bufobj), MA_OWNED);
 	bp->b_vflags |= BV_INFREECNT;
 	old = atomic_fetchadd_int(&numfreebuffers, 1);
 	KASSERT(old >= 0 && old < nbuf,
@@ -690,6 +692,8 @@ bremfree(struct buf *bp)
 	if ((bp->b_flags & B_INVAL) || (bp->b_flags & B_DELWRI) == 0) {
 		KASSERT((bp->b_vflags & BV_INFREECNT) != 0,
 		    ("buf %p not counted in numfreebuffers", bp));
+		if (bp->b_bufobj != NULL)
+			mtx_assert(BO_MTX(bp->b_bufobj), MA_OWNED);
 		bp->b_vflags &= ~BV_INFREECNT;
 		old = atomic_fetchadd_int(&numfreebuffers, -1);
 		KASSERT(old > 0, ("numfreebuffers dropped to %d", old - 1));
@@ -746,6 +750,8 @@ bremfreel(struct buf *bp)
 	if ((bp->b_flags & B_INVAL) || (bp->b_flags & B_DELWRI) == 0) {
 		KASSERT((bp->b_vflags & BV_INFREECNT) != 0,
 		    ("buf %p not counted in numfreebuffers", bp));
+		if (bp->b_bufobj != NULL)
+			mtx_assert(BO_MTX(bp->b_bufobj), MA_OWNED);
 		bp->b_vflags &= ~BV_INFREECNT;
 		old = atomic_fetchadd_int(&numfreebuffers, -1);
 		KASSERT(old > 0, ("numfreebuffers dropped to %d", old - 1));
@@ -1390,8 +1396,16 @@ brelse(struct buf *bp)
 	/* enqueue */
 	mtx_lock(&bqlock);
 	/* Handle delayed bremfree() processing. */
-	if (bp->b_flags & B_REMFREE)
+	if (bp->b_flags & B_REMFREE) {
+		struct bufobj *bo;
+
+		bo = bp->b_bufobj;
+		if (bo != NULL)
+			BO_LOCK(bo);
 		bremfreel(bp);
+		if (bo != NULL)
+			BO_UNLOCK(bo);
+	}
 	if (bp->b_qindex != QUEUE_NONE)
 		panic("brelse: free buffer onto another queue???");
 
@@ -1452,8 +1466,16 @@ brelse(struct buf *bp)
 	 * if B_INVAL is set ).
 	 */
 
-	if (!(bp->b_flags & B_DELWRI))
+	if (!(bp->b_flags & B_DELWRI)) {
+		struct bufobj *bo;
+
+		bo = bp->b_bufobj;
+		if (bo != NULL)
+			BO_LOCK(bo);
 		bufcountwakeup(bp);
+		if (bo != NULL)
+			BO_UNLOCK(bo);
+	}
 
 	/*
 	 * Something we can maybe free or reuse
@@ -1482,6 +1504,8 @@ brelse(struct buf *bp)
 void
 bqrelse(struct buf *bp)
 {
+	struct bufobj *bo;
+
 	CTR3(KTR_BUF, "bqrelse(%p) vp %p flags %X", bp, bp->b_vp, bp->b_flags);
 	KASSERT(!(bp->b_flags & (B_CLUSTER|B_PAGING)),
 	    ("bqrelse: inappropriate B_PAGING or B_CLUSTER bp %p", bp));
@@ -1492,10 +1516,15 @@ bqrelse(struct buf *bp)
 		return;
 	}
 
+	bo = bp->b_bufobj;
 	if (bp->b_flags & B_MANAGED) {
 		if (bp->b_flags & B_REMFREE) {
 			mtx_lock(&bqlock);
+			if (bo != NULL)
+				BO_LOCK(bo);
 			bremfreel(bp);
+			if (bo != NULL)
+				BO_UNLOCK(bo);
 			mtx_unlock(&bqlock);
 		}
 		bp->b_flags &= ~(B_ASYNC | B_NOCACHE | B_AGE | B_RELBUF);
@@ -1505,8 +1534,13 @@ bqrelse(struct buf *bp)
 
 	mtx_lock(&bqlock);
 	/* Handle delayed bremfree() processing. */
-	if (bp->b_flags & B_REMFREE)
+	if (bp->b_flags & B_REMFREE) {
+		if (bo != NULL)
+			BO_LOCK(bo);
 		bremfreel(bp);
+		if (bo != NULL)
+			BO_UNLOCK(bo);
+	}
 	if (bp->b_qindex != QUEUE_NONE)
 		panic("bqrelse: free buffer onto another queue???");
 	/* buffers with stale but valid contents */
@@ -1541,8 +1575,13 @@ bqrelse(struct buf *bp)
 	}
 	mtx_unlock(&bqlock);
 
-	if ((bp->b_flags & B_INVAL) || !(bp->b_flags & B_DELWRI))
+	if ((bp->b_flags & B_INVAL) || !(bp->b_flags & B_DELWRI)) {
+		if (bo != NULL)
+			BO_LOCK(bo);
 		bufcountwakeup(bp);
+		if (bo != NULL)
+			BO_UNLOCK(bo);
+	}
 
 	/*
 	 * Something we can maybe free or reuse.
@@ -1878,7 +1917,11 @@ restart:
 
 		KASSERT((bp->b_flags & B_DELWRI) == 0, ("delwri buffer %p found in queue %d", bp, qindex));
 
+		if (bp->b_bufobj != NULL)
+			BO_LOCK(bp->b_bufobj);
 		bremfreel(bp);
+		if (bp->b_bufobj != NULL)
+			BO_UNLOCK(bp->b_bufobj);
 		mtx_unlock(&bqlock);
 
 		if (qindex == QUEUE_CLEAN) {
@@ -2613,7 +2656,9 @@ loop:
 			bp->b_flags &= ~B_CACHE;
 		else if ((bp->b_flags & (B_VMIO | B_INVAL)) == 0)
 			bp->b_flags |= B_CACHE;
+		BO_LOCK(bo);
 		bremfree(bp);
+		BO_UNLOCK(bo);
 
 		/*
 		 * check for size inconsistancies for non-VMIO case.

Modified: stable/8/sys/kern/vfs_subr.c
==============================================================================
--- stable/8/sys/kern/vfs_subr.c	Mon Sep 13 20:11:18 2010	(r212582)
+++ stable/8/sys/kern/vfs_subr.c	Mon Sep 13 20:29:09 2010	(r212583)
@@ -1249,13 +1249,17 @@ flushbuflist( struct bufv *bufv, int fla
 		 */
 		if (((bp->b_flags & (B_DELWRI | B_INVAL)) == B_DELWRI) &&
 		    (flags & V_SAVE)) {
+			BO_LOCK(bo);
 			bremfree(bp);
+			BO_UNLOCK(bo);
 			bp->b_flags |= B_ASYNC;
 			bwrite(bp);
 			BO_LOCK(bo);
 			return (EAGAIN);	/* XXX: why not loop ? */
 		}
+		BO_LOCK(bo);
 		bremfree(bp);
+		BO_UNLOCK(bo);
 		bp->b_flags |= (B_INVAL | B_RELBUF);
 		bp->b_flags &= ~B_ASYNC;
 		brelse(bp);
@@ -1307,7 +1311,9 @@ restart:
 			    BO_MTX(bo)) == ENOLCK)
 				goto restart;
 
+			BO_LOCK(bo);
 			bremfree(bp);
+			BO_UNLOCK(bo);
 			bp->b_flags |= (B_INVAL | B_RELBUF);
 			bp->b_flags &= ~B_ASYNC;
 			brelse(bp);
@@ -1329,7 +1335,9 @@ restart:
 			    LK_EXCLUSIVE | LK_SLEEPFAIL | LK_INTERLOCK,
 			    BO_MTX(bo)) == ENOLCK)
 				goto restart;
+			BO_LOCK(bo);
 			bremfree(bp);
+			BO_UNLOCK(bo);
 			bp->b_flags |= (B_INVAL | B_RELBUF);
 			bp->b_flags &= ~B_ASYNC;
 			brelse(bp);
@@ -1361,7 +1369,9 @@ restartsync:
 			VNASSERT((bp->b_flags & B_DELWRI), vp,
 			    ("buf(%p) on dirty queue without DELWRI", bp));
 
+			BO_LOCK(bo);
 			bremfree(bp);
+			BO_UNLOCK(bo);
 			bawrite(bp);
 			BO_LOCK(bo);
 			goto restartsync;

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009132029.o8DKTAUa030348>