From 0b46af29254edfa40d600cef67557560a6e2b99a Mon Sep 17 00:00:00 2001 From: Steve Atherton Date: Wed, 14 Oct 2020 21:22:25 -0700 Subject: [PATCH] Added a simulation-only check for pager remap cleanup writing the same page twice in a cleanup cycle, which should never happen, but a bug leading to this was fixed recently. Adjusted some buggify logic to widen edge case coverage around remap cleanup parameters. --- fdbserver/VersionedBTree.actor.cpp | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/fdbserver/VersionedBTree.actor.cpp b/fdbserver/VersionedBTree.actor.cpp index 2af0a8ab98..40a7763c2c 100644 --- a/fdbserver/VersionedBTree.actor.cpp +++ b/fdbserver/VersionedBTree.actor.cpp @@ -1472,10 +1472,11 @@ public: // If the page is still being read then it's not also being written because a write places // the new content into readFuture when the write is launched, not when it is completed. - // Read/write ordering is being enforced waiting readers will not see the new write. This + // Read/write ordering is being enforced so waiting readers will not see the new write. This // is necessary for remap erasure to work correctly since the oldest version of a page, located - // at the original page ID, could have a pending read when that version is expired and the write - // of the next newest version over top of the original page begins. + // at the original page ID, could have a pending read when that version is expired (after which + // future reads of the version are not allowed) and the write of the next newest version over top + // of the original page begins. if (!cacheEntry.initialized()) { cacheEntry.writeFuture = writePhysicalPage(pageID, data); } else if (cacheEntry.reading()) { @@ -1767,6 +1768,10 @@ public: self->filename.c_str(), p.toString().c_str(), secondType, ::toString(*iVersionPagePair).c_str(), oldestRetainedVersion); if(copyNewToOriginal) { + if(g_network->isSimulated()) { + ASSERT(self->remapDestinationsSimOnly.count(p.originalPageID) == 0); + self->remapDestinationsSimOnly.insert(p.originalPageID); + } debug_printf("DWALPager(%s) remapCleanup copy %s\n", self->filename.c_str(), p.toString().c_str()); // Read the data from the page that the original was mapped to @@ -1829,7 +1834,8 @@ public: state RemappedPage cutoff(oldestRetainedVersion - self->remapCleanupWindow); // Minimum version we must pop to before obeying stop command. - state Version minStopVersion = cutoff.version - (self->remapCleanupWindow * SERVER_KNOBS->REDWOOD_REMAP_CLEANUP_LAG); + state Version minStopVersion = cutoff.version - (BUGGIFY ? deterministicRandom()->randomInt(0, 10) : (self->remapCleanupWindow * SERVER_KNOBS->REDWOOD_REMAP_CLEANUP_LAG)); + self->remapDestinationsSimOnly.clear(); loop { state Optional p = wait(self->remapQueue.pop(cutoff)); @@ -2142,6 +2148,7 @@ private: RemapQueueT remapQueue; Version remapCleanupWindow; + std::unordered_set remapDestinationsSimOnly; struct SnapshotEntry { Version version; @@ -5686,12 +5693,13 @@ public: : m_filePrefix(filePrefix), m_concurrentReads(new FlowLock(SERVER_KNOBS->REDWOOD_KVSTORE_CONCURRENT_READS)) { // TODO: This constructor should really just take an IVersionedStore + int pageSize = BUGGIFY ? deterministicRandom()->randomInt(1000, 4096*4) : SERVER_KNOBS->REDWOOD_DEFAULT_PAGE_SIZE; int64_t pageCacheBytes = g_network->isSimulated() - ? (BUGGIFY ? FLOW_KNOBS->BUGGIFY_SIM_PAGE_CACHE_4K : FLOW_KNOBS->SIM_PAGE_CACHE_4K) + ? (BUGGIFY ? deterministicRandom()->randomInt(pageSize, FLOW_KNOBS->BUGGIFY_SIM_PAGE_CACHE_4K) : FLOW_KNOBS->SIM_PAGE_CACHE_4K) : FLOW_KNOBS->PAGE_CACHE_4K; Version remapCleanupWindow = BUGGIFY ? deterministicRandom()->randomInt64(0, 1000) : SERVER_KNOBS->REDWOOD_REMAP_CLEANUP_WINDOW; - IPager2* pager = new DWALPager(SERVER_KNOBS->REDWOOD_DEFAULT_PAGE_SIZE, filePrefix, pageCacheBytes, remapCleanupWindow); + IPager2* pager = new DWALPager(pageSize, filePrefix, pageCacheBytes, remapCleanupWindow); m_tree = new VersionedBTree(pager, filePrefix); m_init = catchError(init_impl(this)); } @@ -7233,7 +7241,7 @@ TEST_CASE("!/redwood/correctness/btree") { shortTest ? 200 : (deterministicRandom()->coinflip() ? 4096 : deterministicRandom()->randomInt(200, 400)); state int64_t targetPageOps = shortTest ? 50000 : 1000000; - state bool pagerMemoryOnly = shortTest && (deterministicRandom()->random01() < .01); + state bool pagerMemoryOnly = shortTest && (deterministicRandom()->random01() < .001); state int maxKeySize = deterministicRandom()->randomInt(1, pageSize * 2); state int maxValueSize = randomSize(pageSize * 25); state int maxCommitSize = shortTest ? 1000 : randomSize(std::min((maxKeySize + maxValueSize) * 20000, 10e6)); @@ -7242,10 +7250,9 @@ TEST_CASE("!/redwood/correctness/btree") { state double clearPostSetProbability = deterministicRandom()->random01() * .1; state double coldStartProbability = pagerMemoryOnly ? 0 : (deterministicRandom()->random01() * 0.3); state double advanceOldVersionProbability = deterministicRandom()->random01(); - state int64_t cacheSizeBytes = - pagerMemoryOnly ? 2e9 : (BUGGIFY ? deterministicRandom()->randomInt(1, 10 * pageSize) : 0); + state int64_t cacheSizeBytes = pagerMemoryOnly ? 2e9 : (pageSize * deterministicRandom()->randomInt(1, (BUGGIFY ? 2 : 10000) + 1)); state Version versionIncrement = deterministicRandom()->randomInt64(1, 1e8); - state Version remapCleanupWindow = deterministicRandom()->randomInt64(0, versionIncrement * 50); + state Version remapCleanupWindow = BUGGIFY ? 0 : deterministicRandom()->randomInt64(1, versionIncrement * 50); state int maxVerificationMapEntries = 300e3; printf("\n");