Fix potential busy loop in keepFreeSpaceRatioFunc

src/Interpreters/Cache/FileCache.cpp

@@ -998,18 +998,19 @@ void FileCache::freeSpaceRatioKeepingThreadFunc()
     FileCacheReserveStat stat;
     EvictionCandidates eviction_candidates;
 
-    bool limits_satisfied = true;
+    IFileCachePriority::DesiredSizeStatus desired_size_status;
     try
     {
         /// Collect at most `keep_up_free_space_remove_batch` elements to evict,
         /// (we use batches to make sure we do not block cache for too long,
         /// by default the batch size is quite small).
-        limits_satisfied = main_priority->collectCandidatesForEviction(
+        desired_size_status = main_priority->collectCandidatesForEviction(
             desired_size, desired_elements_num, keep_up_free_space_remove_batch, stat, eviction_candidates, lock);
 
 #ifdef DEBUG_OR_SANITIZER_BUILD
         /// Let's make sure that we correctly processed the limits.
-        if (limits_satisfied && eviction_candidates.size() < keep_up_free_space_remove_batch)
+        if (desired_size_status == IFileCachePriority::DesiredSizeStatus::SUCCESS
+            && eviction_candidates.size() < keep_up_free_space_remove_batch)
         {
             const auto current_size = main_priority->getSize(lock);
             chassert(current_size >= stat.total_stat.releasable_size);
@@ -1063,13 +1064,24 @@ void FileCache::freeSpaceRatioKeepingThreadFunc()
     watch.stop();
     ProfileEvents::increment(ProfileEvents::FilesystemCacheFreeSpaceKeepingThreadWorkMilliseconds, watch.elapsedMilliseconds());
 
-    LOG_TRACE(log, "Free space ratio keeping thread finished in {} ms", watch.elapsedMilliseconds());
+    LOG_TRACE(log, "Free space ratio keeping thread finished in {} ms (status: {})",
+              watch.elapsedMilliseconds(), desired_size_status);
 
     [[maybe_unused]] bool scheduled = false;
-    if (limits_satisfied)
+    switch (desired_size_status)
-        scheduled = keep_up_free_space_ratio_task->scheduleAfter(general_reschedule_ms);
+    {
-    else
+        case IFileCachePriority::DesiredSizeStatus::SUCCESS: [[fallthrough]];
-        scheduled = keep_up_free_space_ratio_task->schedule();
+        case IFileCachePriority::DesiredSizeStatus::CANNOT_EVICT:
+        {
+            scheduled = keep_up_free_space_ratio_task->scheduleAfter(general_reschedule_ms);
+            break;
+        }
+        case IFileCachePriority::DesiredSizeStatus::REACHED_MAX_CANDIDATES_LIMIT:
+        {
+            scheduled = keep_up_free_space_ratio_task->schedule();
+            break;
+        }
+    }
     chassert(scheduled);
 }
 
@@ -1546,7 +1558,7 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings,
             FileCacheReserveStat stat;
             if (main_priority->collectCandidatesForEviction(
                     new_settings.max_size, new_settings.max_elements, 0/* max_candidates_to_evict */,
-                    stat, eviction_candidates, cache_lock))
+                    stat, eviction_candidates, cache_lock) == IFileCachePriority::DesiredSizeStatus::SUCCESS)
             {
                 if (eviction_candidates.size() == 0)
                 {

src/Interpreters/Cache/IFileCachePriority.h

@@ -151,7 +151,13 @@ public:
     /// and `desired_elements_num` as current cache state.
     /// Collect no more than `max_candidates_to_evict` elements.
     /// Return `true` if the first condition is satisfied.
-    virtual bool collectCandidatesForEviction(
+    enum class DesiredSizeStatus
+    {
+        SUCCESS,
+        CANNOT_EVICT,
+        REACHED_MAX_CANDIDATES_LIMIT,
+    };
+    virtual DesiredSizeStatus collectCandidatesForEviction(
         size_t desired_size,
         size_t desired_elements_count,
         size_t max_candidates_to_evict,

src/Interpreters/Cache/LRUFileCachePriority.cpp

@@ -323,7 +323,7 @@ bool LRUFileCachePriority::collectCandidatesForEviction(
     }
 }
 
-bool LRUFileCachePriority::collectCandidatesForEviction(
+IFileCachePriority::DesiredSizeStatus LRUFileCachePriority::collectCandidatesForEviction(
     size_t desired_size,
     size_t desired_elements_count,
     size_t max_candidates_to_evict,
@@ -336,12 +336,24 @@ bool LRUFileCachePriority::collectCandidatesForEviction(
         return canFit(0, 0, stat.total_stat.releasable_size, stat.total_stat.releasable_count,
                       lock, &desired_size, &desired_elements_count);
     };
+    auto status = DesiredSizeStatus::CANNOT_EVICT;
     auto stop_condition = [&]()
     {
-        return desired_limits_satisfied() || (max_candidates_to_evict && res.size() >= max_candidates_to_evict);
+        if (desired_limits_satisfied())
+        {
+            status = DesiredSizeStatus::SUCCESS;
+            return true;
+        }
+        if (max_candidates_to_evict && res.size() >= max_candidates_to_evict)
+        {
+            status = DesiredSizeStatus::REACHED_MAX_CANDIDATES_LIMIT;
+            return true;
+        }
+        return false;
     };
     iterateForEviction(res, stat, stop_condition, lock);
-    return desired_limits_satisfied();
+    chassert(status != DesiredSizeStatus::SUCCESS || stop_condition());
+    return status;
 }
 
 void LRUFileCachePriority::iterateForEviction(
@@ -350,6 +362,9 @@ void LRUFileCachePriority::iterateForEviction(
     StopConditionFunc stop_condition,
     const CachePriorityGuard::Lock & lock)
 {
+    if (stop_condition())
+        return;
+
     ProfileEvents::increment(ProfileEvents::FilesystemCacheEvictionTries);
 
     IterateFunc iterate_func = [&](LockedKey & locked_key, const FileSegmentMetadataPtr & segment_metadata)

src/Interpreters/Cache/LRUFileCachePriority.h

@@ -63,7 +63,7 @@ public:
         const UserID & user_id,
         const CachePriorityGuard::Lock &) override;
 
-    bool collectCandidatesForEviction(
+    DesiredSizeStatus collectCandidatesForEviction(
         size_t desired_size,
         size_t desired_elements_count,
         size_t max_candidates_to_evict,

src/Interpreters/Cache/SLRUFileCachePriority.cpp

@@ -256,7 +256,7 @@ bool SLRUFileCachePriority::collectCandidatesForEvictionInProtected(
     return true;
 }
 
-bool SLRUFileCachePriority::collectCandidatesForEviction(
+IFileCachePriority::DesiredSizeStatus SLRUFileCachePriority::collectCandidatesForEviction(
     size_t desired_size,
     size_t desired_elements_count,
     size_t max_candidates_to_evict,
@@ -268,7 +268,7 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     const auto desired_probationary_elements_num = getRatio(desired_elements_count, 1 - size_ratio);
 
     FileCacheReserveStat probationary_stat;
-    const bool probationary_limit_satisfied = probationary_queue.collectCandidatesForEviction(
+    const auto probationary_desired_size_status = probationary_queue.collectCandidatesForEviction(
         desired_probationary_size, desired_probationary_elements_num,
         max_candidates_to_evict, probationary_stat, res, lock);
 
@@ -285,14 +285,14 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     chassert(!max_candidates_to_evict || res.size() <= max_candidates_to_evict);
     chassert(res.size() == stat.total_stat.releasable_count);
 
-    if (max_candidates_to_evict && res.size() >= max_candidates_to_evict)
+    if (probationary_desired_size_status == DesiredSizeStatus::REACHED_MAX_CANDIDATES_LIMIT)
-        return probationary_limit_satisfied;
+        return probationary_desired_size_status;
 
     const auto desired_protected_size = getRatio(desired_size, size_ratio);
     const auto desired_protected_elements_num = getRatio(desired_elements_count, size_ratio);
 
     FileCacheReserveStat protected_stat;
-    const bool protected_limit_satisfied = protected_queue.collectCandidatesForEviction(
+    const auto protected_desired_size_status = protected_queue.collectCandidatesForEviction(
         desired_protected_size, desired_protected_elements_num,
         max_candidates_to_evict - res.size(), protected_stat, res, lock);
 
@@ -306,7 +306,10 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
              desired_protected_size, desired_protected_elements_num,
              protected_queue.getStateInfoForLog(lock));
 
-    return probationary_limit_satisfied && protected_limit_satisfied;
+    if (probationary_desired_size_status == DesiredSizeStatus::SUCCESS)
+        return protected_desired_size_status;
+    else
+        return probationary_desired_size_status;
 }
 
 void SLRUFileCachePriority::downgrade(IteratorPtr iterator, const CachePriorityGuard::Lock & lock)

src/Interpreters/Cache/SLRUFileCachePriority.h

@@ -58,7 +58,7 @@ public:
         const UserID & user_id,
         const CachePriorityGuard::Lock &) override;
 
-    bool collectCandidatesForEviction(
+    DesiredSizeStatus collectCandidatesForEviction(
         size_t desired_size,
         size_t desired_elements_count,
         size_t max_candidates_to_evict,