From db7c6875233f429e666e81196b5d9e35dbc866bd Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Mon, 16 Dec 2019 13:13:42 -0800 Subject: [PATCH] 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` 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 --- db/memtable_list.cc | 27 +++++++++++++++++---------- db/memtable_list.h | 22 ++++++++++++++++++---- db/write_batch.cc | 2 +- 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/db/memtable_list.cc b/db/memtable_list.cc index 2d4c514e1..3281c96d0 100644 --- a/db/memtable_list.cc +++ b/db/memtable_list.cc @@ -503,7 +503,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; } @@ -544,14 +544,14 @@ void MemTableList::Add(MemTable* m, autovector* to_delete) { if (num_flush_not_started_ == 1) { imm_flush_needed.store(true, std::memory_order_release); } - UpdateMemoryUsageExcludingLast(); + UpdateCachedValuesFromMemTableListVersion(); ResetTrimHistoryNeeded(); } void MemTableList::TrimHistory(autovector* to_delete, size_t usage) { InstallNewVersion(); current_->TrimHistory(to_delete, usage); - UpdateMemoryUsageExcludingLast(); + UpdateCachedValuesFromMemTableListVersion(); ResetTrimHistoryNeeded(); } @@ -567,17 +567,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 { @@ -707,7 +714,7 @@ Status InstallMemtableAtomicFlushResults( cfds[i]->GetName().c_str(), m->GetFileNumber(), mem_id); imm->current_->Remove(m, to_delete); - imm->UpdateMemoryUsageExcludingLast(); + imm->UpdateCachedValuesFromMemTableListVersion(); imm->ResetTrimHistoryNeeded(); } } @@ -757,7 +764,7 @@ void MemTableList::RemoveOldMemTables(uint64_t log_number, } } - UpdateMemoryUsageExcludingLast(); + UpdateCachedValuesFromMemTableListVersion(); ResetTrimHistoryNeeded(); } diff --git a/db/memtable_list.h b/db/memtable_list.h index aad4fddea..cbf42ac08 100644 --- a/db/memtable_list.h +++ b/db/memtable_list.h @@ -164,6 +164,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. @@ -211,7 +215,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(); } @@ -266,11 +271,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 @@ -388,7 +398,11 @@ class MemTableList { // The current memory usage. size_t current_memory_usage_; + // Cached value of current_->ApproximateMemoryUsageExcludingLast(). std::atomic current_memory_usage_excluding_last_; + + // Cached value of current_->HasHistory(). + std::atomic current_has_history_; }; // Installs memtable atomic flush results. diff --git a/db/write_batch.cc b/db/write_batch.cc index 71104ea35..5988a9beb 100644 --- a/db/write_batch.cc +++ b/db/write_batch.cc @@ -1795,7 +1795,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);