On Sat, Jan 20, 2001 at 06:25:12PM +0100, Marcus Brinkmann wrote:
> On Sun, Jan 14, 2001 at 10:33:51PM -0500, Neal H Walfield wrote:
> >
> > -/* Add a reference to node NP, which must be locked by the caller. */
> > +/* Add a reference to node NP (NP does not have to be locked). */
> > void netfs_nref (struct node *np);
> >
> > /* Releases a node. Drops a reference to node NP, which must not be
>
> Can you explain this change? If it is to match the code in
> check_lookup_cache in nfs/name-cache.c, I think that the code is in error
> and should be fixed to lock the node before acquiring the reference.
> All other callers lock the node, and I looking at nput or nrele, I find
> not having the node locked is rather dangerous. (Note that in nfs/name-cache.c,
> there is an indirect guarantee that REFERENCES doesn't drop to zero because
> one reference is hold for the cached node, and the cache lock is hold during
> the entire operation).
I have thought about this. The code in check_lookup_cache works because as
long as we don't release the cache_look, we still have a reference to the
node and it won't go away. This reference avoids a race with
netfs_nrele/netfs_nput, too. So I think adding a comment to the code
explaining this is enough for now.
The change I had in mind is moving the netfs_nref call after locking the
node. In this case, releasing the cache_lock must be delayed, too, because
we must make sure that we hold a reference for the whole time (otherwise it
might go away before we have a chance to lock it and acquire a new
reference). This results in the following code:
else
{
struct node *np;
np = c->np;
register_pos_hit (c->stati);
mutex_unlock (&dir->lock);
mutex_lock (&np->lock);
netfs_nref (np);
spin_unlock (&cache_lock);
return np;
}
I think this would be the proper fix, because it complys with the semantics
of the netfs interface without relying on the concrete implementation.
However, I am not sure if it is a problem to delay unlocking the cache_lock
so much. (Maybe it is even possible that the holder of the node lock waits
for the cache_lock, which would result in a dead lock!). That's why a comment
is probably the best solution for now.
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org [EMAIL PROTECTED]
Marcus Brinkmann GNU http://www.gnu.org [EMAIL PROTECTED]
[EMAIL PROTECTED]
http://www.marcus-brinkmann.de
_______________________________________________
Bug-hurd mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-hurd
