Re: vnode lock and v_numoutput

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Mon, 26 Jan 2015 12:10:53 +0100
   From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>

   I'm not sure I understand; the "bump v_numoutput" is always needed;
   but if skipped is 0 the last nestiobuf_done() call is done by
   biodone() callback and not by the called. So the bump has to happen
   before the last buf is queued.

Ah, yes, sorry -- we need one bump for every nested buf that will be
queued and biodone'd, and always one bump for the parent which will
always be biodone'd either by a nested buf or by nestiobuf_done on the
parent.

However, I think there's a bug in your new patch too: suppose VOP_BMAP
fails on one of the middle blocks -- then we won't call VOP_STRATEGY
for the last block even if VOP_BMAP succeeds for that block.

I believe the attached patch will fix that, but I haven't tested it.

1. Guarantee to call VOP_STRATEGY on every nestiobuf.
2. Guarantee no blocking between nestiobuf_setup or v_numoutput++ and
the corresponding VOP_STRATEGY or nestiobuf_done.
3. Guarantee one nestiobuf_setup or v_numoutput++ per VOP_STRATEGY or
nestiobuf_done.

diff -c /home/riastradh/netbsd/nbdev/src/sys/dev/vnd.c /tmp/riastradh/buffer-content-14028wVF
--- /home/riastradh/netbsd/nbdev/src/sys/dev/vnd.c	2015-01-07 01:37:45.000000000 +0000
+++ /tmp/riastradh/buffer-content-14028wVF	2015-01-26 18:49:09.000000000 +0000
@@ -795,6 +795,7 @@
 	size_t resid, sz;
 	off_t bn, offset;
 	struct vnode *vp;
+	struct buf *nbp = NULL;
 
 	flags = obp->b_flags;
 
@@ -822,10 +823,14 @@
 	bp->b_resid = bp->b_bcount;
 	for (offset = 0, resid = bp->b_resid; resid;
 	    resid -= sz, offset += sz) {
-		struct buf *nbp;
 		daddr_t nbn;
 		int off, nra;
 
+		if (nbp) {
+			VOP_STRATEGY(vp, nbp);
+			nbp = NULL;
+		}
+
 		nra = 0;
 		vn_lock(vnd->sc_vp, LK_EXCLUSIVE | LK_RETRY);
 		error = VOP_BMAP(vnd->sc_vp, bn / bsize, &vp, &nbn, &nra);
@@ -862,6 +867,7 @@
 			    nbn, sz);
 #endif
 
+		KASSERT(nbp == NULL);
 		nbp = getiobuf(vp, true);
 		nestiobuf_setup(bp, nbp, offset, sz);
 		nbp->b_blkno = nbn + btodb(off);
@@ -875,9 +881,21 @@
 			    nbp->vb_buf.b_flags, nbp->vb_buf.b_data,
 			    nbp->vb_buf.b_bcount);
 #endif
-		VOP_STRATEGY(vp, nbp);
 		bn += sz;
 	}
+	/*
+	 * Bump v_numoutput for the nested I/O master only after we
+	 * have done all the blocking we will do and before submitting
+	 * the last buf to disk.
+	 */
+	if (!(flags & B_READ)) {
+		vp = bp->b_vp;
+		mutex_enter(vp->v_interlock);
+		vp->v_numoutput++;
+		mutex_exit(vp->v_interlock);
+	}
+	if (nbp)
+		VOP_STRATEGY(vp, nbp);
 	nestiobuf_done(bp, skipped, error);
 }
 

Diff finished.  Mon Jan 26 18:49:09 2015