From df4130ad855ca2df9a19020fa4d034e3a824d2c6 Mon Sep 17 00:00:00 2001
From: Laurent Demailly <ldemailly@fb.com>
Date: Tue, 5 May 2015 16:54:47 -0700
Subject: [PATCH] fix crashes in stats and compaction filter for db_ttl_impl

Summary: fix crashes in stats and compaction filter for db_ttl_impl

Test Plan:
Ran build with lots of debugging
https://reviews.facebook.net/differential/diff/194175/

Reviewers: yhchiang, igor, rven

Reviewed By: igor

Subscribers: rven, dhruba

Differential Revision: https://reviews.facebook.net/D38001
---
 db/db_impl.cc                           | 18 ++++++++++++++++--
 db/db_impl.h                            |  2 +-
 include/rocksdb/statistics.h            |  3 ++-
 include/rocksdb/utilities/convenience.h |  3 ++-
 utilities/convenience/convenience.cc    |  4 ++--
 utilities/ttl/db_ttl_impl.cc            |  7 ++++++-
 6 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/db/db_impl.cc b/db/db_impl.cc
index a6ab4b3a1..86f35eb15 100644
--- a/db/db_impl.cc
+++ b/db/db_impl.cc
@@ -265,8 +265,18 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname)
   LogFlush(db_options_.info_log);
 }
 
-void DBImpl::CancelAllBackgroundWork() {
+// Will only lock the mutex_ and wait for completion if wait is true
+void DBImpl::CancelAllBackgroundWork(bool wait) {
   shutting_down_.store(true, std::memory_order_release);
+  if (!wait) {
+    return;
+  }
+  // Wait for background work to finish
+  mutex_.Lock();
+  while (bg_compaction_scheduled_ || bg_flush_scheduled_ || notifying_events_) {
+    bg_cv_.Wait();
+  }
+  mutex_.Unlock();
 }
 
 DBImpl::~DBImpl() {
@@ -285,7 +295,11 @@ DBImpl::~DBImpl() {
     }
     versions_->GetColumnFamilySet()->FreeDeadColumnFamilies();
   }
-  CancelAllBackgroundWork();
+  // CancelAllBackgroundWork called with false means we just set the
+  // shutdown marker, while holding the mutex_ here. After which we
+  // do a variant of the waiting after we release the lock and unschedule work
+  // (to consider: moving all the waiting into CancelAllBackgroundWork(true))
+  CancelAllBackgroundWork(false);
   mutex_.Unlock();
   int compactions_unscheduled = env_->UnSchedule(this, Env::Priority::LOW);
   int flushes_unscheduled = env_->UnSchedule(this, Env::Priority::HIGH);
diff --git a/db/db_impl.h b/db/db_impl.h
index 2a1f1b088..e17f404ef 100644
--- a/db/db_impl.h
+++ b/db/db_impl.h
@@ -275,7 +275,7 @@ class DBImpl : public DB {
 
   const SnapshotList& snapshots() const { return snapshots_; }
 
-  void CancelAllBackgroundWork();
+  void CancelAllBackgroundWork(bool wait);
 
  protected:
   Env* const env_;
diff --git a/include/rocksdb/statistics.h b/include/rocksdb/statistics.h
index 09bf68739..4e06bf6a4 100644
--- a/include/rocksdb/statistics.h
+++ b/include/rocksdb/statistics.h
@@ -240,7 +240,7 @@ enum Histograms : uint32_t {
   NUM_FILES_IN_SINGLE_COMPACTION,
   DB_SEEK,
   WRITE_STALL,
-  HISTOGRAM_ENUM_MAX,
+  HISTOGRAM_ENUM_MAX,  // TODO(ldemailly): enforce HistogramsNameMap match
 };
 
 const std::vector<std::pair<Histograms, std::string>> HistogramsNameMap = {
@@ -263,6 +263,7 @@ const std::vector<std::pair<Histograms, std::string>> HistogramsNameMap = {
   { SOFT_RATE_LIMIT_DELAY_COUNT, "rocksdb.soft.rate.limit.delay.count"},
   { NUM_FILES_IN_SINGLE_COMPACTION, "rocksdb.numfiles.in.singlecompaction" },
   { DB_SEEK, "rocksdb.db.seek.micros" },
+  { WRITE_STALL, "rocksdb.db.write.stall" },
 };
 
 struct HistogramData {
diff --git a/include/rocksdb/utilities/convenience.h b/include/rocksdb/utilities/convenience.h
index e729dcf08..1c1057d3a 100644
--- a/include/rocksdb/utilities/convenience.h
+++ b/include/rocksdb/utilities/convenience.h
@@ -56,7 +56,8 @@ Status GetBlockBasedTableOptionsFromString(
 Status GetOptionsFromString(const Options& base_options,
                             const std::string& opts_str, Options* new_options);
 
-void CancelAllBackgroundWork(DB* db);
+/// Request stopping background work, if wait is true wait until it's done
+void CancelAllBackgroundWork(DB* db, bool wait = false);
 #endif  // ROCKSDB_LITE
 
 }  // namespace rocksdb
diff --git a/utilities/convenience/convenience.cc b/utilities/convenience/convenience.cc
index fc60b0e3a..b91bc9c54 100644
--- a/utilities/convenience/convenience.cc
+++ b/utilities/convenience/convenience.cc
@@ -15,8 +15,8 @@
 
 namespace rocksdb {
 
-void CancelAllBackgroundWork(DB* db) {
-  (dynamic_cast<DBImpl*>(db))->CancelAllBackgroundWork();
+void CancelAllBackgroundWork(DB* db, bool wait) {
+  (dynamic_cast<DBImpl*>(db))->CancelAllBackgroundWork(wait);
 }
 }  // namespace rocksdb
 
diff --git a/utilities/ttl/db_ttl_impl.cc b/utilities/ttl/db_ttl_impl.cc
index a3bdf97b5..f3d941759 100644
--- a/utilities/ttl/db_ttl_impl.cc
+++ b/utilities/ttl/db_ttl_impl.cc
@@ -5,6 +5,7 @@
 
 #include "utilities/ttl/db_ttl_impl.h"
 
+#include "rocksdb/utilities/convenience.h"
 #include "rocksdb/utilities/db_ttl.h"
 #include "db/filename.h"
 #include "db/write_batch_internal.h"
@@ -34,7 +35,11 @@ void DBWithTTLImpl::SanitizeOptions(int32_t ttl, ColumnFamilyOptions* options,
 // Open the db inside DBWithTTLImpl because options needs pointer to its ttl
 DBWithTTLImpl::DBWithTTLImpl(DB* db) : DBWithTTL(db) {}
 
-DBWithTTLImpl::~DBWithTTLImpl() { delete GetOptions().compaction_filter; }
+DBWithTTLImpl::~DBWithTTLImpl() {
+  // Need to stop background compaction before getting rid of the filter
+  CancelAllBackgroundWork(db_, /* wait = */ true);
+  delete GetOptions().compaction_filter;
+}
 
 Status UtilityDB::OpenTtlDB(const Options& options, const std::string& dbname,
                             StackableDB** dbptr, int32_t ttl, bool read_only) {