From f1c8862479a9eebbfca7ca27ff51c8b3014c14dc Mon Sep 17 00:00:00 2001
From: Igor Canadi <icanadi@fb.com>
Date: Mon, 26 Jan 2015 11:48:07 -0800
Subject: [PATCH] Fix data race #1

Summary:
This is first in a series of diffs that fixes data races detected by thread sanitizer.

Here the problem is that we call Ref() on a column family during a single-threaded write, without holding a mutex.

Test Plan: TSAN is no longer complaining about LevelLimitReopen.

Reviewers: yhchiang, rven, sdong

Reviewed By: sdong

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D32121
---
 db/column_family.cc  |  4 ++--
 db/column_family.h   | 15 ++++++++++-----
 db/flush_scheduler.h |  1 +
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/db/column_family.cc b/db/column_family.cc
index 19bb09564..e6e75aad9 100644
--- a/db/column_family.cc
+++ b/db/column_family.cc
@@ -299,7 +299,7 @@ ColumnFamilyData::ColumnFamilyData(
 
 // DB mutex held
 ColumnFamilyData::~ColumnFamilyData() {
-  assert(refs_ == 0);
+  assert(refs_.load(std::memory_order_relaxed) == 0);
   // remove from linked list
   auto prev = prev_;
   auto next = next_;
@@ -731,7 +731,7 @@ ColumnFamilyData* ColumnFamilySet::CreateColumnFamily(
 void ColumnFamilySet::FreeDeadColumnFamilies() {
   autovector<ColumnFamilyData*> to_delete;
   for (auto cfd = dummy_cfd_->next_; cfd != dummy_cfd_; cfd = cfd->next_) {
-    if (cfd->refs_ == 0) {
+    if (cfd->refs_.load(std::memory_order_relaxed) == 0) {
       to_delete.push_back(cfd);
     }
   }
diff --git a/db/column_family.h b/db/column_family.h
index 1c987a3f0..a1a9e8034 100644
--- a/db/column_family.h
+++ b/db/column_family.h
@@ -134,14 +134,18 @@ class ColumnFamilyData {
   // thread-safe
   const std::string& GetName() const { return name_; }
 
-  void Ref() { ++refs_; }
+  // Ref() can only be called whily holding a DB mutex or during a
+  // single-threaded write.
+  void Ref() { refs_.fetch_add(1, std::memory_order_relaxed); }
   // will just decrease reference count to 0, but will not delete it. returns
   // true if the ref count was decreased to zero. in that case, it can be
   // deleted by the caller immediately, or later, by calling
   // FreeDeadColumnFamilies()
+  // Unref() can only be called while holding a DB mutex
   bool Unref() {
-    assert(refs_ > 0);
-    return --refs_ == 0;
+    int old_refs = refs_.fetch_sub(1, std::memory_order_relaxed);
+    assert(old_refs > 0);
+    return old_refs == 1;
   }
 
   // SetDropped() can only be called under following conditions:
@@ -290,7 +294,7 @@ class ColumnFamilyData {
   Version* dummy_versions_;  // Head of circular doubly-linked list of versions.
   Version* current_;         // == dummy_versions->prev_
 
-  int refs_;                   // outstanding references to ColumnFamilyData
+  std::atomic<int> refs_;      // outstanding references to ColumnFamilyData
   bool dropped_;               // true if client dropped it
 
   const InternalKeyComparator internal_comparator_;
@@ -373,7 +377,8 @@ class ColumnFamilySet {
       // dummy is never dead or dropped, so this will never be infinite
       do {
         current_ = current_->next_;
-      } while (current_->refs_ == 0 || current_->IsDropped());
+      } while (current_->refs_.load(std::memory_order_relaxed) == 0 ||
+               current_->IsDropped());
       return *this;
     }
     bool operator!=(const iterator& other) {
diff --git a/db/flush_scheduler.h b/db/flush_scheduler.h
index 201e4a13c..0c96709b9 100644
--- a/db/flush_scheduler.h
+++ b/db/flush_scheduler.h
@@ -23,6 +23,7 @@ class FlushScheduler {
 
   void ScheduleFlush(ColumnFamilyData* cfd);
   // Returns Ref()-ed column family. Client needs to Unref()
+  // REQUIRES: db mutex is held (exception is single-threaded recovery)
   ColumnFamilyData* GetNextColumnFamily();
 
   bool Empty();