Re: Vnode API change: VOP_LOOKUP

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

   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.

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

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