From 259d50c57b6227b2a078effcef8de19cd23c346f Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 8 Apr 2024 14:36:07 +0200 Subject: [PATCH] Add more comments --- src/Interpreters/Cache/EvictionCandidates.cpp | 22 ++++-- src/Interpreters/Cache/EvictionCandidates.h | 8 +- src/Interpreters/Cache/FileCache.cpp | 77 ++++++++++++------- src/Interpreters/Cache/FileCacheFactory.cpp | 8 +- .../Cache/LRUFileCachePriority.cpp | 2 +- src/Interpreters/Cache/Metadata.cpp | 72 +++++++++-------- 6 files changed, 112 insertions(+), 77 deletions(-) diff --git a/src/Interpreters/Cache/EvictionCandidates.cpp b/src/Interpreters/Cache/EvictionCandidates.cpp index f9f9bdfe662..d20ae77d720 100644 --- a/src/Interpreters/Cache/EvictionCandidates.cpp +++ b/src/Interpreters/Cache/EvictionCandidates.cpp @@ -17,6 +17,11 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; } +EvictionCandidates::EvictionCandidates() + : log(getLogger("EvictionCandidates")) +{ +} + EvictionCandidates::~EvictionCandidates() { /// Here `queue_entries_to_invalidate` contains queue entries @@ -64,8 +69,11 @@ void EvictionCandidates::add( void EvictionCandidates::removeQueueEntries(const CachePriorityGuard::Lock & lock) { - auto log = getLogger("EvictionCandidates"); + /// Remove queue entries of eviction candidates. + /// This will release space we consider to be hold for them. + LOG_TEST(log, "Will remove {} eviction candidates", size()); + for (const auto & [key, key_candidates] : candidates) { for (const auto & candidate : key_candidates.candidates) @@ -87,6 +95,7 @@ void EvictionCandidates::evict() auto timer = DB::CurrentThread::getProfileEvents().timer(ProfileEvents::FilesystemCacheEvictMicroseconds); + /// If queue entries are already removed, then nothing to invalidate. if (!removed_queue_entries) queue_entries_to_invalidate.reserve(candidates_size); @@ -184,6 +193,12 @@ void EvictionCandidates::finalize( on_finalize.clear(); } +bool EvictionCandidates::needFinalize() const +{ + /// Do we need to call finalize()? + return !on_finalize.empty() || !queue_entries_to_invalidate.empty(); +} + void EvictionCandidates::setSpaceHolder( size_t size, size_t elements, @@ -196,9 +211,4 @@ void EvictionCandidates::setSpaceHolder( hold_space = std::make_unique(size, elements, priority, lock); } -void EvictionCandidates::insert(EvictionCandidates && other, const CachePriorityGuard::Lock &) -{ - candidates.insert(make_move_iterator(other.candidates.begin()), make_move_iterator(other.candidates.end())); -} - } diff --git a/src/Interpreters/Cache/EvictionCandidates.h b/src/Interpreters/Cache/EvictionCandidates.h index baacbc0cfae..0dcc6bc0dda 100644 --- a/src/Interpreters/Cache/EvictionCandidates.h +++ b/src/Interpreters/Cache/EvictionCandidates.h @@ -9,7 +9,7 @@ class EvictionCandidates : private boost::noncopyable public: using FinalizeEvictionFunc = std::function; - EvictionCandidates() = default; + EvictionCandidates(); ~EvictionCandidates(); void add( @@ -17,8 +17,6 @@ public: LockedKey & locked_key, const CachePriorityGuard::Lock &); - void insert(EvictionCandidates && other, const CachePriorityGuard::Lock &); - void evict(); void removeQueueEntries(const CachePriorityGuard::Lock &); @@ -29,6 +27,8 @@ public: FileCacheQueryLimit::QueryContext * query_context, const CachePriorityGuard::Lock &); + bool needFinalize() const; + size_t size() const { return candidates_size; } auto begin() const { return candidates.begin(); } @@ -57,6 +57,8 @@ private: bool removed_queue_entries = false; IFileCachePriority::HoldSpacePtr hold_space; + + LoggerPtr log; }; using EvictionCandidatesPtr = std::unique_ptr; diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 12ea2c178bc..29f2ebeca55 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -1389,7 +1389,18 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings, || new_settings.max_elements != actual_settings.max_elements) { EvictionCandidates eviction_candidates; - bool limits_satisfied = false; + bool modified_size_limit = false; + + /// In order to not block cache for the duration of cache resize, + /// we do: + /// a. Take a cache lock. + /// 1. Collect eviction candidates, + /// 2. Remove queue entries of eviction candidates. + /// This will release space we consider to be hold for them, + /// so that we can safely modify size limits. + /// 3. Modify size limits of cache. + /// b. Release a cache lock. + /// 1. Do actual eviction from filesystem. { cache_is_being_resized.store(true, std::memory_order_relaxed); SCOPE_EXIT({ @@ -1399,38 +1410,45 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings, auto cache_lock = lockCache(); FileCacheReserveStat stat; - limits_satisfied = main_priority->collectCandidatesForEviction( - new_settings.max_size, new_settings.max_elements, 0/* max_candidates_to_evict */, stat, eviction_candidates, cache_lock); - - eviction_candidates.removeQueueEntries(cache_lock); - - if (limits_satisfied) + if (main_priority->collectCandidatesForEviction( + new_settings.max_size, new_settings.max_elements, 0/* max_candidates_to_evict */, + stat, eviction_candidates, cache_lock)) { + /// Remove only queue entries of eviction candidates. + eviction_candidates.removeQueueEntries(cache_lock); + /// Note that (in-memory) metadata about corresponding file segments + /// (e.g. file segment info in CacheMetadata) will be removed + /// only after eviction from filesystem. This is needed to avoid + /// a race on removal of file from filesystsem and + /// addition of the same file as part of a newly cached file segment. + + /// Modify cache size limits. + /// From this point cache eviction will follow them. main_priority->modifySizeLimits( - new_settings.max_size, new_settings.max_elements, new_settings.slru_size_ratio, cache_lock); + new_settings.max_size, new_settings.max_elements, + new_settings.slru_size_ratio, cache_lock); + + modified_size_limit = true; } - else + } + + if (modified_size_limit) + { + try { - LOG_WARNING(log, "Unable to modify size limit from {} to {}, " - "elements limit from {} to {}", - actual_settings.max_size, new_settings.max_size, - actual_settings.max_elements, new_settings.max_elements); + /// Do actual eviction from filesystem. + eviction_candidates.evict(); + } + catch (...) + { + if (eviction_candidates.needFinalize()) + eviction_candidates.finalize(nullptr, lockCache()); + throw; } - } - try - { - eviction_candidates.evict(); - } - catch (...) - { - auto cache_lock = lockCache(); - eviction_candidates.finalize(nullptr, cache_lock); - throw; - } + if (eviction_candidates.needFinalize()) + eviction_candidates.finalize(nullptr, lockCache()); - if (limits_satisfied) - { LOG_INFO(log, "Changed max_size from {} to {}, max_elements from {} to {}", actual_settings.max_size, new_settings.max_size, actual_settings.max_elements, new_settings.max_elements); @@ -1438,6 +1456,13 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings, actual_settings.max_size = new_settings.max_size; actual_settings.max_elements = new_settings.max_elements; } + else + { + LOG_WARNING(log, "Unable to modify size limit from {} to {}, " + "elements limit from {} to {}", + actual_settings.max_size, new_settings.max_size, + actual_settings.max_elements, new_settings.max_elements); + } } if (new_settings.max_file_segment_size != actual_settings.max_file_segment_size) diff --git a/src/Interpreters/Cache/FileCacheFactory.cpp b/src/Interpreters/Cache/FileCacheFactory.cpp index 747b31bff64..a7a5834f03d 100644 --- a/src/Interpreters/Cache/FileCacheFactory.cpp +++ b/src/Interpreters/Cache/FileCacheFactory.cpp @@ -142,10 +142,8 @@ void FileCacheFactory::updateSettingsFromConfig(const Poco::Util::AbstractConfig caches_by_name_copy = caches_by_name; } - auto * log = &Poco::Logger::get("FileCacheFactory"); - std::unordered_set checked_paths; - for (const auto & [cache_name, cache_info] : caches_by_name_copy) + for (const auto & [_, cache_info] : caches_by_name_copy) { if (cache_info->config_path.empty() || checked_paths.contains(cache_info->config_path)) continue; @@ -158,10 +156,12 @@ void FileCacheFactory::updateSettingsFromConfig(const Poco::Util::AbstractConfig FileCacheSettings old_settings = cache_info->getSettings(); if (old_settings == new_settings) { - LOG_TRACE(log, "No settings changes for cache: {}", cache_name); continue; } + /// FIXME: registerDiskCache modifies `path` setting of FileCacheSettings if path is relative. + /// This can lead to calling applySettingsIfPossible even though nothing changed, which is avoidable. + // LOG_TRACE(log, "Will apply settings changes for cache {}. " // "Settings changes: {} (new settings: {}, old_settings: {})", // cache_name, fmt::join(new_settings.getSettingsDiff(old_settings), ", "), diff --git a/src/Interpreters/Cache/LRUFileCachePriority.cpp b/src/Interpreters/Cache/LRUFileCachePriority.cpp index e859529f5e7..1a2040f9ed2 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.cpp +++ b/src/Interpreters/Cache/LRUFileCachePriority.cpp @@ -280,7 +280,7 @@ bool LRUFileCachePriority::collectCandidatesForEviction( auto can_fit = [&] { - return canFit(size, 1, stat.total_stat.releasable_size, stat.total_stat.releasable_count, lock); + return canFit(size, elements, stat.total_stat.releasable_size, stat.total_stat.releasable_count, lock); }; iterateForEviction(res, stat, can_fit, lock); diff --git a/src/Interpreters/Cache/Metadata.cpp b/src/Interpreters/Cache/Metadata.cpp index 631c1aa2ae6..2cbd56ba0bc 100644 --- a/src/Interpreters/Cache/Metadata.cpp +++ b/src/Interpreters/Cache/Metadata.cpp @@ -941,49 +941,47 @@ KeyMetadata::iterator LockedKey::removeFileSegmentImpl( file_segment->detach(segment_lock, *this); + try { - try + const auto path = key_metadata->getFileSegmentPath(*file_segment); + if (file_segment->segment_kind == FileSegmentKind::Temporary) { - const auto path = key_metadata->getFileSegmentPath(*file_segment); - if (file_segment->segment_kind == FileSegmentKind::Temporary) - { - /// FIXME: For temporary file segment the requirement is not as strong because - /// the implementation of "temporary data in cache" creates files in advance. - if (fs::exists(path)) - fs::remove(path); - } - else if (file_segment->downloaded_size == 0) - { - chassert(!fs::exists(path)); - } - else if (fs::exists(path)) - { + /// FIXME: For temporary file segment the requirement is not as strong because + /// the implementation of "temporary data in cache" creates files in advance. + if (fs::exists(path)) fs::remove(path); - - /// Clear OpenedFileCache to avoid reading from incorrect file descriptor. - int flags = file_segment->getFlagsForLocalRead(); - /// Files are created with flags from file_segment->getFlagsForLocalRead() - /// plus optionally O_DIRECT is added, depends on query setting, so remove both. - OpenedFileCache::instance().remove(path, flags); - OpenedFileCache::instance().remove(path, flags | O_DIRECT); - - LOG_TEST(key_metadata->logger(), "Removed file segment at path: {}", path); - } - else if (!can_be_broken) - { -#ifdef ABORT_ON_LOGICAL_ERROR - throw Exception(ErrorCodes::LOGICAL_ERROR, "Expected path {} to exist", path); -#else - LOG_WARNING(key_metadata->logger(), "Expected path {} to exist, while removing {}:{}", - path, getKey(), file_segment->offset()); -#endif - } } - catch (...) + else if (file_segment->downloaded_size == 0) { - tryLogCurrentException(__PRETTY_FUNCTION__); - chassert(false); + chassert(!fs::exists(path)); } + else if (fs::exists(path)) + { + fs::remove(path); + + /// Clear OpenedFileCache to avoid reading from incorrect file descriptor. + int flags = file_segment->getFlagsForLocalRead(); + /// Files are created with flags from file_segment->getFlagsForLocalRead() + /// plus optionally O_DIRECT is added, depends on query setting, so remove both. + OpenedFileCache::instance().remove(path, flags); + OpenedFileCache::instance().remove(path, flags | O_DIRECT); + + LOG_TEST(key_metadata->logger(), "Removed file segment at path: {}", path); + } + else if (!can_be_broken) + { +#ifdef ABORT_ON_LOGICAL_ERROR + throw Exception(ErrorCodes::LOGICAL_ERROR, "Expected path {} to exist", path); +#else + LOG_WARNING(key_metadata->logger(), "Expected path {} to exist, while removing {}:{}", + path, getKey(), file_segment->offset()); +#endif + } + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + chassert(false); } return key_metadata->erase(it);