On Jan 20, 2014, at 5:02 PM, Taylor R Campbell <[email protected]> wrote:
> Date: Mon, 20 Jan 2014 16:01:07 +0100 > From: J. Hannken-Illjes <[email protected]> > > 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 - [email protected] - TU Braunschweig (Germany)
