From: James Harrison <00jamesh@gmail.com> Date: Fri, 19 May 2017 11:23:53 +0000 (+0100) Subject: MB-24376: Stop rangeRead on invalid Seqno X-Git-Url: http://review.couchbase.org/gitweb?p=ep-engine.git;a=commitdiff_plain;h=41c8ca3a39613d10598f1a24823c5579b975c07f MB-24376: Stop rangeRead on invalid Seqno An exception is thrown if the start of the readRange is updated with a value x < 0 || x > endSeqno, as this should not be done. When updating an item which is currently covered by a rangeRead, the following states occur in the seqList: A₁ B₂ C₃ Initial items [A₁ B₂ C₃] rangeRead to highSeqno { Write Lock [A₁ B₂ C₃] A₋₁ ** Update; A₁ marked stale, new OSV appended. NB: this is before queueDirty updates seqNo [A₁ B₂ C₃] A₄ Seqno updated in queueDirty } RangeReads do not hold the write lock, and stop only upon either reaching the end of the seqList, or reading an item with seqno > endSeqno. There is a brief window at the state marked ** in which a rangeRead may try to update the start of the readRange with -1: - rangeRead (endSeqno 3) reads C₃ normally - proceeds to next item in the seqList - checks if seqno > endSeqno - !(-1 > 3), don't stop yet - update the start of the readRange with -1 This patch ends the readRange if the current item has a seqno < 0. Change-Id: Ia1e53e4c40705b673c1f01e1b15fecbed5dff2fc Reviewed-on: http://review.couchbase.org/78353 Tested-by: Build Bot Reviewed-by: Manu Dhundi Reviewed-by: Dave Rigby --- diff --git a/src/linked_list.cc b/src/linked_list.cc index b7c8a9b..95402fe 100644 --- a/src/linked_list.cc +++ b/src/linked_list.cc @@ -123,19 +123,22 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) { std::vector items; for (const auto& osv : seqList) { - if (osv.getBySeqno() > end) { - /* we are done */ + int64_t currSeqno(osv.getBySeqno()); + + if (currSeqno > end || currSeqno < 0) { + /* We have read all the items in the requested range, or the osv + * does not yet have a valid seqno; either way we are done */ break; } { std::lock_guard lh(rangeLock); - readRange.setBegin(osv.getBySeqno()); /* [EPHE TODO]: should we + readRange.setBegin(currSeqno); /* [EPHE TODO]: should we update the min every time ? */ } - if (osv.getBySeqno() < start) { + if (currSeqno < start) { /* skip this item */ continue; } @@ -166,7 +169,7 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) { "(vb %d) ENOMEM while trying to copy " "item with seqno %" PRIi64 "before streaming it", vbid, - osv.getBySeqno()); + currSeqno); return std::make_tuple(ENGINE_ENOMEM, std::move(empty), 0); } } diff --git a/tests/module_tests/ephemeral_vb_test.cc b/tests/module_tests/ephemeral_vb_test.cc index 46626ce..7359729 100644 --- a/tests/module_tests/ephemeral_vb_test.cc +++ b/tests/module_tests/ephemeral_vb_test.cc @@ -677,3 +677,31 @@ TEST_F(EphemeralVBucketTest, SnapshotHasNoDuplicatesWithMultipleStale) { EXPECT_EQ(numItems * (updateIterations + 1), std::get<2>(res)); // extended end of readRange } + +TEST_F(EphemeralVBucketTest, RangeReadStopsOnInvalidSeqno) { + /* MB-24376: rangeRead has to stop if it encounters an OSV with a seqno of + * -1; this item is definitely past the end of the rangeRead, and has not + * yet had its seqno updated in queueDirty */ + const int numItems = 2; + + // store two items + auto keys = generateKeys(numItems); + setMany(keys, MutationStatus::WasClean); + + auto lastKey = makeStoredDocKey(std::to_string(numItems + 1)); + Item i(lastKey, 0, 0, lastKey.data(), lastKey.size()); + + // set item with no queueItemCtx - will not be queueDirty'd, and will + // keep seqno -1 + EXPECT_EQ(MutationStatus::WasClean, + public_processSet(i, i.getCas(), false)); + + EXPECT_EQ(-1, mockEpheVB->getLL()->getSeqList().back().getBySeqno()); + + auto res = mockEpheVB->inMemoryBackfill( + 1, std::numeric_limits::max()); + + EXPECT_EQ(ENGINE_SUCCESS, std::get<0>(res)); + EXPECT_EQ(numItems, std::get<1>(res).size()); + EXPECT_EQ(numItems, std::get<2>(res)); +} \ No newline at end of file diff --git a/tests/module_tests/vbucket_test.cc b/tests/module_tests/vbucket_test.cc index b54fb03..0b94377 100644 --- a/tests/module_tests/vbucket_test.cc +++ b/tests/module_tests/vbucket_test.cc @@ -122,7 +122,9 @@ std::pair VBucketTest::lockAndFind( return std::make_pair(std::move(hbl), storedVal); } -MutationStatus VBucketTest::public_processSet(Item& itm, const uint64_t cas) { +MutationStatus VBucketTest::public_processSet(Item& itm, + const uint64_t cas, + bool withCtx) { auto hbl_sv = lockAndFind(itm.getKey()); VBQueueItemCtx queueItmCtx(GenerateBySeqno::Yes, GenerateCas::No, @@ -136,7 +138,7 @@ MutationStatus VBucketTest::public_processSet(Item& itm, const uint64_t cas) { cas, true, false, - &queueItmCtx) + withCtx ? &queueItmCtx : nullptr) .first; } diff --git a/tests/module_tests/vbucket_test.h b/tests/module_tests/vbucket_test.h index 9f697a1..1ef4e56 100644 --- a/tests/module_tests/vbucket_test.h +++ b/tests/module_tests/vbucket_test.h @@ -76,7 +76,9 @@ protected: std::pair lockAndFind( const StoredDocKey& key); - MutationStatus public_processSet(Item& itm, const uint64_t cas); + MutationStatus public_processSet(Item& itm, + const uint64_t cas, + bool withCtx = true); AddStatus public_processAdd(Item& itm);