Re: Vnode API change: mnt_vnodelist traversal

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

On Mar 3, 2014, at 3:28 PM, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

>   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;
> };

No.  I want to keep the state opaque to the caller.  There is no need for
the calling party to know anything about state internals.

> 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?

I prefer bool/int result and returning data through argument list.

> 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.

Ok.

>                   /*
>                    * 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.

Flag WRITECLOSE is used for r/w -> ro mount update.  This has much more
problems, if I remember right there is even a PR open.  So not now ...

New diff at http://www.netbsd.org/~hannken/vnode-pass4-2.diff

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)