MB-33918: Fix cursor dropping trigger issue

There was an issue where we were doing an integer divide in the
calculation used to determine whether cursor dropping should be
triggered and also in the calculation used to determine the amount of
memory to recover.

This resulted in cursor dropping being triggered too aggressively and
thinking it need to recover more memory than memory it could.

Change-Id: I4571b51739a3e778fcf5463be361db5e0fe92da9
Reviewed-on: http://review.couchbase.org/108266
Well-Formed: Build Bot <[email protected]>
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: owend74

engines/ep/src/checkpoint_remover.cc

@@ -28,6 +28,8 @@
 #include <phosphor/phosphor.h>
 #include <platform/make_unique.h>
 
+#include <utility>
+
 /**
  * Remove all the closed unreferenced checkpoints for each vbucket.
  */
@@ -90,7 +92,8 @@ class CheckpointVisitor : public VBucketVisitor {
     std::atomic<bool>         &stateFinalizer;
 };
 
-void ClosedUnrefCheckpointRemoverTask::cursorDroppingIfNeeded(void) {
+std::pair<bool, size_t>
+ClosedUnrefCheckpointRemoverTask::isCursorDroppingNeeded() const {
     /**
      * Cursor dropping will commence if one of the following conditions is met:
      * 1. if the total memory used is greater than the upper threshold which is
@@ -112,8 +115,8 @@ void ClosedUnrefCheckpointRemoverTask::cursorDroppingIfNeeded(void) {
                     .getActiveVBucketsTotalCheckpointMemoryUsage();
 
     const auto chkptMemLimit =
-            bucketQuota *
-            (config.getCursorDroppingCheckpointMemUpperMark() / 100);
+            (bucketQuota * config.getCursorDroppingCheckpointMemUpperMark()) /
+            100;
 
     const bool hitCheckpointMemoryThreshold =
             activeVBChkptMemSize >= chkptMemLimit;
@@ -140,8 +143,9 @@ void ClosedUnrefCheckpointRemoverTask::cursorDroppingIfNeeded(void) {
             // memory usage threshold.
             amountOfMemoryToClear =
                     stats.getEstimatedTotalMemoryUsed() -
-                    (bucketQuota *
-                     (config.getCursorDroppingCheckpointMemLowerMark() / 100));
+                    ((bucketQuota *
+                      config.getCursorDroppingCheckpointMemLowerMark()) /
+                     100);
             LOG(EXTENSION_LOG_INFO,
                 "Triggering cursor dropping as checkpoint_memory (%lu MB) "
                 "exceeds cursor_dropping_checkpoint_mem_upper_mark (%lu%%, "
@@ -163,43 +167,49 @@ void ClosedUnrefCheckpointRemoverTask::cursorDroppingIfNeeded(void) {
                 toMB(stats.cursorDroppingUThreshold.load()),
                 toMB(amountOfMemoryToClear));
         }
+        // Cursor dropping is required.
+        return std::make_pair(true, amountOfMemoryToClear);
+    }
+    // Cursor dropping is not required.
+    return std::make_pair(false, 0);
+}
 
-        size_t memoryCleared = 0;
-        KVBucketIface* kvBucket = engine->getKVBucket();
-        // Get a list of active vbuckets sorted by memory usage
-        // of their respective checkpoint managers.
-        auto vbuckets =
-                   kvBucket->getVBuckets().getActiveVBucketsSortedByChkMgrMem();
-        for (const auto& it: vbuckets) {
-            if (memoryCleared < amountOfMemoryToClear) {
-                uint16_t vbid = it.first;
-                VBucketPtr vb = kvBucket->getVBucket(vbid);
-                if (vb) {
-                    // Get a list of cursors that can be dropped from the
-                    // vbucket's checkpoint manager, so as to unreference
-                    // an estimated number of checkpoints.
-                    std::vector<std::string> cursors =
-                            vb->checkpointManager->getListOfCursorsToDrop();
-                    std::vector<std::string>::iterator itr = cursors.begin();
-                    for (; itr != cursors.end(); ++itr) {
-                        if (memoryCleared < amountOfMemoryToClear) {
-                            if (engine->getDcpConnMap().handleSlowStream(vbid,
-                                                                        *itr))
-                            {
-                                auto memoryFreed =
-                                        vb->getChkMgrMemUsageOfUnrefCheckpoints();
-                                ++stats.cursorsDropped;
-                                stats.cursorMemoryFreed += memoryFreed;
-                                memoryCleared += memoryFreed;
-                            }
-                        } else {
-                            break;
+void ClosedUnrefCheckpointRemoverTask::attemptCursorDropping(
+        size_t amountOfMemoryToClear) {
+    size_t memoryCleared = 0;
+    KVBucketIface* kvBucket = engine->getKVBucket();
+    // Get a list of active vbuckets sorted by memory usage
+    // of their respective checkpoint managers.
+    auto vbuckets =
+            kvBucket->getVBuckets().getActiveVBucketsSortedByChkMgrMem();
+    for (const auto& it : vbuckets) {
+        if (memoryCleared < amountOfMemoryToClear) {
+            uint16_t vbid = it.first;
+            VBucketPtr vb = kvBucket->getVBucket(vbid);
+            if (vb) {
+                // Get a list of cursors that can be dropped from the
+                // vbucket's checkpoint manager, so as to unreference
+                // an estimated number of checkpoints.
+                std::vector<std::string> cursors =
+                        vb->checkpointManager->getListOfCursorsToDrop();
+                std::vector<std::string>::iterator itr = cursors.begin();
+                for (; itr != cursors.end(); ++itr) {
+                    if (memoryCleared < amountOfMemoryToClear) {
+                        if (engine->getDcpConnMap().handleSlowStream(vbid,
+                                                                     *itr)) {
+                            auto memoryFreed =
+                                    vb->getChkMgrMemUsageOfUnrefCheckpoints();
+                            ++stats.cursorsDropped;
+                            stats.cursorMemoryFreed += memoryFreed;
+                            memoryCleared += memoryFreed;
                         }
+                    } else {
+                        break;
                     }
                 }
-            } else {    // memoryCleared >= amountOfMemoryToClear
-                break;
             }
+        } else { // memoryCleared >= amountOfMemoryToClear
+            break;
         }
     }
 }
@@ -208,7 +218,13 @@ bool ClosedUnrefCheckpointRemoverTask::run(void) {
     TRACE_EVENT0("ep-engine/task", "ClosedUnrefCheckpointRemoverTask");
     bool inverse = true;
     if (available.compare_exchange_strong(inverse, false)) {
-        cursorDroppingIfNeeded();
+        bool shouldTriggerCursorDropping{false};
+        size_t amountOMemoryToClear{0};
+        std::tie(shouldTriggerCursorDropping, amountOMemoryToClear) =
+                isCursorDroppingNeeded();
+        if (shouldTriggerCursorDropping) {
+            attemptCursorDropping(amountOMemoryToClear);
+        }
         KVBucketIface* kvBucket = engine->getKVBucket();
         auto pv =
                 std::make_unique<CheckpointVisitor>(kvBucket, stats, available);

engines/ep/src/checkpoint_remover.h

@@ -42,7 +42,7 @@ class ClosedUnrefCheckpointRemoverTask : public GlobalTask {
         GlobalTask(e, TaskId::ClosedUnrefCheckpointRemoverTask, interval, false),
         engine(e), stats(st), sleepTime(interval), available(true) {}
 
-    void cursorDroppingIfNeeded(void);
+    void attemptCursorDropping(size_t amountOfMemoryToClear);
 
     bool run(void);
 
@@ -56,6 +56,8 @@ class ClosedUnrefCheckpointRemoverTask : public GlobalTask {
         return std::chrono::milliseconds(25);
     }
 
+    std::pair<bool, size_t> isCursorDroppingNeeded() const;
+
 private:
     EventuallyPersistentEngine *engine;
     EPStats                   &stats;

engines/ep/tests/module_tests/checkpoint_remover_test.cc

@@ -207,4 +207,55 @@ TEST_F(CheckpointRemoverEPTest, CursorDropMemoryFreed) {
 
     // There should only be the one checkpoint cursor now for persistence
     ASSERT_EQ(1, checkpointManager->getNumOfCursors());
+}
+
+// Test that we correctly determine whether to trigger cursor dropping.
+TEST_F(CheckpointRemoverEPTest, cursorDroppingTriggerTest) {
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+    auto& config = engine->getConfiguration();
+    const auto& task = std::make_shared<ClosedUnrefCheckpointRemoverTask>(
+            engine.get(),
+            engine->getEpStats(),
+            engine->getConfiguration().getChkRemoverStime());
+
+    bool shouldTriggerCursorDropping{false};
+    size_t amountOfMemoryToClear{0};
+
+    /*
+     * With a large max size (with no other changes) we should
+     * conclude the cursor dropping is not required.
+     */
+    config.setMaxSize(10240);
+
+    std::tie(shouldTriggerCursorDropping, amountOfMemoryToClear) =
+            task->isCursorDroppingNeeded();
+    EXPECT_FALSE(shouldTriggerCursorDropping);
+    EXPECT_EQ(0, amountOfMemoryToClear);
+
+    /*
+     * Trigger first condition for cursor dropping:
+     * the total memory used is greater than the upper threshold which is
+     * a percentage of the quota, specified by cursor_dropping_upper_mark
+     */
+    config.setMaxSize(1024);
+
+    std::tie(shouldTriggerCursorDropping, amountOfMemoryToClear) =
+            task->isCursorDroppingNeeded();
+    EXPECT_TRUE(shouldTriggerCursorDropping);
+    EXPECT_LT(0, amountOfMemoryToClear);
+
+    /*
+     * Trigger second condition for cursor dropping:
+     * the overall checkpoint memory usage goes above a certain % of the
+     * bucket quota, specified by cursor_dropping_checkpoint_mem_upper_mark and
+     * the checkpoint memory usage is above the memory low watermark.
+     */
+    config.setMaxSize(10240);
+    engine->getEpStats().mem_low_wat.store(1);
+    config.setCursorDroppingCheckpointMemUpperMark(1);
+
+    std::tie(shouldTriggerCursorDropping, amountOfMemoryToClear) =
+            task->isCursorDroppingNeeded();
+    EXPECT_TRUE(shouldTriggerCursorDropping);
+    EXPECT_LT(0, amountOfMemoryToClear);
 }