Skip to content

Commit da6ff4d

Browse files
github-actions[bot]liaoxin01
github-actions[bot]
and
liaoxin01
authored
branch-4.1: [fix](filecache) clean empty v3 cache dirs #63344 (#63616)
Cherry-picked from #63344 Co-authored-by: Xin Liao <liaoxin@selectdb.com>
1 parent a8bbe79 commit da6ff4d

5 files changed

Lines changed: 127 additions & 9 deletions

File tree

‎be/src/io/cache/fs_file_cache_storage.cpp‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -279,25 +279,39 @@ Status FSFileCacheStorage::read(const FileCacheKey& key, size_t value_offset, Sl
279279
}
280280

281281
Status FSFileCacheStorage::remove(const FileCacheKey& key) {
282-
std::string dir = get_path_in_local_cache_v3(key.hash);
283-
std::string file = get_path_in_local_cache_v3(dir, key.offset);
282+
const std::string v3_dir = get_path_in_local_cache_v3(key.hash);
283+
const std::string v3_file = get_path_in_local_cache_v3(v3_dir, key.offset);
284284
FDCache::instance()->remove_file_reader(std::make_pair(key.hash, key.offset));
285-
RETURN_IF_ERROR(fs->delete_file(file));
285+
RETURN_IF_ERROR(fs->delete_file(v3_file));
286286
// return OK not means the file is deleted, it may be not exist
287287

288+
std::string v2_dir;
288289
{ // try to detect the file with old v2 format
289-
dir = get_path_in_local_cache_v2(key.hash, key.meta.expiration_time);
290-
file = get_path_in_local_cache_v2(dir, key.offset, key.meta.type);
291-
RETURN_IF_ERROR(fs->delete_file(file));
290+
v2_dir = get_path_in_local_cache_v2(key.hash, key.meta.expiration_time);
291+
const std::string v2_file = get_path_in_local_cache_v2(v2_dir, key.offset, key.meta.type);
292+
RETURN_IF_ERROR(fs->delete_file(v2_file));
292293
}
293294

294295
BlockMetaKey mkey(key.meta.tablet_id, UInt128Wrapper(key.hash), key.offset);
295296
_meta_store->delete_key(mkey);
296297
std::vector<FileInfo> files;
297298
bool exists {false};
298-
RETURN_IF_ERROR(fs->list(dir, true, &files, &exists));
299-
if (files.empty()) {
300-
RETURN_IF_ERROR(fs->delete_directory(dir));
299+
RETURN_IF_ERROR(fs->list(v2_dir, true, &files, &exists));
300+
if (exists && files.empty()) {
301+
auto st = fs->delete_empty_directory(v2_dir);
302+
if (!st.ok()) {
303+
LOG_WARNING("failed to remove cache directory {}", v2_dir).error(st);
304+
}
305+
}
306+
307+
files.clear();
308+
exists = false;
309+
RETURN_IF_ERROR(fs->list(v3_dir, true, &files, &exists));
310+
if (exists && files.empty()) {
311+
auto st = fs->delete_empty_directory(v3_dir);
312+
if (!st.ok()) {
313+
LOG_WARNING("failed to remove cache directory {}", v3_dir).error(st);
314+
}
301315
}
302316
return Status::OK();
303317
}

‎be/src/io/fs/local_file_system.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,26 @@ Status LocalFileSystem::delete_directory_or_file(const Path& path) {
167167
FILESYSTEM_M(delete_directory_or_file_impl(path));
168168
}
169169

170+
Status LocalFileSystem::delete_empty_directory(const Path& dir) {
171+
FILESYSTEM_M(delete_empty_directory_impl(dir));
172+
}
173+
174+
Status LocalFileSystem::delete_empty_directory_impl(const Path& dir) {
175+
Path path;
176+
RETURN_IF_ERROR(absolute_path(dir, path));
177+
VLOG_DEBUG << "delete empty directory: " << path.native();
178+
int ret = 0;
179+
RETRY_ON_EINTR(ret, rmdir(path.c_str()));
180+
if (ret != 0) {
181+
std::error_code ec(errno, std::generic_category());
182+
if (ec == std::errc::no_such_file_or_directory) {
183+
return Status::OK();
184+
}
185+
return localfs_error(ec, fmt::format("failed to delete empty directory {}", path.native()));
186+
}
187+
return Status::OK();
188+
}
189+
170190
Status LocalFileSystem::delete_directory_or_file_impl(const Path& path) {
171191
bool is_dir;
172192
RETURN_IF_ERROR(is_directory(path, &is_dir));

‎be/src/io/fs/local_file_system.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ class LocalFileSystem final : public FileSystem {
6363
static bool equal_or_sub_path(const Path& parent, const Path& child);
6464
// delete dir or file
6565
Status delete_directory_or_file(const Path& path);
66+
// delete directory only if it is empty
67+
Status delete_empty_directory(const Path& dir);
6668
// change the file permission of the given path
6769
Status permission(const Path& file, std::filesystem::perms prms);
6870

@@ -87,6 +89,7 @@ class LocalFileSystem final : public FileSystem {
8789
Status delete_file_impl(const Path& file) override;
8890
Status delete_directory_impl(const Path& dir) override;
8991
Status delete_directory_or_file_impl(const Path& path);
92+
Status delete_empty_directory_impl(const Path& dir);
9093
Status batch_delete_impl(const std::vector<Path>& files) override;
9194
Status exists_impl(const Path& path, bool* res) const override;
9295
Status file_size_impl(const Path& file, int64_t* file_size) const override;

‎be/test/io/cache/fs_file_cache_storage_leak_cleaner_test.cpp‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,63 @@ TEST_F(FSFileCacheLeakCleanerTest, remove_orphan_and_tmp_files) {
288288
fs::remove_all(dir, ec);
289289
}
290290

291+
TEST_F(FSFileCacheLeakCleanerTest, remove_cleans_empty_v2_and_v3_key_directories) {
292+
auto dir = prepare_test_dir();
293+
FileCacheSettings settings = default_settings();
294+
BlockFileCache mgr(dir.string(), settings);
295+
FSFileCacheStorage storage;
296+
setup_storage(storage, mgr, dir);
297+
298+
FileCacheKey key;
299+
key.hash = BlockFileCache::hash("remove_empty_v3_dir");
300+
key.offset = 0;
301+
key.meta.expiration_time = 123456789;
302+
key.meta.type = FileCacheType::NORMAL;
303+
key.meta.tablet_id = 0;
304+
305+
auto key_dir = storage.get_path_in_local_cache_v3(key.hash);
306+
auto block_path = FSFileCacheStorage::get_path_in_local_cache_v3(key_dir, key.offset);
307+
auto old_key_dir = storage.get_path_in_local_cache_v2(key.hash, key.meta.expiration_time);
308+
auto old_block_path =
309+
FSFileCacheStorage::get_path_in_local_cache_v2(old_key_dir, key.offset, key.meta.type);
310+
create_regular_file(block_path, 'r');
311+
create_regular_file(old_block_path, 'o');
312+
313+
ASSERT_TRUE(storage.remove(key).ok());
314+
315+
EXPECT_FALSE(fs::exists(block_path));
316+
EXPECT_FALSE(fs::exists(key_dir));
317+
EXPECT_FALSE(fs::exists(old_block_path));
318+
EXPECT_FALSE(fs::exists(old_key_dir));
319+
}
320+
321+
TEST_F(FSFileCacheLeakCleanerTest, remove_v3_block_when_v2_directory_missing) {
322+
auto dir = prepare_test_dir();
323+
FileCacheSettings settings = default_settings();
324+
BlockFileCache mgr(dir.string(), settings);
325+
FSFileCacheStorage storage;
326+
setup_storage(storage, mgr, dir);
327+
328+
FileCacheKey key;
329+
key.hash = BlockFileCache::hash("remove_v3_without_v2_dir");
330+
key.offset = 0;
331+
key.meta.expiration_time = 123456789;
332+
key.meta.type = FileCacheType::NORMAL;
333+
key.meta.tablet_id = 0;
334+
335+
auto key_dir = storage.get_path_in_local_cache_v3(key.hash);
336+
auto block_path = FSFileCacheStorage::get_path_in_local_cache_v3(key_dir, key.offset);
337+
auto old_key_dir = storage.get_path_in_local_cache_v2(key.hash, key.meta.expiration_time);
338+
create_regular_file(block_path, 'r');
339+
ASSERT_FALSE(fs::exists(old_key_dir));
340+
341+
ASSERT_TRUE(storage.remove(key).ok());
342+
343+
EXPECT_FALSE(fs::exists(block_path));
344+
EXPECT_FALSE(fs::exists(key_dir));
345+
EXPECT_FALSE(fs::exists(old_key_dir));
346+
}
347+
291348
TEST_F(FSFileCacheLeakCleanerTest, snapshot_metadata_for_hash_offsets_handles_missing_hash) {
292349
auto dir = prepare_test_dir();
293350
FileCacheSettings settings = default_settings();

‎be/test/io/fs/local_file_system_test.cpp‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,30 @@ TEST_F(LocalFileSystemTest, Delete) {
212212
ASSERT_TRUE(check_exist(fmt::format("{}/dir/dir1", test_dir))); // Parent should exist
213213
}
214214

215+
TEST_F(LocalFileSystemTest, DeleteEmptyDirectory) {
216+
auto empty_dir = fmt::format("{}/empty_dir", test_dir);
217+
auto st = io::global_local_filesystem()->create_directory(empty_dir);
218+
ASSERT_TRUE(st.ok()) << st;
219+
220+
st = io::global_local_filesystem()->delete_empty_directory(empty_dir);
221+
ASSERT_TRUE(st.ok()) << st;
222+
ASSERT_FALSE(check_exist(empty_dir));
223+
st = io::global_local_filesystem()->delete_empty_directory(empty_dir);
224+
ASSERT_TRUE(st.ok()) << st;
225+
226+
auto non_empty_dir = fmt::format("{}/non_empty_dir", test_dir);
227+
auto child_file = fmt::format("{}/child", non_empty_dir);
228+
st = io::global_local_filesystem()->create_directory(non_empty_dir);
229+
ASSERT_TRUE(st.ok()) << st;
230+
st = save_string_file(child_file, "abc");
231+
ASSERT_TRUE(st.ok()) << st;
232+
233+
st = io::global_local_filesystem()->delete_empty_directory(non_empty_dir);
234+
ASSERT_FALSE(st.ok()) << st;
235+
ASSERT_TRUE(check_exist(non_empty_dir));
236+
ASSERT_TRUE(check_exist(child_file));
237+
}
238+
215239
TEST_F(LocalFileSystemTest, AbnormalFileWriter) {
216240
auto fname = fmt::format("{}/abc", test_dir);
217241
{

0 commit comments

Comments
 (0)