Re: [DOC-FIX] netfs.h

[Marcus Brinkmann]

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 brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de