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 8f053b6ef649663c548763291937921f1184e6e2..4b94afae8c85dd70cb6f6411d3da2d65b76ea3a1 100644 (file)
--- 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 44031ca4058babc2fb3adcd35c8684d791cb1895..cc8b648bc1fba72b757fdb2b14140f354103c370 100644 (file)
--- 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 c8a7551c507c1672c86b70d6fe03771204768d8e..76390ef83b569aa2f95b540fc0fa1a6e6f7cc3ee 100644 (file)
--- 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);