From ac69da7845bdfe3bb9c861fe242c979b7f11edbb Mon Sep 17 00:00:00 2001 From: James Harrison <00jamesh@gmail.com> Date: Wed, 2 Dec 2020 14:59:02 +0000 Subject: [PATCH] MB-43055: Ensure ItemPager available is not left set to false 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/+/141367 Well-Formed: Build Bot Tested-by: Build Bot Reviewed-by: Dave Rigby --- engines/ep/src/fakes/fake_executorpool.h | 36 ++++++++++--- engines/ep/src/item_pager.cc | 22 +++++--- .../ep/tests/module_tests/item_pager_test.cc | 53 ++++++++++++++++++- 3 files changed, 94 insertions(+), 17 deletions(-) diff --git a/engines/ep/src/fakes/fake_executorpool.h b/engines/ep/src/fakes/fake_executorpool.h index 81e446ec67..4dab4d7269 100644 --- a/engines/ep/src/fakes/fake_executorpool.h +++ b/engines/ep/src/fakes/fake_executorpool.h @@ -175,7 +175,6 @@ 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") { @@ -183,6 +182,11 @@ class CheckedExecutor : public ExecutorThread { // 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); @@ -226,17 +230,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); } /* diff --git a/engines/ep/src/item_pager.cc b/engines/ep/src/item_pager.cc index 8f13ad7941..b210ce0289 100644 --- a/engines/ep/src/item_pager.cc +++ b/engines/ep/src/item_pager.cc @@ -105,23 +105,29 @@ bool ItemPager::run(void) { double lower = engine.getKVBucket()->getPageableMemLowWatermark(); 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() == EvictionPolicy::Value) { doEvict = true; } ++stats.pagerRuns; - if (current <= lower) { - // early exit - no need to run a paging visitor - return true; - } - VBucketFilter replicaFilter; VBucketFilter activePendingFilter; diff --git a/engines/ep/tests/module_tests/item_pager_test.cc b/engines/ep/tests/module_tests/item_pager_test.cc index f327c26c1b..98b2156ea8 100644 --- a/engines/ep/tests/module_tests/item_pager_test.cc +++ b/engines/ep/tests/module_tests/item_pager_test.cc @@ -168,7 +168,7 @@ class STBucketQuotaTest : public STParameterizedBucketTest { return count; } - void populateUntilAboveHighWaterMark(Vbid vbid) { + int populateUntilAboveHighWaterMark(Vbid vbid) { bool populate = true; int count = 0; auto& stats = engine->getEpStats(); @@ -182,6 +182,7 @@ class STBucketQuotaTest : public STParameterizedBucketTest { populate = stats.getEstimatedTotalMemoryUsed() <= stats.mem_high_wat.load(); } + return count; } /** @@ -1200,6 +1201,56 @@ TEST_P(STItemPagerTest, ItemPagerEvictionOrderIsSafe) { } } +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.