diff --git a/be/src/io/cache/fs_file_cache_storage.cpp b/be/src/io/cache/fs_file_cache_storage.cpp index 029d37425e588a..5ed82dcaf431ad 100644 --- a/be/src/io/cache/fs_file_cache_storage.cpp +++ b/be/src/io/cache/fs_file_cache_storage.cpp @@ -279,25 +279,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)); - if (files.empty()) { - RETURN_IF_ERROR(fs->delete_directory(dir)); + RETURN_IF_ERROR(fs->list(v2_dir, true, &files, &exists)); + if (exists && 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 (exists && files.empty()) { + 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..4f9b6baf2a75cf 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -167,6 +167,26 @@ 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) { + FILESYSTEM_M(delete_empty_directory_impl(dir)); +} + +Status LocalFileSystem::delete_empty_directory_impl(const Path& dir) { + Path path; + RETURN_IF_ERROR(absolute_path(dir, path)); + VLOG_DEBUG << "delete empty directory: " << path.native(); + int ret = 0; + RETRY_ON_EINTR(ret, rmdir(path.c_str())); + if (ret != 0) { + std::error_code ec(errno, std::generic_category()); + if (ec == std::errc::no_such_file_or_directory) { + return Status::OK(); + } + return localfs_error(ec, 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..b1546bc473426f 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); @@ -87,6 +89,7 @@ class LocalFileSystem final : public FileSystem { Status delete_file_impl(const Path& file) override; Status delete_directory_impl(const Path& dir) override; Status delete_directory_or_file_impl(const Path& path); + Status delete_empty_directory_impl(const Path& dir); Status batch_delete_impl(const std::vector& files) override; Status exists_impl(const Path& path, bool* res) const override; Status file_size_impl(const Path& file, int64_t* file_size) const override; 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..e4e066ad458605 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,63 @@ 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, remove_v3_block_when_v2_directory_missing) { + 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_v3_without_v2_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); + create_regular_file(block_path, 'r'); + ASSERT_FALSE(fs::exists(old_key_dir)); + + ASSERT_TRUE(storage.remove(key).ok()); + + EXPECT_FALSE(fs::exists(block_path)); + EXPECT_FALSE(fs::exists(key_dir)); + 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..5947dffef9cf16 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -212,6 +212,30 @@ 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)); + st = io::global_local_filesystem()->delete_empty_directory(empty_dir); + ASSERT_TRUE(st.ok()) << st; + + 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); {