From 9c0e66ce9811bf16e56254d85231a5b980ef4571 Mon Sep 17 00:00:00 2001
From: Igor Canadi <icanadi@fb.com>
Date: Thu, 11 Sep 2014 16:24:16 -0700
Subject: [PATCH] Don't run background jobs (flush, compactions) when bg_error_
 is set

Summary:
If bg_error_ is set, that means that we mark DB read only. However, current behavior still continues the flushes and compactions, even though bg_error_ is set.

On the other hand, if bg_error_ is set, we will return Status::OK() from CompactRange(), although the compaction didn't actually succeed.

This is clearly not desired behavior. I found this when I was debugging t5132159, although I'm pretty sure these aren't related.

Also, when we're shutting down, it's dangerous to exit RunManualCompaction(), since that will destruct ManualCompaction object. Background compaction job might still hold a reference to manual_compaction_ and this will lead to undefined behavior. I changed the behavior so that we only exit RunManualCompaction when manual compaction job is marked done.

Test Plan: make check

Reviewers: sdong, ljin, yhchiang

Reviewed By: yhchiang

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D23223
---
 db/db_impl.cc | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/db/db_impl.cc b/db/db_impl.cc
index 74c114bfd2d..3a9596686d3 100644
--- a/db/db_impl.cc
+++ b/db/db_impl.cc
@@ -1891,7 +1891,10 @@ Status DBImpl::RunManualCompaction(ColumnFamilyData* cfd, int input_level,
   Log(db_options_.info_log, "[%s] Manual compaction starting",
       cfd->GetName().c_str());
 
-  while (!manual.done && !shutting_down_.Acquire_Load() && bg_error_.ok()) {
+  // We don't check bg_error_ here, because if we get the error in compaction,
+  // the compaction will set manual.status to bg_error_ and set manual.done to
+  // true.
+  while (!manual.done) {
     assert(bg_manual_only_ > 0);
     if (manual_compaction_ != nullptr) {
       // Running either this or some other manual compaction
@@ -2041,6 +2044,11 @@ Status DBImpl::BackgroundFlush(bool* madeProgress,
                                DeletionState& deletion_state,
                                LogBuffer* log_buffer) {
   mutex_.AssertHeld();
+
+  if (!bg_error_.ok()) {
+    return bg_error_;
+  }
+
   // call_status is failure if at least one flush was a failure. even if
   // flushing one column family reports a failure, we will continue flushing
   // other column families. however, call_status will be a failure in that case.
@@ -2228,6 +2236,16 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress,
   bool is_manual = (manual_compaction_ != nullptr) &&
                    (manual_compaction_->in_progress == false);
 
+  if (!bg_error_.ok()) {
+    if (is_manual) {
+      manual_compaction_->status = bg_error_;
+      manual_compaction_->done = true;
+      manual_compaction_->in_progress = false;
+      manual_compaction_ = nullptr;
+    }
+    return bg_error_;
+  }
+
   if (is_manual) {
     // another thread cannot pick up the same work
     manual_compaction_->in_progress = true;