Release trash accounting on immediate retry and after dir sync failure (#15352)

Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15352

DeleteScheduler now releases a trash file's accounting in two more cases where the file is gone but its size stayed counted until restart.

**Why it matters:** Leaked bytes stay in the total trash size. Once they pass `max_trash_db_ratio` of the tracked DB size, every later deletion skips rate limiting and is deleted immediately, which can cause I/O stalls.

- **Immediate retry:** A failed accounted trash deletion is retried through `DeleteExistingTrashFileImpl`. When the retry deletes immediately, for example with rate limiting disabled, and the file is already gone, `DeleteFileImmediately` skipped `OnDeleteFile` and the retry entry was put back. It now untracks the file and subtracts the entry's size. The NotFound status is still returned.
- **Dir sync failure:** If the unlink worked but `NewDirectory` or the dir fsync then failed, `DeleteTrashFile` reported 0 deleted bytes and skipped `OnDeleteFile`, so the accounting stayed. It now records the deleted bytes and untracks the file right after the unlink, and reports `file_gone` so `BackgroundEmptyTrash` releases the rest without parking a retry entry. The sync error is still reported as a background error.
- **Details:** Adds an `IsFileGone` helper for NotFound/PathNotFound and a `DeleteScheduler::DeleteTrashFile:BeforeSyncDir` sync point for tests. Both new tests keep one more tracked file, so they check that only the gone file's bytes are released.

Reviewed By: anand1976

Differential Revision: D124000703

fbshipit-source-id: 54ed8be66d5670709054a46a258ceaf72b10d899

file/delete_scheduler.cc

@@ -22,6 +22,12 @@
 
 namespace ROCKSDB_NAMESPACE {
 
+namespace {
+bool IsFileGone(const Status& s) {
+  return s.IsNotFound() || s.IsPathNotFound();
+}
+}  // namespace
+
 DeleteScheduler::DeleteScheduler(SystemClock* clock, FileSystem* fs,
                                  int64_t rate_bytes_per_sec, Logger* info_log,
                                  SstFileManagerImpl* sst_file_manager,
@@ -150,7 +156,7 @@ Status DeleteScheduler::DeleteExistingTrashFileImpl(
       (!accounted && !force_bg && num_hard_links > 1)) {
     Status s = DeleteFileImmediately(file_path_to_delete, accounted);
     InstrumentedMutexLock l(&mu_);
-    if (s.ok()) {
+    if (s.ok() || IsFileGone(s)) {
       if (accounted_trash_size.has_value()) {
         total_trash_size_.fetch_sub(accounted_trash_size.value());
       }
@@ -181,6 +187,8 @@ Status DeleteScheduler::DeleteFileImmediately(const std::string& file_path,
     s = OnDeleteFile(file_path, accounted);
     InstrumentedMutexLock l(&mu_);
     RecordTick(stats_.get(), FILES_DELETED_IMMEDIATELY);
+  } else if (IsFileGone(s)) {
+    OnDeleteFile(file_path, accounted).PermitUncheckedError();
   }
   return s;
 }
@@ -381,14 +389,15 @@ void DeleteScheduler::BackgroundEmptyTrash() {
       mu_.Unlock();
       uint64_t deleted_bytes = 0;
       bool is_complete = true;
+      bool trash_file_gone = false;
       // Delete file from trash and update total_penlty value
-      Status s = DeleteTrashFile(path_in_trash, dir_to_sync, accounted,
-                                 &deleted_bytes, &is_complete);
+      Status s =
+          DeleteTrashFile(path_in_trash, dir_to_sync, accounted, &deleted_bytes,
+                          &is_complete, &trash_file_gone);
       total_deleted_bytes += deleted_bytes;
       mu_.Lock();
-      // A trash file that is already gone (e.g. its directory was renamed)
-      // can never be retried, so release all of its remaining accounting.
-      const bool trash_file_gone = s.IsNotFound() || s.IsPathNotFound();
+      // A trash file that is already gone can never be retried, so release
+      // all of its remaining accounting.
       const uint64_t accounted_deleted_bytes =
           trash_file_gone ? accounted_trash_size
                           : std::min(deleted_bytes, accounted_trash_size);
@@ -472,10 +481,11 @@ void DeleteScheduler::BackgroundEmptyTrash() {
 Status DeleteScheduler::DeleteTrashFile(const std::string& path_in_trash,
                                         const std::string& dir_to_sync,
                                         bool accounted, uint64_t* deleted_bytes,
-                                        bool* is_complete) {
+                                        bool* is_complete, bool* file_gone) {
   uint64_t file_size;
   Status s = fs_->GetFileSize(path_in_trash, IOOptions(), &file_size, nullptr);
   *is_complete = true;
+  *file_gone = false;
   TEST_SYNC_POINT("DeleteScheduler::DeleteTrashFile:DeleteFile");
   TEST_SYNC_POINT_CALLBACK("DeleteScheduler::DeleteTrashFile::cb",
                            const_cast<std::string*>(&path_in_trash));
@@ -533,7 +543,14 @@ Status DeleteScheduler::DeleteTrashFile(const std::string& path_in_trash,
       if (s.ok()) {
         s = fs_->DeleteFile(path_in_trash, IOOptions(), nullptr);
       }
-      if (!dir_to_sync.empty()) {
+      if (s.ok()) {
+        *deleted_bytes = file_size;
+        *file_gone = true;
+        s = OnDeleteFile(path_in_trash, accounted);
+      }
+      if (s.ok() && !dir_to_sync.empty()) {
+        TEST_SYNC_POINT_CALLBACK(
+            "DeleteScheduler::DeleteTrashFile:BeforeSyncDir", &s);
         std::unique_ptr<FSDirectory> dir_obj;
         if (s.ok()) {
           s = fs_->NewDirectory(dir_to_sync, IOOptions(), &dir_obj, nullptr);
@@ -547,20 +564,19 @@ Status DeleteScheduler::DeleteTrashFile(const std::string& path_in_trash,
               static_cast<void*>(const_cast<std::string*>(&dir_to_sync)));
         }
       }
-      if (s.ok()) {
-        *deleted_bytes = file_size;
-        s = OnDeleteFile(path_in_trash, accounted);
-      }
     }
   }
   if (!s.ok()) {
-    // Error while getting file size or while deleting
+    // Error while getting file size, deleting, or syncing the directory
     ROCKS_LOG_ERROR(info_log_, "Failed to delete %s from trash -- %s",
                     path_in_trash.c_str(), s.ToString().c_str());
-    *deleted_bytes = 0;
-    if (s.IsNotFound() || s.IsPathNotFound()) {
+    if (!*file_gone && IsFileGone(s)) {
+      *file_gone = true;
       OnDeleteFile(path_in_trash, accounted).PermitUncheckedError();
     }
+    if (!*file_gone) {
+      *deleted_bytes = 0;
+    }
   }
 
   return s;

file/delete_scheduler.h

@@ -145,7 +145,8 @@ class DeleteScheduler {
 
   Status DeleteTrashFile(const std::string& path_in_trash,
                          const std::string& dir_to_sync, bool accounted,
-                         uint64_t* deleted_bytes, bool* is_complete);
+                         uint64_t* deleted_bytes, bool* is_complete,
+                         bool* file_gone);
 
   Status OnDeleteFile(const std::string& file_path, bool accounted);
 

file/delete_scheduler_test.cc

@@ -854,6 +854,63 @@ TEST_F(DeleteSchedulerTest, RetryFailedAccountedTrashDeletion) {
   }
 }
 
+TEST_F(DeleteSchedulerTest, ImmediateRetryOfVanishedFailedTrashDeletion) {
+  rate_bytes_per_sec_ = 1024 * 1024;  // 1 MB / s
+  NewDeleteScheduler();
+
+  constexpr uint64_t kept_file_size = 2048;
+  NewDummyFile("kept.data", kept_file_size);
+  const std::string file = NewDummyFile("retry.data", 1024);
+  const std::string trash_file = file + DeleteScheduler::kTrashExtension;
+  std::atomic<bool> fail_next_delete{true};
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
+      "DeleteScheduler::DeleteTrashFile:BeforeDeleteFile", [&](void* arg) {
+        if (fail_next_delete.exchange(false)) {
+          *static_cast<Status*>(arg) = Status::IOError("injected delete error");
+        }
+      });
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
+
+  ASSERT_OK(delete_scheduler_->DeleteFile(file, dummy_files_dirs_[0],
+                                          /*force_bg=*/true));
+  delete_scheduler_->WaitForEmptyTrashInDirectory(dummy_files_dirs_[0]);
+  ASSERT_OK(env_->FileExists(trash_file));
+
+  ASSERT_OK(env_->DeleteFile(trash_file));
+  delete_scheduler_->SetRateBytesPerSecond(0);
+  ASSERT_TRUE(sst_file_mgr_
+                  ->ScheduleExistingTrashFileDeletion(
+                      trash_file, dummy_files_dirs_[0], /*force_bg=*/false)
+                  .IsPathNotFound());
+
+  ASSERT_EQ(0, delete_scheduler_->GetTotalTrashSize());
+  ASSERT_EQ(kept_file_size, sst_file_mgr_->GetTotalSize());
+}
+
+TEST_F(DeleteSchedulerTest, DirSyncFailureAfterTrashFileDeleted) {
+  rate_bytes_per_sec_ = 1024 * 1024;  // 1 MB / s
+  NewDeleteScheduler();
+
+  constexpr uint64_t kept_file_size = 2048;
+  NewDummyFile("kept.data", kept_file_size);
+  const std::string file = NewDummyFile("synced.data", 1024);
+  const std::string trash_file = file + DeleteScheduler::kTrashExtension;
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
+      "DeleteScheduler::DeleteTrashFile:BeforeSyncDir", [](void* arg) {
+        *static_cast<Status*>(arg) = Status::IOError("injected sync error");
+      });
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
+
+  ASSERT_OK(delete_scheduler_->DeleteFile(file, dummy_files_dirs_[0],
+                                          /*force_bg=*/true));
+  delete_scheduler_->WaitForEmptyTrashInDirectory(dummy_files_dirs_[0]);
+
+  ASSERT_TRUE(env_->FileExists(trash_file).IsNotFound());
+  ASSERT_EQ(1, delete_scheduler_->GetBackgroundErrors().size());
+  ASSERT_EQ(0, delete_scheduler_->GetTotalTrashSize());
+  ASSERT_EQ(kept_file_size, sst_file_mgr_->GetTotalSize());
+}
+
 TEST_F(DeleteSchedulerTest, ConcurrentDirectoryWaitStress) {
   rate_bytes_per_sec_ = 64 * 1024 * 1024;
   NewDeleteScheduler();

unreleased_history/bug_fixes/delete_scheduler_unlinked_trash_accounting.md

@@ -0,0 +1 @@
+Fixed two more `SstFileManager` trash accounting leaks: retrying a failed trash deletion immediately (for example with rate limiting disabled) after the file had disappeared, and a trash file that was deleted but whose directory sync then failed. In both cases the file's size stayed in the total trash size and tracked size until restart.