Re: Vnode API cleanup pass 2a

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

   Date: Mon, 20 Jan 2014 16:01:07 +0100
   From: J. Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>

   Back to vnode creation operations returning unlocked vnodes I put a new
   diff to http://www.netbsd.org/~hannken/vnode-pass2a-4.diff

   Any more objections or OK to commit?

On further consideration, this looks to me like the right approach
now.  I gave it a quick skim, but not a thorough review.  It looks
pretty good, modulo a few little nits below.  I assume all the atf
tests and your other stress tests still pass?

   @@ -517,8 +517,10 @@ zfs_replay_create(zfsvfs_t *zfsvfs, lr_c
                   error = VOP_MKDIR(ZTOV(dzp), &vp, &cn, &xva.xva_vattr 
/*,vflg*/);
                   break;
           case TX_MKXATTR:
                   error = zfs_make_xattrdir(dzp, &xva.xva_vattr, &vp, kcred);
   +               if (error == 0 && vp != NULL)
   +                       VOP_UNLOCK(vp);

This looks suspicious -- does zfs_make_xattrdir return a locked vnode?
I don't immediately see evidence that it does.  (Also, if it turns out
this branch is appropriate, shouldn't ((error == 0) == (vp != NULL))
be guaranteed?)

   @@ -1563,10 +1553,11 @@ coda_symlink(void *v)
           saved_cn_flags = cnp->cn_flags;
           cnp->cn_flags &= ~(MODMASK | OPMASK);
           cnp->cn_flags |= LOOKUP;
           error = VOP_LOOKUP(dvp, ap->a_vpp, cnp);
   +       if (error == 0)
   +               VOP_UNLOCK(*ap->a_vpp);

Can you leave an XXX comment here marking it as a provisional kludge
until VOP_LOOKUP becomes sane (i.e., returns the vnode unlocked)?

   @@ -601,8 +601,10 @@ smbfs_create(void *v)
           error = smbfs_smb_lookup(dnp, name, nmlen, &fattr, &scred);
           if (error)
                   goto out;
           error = smbfs_nget(VTOVFS(dvp), dvp, name, nmlen, &fattr, ap->a_vpp);
   +       if (error == 0)
   +               VOP_UNLOCK(*ap->a_vpp);
           if (error)
                   goto out;

Can you do the VOP_UNLOCK unconditionally after the `if (error)'
branch?  Generally I find it much easier to read `branch on error'
than `branch on success' and I try to adhere to that in new code.
There are a few other cases of this in your patch, although some match
the surrounding style of doing `if (error == 0)' or would require new
labels so I wouldn't change those.

   --- sys/rump/librump/rumpvfs/rumpfs.c        17 Jan 2014 10:55:03 -0000      
1.122
   +++ sys/rump/librump/rumpvfs/rumpfs.c        20 Jan 2014 09:10:53 -0000
   @@ -564,9 +564,10 @@ makeprivate(enum vtype vt, mode_t mode, 
           return rn;
    }

    static int
   -makevnode(struct mount *mp, struct rumpfs_node *rn, struct vnode **vpp)
   +makevnode(struct mount *mp, struct rumpfs_node *rn, struct vnode **vpp,
   +    bool lock_result)

There are only a couple cases of makevnode(..., true) in your patch,
in rump_vop_lookup and rumpfs_mountfs.  Instead of adding an argument
to makevnode, why not just call vn_lock after makevnode in those
cases?

   @@ -183,16 +184,17 @@ ext2fs_mknod(void *v)
            * Remove inode so that it will be reloaded by VFS_VGET and
            * checked to see if it is an alias of an existing entry in
            * the inode cache.
            */
   -       VOP_UNLOCK(*vpp);
           (*vpp)->v_type = VNON;
   +       VOP_UNLOCK(*vpp);

Can you do this one in a separate tiny commit?  It looks like an
unrelated bug fix.