From 60be89fecffd4b6042052b3cefa728df6f4d9c18 Mon Sep 17 00:00:00 2001
From: fanmin shi <fanmin.shi@coreos.com>
Date: Wed, 26 Jul 2017 14:34:36 -0700
Subject: [PATCH] mvcc: fix hash mismatch

This pr adds a check in compaction to ensure that setting keep to nil only if this is the latest compaction in queue and
merges the keep from prev ongoing compaction to this scheduled compaction allows HashKV to computes Hash from previous keep.

Scenario #1

Suppose that HashKV() starts after compact(A) and compact(B) has scheduled and after
completion of compact(A) but before the completion of compact(B).

Before this PR:
Completion of compact(A) sets keep to nil to indicate that there are no ongoing compactions.
This assumption causes HashKV to hash all mvcc keys including keys that
hasn't been deleted by compact(B) up to a rev. Hence, HashKV again after compact(B) returns
a different Hash that during compact(B).

After this PR:
Completion of compact(A) sets keep to nil if current compactRev == compactRev(compact(A)).
Since calling compact(B) changes current compactRev, then current compactRev != compactRev(compact(A)).
Then HashKV will base on the keep set during compact(B) instead of nil which return the correct hash
during compact(B).

Scenario #2

Suppose that HashKV() starts after compact(A) and compact(B) has scheduled and before the
completion of compact(A) and compact(B).

Before this PR:
The start of compact(B) sets keep to compact(B) and overrides the keep from compact(A).
Since compact(A) hasn't finished yet, HashKV needs to hash revision from keep of compact(A) but it
is overridden. Hence HashKV misses revision from keep of compact(A) would result hash difference.

After this PR:
The start of compact(B) merges keep from compact(B) and compact(A). Even though compact(A) hasn't finished,
HashKV can retrieve from keep of compact(A). Hence HashKV doesn't miss any revisions and will compute Hash correctly.
---
 mvcc/kvstore.go | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/mvcc/kvstore.go b/mvcc/kvstore.go
index 618ca0786de..d38d21ae990 100644
--- a/mvcc/kvstore.go
+++ b/mvcc/kvstore.go
@@ -258,7 +258,13 @@ func (s *store) Compact(rev int64) (<-chan struct{}, error) {
 
 	keep := s.kvindex.Compact(rev)
 	s.keepMu.Lock()
-	s.keep = keep
+	// merge keep from the previous ongoing compaction
+	// allows HashKV to include revisions from previous keep.
+	if len(s.keep) > 0 {
+		for k, v := range keep {
+			s.keep[k] = v
+		}
+	}
 	s.keepMu.Unlock()
 	ch := make(chan struct{})
 	var j = func(ctx context.Context) {
@@ -271,9 +277,16 @@ func (s *store) Compact(rev int64) (<-chan struct{}, error) {
 			return
 		}
 		close(ch)
-		s.keepMu.Lock()
-		s.keep = nil
-		s.keepMu.Unlock()
+		s.revMu.RLock()
+		// set the keep to nil if this is the last compaction
+		// to indicate there aren't any ongoing compactions.
+		// correctness of HashKV bases on state of keep.
+		if rev == s.compactMainRev {
+			s.keepMu.Lock()
+			s.keep = nil
+			s.keepMu.Unlock()
+		}
+		s.revMu.RUnlock()
 	}
 
 	s.fifoSched.Schedule(j)