Mark files as trash by using .trash extension

Summary:
SstFileManager move files that need to be deleted into a trash directory.
Deprecate this behaviour and instead add ".trash" extension to files that need to be deleted
Closes https://github.com/facebook/rocksdb/pull/2970

Differential Revision: D5976805

Pulled By: IslamAbdelRahman

fbshipit-source-id: 27374ece4315610b2792c30ffcd50232d4c9a343

HISTORY.md

@@ -2,6 +2,7 @@
 ## Unreleased
 ### Public API Change
 * `BackupableDBOptions::max_valid_backups_to_open == 0` now means no backups will be opened during BackupEngine initialization. Previously this condition disabled limiting backups opened.
+* Deprecate trash_dir param in NewSstFileManager, right now we will rename deleted files to <name>.trash instead of moving them to trash directory
 
 ### New Features
 * `DBOptions::bytes_per_sync` and `DBOptions::wal_bytes_per_sync` can now be changed dynamically, `DBOptions::wal_bytes_per_sync` will flush all memtables and switch to a new WAL file.

db/db_impl_open.cc

@@ -124,6 +124,17 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) {
     result.avoid_flush_during_recovery = false;
   }
 
+#ifndef ROCKSDB_LITE
+  // When the DB is stopped, it's possible that there are some .trash files that
+  // were not deleted yet, when we open the DB we will find these .trash files
+  // and schedule them to be deleted (or delete immediately if SstFileManager
+  // was not used)
+  auto sfm = static_cast<SstFileManagerImpl*>(result.sst_file_manager.get());
+  for (size_t i = 0; i < result.db_paths.size(); i++) {
+    DeleteScheduler::CleanupDirectory(result.env, sfm, result.db_paths[i].path);
+  }
+#endif
+
   return result;
 }
 

db/db_sst_test.cc

@@ -327,11 +327,10 @@ TEST_F(DBSSTTest, RateLimitedDelete) {
   options.disable_auto_compactions = true;
   options.env = env_;
 
-  std::string trash_dir = test::TmpDir(env_) + "/trash";
   int64_t rate_bytes_per_sec = 1024 * 10;  // 10 Kbs / Sec
   Status s;
   options.sst_file_manager.reset(
-      NewSstFileManager(env_, nullptr, trash_dir, 0, false, &s));
+      NewSstFileManager(env_, nullptr, "", 0, false, &s));
   ASSERT_OK(s);
   options.sst_file_manager->SetDeleteRateBytesPerSecond(rate_bytes_per_sec);
   auto sfm = static_cast<SstFileManagerImpl*>(options.sst_file_manager.get());
@@ -394,11 +393,10 @@ TEST_F(DBSSTTest, DeleteSchedulerMultipleDBPaths) {
   options.db_paths.emplace_back(dbname_ + "_2", 1024 * 100);
   options.env = env_;
 
-  std::string trash_dir = test::TmpDir(env_) + "/trash";
   int64_t rate_bytes_per_sec = 1024 * 1024;  // 1 Mb / Sec
   Status s;
-  options.sst_file_manager.reset(NewSstFileManager(
-      env_, nullptr, trash_dir, rate_bytes_per_sec, false, &s));
+  options.sst_file_manager.reset(
+      NewSstFileManager(env_, nullptr, "", rate_bytes_per_sec, false, &s));
   ASSERT_OK(s);
   auto sfm = static_cast<SstFileManagerImpl*>(options.sst_file_manager.get());
   sfm->delete_scheduler()->TEST_SetMaxTrashDBRatio(1.1);
@@ -460,9 +458,8 @@ TEST_F(DBSSTTest, DestroyDBWithRateLimitedDelete) {
   Options options = CurrentOptions();
   options.disable_auto_compactions = true;
   options.env = env_;
-  std::string trash_dir = test::TmpDir(env_) + "/trash";
   options.sst_file_manager.reset(
-      NewSstFileManager(env_, nullptr, trash_dir, 0, false, &s));
+      NewSstFileManager(env_, nullptr, "", 0, false, &s));
   ASSERT_OK(s);
   DestroyAndReopen(options);
 

include/rocksdb/sst_file_manager.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 #include <unordered_map>
+#include <vector>
 
 #include "rocksdb/status.h"
 
@@ -19,6 +20,7 @@ class Logger;
 // SstFileManager is used to track SST files in the DB and control there
 // deletion rate.
 // All SstFileManager public functions are thread-safe.
+// SstFileManager is not extensible.
 class SstFileManager {
  public:
   virtual ~SstFileManager() {}
@@ -64,17 +66,13 @@ class SstFileManager {
 // @param info_log: If not nullptr, info_log will be used to log errors.
 //
 // == Deletion rate limiting specific arguments ==
-// @param trash_dir: Path to the directory where deleted files will be moved
-//    to be deleted in a background thread while applying rate limiting. If this
-//    directory doesn't exist, it will be created. This directory should not be
-//    used by any other process or any other SstFileManager, Set to "" to
-//    disable deletion rate limiting.
+// @param trash_dir: Deprecated, this argument have no effect
 // @param rate_bytes_per_sec: How many bytes should be deleted per second, If
 //    this value is set to 1024 (1 Kb / sec) and we deleted a file of size 4 Kb
 //    in 1 second, we will wait for another 3 seconds before we delete other
 //    files, Set to 0 to disable deletion rate limiting.
-// @param delete_existing_trash: If set to true, the newly created
-//    SstFileManager will delete files that already exist in trash_dir.
+// @param delete_existing_trash: Deprecated, this argument have no effect, but
+//    if user provide trash_dir we will schedule deletes for files in the dir
 // @param status: If not nullptr, status will contain any errors that happened
 //    during creating the missing trash_dir or deleting existing files in trash.
 extern SstFileManager* NewSstFileManager(

util/delete_scheduler.cc

@@ -19,11 +19,10 @@
 
 namespace rocksdb {
 
-DeleteScheduler::DeleteScheduler(Env* env, const std::string& trash_dir,
-                                 int64_t rate_bytes_per_sec, Logger* info_log,
+DeleteScheduler::DeleteScheduler(Env* env, int64_t rate_bytes_per_sec,
+                                 Logger* info_log,
                                  SstFileManagerImpl* sst_file_manager)
     : env_(env),
-      trash_dir_(trash_dir),
       total_trash_size_(0),
       rate_bytes_per_sec_(rate_bytes_per_sec),
       pending_files_(0),
@@ -63,11 +62,11 @@ Status DeleteScheduler::DeleteFile(const std::string& file_path) {
   }
 
   // Move file to trash
-  std::string path_in_trash;
-  s = MoveToTrash(file_path, &path_in_trash);
+  std::string trash_file;
+  s = MarkAsTrash(file_path, &trash_file);
+
   if (!s.ok()) {
-    ROCKS_LOG_ERROR(info_log_, "Failed to move %s to trash directory (%s)",
-                    file_path.c_str(), trash_dir_.c_str());
+    ROCKS_LOG_ERROR(info_log_, "Failed to mark %s as trash", file_path.c_str());
     s = env_->DeleteFile(file_path);
     if (s.ok()) {
       sst_file_manager_->OnDeleteFile(file_path);
@@ -75,10 +74,15 @@ Status DeleteScheduler::DeleteFile(const std::string& file_path) {
     return s;
   }
 
+  // Update the total trash size
+  uint64_t trash_file_size = 0;
+  env_->GetFileSize(trash_file, &trash_file_size);
+  total_trash_size_.fetch_add(trash_file_size);
+
   // Add file to delete queue
   {
     InstrumentedMutexLock l(&mu_);
-    queue_.push(path_in_trash);
+    queue_.push(trash_file);
     pending_files_++;
     if (pending_files_ == 1) {
       cv_.SignalAll();
@@ -92,44 +96,83 @@ std::map<std::string, Status> DeleteScheduler::GetBackgroundErrors() {
   return bg_errors_;
 }
 
-Status DeleteScheduler::MoveToTrash(const std::string& file_path,
-                                    std::string* path_in_trash) {
+const std::string DeleteScheduler::kTrashExtension = ".trash";
+bool DeleteScheduler::IsTrashFile(const std::string& file_path) {
+  return (file_path.size() >= kTrashExtension.size() &&
+          file_path.rfind(kTrashExtension) ==
+              file_path.size() - kTrashExtension.size());
+}
+
+Status DeleteScheduler::CleanupDirectory(Env* env, SstFileManagerImpl* sfm,
+                                         const std::string& path) {
   Status s;
-  // Figure out the name of the file in trash folder
+  // Check if there are any files marked as trash in this path
+  std::vector<std::string> files_in_path;
+  s = env->GetChildren(path, &files_in_path);
+  if (!s.ok()) {
+    return s;
+  }
+  for (const std::string& current_file : files_in_path) {
+    if (!DeleteScheduler::IsTrashFile(current_file)) {
+      // not a trash file, skip
+      continue;
+    }
+
+    Status file_delete;
+    std::string trash_file = path + "/" + current_file;
+    if (sfm) {
+      // We have an SstFileManager that will schedule the file delete
+      sfm->OnAddFile(trash_file);
+      file_delete = sfm->ScheduleFileDeletion(trash_file);
+    } else {
+      // Delete the file immediately
+      file_delete = env->DeleteFile(trash_file);
+    }
+
+    if (s.ok() && !file_delete.ok()) {
+      s = file_delete;
+    }
+  }
+
+  return s;
+}
+
+Status DeleteScheduler::MarkAsTrash(const std::string& file_path,
+                                    std::string* trash_file) {
+  // Sanity check of the path
   size_t idx = file_path.rfind("/");
   if (idx == std::string::npos || idx == file_path.size() - 1) {
     return Status::InvalidArgument("file_path is corrupted");
   }
-  *path_in_trash = trash_dir_ + file_path.substr(idx);
-  std::string unique_suffix = "";
 
-  if (*path_in_trash == file_path) {
-    // This file is already in trash
+  Status s;
+  if (DeleteScheduler::IsTrashFile(file_path)) {
+    // This is already a trash file
     return s;
   }
 
+  *trash_file = file_path + kTrashExtension;
   // TODO(tec) : Implement Env::RenameFileIfNotExist and remove
   //             file_move_mu mutex.
+  int cnt = 0;
   InstrumentedMutexLock l(&file_move_mu_);
   while (true) {
-    s = env_->FileExists(*path_in_trash + unique_suffix);
+    s = env_->FileExists(*trash_file);
     if (s.IsNotFound()) {
       // We found a path for our file in trash
-      *path_in_trash += unique_suffix;
-      s = env_->RenameFile(file_path, *path_in_trash);
+      s = env_->RenameFile(file_path, *trash_file);
       break;
     } else if (s.ok()) {
       // Name conflict, generate new random suffix
-      unique_suffix = env_->GenerateUniqueId();
+      *trash_file = file_path + std::to_string(cnt) + kTrashExtension;
     } else {
       // Error during FileExists call, we cannot continue
       break;
     }
+    cnt++;
   }
   if (s.ok()) {
-    uint64_t trash_file_size = 0;
-    sst_file_manager_->OnMoveFile(file_path, *path_in_trash, &trash_file_size);
-    total_trash_size_.fetch_add(trash_file_size);
+    sst_file_manager_->OnMoveFile(file_path, *trash_file);
   }
   return s;
 }

util/delete_scheduler.h

@@ -24,7 +24,7 @@ class Logger;
 class SstFileManagerImpl;
 
 // DeleteScheduler allows the DB to enforce a rate limit on file deletion,
-// Instead of deleteing files immediately, files are moved to trash_dir
+// Instead of deleteing files immediately, files are marked as trash
 // and deleted in a background thread that apply sleep penlty between deletes
 // if they are happening in a rate faster than rate_bytes_per_sec,
 //
@@ -32,8 +32,7 @@ class SstFileManagerImpl;
 // case DeleteScheduler will delete files immediately.
 class DeleteScheduler {
  public:
-  DeleteScheduler(Env* env, const std::string& trash_dir,
-                  int64_t rate_bytes_per_sec, Logger* info_log,
+  DeleteScheduler(Env* env, int64_t rate_bytes_per_sec, Logger* info_log,
                   SstFileManagerImpl* sst_file_manager);
 
   ~DeleteScheduler();
@@ -46,7 +45,7 @@ class DeleteScheduler {
     return rate_bytes_per_sec_.store(bytes_per_sec);
   }
 
-  // Move file to trash directory and schedule it's deletion
+  // Mark file as trash directory and schedule it's deletion
   Status DeleteFile(const std::string& fname);
 
   // Wait for all files being deleteing in the background to finish or for
@@ -64,8 +63,16 @@ class DeleteScheduler {
     max_trash_db_ratio_ = r;
   }
 
+  static const std::string kTrashExtension;
+  static bool IsTrashFile(const std::string& file_path);
+
+  // Check if there are any .trash filse in path, and schedule their deletion
+  // Or delete immediately if sst_file_manager is nullptr
+  static Status CleanupDirectory(Env* env, SstFileManagerImpl* sfm,
+                                 const std::string& path);
+
  private:
-  Status MoveToTrash(const std::string& file_path, std::string* path_in_trash);
+  Status MarkAsTrash(const std::string& file_path, std::string* path_in_trash);
 
   Status DeleteTrashFile(const std::string& path_in_trash,
                          uint64_t* deleted_bytes);
@@ -73,17 +80,15 @@ class DeleteScheduler {
   void BackgroundEmptyTrash();
 
   Env* env_;
-  // Path to the trash directory
-  std::string trash_dir_;
-  // total size of trash directory
+  // total size of trash files
   std::atomic<uint64_t> total_trash_size_;
   // Maximum number of bytes that should be deleted per second
   std::atomic<int64_t> rate_bytes_per_sec_;
   // Mutex to protect queue_, pending_files_, bg_errors_, closing_
   InstrumentedMutex mu_;
-  // Queue of files in trash that need to be deleted
+  // Queue of trash files that need to be deleted
   std::queue<std::string> queue_;
-  // Number of files in trash that are waiting to be deleted
+  // Number of trash files that are waiting to be deleted
   int32_t pending_files_;
   // Errors that happened in BackgroundEmptyTrash (file_path => error)
   std::map<std::string, Status> bg_errors_;

util/delete_scheduler_test.cc

@@ -30,8 +30,6 @@ class DeleteSchedulerTest : public testing::Test {
   DeleteSchedulerTest() : env_(Env::Default()) {
     dummy_files_dir_ = test::TmpDir(env_) + "/delete_scheduler_dummy_data_dir";
     DestroyAndCreateDir(dummy_files_dir_);
-    trash_dir_ = test::TmpDir(env_) + "/delete_scheduler_trash";
-    DestroyAndCreateDir(trash_dir_);
   }
 
   ~DeleteSchedulerTest() {
@@ -46,11 +44,31 @@ class DeleteSchedulerTest : public testing::Test {
     EXPECT_OK(env_->CreateDir(dir));
   }
 
-  int CountFilesInDir(const std::string& dir) {
+  int CountNormalFiles() {
     std::vector<std::string> files_in_dir;
-    EXPECT_OK(env_->GetChildren(dir, &files_in_dir));
-    // Ignore "." and ".."
-    return static_cast<int>(files_in_dir.size()) - 2;
+    EXPECT_OK(env_->GetChildren(dummy_files_dir_, &files_in_dir));
+
+    int normal_cnt = 0;
+    for (auto& f : files_in_dir) {
+      if (!DeleteScheduler::IsTrashFile(f) && f != "." && f != "..") {
+        printf("%s\n", f.c_str());
+        normal_cnt++;
+      }
+    }
+    return normal_cnt;
+  }
+
+  int CountTrashFiles() {
+    std::vector<std::string> files_in_dir;
+    EXPECT_OK(env_->GetChildren(dummy_files_dir_, &files_in_dir));
+
+    int trash_cnt = 0;
+    for (auto& f : files_in_dir) {
+      if (DeleteScheduler::IsTrashFile(f)) {
+        trash_cnt++;
+      }
+    }
+    return trash_cnt;
   }
 
   std::string NewDummyFile(const std::string& file_name, uint64_t size = 1024) {
@@ -65,9 +83,8 @@ class DeleteSchedulerTest : public testing::Test {
   }
 
   void NewDeleteScheduler() {
-    ASSERT_OK(env_->CreateDirIfMissing(trash_dir_));
     sst_file_mgr_.reset(
-        new SstFileManagerImpl(env_, nullptr, trash_dir_, rate_bytes_per_sec_));
+        new SstFileManagerImpl(env_, nullptr, rate_bytes_per_sec_));
     delete_scheduler_ = sst_file_mgr_->delete_scheduler();
     // Tests in this file are for DeleteScheduler component and dont create any
     // DBs, so we need to use set this value to 100% (instead of default 25%)
@@ -76,7 +93,6 @@ class DeleteSchedulerTest : public testing::Test {
 
   Env* env_;
   std::string dummy_files_dir_;
-  std::string trash_dir_;
   int64_t rate_bytes_per_sec_;
   DeleteScheduler* delete_scheduler_;
   std::unique_ptr<SstFileManagerImpl> sst_file_mgr_;
@@ -124,7 +140,7 @@ TEST_F(DeleteSchedulerTest, BasicRateLimiting) {
     for (int i = 0; i < num_files; i++) {
       ASSERT_OK(delete_scheduler_->DeleteFile(generated_files[i]));
     }
-    ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
+    ASSERT_EQ(CountNormalFiles(), 0);
 
     uint64_t delete_start_time = env_->NowMicros();
     TEST_SYNC_POINT("DeleteSchedulerTest::BasicRateLimiting:1");
@@ -144,7 +160,7 @@ TEST_F(DeleteSchedulerTest, BasicRateLimiting) {
     }
     ASSERT_GT(time_spent_deleting, expected_penlty * 0.9);
 
-    ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+    ASSERT_EQ(CountTrashFiles(), 0);
     rocksdb::SyncPoint::GetInstance()->DisableProcessing();
   }
 }
@@ -226,8 +242,8 @@ TEST_F(DeleteSchedulerTest, RateLimitingMultiThreaded) {
     }
     ASSERT_GT(time_spent_deleting, expected_penlty * 0.9);
 
-    ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
-    ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+    ASSERT_EQ(CountNormalFiles(), 0);
+    ASSERT_EQ(CountTrashFiles(), 0);
     rocksdb::SyncPoint::GetInstance()->DisableProcessing();
   }
 }
@@ -251,8 +267,8 @@ TEST_F(DeleteSchedulerTest, DisableRateLimiting) {
     std::string dummy_file = NewDummyFile("dummy.data");
     ASSERT_OK(delete_scheduler_->DeleteFile(dummy_file));
     ASSERT_TRUE(env_->FileExists(dummy_file).IsNotFound());
-    ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
-    ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+    ASSERT_EQ(CountNormalFiles(), 0);
+    ASSERT_EQ(CountTrashFiles(), 0);
   }
 
   ASSERT_EQ(bg_delete_file, 0);
@@ -281,14 +297,14 @@ TEST_F(DeleteSchedulerTest, ConflictNames) {
     std::string dummy_file = NewDummyFile("conflict.data");
     ASSERT_OK(delete_scheduler_->DeleteFile(dummy_file));
   }
-  ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
+  ASSERT_EQ(CountNormalFiles(), 0);
   // 10 files ("conflict.data" x 10) in trash
-  ASSERT_EQ(CountFilesInDir(trash_dir_), 10);
+  ASSERT_EQ(CountTrashFiles(), 10);
 
   // Hold BackgroundEmptyTrash
   TEST_SYNC_POINT("DeleteSchedulerTest::ConflictNames:1");
   delete_scheduler_->WaitForEmptyTrash();
-  ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+  ASSERT_EQ(CountTrashFiles(), 0);
 
   auto bg_errors = delete_scheduler_->GetBackgroundErrors();
   ASSERT_EQ(bg_errors.size(), 0);
@@ -317,15 +333,15 @@ TEST_F(DeleteSchedulerTest, BackgroundError) {
     std::string file_name = "data_" + ToString(i) + ".data";
     ASSERT_OK(delete_scheduler_->DeleteFile(NewDummyFile(file_name)));
   }
-  ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
-  ASSERT_EQ(CountFilesInDir(trash_dir_), 10);
+  ASSERT_EQ(CountNormalFiles(), 0);
+  ASSERT_EQ(CountTrashFiles(), 10);
 
   // Delete 10 files from trash, this will cause background errors in
   // BackgroundEmptyTrash since we already deleted the files it was
   // goind to delete
   for (int i = 0; i < 10; i++) {
-    std::string file_name = "data_" + ToString(i) + ".data";
-    ASSERT_OK(env_->DeleteFile(trash_dir_ + "/" + file_name));
+    std::string file_name = "data_" + ToString(i) + ".data.trash";
+    ASSERT_OK(env_->DeleteFile(dummy_files_dir_ + "/" + file_name));
   }
 
   // Hold BackgroundEmptyTrash
@@ -359,10 +375,10 @@ TEST_F(DeleteSchedulerTest, StartBGEmptyTrashMultipleTimes) {
       std::string file_name = "data_" + ToString(i) + ".data";
       ASSERT_OK(delete_scheduler_->DeleteFile(NewDummyFile(file_name)));
     }
-    ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
+    ASSERT_EQ(CountNormalFiles(), 0);
     delete_scheduler_->WaitForEmptyTrash();
     ASSERT_EQ(bg_delete_file, 10 * run);
-    ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+    ASSERT_EQ(CountTrashFiles(), 0);
 
     auto bg_errors = delete_scheduler_->GetBackgroundErrors();
     ASSERT_EQ(bg_errors.size(), 0);
@@ -397,35 +413,7 @@ TEST_F(DeleteSchedulerTest, DestructorWithNonEmptyQueue) {
   sst_file_mgr_.reset();
 
   ASSERT_LT(bg_delete_file, 100);
-  ASSERT_GT(CountFilesInDir(trash_dir_), 0);
-
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-}
-
-// 1- Delete the trash directory
-// 2- Delete 10 files using DeleteScheduler
-// 3- Make sure that the 10 files were deleted immediately since DeleteScheduler
-//    failed to move them to trash directory
-TEST_F(DeleteSchedulerTest, MoveToTrashError) {
-  int bg_delete_file = 0;
-  rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "DeleteScheduler::DeleteTrashFile:DeleteFile",
-      [&](void* arg) { bg_delete_file++; });
-  rocksdb::SyncPoint::GetInstance()->EnableProcessing();
-
-  rate_bytes_per_sec_ = 1024;  // 1 Kb / sec
-  NewDeleteScheduler();
-
-  // We will delete the trash directory, that mean that DeleteScheduler wont
-  // be able to move files to trash and will delete files them immediately.
-  ASSERT_OK(test::DestroyDir(env_, trash_dir_));
-  for (int i = 0; i < 10; i++) {
-    std::string file_name = "data_" + ToString(i) + ".data";
-    ASSERT_OK(delete_scheduler_->DeleteFile(NewDummyFile(file_name)));
-  }
-
-  ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
-  ASSERT_EQ(bg_delete_file, 0);
+  ASSERT_GT(CountTrashFiles(), 0);
 
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
@@ -480,7 +468,7 @@ TEST_F(DeleteSchedulerTest, DISABLED_DynamicRateLimiting1) {
     for (int i = 0; i < num_files; i++) {
       ASSERT_OK(delete_scheduler_->DeleteFile(generated_files[i]));
     }
-    ASSERT_EQ(CountFilesInDir(dummy_files_dir_), 0);
+    ASSERT_EQ(CountNormalFiles(), 0);
 
     if (rate_bytes_per_sec_ > 0) {
       uint64_t delete_start_time = env_->NowMicros();
@@ -508,7 +496,7 @@ TEST_F(DeleteSchedulerTest, DISABLED_DynamicRateLimiting1) {
       ASSERT_EQ(fg_delete_file, num_files);
     }
 
-    ASSERT_EQ(CountFilesInDir(trash_dir_), 0);
+    ASSERT_EQ(CountTrashFiles(), 0);
     rocksdb::SyncPoint::GetInstance()->DisableProcessing();
   }
 }
@@ -548,6 +536,27 @@ TEST_F(DeleteSchedulerTest, ImmediateDeleteOn25PercDBSize) {
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
 
+TEST_F(DeleteSchedulerTest, IsTrashCheck) {
+  // Trash files
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("x.trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile(".trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("abc.sst.trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("/a/b/c/abc..sst.trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("log.trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("^^^^^.log.trash"));
+  ASSERT_TRUE(DeleteScheduler::IsTrashFile("abc.t.trash"));
+
+  // Not trash files
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("abc.sst"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("abc.txt"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("/a/b/c/abc.sst"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("/a/b/c/abc.sstrash"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("^^^^^.trashh"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("abc.ttrash"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile(".ttrash"));
+  ASSERT_FALSE(DeleteScheduler::IsTrashFile("abc.trashx"));
+}
+
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {

util/sst_file_manager_impl.cc

@@ -17,14 +17,12 @@ namespace rocksdb {
 
 #ifndef ROCKSDB_LITE
 SstFileManagerImpl::SstFileManagerImpl(Env* env, std::shared_ptr<Logger> logger,
-                                       const std::string& trash_dir,
                                        int64_t rate_bytes_per_sec)
     : env_(env),
       logger_(logger),
       total_files_size_(0),
       max_allowed_space_(0),
-      delete_scheduler_(env, trash_dir, rate_bytes_per_sec, logger.get(),
-                        this) {}
+      delete_scheduler_(env, rate_bytes_per_sec, logger.get(), this) {}
 
 SstFileManagerImpl::~SstFileManagerImpl() {}
 
@@ -132,26 +130,25 @@ SstFileManager* NewSstFileManager(Env* env, std::shared_ptr<Logger> info_log,
                                   int64_t rate_bytes_per_sec,
                                   bool delete_existing_trash, Status* status) {
   SstFileManagerImpl* res =
-      new SstFileManagerImpl(env, info_log, trash_dir, rate_bytes_per_sec);
+      new SstFileManagerImpl(env, info_log, rate_bytes_per_sec);
 
+  // trash_dir is deprecated and not needed anymore, but if user passed it
+  // we will still remove files in it.
   Status s;
-  if (trash_dir != "") {
-    s = env->CreateDirIfMissing(trash_dir);
-    if (s.ok() && delete_existing_trash) {
-      std::vector<std::string> files_in_trash;
-      s = env->GetChildren(trash_dir, &files_in_trash);
-      if (s.ok()) {
-        for (const std::string& trash_file : files_in_trash) {
-          if (trash_file == "." || trash_file == "..") {
-            continue;
-          }
-
-          std::string path_in_trash = trash_dir + "/" + trash_file;
-          res->OnAddFile(path_in_trash);
-          Status file_delete = res->ScheduleFileDeletion(path_in_trash);
-          if (s.ok() && !file_delete.ok()) {
-            s = file_delete;
-          }
+  if (delete_existing_trash && trash_dir != "") {
+    std::vector<std::string> files_in_trash;
+    s = env->GetChildren(trash_dir, &files_in_trash);
+    if (s.ok()) {
+      for (const std::string& trash_file : files_in_trash) {
+        if (trash_file == "." || trash_file == "..") {
+          continue;
+        }
+
+        std::string path_in_trash = trash_dir + "/" + trash_file;
+        res->OnAddFile(path_in_trash);
+        Status file_delete = res->ScheduleFileDeletion(path_in_trash);
+        if (s.ok() && !file_delete.ok()) {
+          s = file_delete;
         }
       }
     }

util/sst_file_manager_impl.h

@@ -25,7 +25,6 @@ class Logger;
 class SstFileManagerImpl : public SstFileManager {
  public:
   explicit SstFileManagerImpl(Env* env, std::shared_ptr<Logger> logger,
-                              const std::string& trash_dir,
                               int64_t rate_bytes_per_sec);
 
   ~SstFileManagerImpl();
@@ -68,7 +67,7 @@ class SstFileManagerImpl : public SstFileManager {
   // Update the delete rate limit in bytes per second.
   virtual void SetDeleteRateBytesPerSecond(int64_t delete_rate) override;
 
-  // Move file to trash directory and schedule it's deletion.
+  // Mark file as trash and schedule it's deletion.
   virtual Status ScheduleFileDeletion(const std::string& file_path);
 
   // Wait for all files being deleteing in the background to finish or for
@@ -94,9 +93,7 @@ class SstFileManagerImpl : public SstFileManager {
   std::unordered_map<std::string, uint64_t> tracked_files_;
   // The maximum allowed space (in bytes) for sst files.
   uint64_t max_allowed_space_;
-  // DeleteScheduler used to throttle file deletition, if SstFileManagerImpl was
-  // created with rate_bytes_per_sec == 0 or trash_dir == "", delete_scheduler_
-  // rate limiting will be disabled and will simply delete the files.
+  // DeleteScheduler used to throttle file deletition.
   DeleteScheduler delete_scheduler_;
 };