Re: Vnode API change: mnt_vnodelist traversal

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

   Date: Mon, 3 Mar 2014 11:11:04 +0100
   From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>

   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.