From f9b62a05671e050f409af2b3bbbe18bd5204f410 Mon Sep 17 00:00:00 2001 From: Andrew Chang Date: Thu, 8 Oct 2026 15:52:29 -0700 Subject: [PATCH] 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 | 46 ++++++++++----- file/delete_scheduler.h | 3 +- file/delete_scheduler_test.cc | 57 +++++++++++++++++++ ...ete_scheduler_unlinked_trash_accounting.md | 1 + 4 files changed, 91 insertions(+), 16 deletions(-) create mode 100644 unreleased_history/bug_fixes/delete_scheduler_unlinked_trash_accounting.md diff --git a/file/delete_scheduler.cc b/file/delete_scheduler.cc index ef234def01..087f1986a1 100644 --- a/file/delete_scheduler.cc +++ b/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(&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 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(const_cast(&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; diff --git a/file/delete_scheduler.h b/file/delete_scheduler.h index 2c51d50980..4e59db877f 100644 --- a/file/delete_scheduler.h +++ b/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); diff --git a/file/delete_scheduler_test.cc b/file/delete_scheduler_test.cc index 9e3d9f8acd..cbeb89a746 100644 --- a/file/delete_scheduler_test.cc +++ b/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 fail_next_delete{true}; + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( + "DeleteScheduler::DeleteTrashFile:BeforeDeleteFile", [&](void* arg) { + if (fail_next_delete.exchange(false)) { + *static_cast(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(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(); diff --git a/unreleased_history/bug_fixes/delete_scheduler_unlinked_trash_accounting.md b/unreleased_history/bug_fixes/delete_scheduler_unlinked_trash_accounting.md new file mode 100644 index 0000000000..cafcd6ae79 --- /dev/null +++ b/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.