Re: Vnode API change: VOP_LOOKUP

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

On Feb 6, 2014, at 4:23 PM, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

>   Date: Mon, 3 Feb 2014 12:04:27 +0100
>   From: J. Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>
> 
>   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 - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)