[committed hurd 02/13] fatfs: use a seperate lock to protect nodehash

[Justus Winter]

Previously, fatfs used diskfs_node_refcnt_lock to serialize access to
the nodehash.

Use a separate lock to protect nodehash.  Adjust the reference
counting accordingly.  Every node in the nodehash carries a light
reference.  When we are asked to give up that light reference, we
reacquire our lock momentarily to check whether someone else
reacquired a reference through the nodehash.

* fatfs/inode.c (nodecache_lock): New lock.
(diskfs_cached_lookup): Use a separate lock to protect nodehash.
Adjust the reference counting accordingly.
(ifind): Likewise.
(diskfs_node_iterate): Likewise.
(diskfs_node_norefs): Move the code removing the node from nodehash...
(diskfs_try_dropping_softrefs): ... here, where we check whether
someone reacquired a reference, and if so hold on to our light
reference.
---
 fatfs/inode.c | 147 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 102 insertions(+), 45 deletions(-)

diff --git a/fatfs/inode.c b/fatfs/inode.c
index ed6f3f0..1d670f5 100644
--- a/fatfs/inode.c
+++ b/fatfs/inode.c
@@ -44,8 +44,19 @@
 #define INOHASH(ino)    (((unsigned)(ino))%INOHSZ)
 #endif
 
+/* The nodehash is a cache of nodes.
+
+   Access to nodehash and nodehash_nr_items is protected by
+   nodecache_lock.
+
+   Every node in the nodehash carries a light reference.  When we are
+   asked to give up that light reference, we reacquire our lock
+   momentarily to check whether someone else reacquired a reference
+   through the nodehash.  */
 static struct node *nodehash[INOHSZ];
 static size_t nodehash_nr_items;
+/* nodecache_lock must be acquired before diskfs_node_refcnt_lock.  */
+static pthread_rwlock_t nodecache_lock = PTHREAD_RWLOCK_INITIALIZER;
 
 static error_t read_node (struct node *np, vm_address_t buf);
 
@@ -58,33 +69,38 @@ inode_init ()
     nodehash[n] = 0;
 }
 
+/* Lookup node with inode number INUM.  Returns NULL if the node is
+   not found in the node cache.  */
+static struct node *
+lookup (ino_t inum)
+{
+  struct node *np;
+  for (np = nodehash[INOHASH(inum)]; np; np = np->dn->hnext)
+    if (np->cache_id == inum)
+      return np;
+  return NULL;
+}
+
 /* Fetch inode INUM, set *NPP to the node structure; gain one user
    reference and lock the node.  */
 error_t
 diskfs_cached_lookup (ino64_t inum, struct node **npp)
 {
   error_t err;
-  struct node *np;
+  struct node *np, *tmp;
   struct disknode *dn;
 
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
-  for (np = nodehash[INOHASH(inum)]; np; np = np->dn->hnext)
-    if (np->cache_id == inum)
-      {
-        np->references++;
-        pthread_spin_unlock (&diskfs_node_refcnt_lock);
-        pthread_mutex_lock (&np->lock);
-        *npp = np;
-        return 0;
-      }
+  pthread_rwlock_rdlock (&nodecache_lock);
+  np = lookup (inum);
+  if (np)
+    goto gotit;
+  pthread_rwlock_unlock (&nodecache_lock);
 
   /* Format specific data for the new node.  */
   dn = malloc (sizeof (struct disknode));
   if (! dn)
-    {
-      pthread_spin_unlock (&diskfs_node_refcnt_lock);
-      return ENOMEM;
-    }
+    return ENOMEM;
+
   dn->pager = 0;
   dn->first = 0;
   dn->last = 0;
@@ -102,15 +118,25 @@ diskfs_cached_lookup (ino64_t inum, struct node **npp)
   pthread_mutex_lock (&np->lock);
   
   /* Put NP in NODEHASH.  */
+  pthread_rwlock_wrlock (&nodecache_lock);
+  tmp = lookup (inum);
+  if (tmp)
+    {
+      /* We lost a race.  */
+      diskfs_nput (np);
+      np = tmp;
+      goto gotit;
+    }
+
   dn->hnext = nodehash[INOHASH(inum)];
   if (dn->hnext)
     dn->hnext->dn->hprevp = &dn->hnext;
   dn->hprevp = &nodehash[INOHASH(inum)];
   nodehash[INOHASH(inum)] = np;
+  diskfs_nref_light (np);
   nodehash_nr_items += 1;
+  pthread_rwlock_unlock (&nodecache_lock);
 
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
-  
   /* Get the contents of NP off disk.  */
   err = read_node (np, 0);
 
@@ -121,6 +147,13 @@ diskfs_cached_lookup (ino64_t inum, struct node **npp)
       *npp = np;
       return 0;
     }
+
+ gotit:
+  diskfs_nref (np);
+  pthread_rwlock_unlock (&nodecache_lock);
+  pthread_mutex_lock (&np->lock);
+  *npp = np;
+  return 0;
 }
 
 /* Fetch inode INUM, set *NPP to the node structure;
@@ -133,24 +166,23 @@ diskfs_cached_lookup_in_dirbuf (int inum, struct node 
**npp, vm_address_t buf)
   struct node *np;
   struct disknode *dn;
 
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
+  pthread_rwlock_rdlock (&nodecache_lock);
   for (np = nodehash[INOHASH(inum)]; np; np = np->dn->hnext)
     if (np->cache_id == inum)
       {
-        np->references++;
-        pthread_spin_unlock (&diskfs_node_refcnt_lock);
+        diskfs_nref (np);
+        pthread_rwlock_unlock (&nodecache_lock);
         pthread_mutex_lock (&np->lock);
         *npp = np;
         return 0;
       }
+  pthread_rwlock_unlock (&nodecache_lock);
 
   /* Format specific data for the new node.  */
   dn = malloc (sizeof (struct disknode));
   if (! dn)
-    {
-      pthread_spin_unlock (&diskfs_node_refcnt_lock);
-      return ENOMEM;
-    }
+    return ENOMEM;
+
   dn->pager = 0;
   dn->first = 0;
   dn->last = 0;
@@ -168,15 +200,16 @@ diskfs_cached_lookup_in_dirbuf (int inum, struct node 
**npp, vm_address_t buf)
   pthread_mutex_lock (&np->lock);
   
   /* Put NP in NODEHASH.  */
+  pthread_rwlock_wrlock (&nodecache_lock);
   dn->hnext = nodehash[INOHASH(inum)];
   if (dn->hnext)
     dn->hnext->dn->hprevp = &dn->hnext;
   dn->hprevp = &nodehash[INOHASH(inum)];
   nodehash[INOHASH(inum)] = np;
+  diskfs_nref_light (np);
   nodehash_nr_items += 1;
+  pthread_rwlock_unlock (&nodecache_lock);
 
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
-  
   /* Get the contents of NP off disk.  */
   err = read_node (np, buf);
 
@@ -196,17 +229,12 @@ ifind (ino_t inum)
 {
   struct node *np;
 
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
-  for (np = nodehash[INOHASH(inum)]; np; np = np->dn->hnext)
-    {
-      if (np->cache_id != inum)
-        continue;
+  pthread_rwlock_rdlock (&nodecache_lock);
+  np = lookup (inum);
+  pthread_rwlock_unlock (&nodecache_lock);
 
-      assert (np->references);
-      pthread_spin_unlock (&diskfs_node_refcnt_lock);
-      return np;
-    }
-  assert (0);
+  assert (np);
+  return np;
 }
 
 /* The last reference to a node has gone away; drop it from the hash
@@ -216,11 +244,6 @@ diskfs_node_norefs (struct node *np)
 {
   struct cluster_chain *last = np->dn->first;
 
-  *np->dn->hprevp = np->dn->hnext;
-  if (np->dn->hnext)
-    np->dn->hnext->dn->hprevp = np->dn->hprevp;
-  nodehash_nr_items -= 1;
-
   while (last)
     {
       struct cluster_chain *next = last->next;
@@ -251,6 +274,36 @@ diskfs_node_norefs (struct node *np)
 void
 diskfs_try_dropping_softrefs (struct node *np)
 {
+  pthread_rwlock_wrlock (&nodecache_lock);
+  if (np->dn->hprevp != NULL)
+    {
+      /* Check if someone reacquired a reference through the
+        nodehash.  */
+      unsigned int references;
+      pthread_spin_lock (&diskfs_node_refcnt_lock);
+      references = np->references;
+      pthread_spin_unlock (&diskfs_node_refcnt_lock);
+
+      /* An additional reference is acquired by libdiskfs across calls
+        to diskfs_try_dropping_softrefs.  */
+      if (references > 1)
+       {
+         /* A reference was reacquired through a hash table lookup.
+            It's fine, we didn't touch anything yet. */
+         pthread_rwlock_unlock (&nodecache_lock);
+         return;
+       }
+
+      *np->dn->hprevp = np->dn->hnext;
+      if (np->dn->hnext)
+       np->dn->hnext->dn->hprevp = np->dn->hprevp;
+      np->dn->hnext = NULL;
+      np->dn->hprevp = NULL;
+      nodehash_nr_items -= 1;
+      diskfs_nrele_light (np);
+    }
+  pthread_rwlock_unlock (&nodecache_lock);
+
   drop_pager_softrefs (np);
 }
 
@@ -554,12 +607,12 @@ diskfs_node_iterate (error_t (*fun)(struct node *))
   size_t num_nodes;
   struct node *node, **node_list, **p;
 
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
+  pthread_rwlock_rdlock (&nodecache_lock);
 
   /* We must copy everything from the hash table into another data structure
      to avoid running into any problems with the hash-table being modified
      during processing (normally we delegate access to hash-table with
-     diskfs_node_refcnt_lock, but we can't hold this while locking the
+     nodecache_lock, but we can't hold this while locking the
      individual node locks).  */
 
   num_nodes = nodehash_nr_items;
@@ -570,10 +623,14 @@ diskfs_node_iterate (error_t (*fun)(struct node *))
     for (node = nodehash[n]; node; node = node->dn->hnext)
       {
         *p++ = node;
-        node->references++;
+
+       /* We acquire a hard reference for node, but without using
+          diskfs_nref.  We do this so that diskfs_new_hardrefs will not
+          get called.  */
+       node->references++;
       }
 
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
+  pthread_rwlock_unlock (&nodecache_lock);
 
   p = node_list;
   while (num_nodes-- > 0)
-- 
2.1.4