From: Levi Tamasi <ltamasi@fb.com>
Date: Mon, 16 Dec 2019 21:13:42 +0000 (-0800)
Subject: Fix a data race related to memtable trimming (#6187)
X-Git-Tag: v6.5.3~2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=c0a5673aa7a58487f059ba2f233b13785eb70b2e;p=rocksdb.git

Fix a data race related to memtable trimming (#6187)

Summary:
https://github.com/facebook/rocksdb/pull/6177 introduced a data race
involving `MemTableList::InstallNewVersion` and `MemTableList::NumFlushed`.
The patch fixes this by caching whether the current version has any
memtable history (i.e. flushed memtables that are kept around for
transaction conflict checking) in an `std::atomic<bool>` member called
`current_has_history_`, similarly to how `current_memory_usage_excluding_last_`
is handled.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6187

Test Plan:
```
make clean
COMPILE_WITH_TSAN=1 make db_test -j24
./db_test
```

Differential Revision: D19084059

Pulled By: ltamasi

fbshipit-source-id: 327a5af9700fb7102baea2cc8903c085f69543b9
---

diff --git a/db/memtable_list.cc b/db/memtable_list.cc
index 8f053b6e..4b94afae 100644
--- a/db/memtable_list.cc
+++ b/db/memtable_list.cc
@@ -480,7 +480,7 @@ Status MemTableList::TryInstallMemtableFlushResults(
                            cfd->GetName().c_str(), m->file_number_, mem_id);
           assert(m->file_number_ > 0);
           current_->Remove(m, to_delete);
-          UpdateMemoryUsageExcludingLast();
+          UpdateCachedValuesFromMemTableListVersion();
           ResetTrimHistoryNeeded();
           ++mem_id;
         }
@@ -521,14 +521,14 @@ void MemTableList::Add(MemTable* m, autovector<MemTable*>* to_delete) {
   if (num_flush_not_started_ == 1) {
     imm_flush_needed.store(true, std::memory_order_release);
   }
-  UpdateMemoryUsageExcludingLast();
+  UpdateCachedValuesFromMemTableListVersion();
   ResetTrimHistoryNeeded();
 }
 
 void MemTableList::TrimHistory(autovector<MemTable*>* to_delete, size_t usage) {
   InstallNewVersion();
   current_->TrimHistory(to_delete, usage);
-  UpdateMemoryUsageExcludingLast();
+  UpdateCachedValuesFromMemTableListVersion();
   ResetTrimHistoryNeeded();
 }
 
@@ -544,17 +544,24 @@ size_t MemTableList::ApproximateUnflushedMemTablesMemoryUsage() {
 size_t MemTableList::ApproximateMemoryUsage() { return current_memory_usage_; }
 
 size_t MemTableList::ApproximateMemoryUsageExcludingLast() const {
-  size_t usage =
+  const size_t usage =
       current_memory_usage_excluding_last_.load(std::memory_order_relaxed);
   return usage;
 }
 
-// Update current_memory_usage_excluding_last_, need to call whenever state
-// changes for MemtableListVersion (whenever InstallNewVersion() is called)
-void MemTableList::UpdateMemoryUsageExcludingLast() {
-  size_t total_memtable_size = current_->ApproximateMemoryUsageExcludingLast();
+bool MemTableList::HasHistory() const {
+  const bool has_history = current_has_history_.load(std::memory_order_relaxed);
+  return has_history;
+}
+
+void MemTableList::UpdateCachedValuesFromMemTableListVersion() {
+  const size_t total_memtable_size =
+      current_->ApproximateMemoryUsageExcludingLast();
   current_memory_usage_excluding_last_.store(total_memtable_size,
                                              std::memory_order_relaxed);
+
+  const bool has_history = current_->HasHistory();
+  current_has_history_.store(has_history, std::memory_order_relaxed);
 }
 
 uint64_t MemTableList::ApproximateOldestKeyTime() const {
@@ -684,7 +691,7 @@ Status InstallMemtableAtomicFlushResults(
                          cfds[i]->GetName().c_str(), m->GetFileNumber(),
                          mem_id);
         imm->current_->Remove(m, to_delete);
-        imm->UpdateMemoryUsageExcludingLast();
+        imm->UpdateCachedValuesFromMemTableListVersion();
         imm->ResetTrimHistoryNeeded();
       }
     }
@@ -727,7 +734,8 @@ void MemTableList::RemoveOldMemTables(uint64_t log_number,
       imm_flush_needed.store(false, std::memory_order_release);
     }
   }
-  UpdateMemoryUsageExcludingLast();
+
+  UpdateCachedValuesFromMemTableListVersion();
   ResetTrimHistoryNeeded();
 }
 
diff --git a/db/memtable_list.h b/db/memtable_list.h
index 44031ca4..cc8b648b 100644
--- a/db/memtable_list.h
+++ b/db/memtable_list.h
@@ -159,6 +159,10 @@ class MemTableListVersion {
   // memory usage above or equal to max_write_buffer_size_to_maintain_
   size_t ApproximateMemoryUsageExcludingLast() const;
 
+  // Whether this version contains flushed memtables that are only kept around
+  // for transaction conflict checking.
+  bool HasHistory() const { return !memlist_history_.empty(); }
+
   bool MemtableLimitExceeded(size_t usage);
 
   // Immutable MemTables that have not yet been flushed.
@@ -206,7 +210,8 @@ class MemTableList {
         commit_in_progress_(false),
         flush_requested_(false),
         current_memory_usage_(0),
-        current_memory_usage_excluding_last_(0) {
+        current_memory_usage_excluding_last_(0),
+        current_has_history_(false) {
     current_->Ref();
   }
 
@@ -260,11 +265,16 @@ class MemTableList {
   // Returns an estimate of the number of bytes of data in use.
   size_t ApproximateMemoryUsage();
 
-  // Returns the cached current_memory_usage_excluding_last_ value
+  // Returns the cached current_memory_usage_excluding_last_ value.
   size_t ApproximateMemoryUsageExcludingLast() const;
 
-  // Update current_memory_usage_excluding_last_ from MemtableListVersion
-  void UpdateMemoryUsageExcludingLast();
+  // Returns the cached current_has_history_ value.
+  bool HasHistory() const;
+
+  // Updates current_memory_usage_excluding_last_ and current_has_history_
+  // from MemTableListVersion. Must be called whenever InstallNewVersion is
+  // called.
+  void UpdateCachedValuesFromMemTableListVersion();
 
   // `usage` is the current size of the mutable Memtable. When
   // max_write_buffer_size_to_maintain is used, total size of mutable and
@@ -382,7 +392,11 @@ class MemTableList {
   // The current memory usage.
   size_t current_memory_usage_;
 
+  // Cached value of current_->ApproximateMemoryUsageExcludingLast().
   std::atomic<size_t> current_memory_usage_excluding_last_;
+
+  // Cached value of current_->HasHistory().
+  std::atomic<bool> current_has_history_;
 };
 
 // Installs memtable atomic flush results.
diff --git a/db/write_batch.cc b/db/write_batch.cc
index c8a7551c..76390ef8 100644
--- a/db/write_batch.cc
+++ b/db/write_batch.cc
@@ -1797,7 +1797,7 @@ class MemTableInserter : public WriteBatch::Handler {
         MemTableList* const imm = cfd->imm();
         assert(imm);
 
-        if (imm->NumFlushed() > 0) {
+        if (imm->HasHistory()) {
           const MemTable* const mem = cfd->mem();
           assert(mem);