MB-43055: [BP] Ensure ItemPager available is not left set to false

Backport of ac69da7 http://review.couchbase.org/c/kv_engine/+/141367

The ItemPager exited early, without scheduling a PagingVisitor, if
current memory usage had already dropped below the low watermark by the
time the ItemPager task was run.

However, this was done _after_ the `available` flag had been set to
false. This was an issue, as the flag is only set back to true by
`PagingVisitor::complete()` - but no PagingVisitor was scheduled.

Fix by exiting before `available` is altered.

Change-Id: Iee11632ff95c7f507935098cc4e75ad58b7e160b
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/142986
Well-Formed: Build Bot <[email protected]>
Tested-by: James Harrison <[email protected]>
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jameseh96

engines/ep/src/fakes/fake_executorpool.h

@@ -164,14 +164,18 @@ class CheckedExecutor : public ExecutorThread {
         // Configure a checker to run, some tasks are subtly different
         if (getTaskName() == "Snapshotting vbucket states" ||
             getTaskName() == "Removing closed unreferenced checkpoints from memory" ||
-            getTaskName() == "Paging out items." ||
             getTaskName() == "Paging expired items." ||
             getTaskName() == "Adjusting hash table sizes." ||
             getTaskName() == "Generating access log") {
             checker = [=](bool taskRescheduled) {
                 // These tasks all schedule one other task
                 this->oneExecutes(taskRescheduled, 1);
             };
+        } else if (getTaskName() == "Paging out items.") {
+            checker = [=](bool taskRescheduled) {
+                // This task _may_ schedule a single task.
+                this->oneExecutes(taskRescheduled, /*min*/ 0, /*max*/ 1);
+            };
         } else {
             checker = [=](bool taskRescheduled) {
                 this->oneExecutes(taskRescheduled, 0);
@@ -217,17 +221,33 @@ class CheckedExecutor : public ExecutorThread {
      *   - request itself to be rescheduled
      *   - schedule other tasks (expectedToBeScheduled)
      */
-    void oneExecutes(bool rescheduled, int expectedToBeScheduled) {
-        if (rescheduled) {
-            // One task executed and was rescheduled, account for it.
-            expectedToBeScheduled++;
+    void oneExecutes(bool rescheduled,
+                     int minExpectedToBeScheduled,
+                     int maxExpectedToBeScheduled) {
+        auto expected = preFutureQueueSize + preReadyQueueSize;
+        auto actual = queue.getFutureQueueSize() + queue.getReadyQueueSize();
+
+        if (!rescheduled) {
+            // the task did _not_ reschedule itself, so expect one fewer task
+            expected--;
         }
 
         // Check that the new sizes of the future and ready tally given
         // one executed and n were scheduled as a side effect.
-        EXPECT_EQ((preFutureQueueSize + preReadyQueueSize) - 1,
-                  (queue.getFutureQueueSize() + queue.getReadyQueueSize()) -
-                  expectedToBeScheduled);
+
+        // there should now be _at least_ minExpectedToBeScheduled extra
+        // tasks
+        EXPECT_GE(actual, expected + minExpectedToBeScheduled);
+        // there should now be _no more than_ maxExpectedToBeScheduled extra
+        // tasks
+        EXPECT_LE(actual, expected + maxExpectedToBeScheduled);
+    }
+
+    void oneExecutes(bool rescheduled, int expectedToBeScheduled) {
+        // expect _exactly_ the specified number of tasks to be scheduled
+        oneExecutes(rescheduled,
+                    /*min*/ expectedToBeScheduled,
+                    /*max*/ expectedToBeScheduled);
     }
 
     /*

engines/ep/src/item_pager.cc

@@ -563,12 +563,22 @@ bool ItemPager::run(void) {
     }
 
     if (current <= lower) {
+        // doEvict may have been set to ensure eviction would continue until the
+        // low watermark was reached - it now has, so clear the flag.
         doEvict = false;
+        // If a PagingVisitor were to be created now, it would visit vbuckets
+        // but not try to evict anything. Stop now instead.
+        return true;
     }
 
-    bool inverse = true;
-    if (((current > upper) || doEvict || wasNotified) &&
-        (*available).compare_exchange_strong(inverse, false)) {
+    if ((current > upper) || doEvict || wasNotified) {
+        bool inverse = true;
+        if (!(*available).compare_exchange_strong(inverse, false)) {
+            // available != true, another PagingVisitor exists and is still
+            // running. Don't create another.
+            return true;
+        }
+        // Note: available is reset to true by PagingVisitor::complete()
         if (kvBucket->getItemEvictionPolicy() == VALUE_ONLY) {
             doEvict = true;
         }

engines/ep/tests/module_tests/item_pager_test.cc

@@ -163,7 +163,7 @@ class STBucketQuotaTest : public STParameterizedBucketTest {
         return count;
     }
 
-    void populateUntilAboveHighWaterMark(uint16_t vbid) {
+    int populateUntilAboveHighWaterMark(uint16_t vbid) {
         bool populate = true;
         int count = 0;
         auto& stats = engine->getEpStats();
@@ -177,6 +177,7 @@ class STBucketQuotaTest : public STParameterizedBucketTest {
             populate = stats.getEstimatedTotalMemoryUsed() <=
                        stats.mem_high_wat.load();
         }
+        return count;
     }
 
     /**
@@ -940,6 +941,56 @@ TEST_P(STItemPagerTest, ActiveEvictedIfReplicaEvictionInsufficient) {
     EXPECT_LT(replicaRR, 5);
 }
 
+TEST_P(STItemPagerTest, MB43055_MemUsedDropDoesNotBreakEviction) {
+    // MB-43055: Test that having current memory usage drop below the low
+    // watermark before the item pager runs does not prevent future item
+    // pager runs.
+
+    if (std::get<1>(GetParam()) == "fail_new_data") {
+        // items are not auto-deleted, so the ItemPager does not run.
+        return;
+    }
+
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+
+    // this triggers eviction, scheduling the ItemPager task
+    auto itemCount = populateUntilAboveHighWaterMark(vbid);
+
+    // now delete some items to lower memory usage
+    for (int i = 0; i < itemCount; i++) {
+        auto key = makeStoredDocKey("key_" + std::to_string(i));
+        uint64_t cas = 0;
+        mutation_descr_t mutation_descr;
+        EXPECT_EQ(ENGINE_SUCCESS,
+                  store->deleteItem(key,
+                                    cas,
+                                    vbid,
+                                    cookie,
+                                    /*itemMeta*/ nullptr,
+                                    mutation_descr));
+    }
+
+    auto& stats = engine->getEpStats();
+    // confirm we are now below the low watermark, and can test the item pager
+    // behaviour
+    ASSERT_LT(stats.getEstimatedTotalMemoryUsed(), stats.mem_low_wat.load());
+
+    auto& lpNonioQ = *task_executor->getLpTaskQ()[NONIO_TASK_IDX];
+    // run the item pager. It should _not_ create and schedule a PagingVisitor
+    runNextTask(lpNonioQ, "Paging out items.");
+
+    EXPECT_EQ(0, lpNonioQ.getReadyQueueSize());
+    EXPECT_EQ(initialNonIoTasks, lpNonioQ.getFutureQueueSize());
+
+    // populate again, and confirm this time that the item pager does shedule
+    // a paging visitor
+    populateUntilAboveHighWaterMark(vbid);
+
+    runNextTask(lpNonioQ, "Paging out items.");
+
+    runNextTask(lpNonioQ, "Item pager on vb 0");
+}
+
 /**
  * Test fixture for Ephemeral-only item pager tests.
  */