Re: kern/60531: looping "vflushbuf: dirty" message

> Date: Sun, 2 Aug 2026 15:25:08 +0000
> From: Taylor R Campbell <[email protected]>
>=20
> I added KASSERT(VOP_ISLOCKED(vp)) to vflushbuf and to the path in
> reassignbuf that puts a buffer on vp->v_dirtyblkhd, and immediately
> hit:
>=20
> # mount -u -o rw,noatime,nodevmtime,log /
> [  20.8447068] panic: kernel diagnostic assertion "VOP_ISLOCKED(vp)" fail=
ed: file "/home/riastradh/netbsd/current/src/sys/kern/vfs_subr.c", line 526
> [  20.8611358] cpu0: Begin traceback...
> [  20.8611358] trace fp ffffc000c64ff680
> ...
> [  20.9297182] fp ffffc000c64ff7d0 bdwrite() at ffffc000006743e8 netbsd:b=
dwrite+0x128
> [  20.9507110] fp ffffc000c64ff800 ffs_nodealloccg() at ffffc00000536178 =
netbsd:ffs_nodealloccg+0x730
> ...
> [  21.0457100] fp ffffc000c64ffa00 vcache_new() at ffffc00000696738 netbs=
d:vcache_new+0x94
> ...
> So I think vflushbuf's assumption that buffers are added to
> v_dirtyblkhd only while the vnode lock is held may not be valid!
>=20
> Now, this path involves creating a new vnode, so that may not be
> relevant here.

dholland and I took a closer look at this path, and it turns out the
assertion failure is not about the vnode being created -- it's about
the block device the file system is mounted from:

   1261 static daddr_t
   1262 ffs_nodealloccg(struct inode *ip, u_int cg, daddr_t ipref, int mode=
, int realsize,
   1263     int flags)
   1264 {
...
   1291 	error =3D bread(ip->i_devvp, FFS_FSBTODB(fs, cgtod(fs, cg)),
   1292 		(int)fs->fs_cgsize, B_MODIFY, &bp);
   1293 	if (error)
   1294 		goto fail;
...
   1416 	if (ibp !=3D NULL) {
   1417 		bwrite(ibp);
   1418 		bwrite(bp);
   1419 	} else
=3D> 1420 		bdwrite(bp);
   1421 	return ((ino_t)(cg * fs->fs_ipg + ipref));

https://nxr.netbsd.org/xref/src/sys/ufs/ffs/ffs_alloc.c?r=3D1.175#1252

Here ip->i_devvp is presumably the block device the file system is
mounted from (why do we even have ip->i_devvp instead of just using
ump->um_devvp?):

   2182 int
   2183 ffs_loadvnode(struct mount *mp, struct vnode *vp,
   2184     const void *key, size_t key_len, const void **new_key)
   2185 {
...
   2212 	ip->i_devvp =3D ump->um_devvp;

https://nxr.netbsd.org/xref/src/sys/ufs/ffs/ffs_vfsops.c?r=3D1.385#2212

   2234 int
   2235 ffs_newvnode(struct mount *mp, struct vnode *dvp, struct vnode *vp,
   2236     struct vattr *vap, kauth_cred_t cred, void *extra,
   2237     size_t *key_len, const void **new_key)
   2238 {
...
   2335 	ip->i_devvp =3D ump->um_devvp;

https://nxr.netbsd.org/xref/src/sys/ufs/ffs/ffs_vfsops.c?r=3D1.385#2335

I suspect there a are a lot of paths like this: bread/bdwrite on the
block device to edit file system metadata.

What are the bad consequences of calling reassignbuf without the vnode
lock held?  Well, vflushbuf can only guarantee completion in bounded
time if either:

(a) other threads are blocked from adding new dirty bufs,
or
(b) vflushbuf can distinguish the bufs that were already on the queue
    when it started from those that were added after it started, e.g.
    with another queue or a flush generation counter.

Without one of those two options, v_numoutput might never reach zero.
On the other hand, once it _has_ reached zero once, we don't really
need to loop at all: the printf is only evidence that the system
_could_ have waited indefinitely in vflushbuf because new bwrites kept
appearing as we waited -- but it _didn't_, because v_numoutput finally
went down to zero.

So I think we should do some combination of:

1. delete the `goto loop' part

2. put the vprint under #ifdef DEBUG

3. vprint only once per call to vflushbuf, or at most twice -- once
   when we notice a potential problem, once when we've finally broken
   out of the loop

4. apply a global rate limit to the vprint

5. put a flush generation number on struct buf and wait for that to
   drain (i.e., wait until there are no dirty bufs queued with the old
   generation number) instead of waiting for v_numoutput to drain


Curiously, this printf was put under #ifdef DIAGNOSTIC in the CSRG BSD
tree back in 1992 when vflushbuf was deleted and open-coded in
ffs_fsync:

 int
 ffs_fsync(ap)
-	struct vop_fsync_args *ap;
+	struct vop_fsync_args /* {
+		struct vnode *a_vp;
+		struct ucred *a_cred;
+		int a_waitfor;
+		struct proc *a_p;
+	} */ *ap;
 {
...
-	vflushbuf(ap->a_vp, ap->a_waitfor =3D=3D MNT_WAIT ? B_SYNC : 0);
+	/*
+	 * Flush all dirty buffers associated with a vnode.
+	 */
+loop:
+	s =3D splbio();
+	for (bp =3D vp->v_dirtyblkhd; bp; bp =3D nbp) {
+		nbp =3D bp->b_blockf;
+		if ((bp->b_flags & B_BUSY))
+			continue;
+		if ((bp->b_flags & B_DELWRI) =3D=3D 0)
+			panic("ffs_fsync: not dirty");
+		bremfree(bp);
+		bp->b_flags |=3D B_BUSY;
+		splx(s);
+		/*
+		 * Wait for I/O associated with indirect blocks to complete,
+		 * since there is no way to quickly wait for them below.
+		 */
+		if (bp->b_vp =3D=3D vp || ap->a_waitfor =3D=3D MNT_NOWAIT)
+			(void) bawrite(bp);
+		else
+			(void) bwrite(bp);
+		goto loop;
+	}
+	if (ap->a_waitfor =3D=3D MNT_WAIT) {
+		while (vp->v_numoutput) {
+			vp->v_flag |=3D VBWAIT;
+			sleep((caddr_t)&vp->v_numoutput, PRIBIO + 1);
+		}
+#ifdef DIAGNOSTIC
+		if (vp->v_dirtyblkhd) {
+			vprint("ffs_fsync: dirty", vp);
+			goto loop;
+		}
+#endif
+	}

https://github.com/robohack/ucb-csrg-bsd/commit/fce3da350147b2a731b529826ce=
af7f89fb33311