[BP] MB-59368: Use getPageableMemCurrent/MemLowWatermark in PagingVisitor

The MB highlighted that for ephemeral auto_delete, paging can be
triggered as the trigger uses getPageableMemCurrent/MemLowWatermark
but the PagingVisitor immediately cancels paging because it compared
getEstimatedTotalMemoryUsed vs stats.mem_low_wat, nothing is freed
and paging is immediately triggered to run again - resulting in a
noticeable increase in CPU but no memory reduction.

Commit changes PagingVisitor to use the Pageable functions when
deciding to stop, memory is now released and no further paging is
triggered.

Conflicts:
engines/ep/src/paging_visitor.cc
engines/ep/tests/module_tests/item_pager_test.cc

Not a clean backport and required some dependent test-code backporting.

Change-Id: Ic83764c693cc35d4e0cc9b36c7aae8cb98e4bb30
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/201918
Reviewed-by: Dave Rigby <[email protected]>
Well-Formed: Restriction Checker
Tested-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/ephemeral_vb.cc

@@ -547,6 +547,8 @@ EphemeralVBucket::softDeleteStoredValue(const HashTable::HashBucketLock& hbl,
                                         const VBQueueItemCtx& queueItmCtx,
                                         uint64_t bySeqno,
                                         DeleteSource deleteSource) {
+    softDeleteStoredValueHook(getStateLock());
+
     std::lock_guard<std::mutex> lh(sequenceLock);
 
     StoredValue* newSv = &v;

engines/ep/src/paging_visitor.cc

@@ -199,8 +199,8 @@ void PagingVisitor::visitBucket(VBucket& vb) {
     }
 
     // skip active vbuckets if active resident ratio is lower than replica
-    auto current = static_cast<double>(stats.getEstimatedTotalMemoryUsed());
-    auto lower = static_cast<double>(stats.mem_low_wat);
+    auto current = static_cast<double>(store.getPageableMemCurrent());
+    auto lower = static_cast<double>(store.getPageableMemLowWatermark());
 
     if (current <= lower) {
         // stop eviction whenever memory usage is below low watermark

engines/ep/src/vbucket.h

@@ -2524,6 +2524,8 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
      */
     const SyncWriteTimeoutHandlerFactory syncWriteTimeoutFactory;
 
+    TestingHook<folly::SharedMutex&> softDeleteStoredValueHook;
+
 private:
     /**
      * The replication topology, set as part of SET_VBUCKET_STATE.

engines/ep/tests/module_tests/evp_store_single_threaded_test.h

@@ -728,6 +728,13 @@ class STParameterizedBucketTest
                 "item_eviction_policy=full_eviction"s);
     }
 
+    static auto ephAutoDeleteConfigValues() {
+        using namespace std::string_literals;
+        return ::testing::Values(
+                "bucket_type=ephemeral:"
+                "ephemeral_full_policy=auto_delete"s);
+    }
+
     bool persistent() const {
         return GetParam().find("persistent") != std::string::npos;
     }

engines/ep/tests/module_tests/item_pager_test.cc

@@ -29,6 +29,7 @@
 #include "test_helpers.h"
 #include "tests/mock/mock_synchronous_ep_engine.h"
 #include "vbucket.h"
+#include "vbucket_utils.h"
 
 #include <folly/portability/GTest.h>
 #include <platform/cb_arena_malloc.h>
@@ -1573,6 +1574,75 @@ TEST_P(STEphemeralItemPagerTest, ReplicaNotPaged) {
     }
 }
 
+/**
+ * Test fixture for Ephemeral item pager tests with
+ * ephemeral_full_policy=auto_delete.
+ */
+class STEphemeralAutoDeleteItemPagerTest : public STItemPagerTest {};
+
+// Test reproduced MB-59368. This was an issue where memory reached the
+// threshold to trigger the pager (which will delete data), but code in the
+// paging visitor was checking different values and stopped the pager. The pager
+// would then just keep getting triggered leading to noticeable CPU increase
+// and no memory reduction.
+TEST_P(STEphemeralAutoDeleteItemPagerTest, MB_59368) {
+    // Need two vbuckets and one must be replica so that we account the replica
+    // memory usage which will affect the result of getPageableMemCurrent
+    const Vbid activeVbid = vbid;
+    const Vbid replicaVbid(1);
+
+    setVBucketState(replicaVbid, vbucket_state_replica);
+
+    auto setReplica = [this](Vbid replicaVbid, int count) {
+        auto key = makeStoredDocKey("replica_" + std::to_string(count));
+        auto item = make_item(replicaVbid, key, "value");
+        item.setCas(1 + count);
+        uint64_t seqno;
+        ASSERT_EQ(cb::engine_errc::success,
+                  store->setWithMeta(item,
+                                     0,
+                                     &seqno,
+                                     cookie,
+                                     {vbucket_state_replica},
+                                     CheckConflicts::No,
+                                     true,
+                                     GenerateBySeqno::Yes,
+                                     GenerateCas::Yes));
+    };
+
+    auto setActive = [this](Vbid activeVbid, int count) {
+        auto key = makeStoredDocKey("active_" + std::to_string(count));
+        auto item = make_item(activeVbid, key, "value");
+        item.setFreqCounterValue(0);
+        storeItem(item);
+    };
+
+    auto& stats = engine->getEpStats();
+    int count = 0;
+    do {
+        // Write the active last as the path it executes will do the memory
+        // check that schedules the pager.
+        setReplica(replicaVbid, count);
+        setActive(activeVbid, count);
+        ++count;
+    } while (store->getPageableMemCurrent() <
+             store->getPageableMemHighWatermark());
+
+    // This is the condition which produced the issue.
+    ASSERT_LT(stats.getPreciseTotalMemoryUsed(), stats.mem_low_wat);
+
+    auto& vb = *store->getVBucket(activeVbid);
+    bool softDeleteCalled{false};
+    VBucketTestIntrospector::setSoftDeleteStoredValueHook(
+            vb, {[&softDeleteCalled](folly::SharedMutex& vbStateLock) {
+                softDeleteCalled = true;
+            }});
+
+    // Run the pager and check that items got deleted.
+    runHighMemoryPager();
+    EXPECT_TRUE(softDeleteCalled) << "Nothing was deleted";
+}
+
 /**
  * Test fixture for expiry pager tests - enables the Expiry Pager (in addition
  * to what the parent class does).
@@ -2684,6 +2754,11 @@ INSTANTIATE_TEST_SUITE_P(PersistentFullValue,
                          STParameterizedBucketTest::persistentConfigValues(),
                          STParameterizedBucketTest::PrintToStringParamName);
 
+INSTANTIATE_TEST_SUITE_P(EphemeralAutoDelete,
+                         STEphemeralAutoDeleteItemPagerTest,
+                         STParameterizedBucketTest::ephAutoDeleteConfigValues(),
+                         STParameterizedBucketTest::PrintToStringParamName);
+
 #else
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(STItemPagerTest);
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(MultiPagingVisitorTest);
@@ -2693,4 +2768,6 @@ GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(STValueEvictionExpiryPagerTest);
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(MB_32669);
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(STEphemeralItemPagerTest);
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(MB_36087);
+GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(
+        STEphemeralAutoDeleteItemPagerTest);
 #endif

engines/ep/tests/module_tests/vbucket_utils.h

@@ -38,4 +38,9 @@ class VBucketTestIntrospector {
     static void destroyDM(VBucket& vb) {
         vb.durabilityMonitor.reset();
     }
+
+    static void setSoftDeleteStoredValueHook(
+            VBucket& vb, TestingHook<folly::SharedMutex&> hook) {
+        vb.softDeleteStoredValueHook = hook;
+    }
 };