On Feb 6, 2014, at 4:23 PM, Taylor R Campbell
<campbell+netbsd-tech-k...@mumble.net> wrote:
> Date: Mon, 3 Feb 2014 12:04:27 +0100
> From: J. Hannken-Illjes <hann...@eis.cs.tu-bs.de>
>
> As announced some weeks ago here comes the API change to VOP_LOOKUP:
>
> Change vnode operation lookup to return the resulting vnode *vpp locked.
> Change cache_lookup() to return an unlocked vnode.
>
> Sounds like the right thing to me. I skimmed through the patch, and I
> have one question and a few little nits.
>
> @@ -571,8 +571,14 @@ zfs_replay_remove(zfsvfs_t *zfsvfs, lr_r
> if (error != 0) {
> VOP_UNLOCK(ZTOV(dzp));
> goto fail;
> }
> + error = vn_lock(vp, LK_EXCLUSIVE);
>
> Why LK_EXCLUSIVE and not (LK_EXCLUSIVE | LK_RETRY)? There are a few
> other cases of this too. Are you mixing this up with a change to
> allow vn_lock to fail everywhere so we can use that for transactions
> and forced unmount? While I think that will eventually be the right
> thing, that should be in a separate patch so it can be systematically
> done for all vn_locks.
Where possible I prefer the variant without LK_RETRY for new code as the
next vnode operation would return an error anyway when run on a dead vnode.
> @@ -464,29 +464,9 @@ cache_lookup(struct vnode *dvp, const ch
> */
> [...]
> - }
> + KASSERT(error == 0);
>
> This kassert is superfluous -- the `if (error) ... return' branch is
> right above it.
OK.
> @@ -380,12 +380,12 @@ layer_lookup(void *v)
> vref(dvp);
> *ap->a_vpp = dvp;
> vrele(lvp);
> } else if (lvp != NULL) {
> - /* Note: dvp, ldvp and lvp are all locked. */
> + /* Note: dvp and ldvp are all locked. */
>
> s/all/both/1
OK.
> @@ -519,8 +519,10 @@ kernfs_lookup(void *v)
> break;
>
> found:
> error = kernfs_allocvp(dvp->v_mount, vpp, kt->kt_tag, kt,
> 0);
> + if (error == 0)
> + VOP_UNLOCK(*vpp);
> return (error);
>
> I would rather write this (and a few other cases like it in the patch)
> as
>
> error = kernfs_allocvp(...);
> if (error)
> return error;
> VOP_UNLOCK(*vpp);
> return 0;
>
> in order to consistently set error branches apart and make the success
> case the main flow of the code.
OK.
--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
