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

The following reply was made to PR kern/60531; it has been noted by GNATS.

From: Taylor R Campbell <[email protected]>
To: [email protected], [email protected]
Cc: [email protected], [email protected]
Subject: Re: kern/60531: looping "vflushbuf: dirty" message
Date: Sun, 2 Aug 2026 19:43:58 +0000

 > 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