From 2a1cb8c07377ca61839216eb47c62f3e3e975d0e Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Mon, 18 May 2026 15:01:35 +0800 Subject: [PATCH] [fix](filecache) clean empty v3 cache dirs --- be/src/io/cache/fs_file_cache_storage.cpp | 30 ++++++++++++++----- be/src/io/fs/local_file_system.cpp | 11 +++++++ be/src/io/fs/local_file_system.h | 2 ++ ...s_file_cache_storage_leak_cleaner_test.cpp | 30 +++++++++++++++++++ be/test/io/fs/local_file_system_test.cpp | 22 ++++++++++++++ 5 files changed, 87 insertions(+), 8 deletions(-) diff --git a/be/src/io/cache/fs_file_cache_storage.cpp b/be/src/io/cache/fs_file_cache_storage.cpp index 43b9c4ae4cb360..069c991039ea32 100644 --- a/be/src/io/cache/fs_file_cache_storage.cpp +++ b/be/src/io/cache/fs_file_cache_storage.cpp @@ -281,25 +281,39 @@ Status FSFileCacheStorage::read(const FileCacheKey& key, size_t value_offset, Sl } Status FSFileCacheStorage::remove(const FileCacheKey& key) { - std::string dir = get_path_in_local_cache_v3(key.hash); - std::string file = get_path_in_local_cache_v3(dir, key.offset); + const std::string v3_dir = get_path_in_local_cache_v3(key.hash); + const std::string v3_file = get_path_in_local_cache_v3(v3_dir, key.offset); FDCache::instance()->remove_file_reader(std::make_pair(key.hash, key.offset)); - RETURN_IF_ERROR(fs->delete_file(file)); + RETURN_IF_ERROR(fs->delete_file(v3_file)); // return OK not means the file is deleted, it may be not exist + std::string v2_dir; { // try to detect the file with old v2 format - dir = get_path_in_local_cache_v2(key.hash, key.meta.expiration_time); - file = get_path_in_local_cache_v2(dir, key.offset, key.meta.type); - RETURN_IF_ERROR(fs->delete_file(file)); + v2_dir = get_path_in_local_cache_v2(key.hash, key.meta.expiration_time); + const std::string v2_file = get_path_in_local_cache_v2(v2_dir, key.offset, key.meta.type); + RETURN_IF_ERROR(fs->delete_file(v2_file)); } BlockMetaKey mkey(key.meta.tablet_id, UInt128Wrapper(key.hash), key.offset); _meta_store->delete_key(mkey); std::vector files; bool exists {false}; - RETURN_IF_ERROR(fs->list(dir, true, &files, &exists)); + RETURN_IF_ERROR(fs->list(v2_dir, true, &files, &exists)); + if (files.empty()) { + auto st = fs->delete_empty_directory(v2_dir); + if (!st.ok()) { + LOG_WARNING("failed to remove cache directory {}", v2_dir).error(st); + } + } + + files.clear(); + exists = false; + RETURN_IF_ERROR(fs->list(v3_dir, true, &files, &exists)); if (files.empty()) { - RETURN_IF_ERROR(fs->delete_directory(dir)); + auto st = fs->delete_empty_directory(v3_dir); + if (!st.ok()) { + LOG_WARNING("failed to remove cache directory {}", v3_dir).error(st); + } } return Status::OK(); } diff --git a/be/src/io/fs/local_file_system.cpp b/be/src/io/fs/local_file_system.cpp index 7edabd836c12e9..f67b38304e3bb2 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -167,6 +167,17 @@ Status LocalFileSystem::delete_directory_or_file(const Path& path) { FILESYSTEM_M(delete_directory_or_file_impl(path)); } +Status LocalFileSystem::delete_empty_directory(const Path& dir) { + Path path; + RETURN_IF_ERROR(absolute_path(dir, path)); + VLOG_DEBUG << "delete empty directory: " << path.native(); + if (rmdir(path.c_str()) != 0) { + return localfs_error(errno, + fmt::format("failed to delete empty directory {}", path.native())); + } + return Status::OK(); +} + Status LocalFileSystem::delete_directory_or_file_impl(const Path& path) { bool is_dir; RETURN_IF_ERROR(is_directory(path, &is_dir)); diff --git a/be/src/io/fs/local_file_system.h b/be/src/io/fs/local_file_system.h index c1ffbd22949252..e1d72b11cdee24 100644 --- a/be/src/io/fs/local_file_system.h +++ b/be/src/io/fs/local_file_system.h @@ -63,6 +63,8 @@ class LocalFileSystem final : public FileSystem { static bool equal_or_sub_path(const Path& parent, const Path& child); // delete dir or file Status delete_directory_or_file(const Path& path); + // delete directory only if it is empty + Status delete_empty_directory(const Path& dir); // change the file permission of the given path Status permission(const Path& file, std::filesystem::perms prms); diff --git a/be/test/io/cache/fs_file_cache_storage_leak_cleaner_test.cpp b/be/test/io/cache/fs_file_cache_storage_leak_cleaner_test.cpp index 49ef8587ced2e9..1f9c06d96190a5 100644 --- a/be/test/io/cache/fs_file_cache_storage_leak_cleaner_test.cpp +++ b/be/test/io/cache/fs_file_cache_storage_leak_cleaner_test.cpp @@ -288,6 +288,36 @@ TEST_F(FSFileCacheLeakCleanerTest, remove_orphan_and_tmp_files) { fs::remove_all(dir, ec); } +TEST_F(FSFileCacheLeakCleanerTest, remove_cleans_empty_v2_and_v3_key_directories) { + auto dir = prepare_test_dir(); + FileCacheSettings settings = default_settings(); + BlockFileCache mgr(dir.string(), settings); + FSFileCacheStorage storage; + setup_storage(storage, mgr, dir); + + FileCacheKey key; + key.hash = BlockFileCache::hash("remove_empty_v3_dir"); + key.offset = 0; + key.meta.expiration_time = 123456789; + key.meta.type = FileCacheType::NORMAL; + key.meta.tablet_id = 0; + + auto key_dir = storage.get_path_in_local_cache_v3(key.hash); + auto block_path = FSFileCacheStorage::get_path_in_local_cache_v3(key_dir, key.offset); + auto old_key_dir = storage.get_path_in_local_cache_v2(key.hash, key.meta.expiration_time); + auto old_block_path = + FSFileCacheStorage::get_path_in_local_cache_v2(old_key_dir, key.offset, key.meta.type); + create_regular_file(block_path, 'r'); + create_regular_file(old_block_path, 'o'); + + ASSERT_TRUE(storage.remove(key).ok()); + + EXPECT_FALSE(fs::exists(block_path)); + EXPECT_FALSE(fs::exists(key_dir)); + EXPECT_FALSE(fs::exists(old_block_path)); + EXPECT_FALSE(fs::exists(old_key_dir)); +} + TEST_F(FSFileCacheLeakCleanerTest, snapshot_metadata_for_hash_offsets_handles_missing_hash) { auto dir = prepare_test_dir(); FileCacheSettings settings = default_settings(); diff --git a/be/test/io/fs/local_file_system_test.cpp b/be/test/io/fs/local_file_system_test.cpp index 450c3daea8f2f6..5f66bec050b251 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -212,6 +212,28 @@ TEST_F(LocalFileSystemTest, Delete) { ASSERT_TRUE(check_exist(fmt::format("{}/dir/dir1", test_dir))); // Parent should exist } +TEST_F(LocalFileSystemTest, DeleteEmptyDirectory) { + auto empty_dir = fmt::format("{}/empty_dir", test_dir); + auto st = io::global_local_filesystem()->create_directory(empty_dir); + ASSERT_TRUE(st.ok()) << st; + + st = io::global_local_filesystem()->delete_empty_directory(empty_dir); + ASSERT_TRUE(st.ok()) << st; + ASSERT_FALSE(check_exist(empty_dir)); + + auto non_empty_dir = fmt::format("{}/non_empty_dir", test_dir); + auto child_file = fmt::format("{}/child", non_empty_dir); + st = io::global_local_filesystem()->create_directory(non_empty_dir); + ASSERT_TRUE(st.ok()) << st; + st = save_string_file(child_file, "abc"); + ASSERT_TRUE(st.ok()) << st; + + st = io::global_local_filesystem()->delete_empty_directory(non_empty_dir); + ASSERT_FALSE(st.ok()) << st; + ASSERT_TRUE(check_exist(non_empty_dir)); + ASSERT_TRUE(check_exist(child_file)); +} + TEST_F(LocalFileSystemTest, AbnormalFileWriter) { auto fname = fmt::format("{}/abc", test_dir); {