Re: null_lookup() vnode locking wierdness

On 23 Nov, I wrote:
> I was trying to figure out why the VOP_UNLOCK() call in null_lookup()
> was violating a vnode locking assertion, so I tossed a bunch of
> ASSERT_VOP_LOCKED() calls into null_lookup().  I found something I don't
> understand ...
> 
>         ASSERT_VOP_LOCKED(dvp, "null_lookup 1");
>         if ((flags & ISLASTCN) && (dvp->v_mount->mnt_flag & MNT_RDONLY) &&
>             (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME))
>                 return (EROFS);
>         /*
>          * Although it is possible to call null_bypass(), we'll do
>          * a direct call to reduce overhead
>          */
>         ASSERT_VOP_LOCKED(dvp, "null_lookup 2");
>         ldvp = NULLVPTOLOWERVP(dvp);
>         ASSERT_VOP_LOCKED(dvp, "null_lookup 3");
>         vp = lvp = NULL;  
>         error = VOP_LOOKUP(ldvp, &lvp, cnp);
>         ASSERT_VOP_LOCKED(dvp, "null_lookup 4");
>         if (error == EJUSTRETURN && (flags & ISLASTCN) &&
>             (dvp->v_mount->mnt_flag & MNT_RDONLY) &&
>             (cnp->cn_nameiop == CREATE || cnp->cn_nameiop == RENAME))  
>                 error = EROFS;
>    
>         /*
>          * Rely only on the PDIRUNLOCK flag which should be carefully
>          * tracked by underlying filesystem.
>          */
>         if (cnp->cn_flags & PDIRUNLOCK)
>                 VOP_UNLOCK(dvp, LK_THISLAYER, td);
> 
> The null_lookup {1,2,3} assertions pass, but null_lookup 4 fails.  It
> appears that the VOP_LOOKUP() call to the underlying file system is
> unlocking the directory vnode in the nullfs file system.  How can that
> happen?

I think I just answered my own question.  It appears that both vnodes
can share the same lock according to the following code fragment in
null_nodeget():

        /*
         * From NetBSD:
         * Now lock the new node. We rely on the fact that we were passed
         * a locked vnode. If the lower node is exporting a struct lock
         * (v_vnlock != NULL) then we just set the upper v_vnlock to the
         * lower one, and both are now locked. If the lower node is exporting
         * NULL, then we copy that up and manually lock the new vnode.
         */

        vp->v_vnlock = lowervp->v_vnlock;
        error = VOP_LOCK(vp, LK_EXCLUSIVE | LK_THISLAYER, td);

It looks like the easiest fix is to skip the VOP_UNLOCK() call in
null_lookup() if dvp->v_vnlock == ldvp->v_vnlock.

Index: sys/fs/nullfs/null_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/nullfs/null_vnops.c,v
retrieving revision 1.63
diff -u -r1.63 null_vnops.c
--- sys/fs/nullfs/null_vnops.c	17 Jun 2003 08:52:45 -0000	1.63
+++ sys/fs/nullfs/null_vnops.c	24 Nov 2003 00:10:41 -0000
_at__at_ -392,7 +392,7 _at__at_
 	 * Rely only on the PDIRUNLOCK flag which should be carefully
 	 * tracked by underlying filesystem.
 	 */
-	if (cnp->cn_flags & PDIRUNLOCK)
+	if ((cnp->cn_flags & PDIRUNLOCK) && dvp->v_vnlock != ldvp->v_vnlock)
 		VOP_UNLOCK(dvp, LK_THISLAYER, td);
 	if ((error == 0 || error == EJUSTRETURN) && lvp != NULL) {
 		if (ldvp == lvp) {