[hurd] 04/20: tmpfs: use a seperate lock to protect all_nodes

[Samuel Thibault]

This is an automated email from the git hooks/post-receive script.

sthibault pushed a commit to branch upstream
in repository hurd.

commit 8c050fb080c6e1981dc8e5a97a2313cd24e9b4b4
Author: Justus Winter <address@hidden>
Date:   Tue May 13 15:35:42 2014 +0200

    tmpfs: use a seperate lock to protect all_nodes
    
    Previously, tmpfs used diskfs_node_refcnt_lock to serialize access to
    the all_nodes and some other related global state related to memory
    consumption.
    
    Use a separate lock to protect all_nodes, and atomic operations to
    access the state related to memory consumption. Adjust the reference
    counting accordingly.  Every node in the all_nodes 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 all_nodes.
    
    * tmpfs/tmpfs.h (num_files, tmpfs_space_used): Use atomic operations
    for these variables.
    (adjust_used): Use atomic operations.
    (get_used): New convenience function to atomically retrieve
    tmpfs_space_used.
    * tmpfs/node.c (all_nodes_lock): New lock.
    (diskfs_alloc_node): Use a separate lock to protect all_nodes.
    Adjust the reference counting accordingly.
    (diskfs_free_node): Likewise.
    (diskfs_cached_lookup):Likewise.
    (diskfs_node_iterate): Likewise.
    (diskfs_node_norefs): Do not remove the node from all_nodes.  This
    actually looks like a mistake, I do not know why they did that here as
    well as in diskfs_free_node.
    (diskfs_try_dropping_softrefs): Check whether someone reacquired a
    reference, and if so hold on to our light reference.
    (diskfs_grow): Use atomic operations.
    * tmpfs/tmpfs.c (diskfs_set_statfs): Likewise.
---
 tmpfs/node.c  | 107 ++++++++++++++++++++++++++++++++++++++++++----------------
 tmpfs/tmpfs.c |   6 ++--
 tmpfs/tmpfs.h |  20 +++++++----
 3 files changed, 94 insertions(+), 39 deletions(-)

diff --git a/tmpfs/node.c b/tmpfs/node.c
index 02d7a60..428b6d9 100644
--- a/tmpfs/node.c
+++ b/tmpfs/node.c
@@ -29,8 +29,19 @@
 unsigned int num_files;
 static unsigned int gen;
 
+/* all_nodes is a list of all nodes.
+
+   Access to all_nodes and all_nodes_nr_items is protected by
+   all_nodes_lock.
+
+   Every node in all_nodes 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.  */
 struct node *all_nodes;
 static size_t all_nodes_nr_items;
+/* all_nodes_lock must be acquired before diskfs_node_refcnt_lock.  */
+pthread_rwlock_t all_nodes_lock = PTHREAD_RWLOCK_INITIALIZER;
 
 error_t
 diskfs_alloc_node (struct node *dp, mode_t mode, struct node **npp)
@@ -40,18 +51,17 @@ diskfs_alloc_node (struct node *dp, mode_t mode, struct 
node **npp)
   dn = calloc (1, sizeof *dn);
   if (dn == 0)
     return ENOSPC;
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
-  if (round_page (tmpfs_space_used + sizeof *dn) / vm_page_size
+
+  if (round_page (get_used () + sizeof *dn) / vm_page_size
       > tmpfs_page_limit)
     {
-      pthread_spin_unlock (&diskfs_node_refcnt_lock);
+      pthread_rwlock_unlock (&all_nodes_lock);
       free (dn);
       return ENOSPC;
     }
   dn->gen = gen++;
-  ++num_files;
-  tmpfs_space_used += sizeof *dn;
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
+  __atomic_add_fetch (&num_files, 1, __ATOMIC_RELAXED);
+  adjust_used (sizeof *dn);
 
   dn->type = IFTODT (mode & S_IFMT);
   return diskfs_cached_lookup ((ino_t) (uintptr_t) dn, npp);
@@ -75,15 +85,19 @@ diskfs_free_node (struct node *np, mode_t mode)
       free (np->dn->u.lnk);
       break;
     }
+
+  pthread_rwlock_wrlock (&all_nodes_lock);
   *np->dn->hprevp = np->dn->hnext;
   if (np->dn->hnext != 0)
     np->dn->hnext->dn->hprevp = np->dn->hprevp;
   all_nodes_nr_items -= 1;
+  pthread_rwlock_unlock (&all_nodes_lock);
+
   free (np->dn);
   np->dn = 0;
 
-  --num_files;
-  tmpfs_space_used -= sizeof *np->dn;
+  __atomic_sub_fetch (&num_files, 1, __ATOMIC_RELAXED);
+  adjust_used (-sizeof *np->dn);
 }
 
 void
@@ -117,14 +131,6 @@ diskfs_node_norefs (struct node *np)
          np->dn->u.chr = np->dn_stat.st_rdev;
          break;
        }
-
-      /* Remove this node from the cache list rooted at `all_nodes'.  */
-      *np->dn->hprevp = np->dn->hnext;
-      if (np->dn->hnext != 0)
-       np->dn->hnext->dn->hprevp = np->dn->hprevp;
-      all_nodes_nr_items -= 1;
-      np->dn->hnext = 0;
-      np->dn->hprevp = 0;
     }
 
   free (np);
@@ -167,30 +173,34 @@ diskfs_cached_lookup (ino_t inum, struct node **npp)
 
   assert (npp);
 
+  pthread_rwlock_rdlock (&all_nodes_lock);
   if (dn->hprevp != 0)         /* There is already a node.  */
-    {
-      np = *dn->hprevp;
-      assert (np->dn == dn);
-      assert (*dn->hprevp == np);
-
-      diskfs_nref (np);
-    }
+    goto gotit;
   else
     /* Create the new node.  */
     {
       struct stat *st;
+      pthread_rwlock_unlock (&all_nodes_lock);
 
       np = diskfs_make_node (dn);
       np->cache_id = (ino_t) (uintptr_t) dn;
 
-      pthread_spin_lock (&diskfs_node_refcnt_lock);
+      pthread_rwlock_wrlock (&all_nodes_lock);
+      if (dn->hprevp != NULL)
+        {
+          /* We lost a race.  */
+          diskfs_nrele (np);
+          goto gotit;
+        }
+
       dn->hnext = all_nodes;
       if (dn->hnext)
        dn->hnext->dn->hprevp = &dn->hnext;
       dn->hprevp = &all_nodes;
       all_nodes = np;
       all_nodes_nr_items += 1;
-      pthread_spin_unlock (&diskfs_node_refcnt_lock);
+      diskfs_nref_light (np);
+      pthread_rwlock_unlock (&all_nodes_lock);
 
       st = &np->dn_stat;
       memset (st, 0, sizeof *st);
@@ -220,6 +230,16 @@ diskfs_cached_lookup (ino_t inum, struct node **npp)
   pthread_mutex_lock (&np->lock);
   *npp = np;
   return 0;
+
+ gotit:
+  np = *dn->hprevp;
+  assert (np->dn == dn);
+  assert (*dn->hprevp == np);
+  diskfs_nref (np);
+  pthread_rwlock_unlock (&all_nodes_lock);
+  pthread_mutex_lock (&np->lock);
+  *npp = np;
+  return 0;
 }
 
 error_t
@@ -229,12 +249,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 (&all_nodes_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
+     all_nodes_lock, but we can't hold this while locking the
      individual node locks).  */
 
   num_nodes = all_nodes_nr_items;
@@ -243,10 +263,14 @@ diskfs_node_iterate (error_t (*fun) (struct node *))
   for (node = all_nodes; node != 0; node = node->dn->hnext)
     {
       *p++ = node;
+
+      /* 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 (&all_nodes_lock);
 
   p = node_list;
   while (num_nodes-- > 0)
@@ -272,6 +296,31 @@ diskfs_node_iterate (error_t (*fun) (struct node *))
 void
 diskfs_try_dropping_softrefs (struct node *np)
 {
+  pthread_rwlock_wrlock (&all_nodes_lock);
+  if (np->cache_id != 0)
+    {
+      /* Check if someone reacquired a reference.  */
+      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.  It's fine, we didn't touch
+            anything yet. */
+         pthread_rwlock_unlock (&all_nodes_lock);
+         return;
+       }
+
+      /* Just let go of the weak reference.  The node will be removed
+        from all_nodes in diskfs_free_node.  */
+      np->cache_id = 0;
+      diskfs_nrele_light (np);
+    }
+  pthread_rwlock_unlock (&all_nodes_lock);
 }
 
 /* The user must define this funcction.  Node NP has some light
@@ -447,7 +496,7 @@ diskfs_grow (struct node *np, off_t size, struct protid 
*cred)
 
   off_t set_size = size;
   size = round_page (size);
-  if (round_page (tmpfs_space_used + size - np->allocsize)
+  if (round_page (get_used () + size - np->allocsize)
       / vm_page_size > tmpfs_page_limit)
     return ENOSPC;
 
diff --git a/tmpfs/tmpfs.c b/tmpfs/tmpfs.c
index 1b5b374..fd1c9aa 100644
--- a/tmpfs/tmpfs.c
+++ b/tmpfs/tmpfs.c
@@ -67,10 +67,8 @@ diskfs_set_statfs (struct statfs *st)
   st->f_bsize = vm_page_size;
   st->f_blocks = tmpfs_page_limit;
 
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
-  st->f_files = num_files;
-  pages = round_page (tmpfs_space_used) / vm_page_size;
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
+  st->f_files = __atomic_load_n (&num_files, __ATOMIC_RELAXED);
+  pages = round_page (get_used ()) / vm_page_size;
 
   st->f_bfree = pages < tmpfs_page_limit ? tmpfs_page_limit - pages : 0;
   st->f_bavail = st->f_bfree;
diff --git a/tmpfs/tmpfs.h b/tmpfs/tmpfs.h
index b3c636d..ad47200 100644
--- a/tmpfs/tmpfs.h
+++ b/tmpfs/tmpfs.h
@@ -69,17 +69,25 @@ struct tmpfs_dirent
   char name[0];
 };
 
-extern unsigned int num_files;
-extern off_t tmpfs_page_limit, tmpfs_space_used;
-
+extern off_t tmpfs_page_limit;
 extern mach_port_t default_pager;
 
+/* These two must be accessed using atomic operations.  */
+extern unsigned int num_files;
+extern off_t tmpfs_space_used;
+
+/* Convenience function to adjust tmpfs_space_used.  */
 static inline void
 adjust_used (off_t change)
 {
-  pthread_spin_lock (&diskfs_node_refcnt_lock);
-  tmpfs_space_used += change;
-  pthread_spin_unlock (&diskfs_node_refcnt_lock);
+  __atomic_add_fetch (&num_files, change, __ATOMIC_RELAXED);
+}
+
+/* Convenience function to get tmpfs_space_used.  */
+static inline off_t
+get_used (void)
+{
+  return __atomic_load_n (&num_files, __ATOMIC_RELAXED);
 }
 
 #endif

-- 
Alioth's /usr/local/bin/git-commit-notice on 
/srv/git.debian.org/git/pkg-hurd/hurd.git