Date: Mon, 3 Mar 2014 11:11:04 +0100 From: "J. Hannken-Illjes" <hann...@eis.cs.tu-bs.de>
Add an interface to iterate over a vnode list: void vfs_vnode_iterator_init(struct mount *mp, void **marker) void vfs_vnode_iterator_destroy(void *marker) bool vfs_vnode_iterator_next(void *marker, struct vnode **vpp) Looks pretty good to me, but let's make the API a little type-safer: /* mount.h */ struct vnode_iterator; /* opaque */ void vfs_vnode_iterator_init(struct mount *mp, struct vnode_iterator **vip); void vfs_vnode_iterator_destroy(struct vnode_iterator *vi); bool vfs_vnode_iterator_next(struct vnode_iterator *vi, struct vnode **vpp); /* vfs_mount.c */ struct vnode_iterator { struct vnode vi_vnode; }; void vfs_vnode_iterator_init(struct mount *mp, struct vnode_iterator **vip) { struct vnode *vp; vp = vnalloc(mp); mutex_enter(&mntvnode_lock); TAILQ_INSERT_HEAD(&mp->mnt_vnodelist, vp, v_mntvnodes); mutex_exit(&mntvnode_lock); *vip = (struct vnode_iterator *)vp; } void vfs_vnode_iterator_destroy(struct vnode_iterator *vi) { struct vnode *vp = &vi->vi_vnode; ... } Why not have vfs_vnode_iterator_next return the vnode pointer or NULL? struct vnode *vfs_vnode_iterator_next(struct vnode_iterator *vi); while ((vp = vfs_vnode_iterator_next(vi)) != NULL) { ... } A couple little comments on the patch: --- sys/kern/vfs_mount.c 27 Feb 2014 13:00:06 -0000 1.26 +++ sys/kern/vfs_mount.c 3 Mar 2014 10:02:29 -0000 @@ -374,8 +374,68 @@ vunmark(vnode_t *mvp) ... +void +vfs_vnode_iterator_destroy(void *marker) +{ + struct vnode *mvp = marker; + + KASSERT((mvp->v_iflag & VI_MARKER) != 0); I'd prefer to say vismarker(mvp) here, or at least ISSET(mvp->v_iflag, VI_MARKER) if you want to kill vismarker -- my eyes cross when I see the mess of (vp->v_iflag & VI_XLXKMRK) {!,}= 0 everywhere. /* * If WRITECLOSE is set, only flush out regular file * vnodes open for writing. */ - if ((flags & WRITECLOSE) && - (vp->v_writecount == 0 || vp->v_type != VREG)) { + if ((flags & WRITECLOSE) && vp->v_type == VREG) { + mutex_enter(vp->v_interlock); + if (vp->v_writecount == 0) { + mutex_exit(vp->v_interlock); + vrele(vp); + continue; + } mutex_exit(vp->v_interlock); - continue; This looks suspect: we make a decision based on vp->v_writecount under the lock, and then drop the lock. Perhaps it doesn't matter if this is an opportunistic thing (and I suspect since we don't hold the vnode lock the original code had the same issue), but a comment to that effect would be nice.