Re: Problems with vnode locking/unlocking

[Alex Hornung <ahornung@xxxxxxxxx>]

Nevermind my previous message, I've fixed it and it seems to be working
properly now.


On Tue, 2009-05-26 at 16:02 -0700, Matthew Dillon wrote:
> :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@backplane.com>