From: Michael Vrable <mvrable@cs.ucsd.edu>
Date: Tue, 3 Aug 2010 21:22:25 +0000 (-0700)
Subject: Fix up reference counting for memory-mapped journal log segments.
X-Git-Url: http://git.vrable.net/?p=bluesky.git;a=commitdiff_plain;h=4fa8649924cb61b3d77ced461fe716a7ee18fc1d

Fix up reference counting for memory-mapped journal log segments.
---

diff --git a/bluesky/bluesky-private.h b/bluesky/bluesky-private.h
index 2932417..0f86f45 100644
--- a/bluesky/bluesky-private.h
+++ b/bluesky/bluesky-private.h
@@ -250,11 +250,23 @@ struct _BlueSkyLog {
     GHashTable *mmap_cache;
 };
 
+/* Reference-counted blocks of memory, used for passing data in and out of
+ * storage backends and in other places.  This may also refer to read-only
+ * mmaped data. */
+struct _BlueSkyMmap {
+    gint refcount;
+    int log_seq;
+    const char *addr;
+    size_t len;
+    BlueSkyLog *log;
+};
+
 BlueSkyLog *bluesky_log_new(const char *log_directory);
 void bluesky_log_item_submit(BlueSkyCloudLog *item, BlueSkyLog *log);
 void bluesky_log_finish_all(GList *log_items);
 BlueSkyRCStr *bluesky_log_map_object(BlueSkyLog *log, int log_seq,
                                      int log_offset, int log_size);
+void bluesky_mmap_unref(BlueSkyMmap *mmap);
 
 #ifdef __cplusplus
 }
diff --git a/bluesky/bluesky.h b/bluesky/bluesky.h
index 482c39a..8846488 100644
--- a/bluesky/bluesky.h
+++ b/bluesky/bluesky.h
@@ -63,14 +63,8 @@ void bluesky_init(void);
 
 gchar *bluesky_lowercase(const gchar *s);
 
-/* Reference-counted blocks of memory, used for passing data in and out of
- * storage backends and in other places.  This may also refer to read-only
- * mmaped data. */
-typedef struct {
-    gint refcount;
-    const char *addr;
-    size_t len;
-} BlueSkyMmap;
+struct _BlueSkyMmap;
+typedef struct _BlueSkyMmap BlueSkyMmap;
 
 typedef struct {
     gint refcount;
diff --git a/bluesky/log.c b/bluesky/log.c
index 9b4b62b..55daf7a 100644
--- a/bluesky/log.c
+++ b/bluesky/log.c
@@ -282,10 +282,12 @@ BlueSkyRCStr *bluesky_log_map_object(BlueSkyLog *log,
         map = g_new0(BlueSkyMmap, 1);
 
         off_t length = lseek(fd, 0, SEEK_END);
+        map->log_seq = log_seq;
         map->addr = (const char *)mmap(NULL, length, PROT_READ, MAP_SHARED,
                                        fd, 0);
         map->len = length;
-        g_atomic_int_set(&map->refcount, 1);
+        map->log = log;
+        g_atomic_int_set(&map->refcount, 0);
 
         g_hash_table_insert(log->mmap_cache, GINT_TO_POINTER(log_seq), map);
 
@@ -296,3 +298,31 @@ BlueSkyRCStr *bluesky_log_map_object(BlueSkyLog *log,
 
     return bluesky_string_new_from_mmap(map, log_offset, log_size);
 }
+
+void bluesky_mmap_unref(BlueSkyMmap *mmap)
+{
+    if (mmap == NULL)
+        return;
+
+    if (g_atomic_int_dec_and_test(&mmap->refcount)) {
+        /* There is a potential race condition here: the BlueSkyLog contains a
+         * hash table of currently-existing BlueSkyMmap objects, which does not
+         * hold a reference.  Some other thread might grab a new reference to
+         * this object after reading it from the hash table.  So, before
+         * destruction we need to grab the lock for the hash table, then check
+         * the reference count again.  If it is still zero, we can proceed with
+         * object destruction. */
+        BlueSkyLog *log = mmap->log;
+        g_mutex_lock(log->mmap_lock);
+        if (g_atomic_int_get(&mmap->refcount) > 0) {
+            g_mutex_unlock(log->mmap_lock);
+            return;
+        }
+
+        g_hash_table_remove(log->mmap_cache, GINT_TO_POINTER(mmap->log_seq));
+        munmap((void *)mmap->addr, mmap->len);
+        g_free(mmap);
+        g_mutex_unlock(log->mmap_lock);
+    }
+}
+
diff --git a/bluesky/util.c b/bluesky/util.c
index 0bc1c6c..f899420 100644
--- a/bluesky/util.c
+++ b/bluesky/util.c
@@ -53,17 +53,6 @@ gboolean bluesky_inode_is_ready(BlueSkyInode *inode)
 
 /**** Reference-counted strings. ****/
 
-void bluesky_mmap_unref(BlueSkyMmap *mmap)
-{
-    if (mmap == NULL)
-        return;
-
-    if (g_atomic_int_dec_and_test(&mmap->refcount)) {
-        munmap((void *)mmap->addr, mmap->len);
-        g_free(mmap);
-    }
-}
-
 /* Create and return a new reference-counted string.  The reference count is
  * initially one.  The newly-returned string takes ownership of the memory
  * pointed at by data, and will call g_free on it when the reference count