move enabling file deletion to be within error handler

facebook · Oct 17, 2023 · 5e45142 · 5e45142
1 parent 8420b9d
commit 5e45142
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 48 deletions.
diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc
@@ -405,38 +405,6 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) {
     }
   }
 
-  std::optional<int> remain_counter;
-  if (s.ok()) {
-    assert(versions_->io_status().ok());
-    int disable_file_deletion_count =
-        error_handler_.GetAndResetDisableFileDeletionCount();
-    if (disable_file_deletion_count) {
-      // If we reach here, we should re-enable file deletions if it was disabled
-      // during previous error handling.
-      remain_counter = EnableFileDeletionsWithLock(disable_file_deletion_count);
-    }
-  }
-
-  JobContext job_context(0);
-  FindObsoleteFiles(&job_context, true);
-  mutex_.Unlock();
-  if (remain_counter.has_value()) {
-    if (remain_counter.value() == 0) {
-      ROCKS_LOG_INFO(immutable_db_options_.info_log, "File Deletions Enabled");
-    } else {
-      ROCKS_LOG_WARN(
-          immutable_db_options_.info_log,
-          "File Deletions Enable, but not really enabled. Counter: %d",
-          remain_counter.value());
-    }
-  }
-  job_context.manifest_file_number = 1;
-  if (job_context.HaveSomethingToDelete()) {
-    PurgeObsoleteFiles(job_context);
-  }
-  job_context.Clean();
-
-  mutex_.Lock();
   if (s.ok()) {
     // This will notify and unblock threads waiting for error recovery to
     // finish. Those previouly waiting threads can now proceed, which may
@@ -449,13 +417,23 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) {
     error_handler_.GetRecoveryError().PermitUncheckedError();
   }
 
+  JobContext job_context(0);
+  FindObsoleteFiles(&job_context, true);
+  mutex_.Unlock();
+  job_context.manifest_file_number = 1;
+  if (job_context.HaveSomethingToDelete()) {
+    PurgeObsoleteFiles(job_context);
+  }
+  job_context.Clean();
+
   if (s.ok()) {
     ROCKS_LOG_INFO(immutable_db_options_.info_log, "Successfully resumed DB");
   } else {
     ROCKS_LOG_INFO(immutable_db_options_.info_log, "Failed to resume DB [%s]",
                    s.ToString().c_str());
   }
 
+  mutex_.Lock();
   // Check for shutdown again before scheduling further compactions,
   // since we released and re-acquired the lock above
   if (shutdown_initiated_) {

diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h
@@ -2373,9 +2373,9 @@ class DBImpl : public DB {
 
   Status DisableFileDeletionsWithLock();
 
-  // Decrease `disable_delete_obsolete_files_` by count while holding lock and
-  // return its remaning value.
-  int EnableFileDeletionsWithLock(int count);
+  // Safely decrease `disable_delete_obsolete_files_` by one while holding lock
+  // and return its remaning value.
+  int EnableFileDeletionsWithLock();
 
   Status IncreaseFullHistoryTsLowImpl(ColumnFamilyData* cfd,
                                       std::string ts_low);

diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc
@@ -100,11 +100,11 @@ Status DBImpl::EnableFileDeletions(bool force) {
   return Status::OK();
 }
 
-int DBImpl::EnableFileDeletionsWithLock(int count) {
+int DBImpl::EnableFileDeletionsWithLock() {
   mutex_.AssertHeld();
   // In case others have called EnableFileDeletions(true /* force */) in between
   disable_delete_obsolete_files_ =
-      std::max(0, disable_delete_obsolete_files_ - count);
+      std::max(0, disable_delete_obsolete_files_ - 1);
   return disable_delete_obsolete_files_;
 }
 

diff --git a/db/error_handler.cc b/db/error_handler.cc
@@ -396,12 +396,13 @@ const Status& ErrorHandler::SetBGError(const Status& bg_status,
   ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s",
                  bg_io_err.ToString().c_str());
 
-  if (BackgroundErrorReason::kManifestWrite == reason ||
-      BackgroundErrorReason::kManifestWriteNoWAL == reason) {
+  if (!recovery_disabled_file_deletion_ &&
+      (BackgroundErrorReason::kManifestWrite == reason ||
+       BackgroundErrorReason::kManifestWriteNoWAL == reason)) {
     // Always returns ok
     ROCKS_LOG_INFO(db_options_.info_log, "Disabling File Deletions");
     db_->DisableFileDeletionsWithLock().PermitUncheckedError();
-    disable_file_deletion_count_.fetch_add(1);
+    recovery_disabled_file_deletion_ = true;
   }
 
   Status new_bg_io_err = bg_io_err;
@@ -561,6 +562,18 @@ Status ErrorHandler::ClearBGError() {
     recovery_error_.PermitUncheckedError();
     recovery_in_prog_ = false;
     soft_error_no_bg_work_ = false;
+    if (recovery_disabled_file_deletion_) {
+      recovery_disabled_file_deletion_ = false;
+      int remain_counter = db_->EnableFileDeletionsWithLock();
+      if (remain_counter == 0) {
+        ROCKS_LOG_INFO(db_options_.info_log, "File Deletions Enabled");
+      } else {
+        ROCKS_LOG_WARN(
+            db_options_.info_log,
+            "File Deletions Enable, but not really enabled. Counter: %d",
+            remain_counter);
+      }
+    }
     EventHelpers::NotifyOnErrorRecoveryEnd(db_options_.listeners, old_bg_error,
                                            bg_error_, db_mutex_);
   }

diff --git a/db/error_handler.h b/db/error_handler.h
@@ -42,7 +42,8 @@ class ErrorHandler {
         recovery_in_prog_(false),
         soft_error_no_bg_work_(false),
         is_db_stopped_(false),
-        bg_error_stats_(db_options.statistics) {
+        bg_error_stats_(db_options.statistics),
+        recovery_disabled_file_deletion_(false) {
     // Clear the checked flag for uninitialized errors
     bg_error_.PermitUncheckedError();
     recovery_error_.PermitUncheckedError();
@@ -80,10 +81,6 @@ class ErrorHandler {
 
   void EndAutoRecovery();
 
-  int GetAndResetDisableFileDeletionCount() {
-    return disable_file_deletion_count_.exchange(0);
-  };
-
  private:
   DBImpl* db_;
   const ImmutableDBOptions& db_options_;
@@ -112,9 +109,9 @@ class ErrorHandler {
   // The pointer of DB statistics.
   std::shared_ptr<Statistics> bg_error_stats_;
 
-  // The number of times this error handler called DisableFileDeletionsWithLock
-  // to disable file deletions.
-  std::atomic<int> disable_file_deletion_count_;
+  // Tracks whether the recovery has disabled file deletion. This boolean flag
+  // is updated while holding db mutex.
+  bool recovery_disabled_file_deletion_;
 
   const Status& HandleKnownErrors(const Status& bg_err,
                                   BackgroundErrorReason reason);