Problems with vnode locking/unlocking

[Matthew Dillon]

:Hi,
:
:I'm running into quite a lot of trouble with vnode locking and
:unlocking, releasing, putting, getting, ... I'm 100% sure I got most if
:not all of the vnode locking wrong and I reallly need some help
:understanding it. It is still unclear to me what vnops need to do what
:with regard to locking/unlocking.
:
:My current code is here:
:http://gitweb.dragonflybsd.org/~alexh/dragonfly.git/tree/9de1a66518d104077521a13d7b13ae958fd79d98:/sys/vfs/devfs
:
:Right now my concerns are mainly in devfs_vnops.c, where all the
:locking/unlocking/... occurs or should occur.
:
:I'd appreciate some insight/comments/corrections/... on this issue!
:
:Alex Hornung

    One thing I noticed is that the devfs_root() code path does not
    look right.  This routine can be called any number of times by
    the kernel, you don't want to allocate a new root vnode every
    time!

    devfs_root()->devfs_allocv()->getnewvnode()-> ...

    If the root node already has a vnode associated with it you have to
    ref and vn_lock and return that vnode, not allocate a new vnode.
    I think you might be overwriting previously acquired vnode pointers
    in that root node and that will certainly mess things up.

    --

    Another thing I noticed is that you need to remember that when you
    return a vnode in *vpp, the caller is expected that vnode to be
    referenced (and possibly also locked), which means you don't release
    the vnode that you are also returning unless you have extra references
    (2 or more) and you need to get them down to one reference for the
    return.

    So for example the devfs_nsymlink() call is calling devfs_allocvp()
    but it is also returning the vp in *ap->a_vpp.  In the case of
    devfs_nsymlink() I believe it is expected to return a referenced
    but NOT locked vnode, so you would unlock it but not dereference it
    before returning.

    You will want to check all the VNOPS that return a vnode in *ap->a_vpp
    for similar issues.

    In the case if nresolve I believe you are doing it correctly... 
    VOP_NRESOLVE() does not return a vnode (there is no ap->a_vpp), 
    it just expects the namecache entry to be resolved and the
    cache_setvp() call doesn't inherit any refs or anything so you unlock
    and release the vnode like you are doing.

					-Matt
					Matthew Dillon 
					<dillon at backplane.com>