From 40d9cf34fb07c87964af8f671551927f40d24b0b Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Thu, 1 Nov 2018 11:45:22 -0700 Subject: [PATCH] md_ops: cache next MD results Instead of hitting the server every single time we need to verify a revoked key. For example, when we have to verify lots of MDs written by the same revoked key when doing garbage collection. Issue: KBFS-3581 Issue: #1888 --- libkbfs/errors.go | 13 ++++++++++ libkbfs/interfaces.go | 10 ++++++++ libkbfs/md_ops.go | 21 +++++++++++++--- libkbfs/md_ops_test.go | 2 ++ libkbfs/mdcache.go | 54 ++++++++++++++++++++++++++++++++++++++++-- libkbfs/mocks_test.go | 27 +++++++++++++++++++++ 6 files changed, 122 insertions(+), 5 deletions(-) diff --git a/libkbfs/errors.go b/libkbfs/errors.go index d3d4f23e7d..c5dd0816f3 100644 --- a/libkbfs/errors.go +++ b/libkbfs/errors.go @@ -1210,3 +1210,16 @@ func (e FolderNotResetOnServer) Error() string { return fmt.Sprintf("Folder %s is not yet reset on the server; "+ "contact Keybase for help", e.h.GetCanonicalPath()) } + +// NextMDNotCachedError indicates we haven't cached the next MD after +// the given Merkle seqno. +type NextMDNotCachedError struct { + TlfID tlf.ID + RootSeqno keybase1.Seqno +} + +// Error implements the Error interface for NextMDNotCachedError. +func (e NextMDNotCachedError) Error() string { + return fmt.Sprintf("The MD following %d for folder %s is not cached", + e.RootSeqno, e.TlfID) +} diff --git a/libkbfs/interfaces.go b/libkbfs/interfaces.go index 438cda6b69..040b8cc547 100644 --- a/libkbfs/interfaces.go +++ b/libkbfs/interfaces.go @@ -1035,6 +1035,16 @@ type MDCache interface { // ChangeHandleForID moves an ID to be under a new handle, if the // ID is cached already. ChangeHandleForID(oldHandle *TlfHandle, newHandle *TlfHandle) + // GetNextMD returns a cached view of the next MD following the + // given global Merkle root. + GetNextMD(tlfID tlf.ID, rootSeqno keybase1.Seqno) ( + nextKbfsRoot *kbfsmd.MerkleRoot, nextMerkleNodes [][]byte, + nextRootSeqno keybase1.Seqno, err error) + // PutNextMD caches a view of the next MD following the given + // global Merkle root. + PutNextMD(tlfID tlf.ID, rootSeqno keybase1.Seqno, + nextKbfsRoot *kbfsmd.MerkleRoot, nextMerkleNodes [][]byte, + nextRootSeqno keybase1.Seqno) error } // KeyCache handles caching for both TLFCryptKeys and BlockCryptKeys. diff --git a/libkbfs/md_ops.go b/libkbfs/md_ops.go index 1a8120b31c..1f81cbd52c 100644 --- a/libkbfs/md_ops.go +++ b/libkbfs/md_ops.go @@ -303,11 +303,26 @@ func (md *MDOpsStandard) checkRevisionCameBeforeMerkle( ctx = context.WithValue(ctx, ctxMDOpsSkipKeyVerification, struct{}{}) kbfsRoot, merkleNodes, rootSeqno, err := - md.config.MDServer().FindNextMD(ctx, rmds.MD.TlfID(), - root.Seqno) - if err != nil { + md.config.MDCache().GetNextMD(rmds.MD.TlfID(), root.Seqno) + switch errors.Cause(err).(type) { + case nil: + case NextMDNotCachedError: + md.log.CDebugf(ctx, "Finding next MD for TLF %s after global root %d", + rmds.MD.TlfID(), root.Seqno) + kbfsRoot, merkleNodes, rootSeqno, err = + md.config.MDServer().FindNextMD(ctx, rmds.MD.TlfID(), root.Seqno) + if err != nil { + return err + } + err = md.config.MDCache().PutNextMD( + rmds.MD.TlfID(), root.Seqno, kbfsRoot, merkleNodes, rootSeqno) + if err != nil { + return err + } + default: return err } + if len(merkleNodes) == 0 { // This can happen legitimately if we are still inside the // error window and no new merkle trees have been made yet, or diff --git a/libkbfs/md_ops_test.go b/libkbfs/md_ops_test.go index 12c9801eca..d0d1b93d92 100644 --- a/libkbfs/md_ops_test.go +++ b/libkbfs/md_ops_test.go @@ -1300,6 +1300,7 @@ func testMDOpsVerifyRevokedDeviceWrite(t *testing.T, ver kbfsmd.MetadataVer) { require.True(t, cacheable) t.Log("Make the server return no information, but outside the max gap") + config.MDCache().(*MDCacheStandard).nextMDLRU.Purge() clock.Add(maxAllowedMerkleGap) // already added one minute above _, err = mdOps.verifyKey( ctx, allRMDSs[0], allRMDSs[0].MD.GetLastModifyingUser(), @@ -1307,6 +1308,7 @@ func testMDOpsVerifyRevokedDeviceWrite(t *testing.T, ver kbfsmd.MetadataVer) { require.Error(t, err) t.Log("Make the server return a root, but which is outside the max gap") + config.MDCache().(*MDCacheStandard).nextMDLRU.Purge() root.Timestamp = clock.Now().Unix() mdServer.nextMerkleRoot = root mdServer.nextMerkleNodes = [][]byte{leafBytes} diff --git a/libkbfs/mdcache.go b/libkbfs/mdcache.go index 08aea50b08..c6954a20fe 100644 --- a/libkbfs/mdcache.go +++ b/libkbfs/mdcache.go @@ -8,6 +8,7 @@ import ( "sync" lru "github.com/hashicorp/golang-lru" + "github.com/keybase/client/go/protocol/keybase1" "github.com/keybase/kbfs/kbfsmd" "github.com/keybase/kbfs/tlf" "github.com/pkg/errors" @@ -21,6 +22,8 @@ type MDCacheStandard struct { lock sync.RWMutex lru *lru.Cache idLRU *lru.Cache + + nextMDLRU *lru.Cache } type mdCacheKey struct { @@ -29,7 +32,10 @@ type mdCacheKey struct { bid kbfsmd.BranchID } -const defaultMDCacheCapacity = 5000 +const ( + defaultMDCacheCapacity = 5000 + defaultNextMDCacheCapacity = 100 +) // NewMDCacheStandard constructs a new MDCacheStandard using the given // cache capacity. @@ -42,7 +48,13 @@ func NewMDCacheStandard(capacity int) *MDCacheStandard { if err != nil { return nil } - return &MDCacheStandard{lru: mdLRU, idLRU: idLRU} + // Hard-code the nextMD cache size to something small, since only + // one entry is used for each revoked device we need to verify. + nextMDLRU, err := lru.New(defaultNextMDCacheCapacity) + if err != nil { + return nil + } + return &MDCacheStandard{lru: mdLRU, idLRU: idLRU, nextMDLRU: nextMDLRU} } // Get implements the MDCache interface for MDCacheStandard. @@ -161,3 +173,41 @@ func (md *MDCacheStandard) ChangeHandleForID( md.idLRU.Add(newKey, tmp) return } + +type mdcacheNextMDKey struct { + tlfID tlf.ID + rootSeqno keybase1.Seqno +} + +type mdcacheNextMDVal struct { + nextKbfsRoot *kbfsmd.MerkleRoot + nextMerkleNodes [][]byte + nextRootSeqno keybase1.Seqno +} + +// GetNextMD implements the MDCache interface for MDCacheStandard. +func (md *MDCacheStandard) GetNextMD(tlfID tlf.ID, rootSeqno keybase1.Seqno) ( + nextKbfsRoot *kbfsmd.MerkleRoot, nextMerkleNodes [][]byte, + nextRootSeqno keybase1.Seqno, err error) { + key := mdcacheNextMDKey{tlfID, rootSeqno} + tmp, ok := md.nextMDLRU.Get(key) + if !ok { + return nil, nil, 0, NextMDNotCachedError{tlfID, rootSeqno} + } + val, ok := tmp.(mdcacheNextMDVal) + if !ok { + return nil, nil, 0, errors.Errorf( + "Bad next MD for %s, seqno=%d", tlfID, rootSeqno) + } + return val.nextKbfsRoot, val.nextMerkleNodes, val.nextRootSeqno, nil +} + +// PutNextMD implements the MDCache interface for MDCacheStandard. +func (md *MDCacheStandard) PutNextMD( + tlfID tlf.ID, rootSeqno keybase1.Seqno, nextKbfsRoot *kbfsmd.MerkleRoot, + nextMerkleNodes [][]byte, nextRootSeqno keybase1.Seqno) error { + key := mdcacheNextMDKey{tlfID, rootSeqno} + val := mdcacheNextMDVal{nextKbfsRoot, nextMerkleNodes, nextRootSeqno} + md.nextMDLRU.Add(key, val) + return nil +} diff --git a/libkbfs/mocks_test.go b/libkbfs/mocks_test.go index 88e5072b5d..6158c2f2dc 100644 --- a/libkbfs/mocks_test.go +++ b/libkbfs/mocks_test.go @@ -3797,6 +3797,33 @@ func (mr *MockMDCacheMockRecorder) ChangeHandleForID(oldHandle, newHandle interf return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ChangeHandleForID", reflect.TypeOf((*MockMDCache)(nil).ChangeHandleForID), oldHandle, newHandle) } +// GetNextMD mocks base method +func (m *MockMDCache) GetNextMD(tlfID tlf.ID, rootSeqno keybase1.Seqno) (*kbfsmd.MerkleRoot, [][]byte, keybase1.Seqno, error) { + ret := m.ctrl.Call(m, "GetNextMD", tlfID, rootSeqno) + ret0, _ := ret[0].(*kbfsmd.MerkleRoot) + ret1, _ := ret[1].([][]byte) + ret2, _ := ret[2].(keybase1.Seqno) + ret3, _ := ret[3].(error) + return ret0, ret1, ret2, ret3 +} + +// GetNextMD indicates an expected call of GetNextMD +func (mr *MockMDCacheMockRecorder) GetNextMD(tlfID, rootSeqno interface{}) *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNextMD", reflect.TypeOf((*MockMDCache)(nil).GetNextMD), tlfID, rootSeqno) +} + +// PutNextMD mocks base method +func (m *MockMDCache) PutNextMD(tlfID tlf.ID, rootSeqno keybase1.Seqno, nextKbfsRoot *kbfsmd.MerkleRoot, nextMerkleNodes [][]byte, nextRootSeqno keybase1.Seqno) error { + ret := m.ctrl.Call(m, "PutNextMD", tlfID, rootSeqno, nextKbfsRoot, nextMerkleNodes, nextRootSeqno) + ret0, _ := ret[0].(error) + return ret0 +} + +// PutNextMD indicates an expected call of PutNextMD +func (mr *MockMDCacheMockRecorder) PutNextMD(tlfID, rootSeqno, nextKbfsRoot, nextMerkleNodes, nextRootSeqno interface{}) *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PutNextMD", reflect.TypeOf((*MockMDCache)(nil).PutNextMD), tlfID, rootSeqno, nextKbfsRoot, nextMerkleNodes, nextRootSeqno) +} + // MockKeyCache is a mock of KeyCache interface type MockKeyCache struct { ctrl *gomock.Controller