Re: Vnode API cleanup pass 2a

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

On Jan 20, 2014, at 5:02 PM, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

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

Sure.  Now even on a 20-core amd64 with linux/kvm as a host.

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

Yes, looks like zfs_make_xattrdir returns an unlocked vnode (which means
the current implementation is wrong).  Removed this part.

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

Done.

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

Done.

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

Done.  rump_vop_lookup changed as proposed -- this lock will go with the
next pass doing the same for VOP_LOOKUP.  rumpfs_mountfs doesn't need a
lock -- an unmount can not happen here:

RCS file: /cvsroot/src/sys/rump/librump/rumpvfs/rumpfs.c,v
@@ -813,4 +812,11 @@ rump_vop_lookup(void *v)
                mutex_exit(&reclock);
                rv = makevnode(dvp->v_mount, rn, vpp);
+               if (rv == 0) {
+                       rv = vn_lock(*vpp, LK_EXCLUSIVE);
+                       if (rv != 0) {
+                               vrele(*vpp);
+                               *vpp = NULL;
+                       }
+               }
        }
        if (dotdot)
@@ -1762,5 +1768,4 @@ rumpfs_mountfs(struct mount *mp)
 
        rfsmp->rfsmp_rvp->v_vflag |= VV_ROOT;
-       VOP_UNLOCK(rfsmp->rfsmp_rvp);
 
        mp->mnt_data = rfsmp;


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


Done.

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