Prevent segfault because SizeUnderCompaction was called without any locks.

Summary:
SizeBeingCompacted was called without any lock protection. This causes
crashes, especially when running db_bench with value_size=128K.
The fix is to compute SizeUnderCompaction while holding the mutex and
passing in these values into the call to Finalize.

(gdb) where
#4  leveldb::VersionSet::SizeBeingCompacted (this=this@entry=0x7f0b490931c0, level=level@entry=4) at db/version_set.cc:1827
#5  0x000000000043a3c8 in leveldb::VersionSet::Finalize (this=this@entry=0x7f0b490931c0, v=v@entry=0x7f0b3b86b480) at db/version_set.cc:1420
#6  0x00000000004418d1 in leveldb::VersionSet::LogAndApply (this=0x7f0b490931c0, edit=0x7f0b3dc8c200, mu=0x7f0b490835b0, new_descriptor_log=<optimized out>) at db/version_set.cc:1016
#7  0x00000000004222b2 in leveldb::DBImpl::InstallCompactionResults (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1473
#8  0x0000000000426027 in leveldb::DBImpl::DoCompactionWork (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1757
#9  0x0000000000426690 in leveldb::DBImpl::BackgroundCompaction (this=this@entry=0x7f0b49083400, madeProgress=madeProgress@entry=0x7f0b41bf2d1e, deletion_state=...) at db/db_impl.cc:1268
#10 0x0000000000428f42 in leveldb::DBImpl::BackgroundCall (this=0x7f0b49083400) at db/db_impl.cc:1170
#11 0x000000000045348e in BGThread (this=0x7f0b49023100) at util/env_posix.cc:941
#12 leveldb::(anonymous namespace)::PosixEnv::BGThreadWrapper (arg=0x7f0b49023100) at util/env_posix.cc:874
#13 0x00007f0b4a7cf10d in start_thread (arg=0x7f0b41bf3700) at pthread_create.c:301
#14 0x00007f0b49b4b11d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115

Test Plan:
make check

I am running db_bench with a value size of 128K to see if the segfault is fixed.

Reviewers: MarkCallaghan, sheki, emayanke

Reviewed By: sheki

CC: leveldb
Differential Revision: https://reviews.facebook.net/D9279

diff --git a/db/version_set.cc b/db/version_set.cc
index 8c1983adcbd81548dc03e0bf07a8b1e010e0e683..23a5b05f63875460d5cc97fd84a55d75859c08df 100644 (file)
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -1009,11 +1009,15 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu,
   // Unlock during expensive MANIFEST log write. New writes cannot get here
   // because &w is ensuring that all new writes get queued.
   {
+    // calculate the amount of data being compacted at every level
+    std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
+    SizeBeingCompacted(size_being_compacted);
+
     mu->Unlock();
 
-    // The calles to Finalize and UpdateFilesBySize are cpu-heavy
+    // The calls to Finalize and UpdateFilesBySize are cpu-heavy
     // and is best called outside the mutex.
-    Finalize(v);
+    Finalize(v, size_being_compacted);
     UpdateFilesBySize(v);
 
     // Write new record to MANIFEST log
@@ -1226,8 +1230,12 @@ Status VersionSet::Recover() {
   if (s.ok()) {
     Version* v = new Version(this, current_version_number_++);
     builder.SaveTo(v);
+
     // Install recovered version
-    Finalize(v);
+    std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
+    SizeBeingCompacted(size_being_compacted);
+    Finalize(v, size_being_compacted);
+
     v->offset_manifest_file_ = manifest_file_size;
     AppendVersion(v);
     manifest_file_number_ = next_file;
@@ -1350,8 +1358,12 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
   if (s.ok()) {
     Version* v = new Version(this, 0);
     builder.SaveTo(v);
+
     // Install recovered version
-    Finalize(v);
+    std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
+    SizeBeingCompacted(size_being_compacted);
+    Finalize(v, size_being_compacted);
+
     AppendVersion(v);
     manifest_file_number_ = next_file;
     next_file_number_ = next_file + 1;
@@ -1374,7 +1386,8 @@ void VersionSet::MarkFileNumberUsed(uint64_t number) {
   }
 }
 
-void VersionSet::Finalize(Version* v) {
+void VersionSet::Finalize(Version* v,
+  std::vector<uint64_t>& size_being_compacted) {
 
   double max_score = 0;
   int max_score_level = 0;
@@ -1417,7 +1430,7 @@ void VersionSet::Finalize(Version* v) {
     } else {
       // Compute the ratio of current size to size limit.
       const uint64_t level_bytes = TotalFileSize(v->files_[level]) -
-                                   SizeBeingCompacted(level);
+                                   size_being_compacted[level];
       score = static_cast<double>(level_bytes) / MaxBytesForLevel(level);
       if (score > 1) {
         // Log(options_->info_log, "XXX score l%d = %d ", level, (int)score);
@@ -1822,19 +1835,22 @@ void VersionSet::ReleaseCompactionFiles(Compaction* c, Status status) {
 }
 
 // The total size of files that are currently being compacted
-uint64_t VersionSet::SizeBeingCompacted(int level) {
-  uint64_t total = 0;
-  for (std::set<Compaction*>::iterator it =
-       compactions_in_progress_[level].begin();
-       it != compactions_in_progress_[level].end();
-       ++it) {
-    Compaction* c = (*it);
-    assert(c->level() == level);
-    for (int i = 0; i < c->num_input_files(0); i++) {
-      total += c->input(0,i)->file_size;
+// at at every level upto the penultimate level.
+void VersionSet::SizeBeingCompacted(std::vector<uint64_t>& sizes) {
+  for (int level = 0; level < NumberLevels()-1; level++) {
+    uint64_t total = 0;
+    for (std::set<Compaction*>::iterator it =
+         compactions_in_progress_[level].begin();
+         it != compactions_in_progress_[level].end();
+         ++it) {
+      Compaction* c = (*it);
+      assert(c->level() == level);
+      for (int i = 0; i < c->num_input_files(0); i++) {
+        total += c->input(0,i)->file_size;
+      }
     }
+    sizes[level] = total;
   }
-  return total;
 }
 
 Compaction* VersionSet::PickCompactionBySize(int level, double score) {
@@ -1916,7 +1932,9 @@ Compaction* VersionSet::PickCompaction() {
 
   // compute the compactions needed. It is better to do it here
   // and also in LogAndApply(), otherwise the values could be stale.
-  Finalize(current_);
+  std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
+  current_->vset_->SizeBeingCompacted(size_being_compacted);
+  Finalize(current_, size_being_compacted);
 
   // We prefer compactions triggered by too much data in a level over
   // the compactions triggered by seeks.

diff --git a/db/version_set.h b/db/version_set.h
index f8ec71620b788054e2678c1cac97c7eff576ae1a..17069b2cf2df598a8fe631adc76b281859eda42e 100644 (file)
--- a/db/version_set.h
+++ b/db/version_set.h
@@ -387,7 +387,7 @@ class VersionSet {
 
   void Init(int num_levels);
 
-  void Finalize(Version* v);
+  void Finalize(Version* v, std::vector<uint64_t>&);
 
   void GetRange(const std::vector<FileMetaData*>& inputs,
                 InternalKey* smallest,
@@ -459,8 +459,8 @@ class VersionSet {
   void operator=(const VersionSet&);
 
   // Return the total amount of data that is undergoing
-  // compactions at this level
-  uint64_t SizeBeingCompacted(int level);
+  // compactions per level
+  void SizeBeingCompacted(std::vector<uint64_t>&);
 
   // Returns true if any one of the parent files are being compacted
   bool ParentRangeInCompaction(const InternalKey* smallest,