From 61438e75654dd737290c88af3d6b706d7a09fac3 Mon Sep 17 00:00:00 2001 From: Vlad Date: Fri, 25 Aug 2023 15:09:50 +0300 Subject: [PATCH 01/14] split accessor cache into recent blocks and ipld requested blocks --- nodebuilder/blob/mocks/api.go | 4 +- nodebuilder/fraud/mocks/api.go | 4 +- share/eds/accessor_cache.go | 130 +++++++++++++++++++++++++++------ share/eds/blockstore.go | 9 ++- share/eds/metrics.go | 2 +- share/eds/store.go | 91 ++++++++++++----------- share/eds/store_test.go | 43 ++++++++--- 7 files changed, 198 insertions(+), 85 deletions(-) diff --git a/nodebuilder/blob/mocks/api.go b/nodebuilder/blob/mocks/api.go index 5cd34b74b6..e8d9f322b5 100644 --- a/nodebuilder/blob/mocks/api.go +++ b/nodebuilder/blob/mocks/api.go @@ -40,7 +40,7 @@ func (m *MockModule) EXPECT() *MockModuleMockRecorder { // Get mocks base method. func (m *MockModule) Get(arg0 context.Context, arg1 uint64, arg2 share.Namespace, arg3 blob.Commitment) (*blob.Blob, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "get", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*blob.Blob) ret1, _ := ret[1].(error) return ret0, ret1 @@ -49,7 +49,7 @@ func (m *MockModule) Get(arg0 context.Context, arg1 uint64, arg2 share.Namespace // Get indicates an expected call of Get. func (mr *MockModuleMockRecorder) Get(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1, arg2, arg3) } // GetAll mocks base method. diff --git a/nodebuilder/fraud/mocks/api.go b/nodebuilder/fraud/mocks/api.go index 399f8746e1..0936a2abb7 100644 --- a/nodebuilder/fraud/mocks/api.go +++ b/nodebuilder/fraud/mocks/api.go @@ -40,7 +40,7 @@ func (m *MockModule) EXPECT() *MockModuleMockRecorder { // Get mocks base method. func (m *MockModule) Get(arg0 context.Context, arg1 fraud0.ProofType) ([]fraud.Proof, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1) + ret := m.ctrl.Call(m, "get", arg0, arg1) ret0, _ := ret[0].([]fraud.Proof) ret1, _ := ret[1].(error) return ret0, ret1 @@ -49,7 +49,7 @@ func (m *MockModule) Get(arg0 context.Context, arg1 fraud0.ProofType) ([]fraud.P // Get indicates an expected call of Get. func (mr *MockModuleMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1) } // Subscribe mocks base method. diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go index cd0f0537fa..a9aa3d3742 100644 --- a/share/eds/accessor_cache.go +++ b/share/eds/accessor_cache.go @@ -15,10 +15,71 @@ import ( ) var ( - defaultCacheSize = 128 - errCacheMiss = errors.New("accessor not found in blockstore cache") + errCacheMiss = errors.New("accessor not found in blockstore cache") ) +// cache is an interface that defines the basic cache operations. +type cache interface { + // Get retrieves an item from the cache. + get(shard.Key) (*accessorWithBlockstore, error) + + // getOrLoad attempts to get an item from the cache and, if not found, invokes + // the provided loader function to load it into the cache. + getOrLoad(ctx context.Context, key shard.Key, loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error)) (*accessorWithBlockstore, error) + + // enableMetrics enables metrics in cache + enableMetrics() error +} + +// multiCache represents a cache that looks into multiple caches one by one. +type multiCache struct { + caches []cache +} + +// newMultiCache creates a new multiCache with the provided caches. +func newMultiCache(caches ...cache) *multiCache { + return &multiCache{caches: caches} +} + +// get looks for an item in all the caches one by one and returns the first found item. +func (mc *multiCache) get(key shard.Key) (*accessorWithBlockstore, error) { + for _, cache := range mc.caches { + accessor, err := cache.get(key) + if err == nil { + return accessor, nil + } + } + + return nil, errCacheMiss +} + +// getOrLoad attempts to get an item from all caches, and if not found, invokes +// the provided loader function to load it into one of the caches. +func (mc *multiCache) getOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), +) (*accessorWithBlockstore, error) { + for _, cache := range mc.caches { + accessor, err := cache.getOrLoad(ctx, key, loader) + if err == nil { + return accessor, nil + } + } + + return nil, errors.New("multicache: unable to get or load accessor") +} + +func (mc *multiCache) enableMetrics() error { + for _, cache := range mc.caches { + err := cache.enableMetrics() + if err != nil { + return err + } + } + return nil +} + // accessorWithBlockstore is the value that we store in the blockstore cache type accessorWithBlockstore struct { sa *dagstore.ShardAccessor @@ -28,6 +89,8 @@ type accessorWithBlockstore struct { } type blockstoreCache struct { + // name is a prefix, that will be used for cache metrics if it is enabled + name string // stripedLocks prevents simultaneous RW access to the blockstore cache for a shard. Instead // of using only one lock or one lock per key, we stripe the shard keys across 256 locks. 256 is // chosen because it 0-255 is the range of values we get looking at the last byte of the key. @@ -39,7 +102,7 @@ type blockstoreCache struct { metrics *cacheMetrics } -func newBlockstoreCache(cacheSize int) (*blockstoreCache, error) { +func newBlockstoreCache(name string, cacheSize int) (*blockstoreCache, error) { bc := &blockstoreCache{} // instantiate the blockstore cache bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) @@ -71,17 +134,17 @@ func (bc *blockstoreCache) evictFn() func(_ interface{}, val interface{}) { // Get retrieves the blockstore for a given shard key from the cache. If the blockstore is not in // the cache, it returns an errCacheMiss -func (bc *blockstoreCache) Get(shardContainingCid shard.Key) (*accessorWithBlockstore, error) { - lk := &bc.stripedLocks[shardKeyToStriped(shardContainingCid)] +func (bc *blockstoreCache) get(key shard.Key) (*accessorWithBlockstore, error) { + lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() defer lk.Unlock() - return bc.unsafeGet(shardContainingCid) + return bc.unsafeGet(key) } -func (bc *blockstoreCache) unsafeGet(shardContainingCid shard.Key) (*accessorWithBlockstore, error) { +func (bc *blockstoreCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, error) { // We've already ensured that the given shard has the cid/multihash we are looking for. - val, ok := bc.cache.Get(shardContainingCid) + val, ok := bc.cache.Get(key) if !ok { return nil, errCacheMiss } @@ -96,22 +159,26 @@ func (bc *blockstoreCache) unsafeGet(shardContainingCid shard.Key) (*accessorWit return accessor, nil } -// Add adds a blockstore for a given shard key to the cache. -func (bc *blockstoreCache) Add( - shardContainingCid shard.Key, - accessor *dagstore.ShardAccessor, +// getOrLoad attempts to get an item from all caches, and if not found, invokes +// the provided loader function to load it into one of the caches. +func (bc *blockstoreCache) getOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), ) (*accessorWithBlockstore, error) { - lk := &bc.stripedLocks[shardKeyToStriped(shardContainingCid)] + lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() defer lk.Unlock() - return bc.unsafeAdd(shardContainingCid, accessor) -} + if accessor, err := bc.unsafeGet(key); err == nil { + return accessor, nil + } + + accessor, err := loader(ctx, key) + if err != nil { + return nil, fmt.Errorf("unable to get accessor: %w", err) + } -func (bc *blockstoreCache) unsafeAdd( - shardContainingCid shard.Key, - accessor *dagstore.ShardAccessor, -) (*accessorWithBlockstore, error) { blockStore, err := accessor.Blockstore() if err != nil { return nil, fmt.Errorf("failed to get blockstore from accessor: %w", err) @@ -121,7 +188,7 @@ func (bc *blockstoreCache) unsafeAdd( bs: blockStore, sa: accessor, } - bc.cache.Add(shardContainingCid, newAccessor) + bc.cache.Add(key, newAccessor) return newAccessor, nil } @@ -135,14 +202,14 @@ type cacheMetrics struct { evictedCounter metric.Int64Counter } -func (bc *blockstoreCache) withMetrics() error { - evictedCounter, err := meter.Int64Counter("eds_blockstore_cache_evicted_counter", +func (bc *blockstoreCache) enableMetrics() error { + evictedCounter, err := meter.Int64Counter("eds_blockstore_cache"+bc.name+"_evicted_counter", metric.WithDescription("eds blockstore cache evicted event counter")) if err != nil { return err } - cacheSize, err := meter.Int64ObservableGauge("eds_blockstore_cache_size", + cacheSize, err := meter.Int64ObservableGauge("eds_blockstore"+bc.name+"_cache_size", metric.WithDescription("total amount of items in blockstore cache"), ) if err != nil { @@ -168,3 +235,20 @@ func (m *cacheMetrics) observeEvicted(failed bool) { m.evictedCounter.Add(context.Background(), 1, metric.WithAttributes( attribute.Bool(failedKey, failed))) } + +type noopCache struct{} + +func (n noopCache) get(shard.Key) (*accessorWithBlockstore, error) { + return nil, errCacheMiss +} + +func (n noopCache) getOrLoad( + context.Context, shard.Key, + func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), +) (*accessorWithBlockstore, error) { + return nil, nil +} + +func (n noopCache) enableMetrics() error { + return nil +} diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 9fc9d7f2f8..ddf94f4d26 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -119,9 +119,16 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d return nil, fmt.Errorf("failed to find shards containing multihash: %w", err) } + // check hash for any of keys + for _, k := range keys { + if accessor, err := bs.store.cache.get(k); err == nil { + return accessor.bs, nil + } + } + // a share can exist in multiple EDSes, so just take the first one. shardKey := keys[0] - accessor, err := bs.store.getCachedAccessor(ctx, shardKey) + accessor, err := bs.cache.getOrLoad(ctx, shardKey, bs.store.getAccessor) if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) } diff --git a/share/eds/metrics.go b/share/eds/metrics.go index 6547e239cd..8da7b6300b 100644 --- a/share/eds/metrics.go +++ b/share/eds/metrics.go @@ -116,7 +116,7 @@ func (s *Store) WithMetrics() error { return err } - if err = s.cache.withMetrics(); err != nil { + if err = s.cache.enableMetrics(); err != nil { return err } s.metrics = &metrics{ diff --git a/share/eds/store.go b/share/eds/store.go index fd679e2591..9af72c6134 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -39,6 +39,9 @@ const ( // shards that are currently available but inactive, or errored. // We don't use transient files right now, so GC is turned off by default. defaultGCInterval = 0 + + defaultRecentBlocksCacheSize = 10 + defaultBlockstoreCacheSize = 128 ) var ErrNotFound = errors.New("eds not found in store") @@ -53,7 +56,7 @@ type Store struct { dgstr *dagstore.DAGStore mounts *mount.Registry - cache *blockstoreCache + cache cache bs bstore.Blockstore carIdx index.FullIndexRepo @@ -105,7 +108,12 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { return nil, fmt.Errorf("failed to create DAGStore: %w", err) } - cache, err := newBlockstoreCache(defaultCacheSize) + recentBlocksCache, err := newBlockstoreCache("recent", defaultRecentBlocksCacheSize) + if err != nil { + return nil, fmt.Errorf("failed to create recent blocks cache: %w", err) + } + + blockstoreCache, err := newBlockstoreCache("blockstore", defaultBlockstoreCacheSize) if err != nil { return nil, fmt.Errorf("failed to create blockstore cache: %w", err) } @@ -117,9 +125,9 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { invertedIdx: invertedIdx, gcInterval: defaultGCInterval, mounts: r, - cache: cache, + cache: newMultiCache(recentBlocksCache, blockstoreCache), } - store.bs = newBlockstore(store, cache) + store.bs = newBlockstore(store, blockstoreCache) return store, nil } @@ -231,17 +239,25 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to initiate shard registration: %w", err) } + var result dagstore.ShardResult select { + case result = <-ch: case <-ctx.Done(): // if context finished before result was received, track result in separate goroutine go trackLateResult("put", ch, s.metrics, time.Minute*5) return ctx.Err() - case result := <-ch: - if result.Error != nil { - return fmt.Errorf("failed to register shard: %w", result.Error) - } - return nil } + + if result.Error != nil { + return fmt.Errorf("failed to register shard: %w", result.Error) + } + + // return accessor in result will be always nil, need to aquire shard to make it available in cache + if _, err := s.cache.getOrLoad(ctx, result.Key, s.getAccessor); err != nil { + log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) + } + + return nil } // waitForResult waits for a result from the res channel for a maximum duration specified by @@ -289,12 +305,17 @@ func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.Reader, err } func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, error) { - key := root.String() - accessor, err := s.getCachedAccessor(ctx, shard.KeyFromString(key)) + key := shard.KeyFromString(root.String()) + accessor, err := s.cache.get(key) + if err == nil { + return accessor.sa.Reader(), nil + } + // if accessor not found in cache, create new one from dagstore + shardAccessor, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) } - return accessor.sa.Reader(), nil + return shardAccessor.Reader(), nil } // Blockstore returns an IPFS blockstore providing access to individual shares/nodes of all EDS @@ -326,11 +347,21 @@ func (s *Store) carBlockstore( root share.DataHash, ) (dagstore.ReadBlockstore, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.getCachedAccessor(ctx, key) + accessor, err := s.cache.get(key) + if err == nil { + return accessor.bs, nil + } + // if accessor not found in cache, create new one from dagstore + sa, err := s.getAccessor(ctx, key) + if err != nil { + return nil, fmt.Errorf("failed to get accessor: %w", err) + } + + bs, err := sa.Blockstore() if err != nil { return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) } - return accessor.bs, nil + return bs, nil } // GetDAH returns the DataAvailabilityHeader for the EDS identified by DataHash. @@ -344,13 +375,12 @@ func (s *Store) GetDAH(ctx context.Context, root share.DataHash) (*share.Root, e } func (s *Store) getDAH(ctx context.Context, root share.DataHash) (*share.Root, error) { - key := shard.KeyFromString(root.String()) - accessor, err := s.getCachedAccessor(ctx, key) + r, err := s.getCAR(ctx, root) if err != nil { return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) } - carHeader, err := carv1.ReadHeader(bufio.NewReader(accessor.sa.Reader())) + carHeader, err := carv1.ReadHeader(bufio.NewReader(r)) if err != nil { return nil, fmt.Errorf("eds/store: failed to read car header: %w", err) } @@ -397,33 +427,6 @@ func (s *Store) getAccessor(ctx context.Context, key shard.Key) (*dagstore.Shard } } -func (s *Store) getCachedAccessor(ctx context.Context, key shard.Key) (*accessorWithBlockstore, error) { - lk := &s.cache.stripedLocks[shardKeyToStriped(key)] - lk.Lock() - defer lk.Unlock() - - tnow := time.Now() - accessor, err := s.cache.unsafeGet(key) - if err != nil && err != errCacheMiss { - log.Errorf("unexpected error while reading key from bs cache %s: %s", key, err) - } - if accessor != nil { - s.metrics.observeGetAccessor(ctx, time.Since(tnow), true, false) - return accessor, nil - } - - // wasn't found in cache, so acquire it and add to cache - shardAccessor, err := s.getAccessor(ctx, key) - if err != nil { - s.metrics.observeGetAccessor(ctx, time.Since(tnow), false, err != nil) - return nil, err - } - - a, err := s.cache.unsafeAdd(key, shardAccessor) - s.metrics.observeGetAccessor(ctx, time.Since(tnow), false, err != nil) - return a, err -} - // Remove removes EDS from Store by the given share.Root hash and cleans up all // the indexing. func (s *Store) Remove(ctx context.Context, root share.DataHash) error { diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 4b263e7062..bcdf06e152 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -2,6 +2,7 @@ package eds import ( "context" + "github.com/ipfs/go-cid" "os" "testing" "time" @@ -145,13 +146,13 @@ func TestEDSStore(t *testing.T) { // key isnt in cache yet, so get returns errCacheMiss shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.Get(shardKey) + _, err = edsStore.cache.get(shardKey) assert.ErrorIs(t, err, errCacheMiss) // now get it, so that the key is in the cache _, err = edsStore.CARBlockstore(ctx, dah.Hash()) assert.NoError(t, err) - _, err = edsStore.cache.Get(shardKey) + _, err = edsStore.cache.get(shardKey) assert.NoError(t, err, errCacheMiss) }) @@ -217,20 +218,38 @@ func Test_BlockstoreCache(t *testing.T) { err = edsStore.Start(ctx) require.NoError(t, err) + // store eds to the store with noopCache to allow clean cache after put + swap := edsStore.cache + edsStore.cache = noopCache{} eds, dah := randomEDS(t) err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // key isnt in cache yet, so get returns errCacheMiss + // get any key from saved eds + bs, err := edsStore.carBlockstore(ctx, dah.Hash()) + require.NoError(t, err) + keys, err := bs.AllKeysChan(ctx) + require.NoError(t, err) + var key cid.Cid + select { + case key = <-keys: + case <-ctx.Done(): + t.Fatal("context timeout") + } + + // swap back original cache + edsStore.cache = swap + + // key isn't in cache yet, so get returns errCacheMiss shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.Get(shardKey) - assert.ErrorIs(t, err, errCacheMiss) + _, err = edsStore.cache.get(shardKey) + require.ErrorIs(t, err, errCacheMiss) // now get it, so that the key is in the cache - _, err = edsStore.getCachedAccessor(ctx, shardKey) - assert.NoError(t, err) - _, err = edsStore.cache.Get(shardKey) - assert.NoError(t, err, errCacheMiss) + _, err = edsStore.bs.Get(ctx, key) + require.NoError(t, err) + _, err = edsStore.cache.get(shardKey) + require.NoError(t, err) } // Test_CachedAccessor verifies that the reader represented by a cached accessor can be read from @@ -249,8 +268,8 @@ func Test_CachedAccessor(t *testing.T) { require.NoError(t, err) shardKey := shard.KeyFromString(dah.String()) - // adds to cache - cachedAccessor, err := edsStore.getCachedAccessor(ctx, shardKey) + // accessor is expected to be in cache + cachedAccessor, err := edsStore.cache.get(shardKey) assert.NoError(t, err) // first read @@ -260,7 +279,7 @@ func Test_CachedAccessor(t *testing.T) { assert.NoError(t, err) // second read - cachedAccessor, err = edsStore.getCachedAccessor(ctx, shardKey) + cachedAccessor, err = edsStore.cache.get(shardKey) assert.NoError(t, err) carReader, err = car.NewCarReader(cachedAccessor.sa.Reader()) assert.NoError(t, err) From 186d1350f1cf4bd86e46cbf5912a73e32392e652 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 29 Aug 2023 09:28:36 +0300 Subject: [PATCH 02/14] - make multicache less abstract - update metrics - fix tests --- share/eds/accessor_cache.go | 101 ++++++++++++++++++++++++++---------- share/eds/blockstore.go | 6 +-- share/eds/metrics.go | 22 -------- share/eds/store.go | 25 +++++---- share/eds/store_test.go | 20 ++++--- 5 files changed, 97 insertions(+), 77 deletions(-) diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go index a9aa3d3742..9d2144a4e8 100644 --- a/share/eds/accessor_cache.go +++ b/share/eds/accessor_cache.go @@ -14,43 +14,49 @@ import ( "go.opentelemetry.io/otel/metric" ) +const cacheFoundKey = "found" + var ( errCacheMiss = errors.New("accessor not found in blockstore cache") ) // cache is an interface that defines the basic cache operations. type cache interface { - // Get retrieves an item from the cache. + // get retrieves an item from the cache. get(shard.Key) (*accessorWithBlockstore, error) // getOrLoad attempts to get an item from the cache and, if not found, invokes // the provided loader function to load it into the cache. getOrLoad(ctx context.Context, key shard.Key, loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error)) (*accessorWithBlockstore, error) + // remove removes an item from cache. + remove(shard.Key) error + // enableMetrics enables metrics in cache enableMetrics() error } // multiCache represents a cache that looks into multiple caches one by one. type multiCache struct { - caches []cache + recentBlocks cache + ipldRequestedBlocks cache } // newMultiCache creates a new multiCache with the provided caches. -func newMultiCache(caches ...cache) *multiCache { - return &multiCache{caches: caches} +func newMultiCache(recentBlocks, ipldRequestedBlocks cache) *multiCache { + return &multiCache{ + recentBlocks: recentBlocks, + ipldRequestedBlocks: ipldRequestedBlocks, + } } // get looks for an item in all the caches one by one and returns the first found item. func (mc *multiCache) get(key shard.Key) (*accessorWithBlockstore, error) { - for _, cache := range mc.caches { - accessor, err := cache.get(key) - if err == nil { - return accessor, nil - } + ac, err := mc.recentBlocks.get(key) + if err == nil { + return ac, nil } - - return nil, errCacheMiss + return mc.ipldRequestedBlocks.get(key) } // getOrLoad attempts to get an item from all caches, and if not found, invokes @@ -60,24 +66,26 @@ func (mc *multiCache) getOrLoad( key shard.Key, loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), ) (*accessorWithBlockstore, error) { - for _, cache := range mc.caches { - accessor, err := cache.getOrLoad(ctx, key, loader) - if err == nil { - return accessor, nil - } + ac, err := mc.recentBlocks.getOrLoad(ctx, key, loader) + if err == nil { + return ac, nil } + return mc.ipldRequestedBlocks.getOrLoad(ctx, key, loader) +} - return nil, errors.New("multicache: unable to get or load accessor") +// remove removes an item from all underlying caches +func (mc *multiCache) remove(key shard.Key) error { + if err := mc.recentBlocks.remove(key); err != nil { + return err + } + return mc.ipldRequestedBlocks.remove(key) } func (mc *multiCache) enableMetrics() error { - for _, cache := range mc.caches { - err := cache.enableMetrics() - if err != nil { - return err - } + if err := mc.recentBlocks.enableMetrics(); err != nil { + return err } - return nil + return mc.ipldRequestedBlocks.enableMetrics() } // accessorWithBlockstore is the value that we store in the blockstore cache @@ -103,7 +111,9 @@ type blockstoreCache struct { } func newBlockstoreCache(name string, cacheSize int) (*blockstoreCache, error) { - bc := &blockstoreCache{} + bc := &blockstoreCache{ + name: name, + } // instantiate the blockstore cache bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) if err != nil { @@ -146,6 +156,7 @@ func (bc *blockstoreCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, er // We've already ensured that the given shard has the cid/multihash we are looking for. val, ok := bc.cache.Get(key) if !ok { + bc.metrics.observeGet(false) return nil, errCacheMiss } @@ -156,6 +167,7 @@ func (bc *blockstoreCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, er reflect.TypeOf(val), )) } + bc.metrics.observeGet(true) return accessor, nil } @@ -192,6 +204,20 @@ func (bc *blockstoreCache) getOrLoad( return newAccessor, nil } +func (bc *blockstoreCache) remove(key shard.Key) error { + lk := &bc.stripedLocks[shardKeyToStriped(key)] + lk.Lock() + defer lk.Unlock() + + ac, err := bc.unsafeGet(key) + if err != nil { + // nothing to remove + return nil + } + + return ac.sa.Close() +} + // shardKeyToStriped returns the index of the lock to use for a given shard key. We use the last // byte of the shard key as the pseudo-random index. func shardKeyToStriped(sk shard.Key) byte { @@ -199,17 +225,24 @@ func shardKeyToStriped(sk shard.Key) byte { } type cacheMetrics struct { + getCounter metric.Int64Counter evictedCounter metric.Int64Counter } func (bc *blockstoreCache) enableMetrics() error { - evictedCounter, err := meter.Int64Counter("eds_blockstore_cache"+bc.name+"_evicted_counter", + evictedCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_evicted_counter", metric.WithDescription("eds blockstore cache evicted event counter")) if err != nil { return err } - cacheSize, err := meter.Int64ObservableGauge("eds_blockstore"+bc.name+"_cache_size", + getCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_get_counter", + metric.WithDescription("eds blockstore cache evicted event counter")) + if err != nil { + return err + } + + cacheSize, err := meter.Int64ObservableGauge("eds_blockstore_cache_"+bc.name+"_size", metric.WithDescription("total amount of items in blockstore cache"), ) if err != nil { @@ -224,7 +257,9 @@ func (bc *blockstoreCache) enableMetrics() error { if err != nil { return err } - bc.metrics = &cacheMetrics{evictedCounter: evictedCounter} + bc.metrics = &cacheMetrics{ + getCounter: getCounter, + evictedCounter: evictedCounter} return nil } @@ -236,6 +271,14 @@ func (m *cacheMetrics) observeEvicted(failed bool) { attribute.Bool(failedKey, failed))) } +func (m *cacheMetrics) observeGet(found bool) { + if m == nil { + return + } + m.getCounter.Add(context.Background(), 1, metric.WithAttributes( + attribute.Bool(cacheFoundKey, found))) +} + type noopCache struct{} func (n noopCache) get(shard.Key) (*accessorWithBlockstore, error) { @@ -249,6 +292,10 @@ func (n noopCache) getOrLoad( return nil, nil } +func (n noopCache) remove(shard.Key) error { + return nil +} + func (n noopCache) enableMetrics() error { return nil } diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index ddf94f4d26..50e48f65f6 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -29,13 +29,11 @@ var ( // implementation to allow for the blockstore operations to be routed to the underlying stores. type blockstore struct { store *Store - cache *blockstoreCache } -func newBlockstore(store *Store, cache *blockstoreCache) *blockstore { +func newBlockstore(store *Store) *blockstore { return &blockstore{ store: store, - cache: cache, } } @@ -128,7 +126,7 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d // a share can exist in multiple EDSes, so just take the first one. shardKey := keys[0] - accessor, err := bs.cache.getOrLoad(ctx, shardKey, bs.store.getAccessor) + accessor, err := bs.store.cache.ipldRequestedBlocks.getOrLoad(ctx, shardKey, bs.store.getAccessor) if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) } diff --git a/share/eds/metrics.go b/share/eds/metrics.go index 8da7b6300b..f9186c5379 100644 --- a/share/eds/metrics.go +++ b/share/eds/metrics.go @@ -12,7 +12,6 @@ import ( const ( failedKey = "failed" sizeKey = "eds_size" - cachedKey = "cached" putResultKey = "result" putOK putResult = "ok" @@ -43,7 +42,6 @@ type metrics struct { getTime metric.Float64Histogram hasTime metric.Float64Histogram listTime metric.Float64Histogram - getAccessorTime metric.Float64Histogram longOpTime metric.Float64Histogram gcTime metric.Float64Histogram @@ -98,12 +96,6 @@ func (s *Store) WithMetrics() error { return err } - getAccessorTime, err := meter.Float64Histogram("eds_store_get_accessor_time_histogram", - metric.WithDescription("eds store get accessor time histogram(s)")) - if err != nil { - return err - } - longOpTime, err := meter.Float64Histogram("eds_store_long_operation_time_histogram", metric.WithDescription("eds store long operation time histogram(s)")) if err != nil { @@ -128,7 +120,6 @@ func (s *Store) WithMetrics() error { getTime: getTime, hasTime: hasTime, listTime: listTime, - getAccessorTime: getAccessorTime, longOpTime: longOpTime, gcTime: gcTime, } @@ -255,16 +246,3 @@ func (m *metrics) observeList(ctx context.Context, dur time.Duration, failed boo m.listTime.Record(ctx, dur.Seconds(), metric.WithAttributes( attribute.Bool(failedKey, failed))) } - -func (m *metrics) observeGetAccessor(ctx context.Context, dur time.Duration, cached, failed bool) { - if m == nil { - return - } - if ctx.Err() != nil { - ctx = context.Background() - } - - m.getAccessorTime.Record(ctx, dur.Seconds(), metric.WithAttributes( - attribute.Bool(cachedKey, cached), - attribute.Bool(failedKey, failed))) -} diff --git a/share/eds/store.go b/share/eds/store.go index 9af72c6134..b9bfb78240 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -55,9 +55,7 @@ type Store struct { dgstr *dagstore.DAGStore mounts *mount.Registry - - cache cache - bs bstore.Blockstore + cache *multiCache carIdx index.FullIndexRepo invertedIdx *simpleInvertedIndex @@ -118,7 +116,7 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { return nil, fmt.Errorf("failed to create blockstore cache: %w", err) } - store := &Store{ + return &Store{ basepath: basepath, dgstr: dagStore, carIdx: fsRepo, @@ -126,9 +124,7 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { gcInterval: defaultGCInterval, mounts: r, cache: newMultiCache(recentBlocksCache, blockstoreCache), - } - store.bs = newBlockstore(store, blockstoreCache) - return store, nil + }, nil } func (s *Store) Start(ctx context.Context) error { @@ -310,7 +306,7 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, err if err == nil { return accessor.sa.Reader(), nil } - // if accessor not found in cache, create new one from dagstore + // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared to other cache put triggers, that the same block to be requested again. shardAccessor, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) @@ -323,7 +319,7 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, err // blocks. We represent `shares` and NMT Merkle proofs as IPFS blocks and IPLD nodes so Bitswap can // access those. func (s *Store) Blockstore() bstore.Blockstore { - return s.bs + return newBlockstore(s) } // CARBlockstore returns an IPFS Blockstore providing access to individual shares/nodes of a @@ -439,9 +435,12 @@ func (s *Store) Remove(ctx context.Context, root share.DataHash) error { } func (s *Store) remove(ctx context.Context, root share.DataHash) (err error) { - key := root.String() + key := shard.KeyFromString(root.String()) + if err := s.cache.remove(key); err != nil { + log.Warnw("remove accessor from cache", "err", err) + } ch := make(chan dagstore.ShardResult, 1) - err = s.dgstr.DestroyShard(ctx, shard.KeyFromString(key), ch, dagstore.DestroyOpts{}) + err = s.dgstr.DestroyShard(ctx, key, ch, dagstore.DestroyOpts{}) if err != nil { return fmt.Errorf("failed to initiate shard destruction: %w", err) } @@ -456,7 +455,7 @@ func (s *Store) remove(ctx context.Context, root share.DataHash) (err error) { return ctx.Err() } - dropped, err := s.carIdx.DropFullIndex(shard.KeyFromString(key)) + dropped, err := s.carIdx.DropFullIndex(key) if !dropped { log.Warnf("failed to drop index for %s", key) } @@ -464,7 +463,7 @@ func (s *Store) remove(ctx context.Context, root share.DataHash) (err error) { return fmt.Errorf("failed to drop index for %s: %w", key, err) } - err = os.Remove(s.basepath + blocksPath + key) + err = os.Remove(s.basepath + blocksPath + root.String()) if err != nil { return fmt.Errorf("failed to remove CAR file: %w", err) } diff --git a/share/eds/store_test.go b/share/eds/store_test.go index bcdf06e152..5aab0cc9ce 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -138,22 +138,15 @@ func TestEDSStore(t *testing.T) { assert.True(t, ok) }) - t.Run("BlockstoreCache", func(t *testing.T) { + t.Run("RecentBlocksCache", func(t *testing.T) { eds, dah := randomEDS(t) - err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // key isnt in cache yet, so get returns errCacheMiss + // check, that the key is in the cache after put shardKey := shard.KeyFromString(dah.String()) _, err = edsStore.cache.get(shardKey) - assert.ErrorIs(t, err, errCacheMiss) - - // now get it, so that the key is in the cache - _, err = edsStore.CARBlockstore(ctx, dah.Hash()) assert.NoError(t, err) - _, err = edsStore.cache.get(shardKey) - assert.NoError(t, err, errCacheMiss) }) t.Run("List", func(t *testing.T) { @@ -193,6 +186,11 @@ func TestEDSStore_GC(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) + // remove links to the shard from cache + key := shard.KeyFromString(share.DataHash(dah.Hash()).String()) + err = edsStore.cache.remove(key) + require.NoError(t, err) + // doesn't exist yet assert.NotContains(t, edsStore.lastGCResult.Load().Shards, shardKey) @@ -220,7 +218,7 @@ func Test_BlockstoreCache(t *testing.T) { // store eds to the store with noopCache to allow clean cache after put swap := edsStore.cache - edsStore.cache = noopCache{} + edsStore.cache = newMultiCache(noopCache{}, noopCache{}) eds, dah := randomEDS(t) err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) @@ -246,7 +244,7 @@ func Test_BlockstoreCache(t *testing.T) { require.ErrorIs(t, err, errCacheMiss) // now get it, so that the key is in the cache - _, err = edsStore.bs.Get(ctx, key) + _, err = edsStore.Blockstore().Get(ctx, key) require.NoError(t, err) _, err = edsStore.cache.get(shardKey) require.NoError(t, err) From 045a0c6f5814e440db65129c35ea220b1f04e898 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 29 Aug 2023 09:35:50 +0300 Subject: [PATCH 03/14] i love linter --- share/eds/accessor_cache.go | 6 +++++- share/eds/store.go | 7 +++++-- share/eds/store_test.go | 3 ++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go index 9d2144a4e8..a57bc724e4 100644 --- a/share/eds/accessor_cache.go +++ b/share/eds/accessor_cache.go @@ -27,7 +27,11 @@ type cache interface { // getOrLoad attempts to get an item from the cache and, if not found, invokes // the provided loader function to load it into the cache. - getOrLoad(ctx context.Context, key shard.Key, loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error)) (*accessorWithBlockstore, error) + getOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), + ) (*accessorWithBlockstore, error) // remove removes an item from cache. remove(shard.Key) error diff --git a/share/eds/store.go b/share/eds/store.go index b9bfb78240..e927a52644 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -248,7 +248,8 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to register shard: %w", result.Error) } - // return accessor in result will be always nil, need to aquire shard to make it available in cache + // accessor returned in result will be always nil, so shard needs to be acquired first, to become + // available in cache if _, err := s.cache.getOrLoad(ctx, result.Key, s.getAccessor); err != nil { log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) } @@ -306,7 +307,9 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, err if err == nil { return accessor.sa.Reader(), nil } - // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared to other cache put triggers, that the same block to be requested again. + // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor + // to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared + // to other cache put triggers, that the same block to be requested again. shardAccessor, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 5aab0cc9ce..0dfcb9b980 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -2,11 +2,12 @@ package eds import ( "context" - "github.com/ipfs/go-cid" "os" "testing" "time" + "github.com/ipfs/go-cid" + "github.com/filecoin-project/dagstore/shard" "github.com/ipfs/go-datastore" ds_sync "github.com/ipfs/go-datastore/sync" From b52c89b4cf4789d64c21d87a4b321b88759ade18 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 29 Aug 2023 11:40:41 +0300 Subject: [PATCH 04/14] - async put into cache - fix merge conflicts --- share/eds/accessor_cache.go | 7 +++---- share/eds/store.go | 25 +++++++++++++++++-------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go index a57bc724e4..bfaba46335 100644 --- a/share/eds/accessor_cache.go +++ b/share/eds/accessor_cache.go @@ -79,10 +79,9 @@ func (mc *multiCache) getOrLoad( // remove removes an item from all underlying caches func (mc *multiCache) remove(key shard.Key) error { - if err := mc.recentBlocks.remove(key); err != nil { - return err - } - return mc.ipldRequestedBlocks.remove(key) + err1 := mc.recentBlocks.remove(key) + err2 := mc.ipldRequestedBlocks.remove(key) + return errors.Join(err1, err2) } func (mc *multiCache) enableMetrics() error { diff --git a/share/eds/store.go b/share/eds/store.go index 8f007cf8f8..33a3520927 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -55,7 +55,9 @@ type Store struct { dgstr *dagstore.DAGStore mounts *mount.Registry - cache *multiCache + + bs *blockstore + cache *multiCache carIdx index.FullIndexRepo invertedIdx *simpleInvertedIndex @@ -125,7 +127,7 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { mounts: r, cache: newMultiCache(recentBlocksCache, blockstoreCache), } - store.bs = newBlockstore(store, cache, ds) + store.bs = newBlockstore(store, ds) return store, nil } @@ -250,11 +252,18 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to register shard: %w", result.Error) } - // accessor returned in result will be always nil, so shard needs to be acquired first, to become - // available in cache - if _, err := s.cache.getOrLoad(ctx, result.Key, s.getAccessor); err != nil { - log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) - } + //accessor returned in result will be nil, so shard needs to be acquired first, to become + // available in cache. It might take some time and result should not affect put operation, so do it + // in goroutine + //TODO: Ideally only recent blocks should be put in cache, but there is no way right now to check + //such condition. + go func() { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + if _, err := s.cache.getOrLoad(ctx, result.Key, s.getAccessor); err != nil { + log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) + } + }() return nil } @@ -324,7 +333,7 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, err // blocks. We represent `shares` and NMT Merkle proofs as IPFS blocks and IPLD nodes so Bitswap can // access those. func (s *Store) Blockstore() bstore.Blockstore { - return newBlockstore(s) + return s.bs } // CARBlockstore returns an IPFS Blockstore providing access to individual shares/nodes of a From c6e769315328c340fd11bac6fe1a66b0d7e8ca0a Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 29 Aug 2023 12:04:50 +0300 Subject: [PATCH 05/14] sort imports --- Makefile | 4 ++-- share/eds/store.go | 4 ++-- share/eds/store_test.go | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index feef6172aa..741cf5c1bf 100644 --- a/Makefile +++ b/Makefile @@ -161,14 +161,14 @@ openrpc-gen: lint-imports: @echo "--> Running imports linter" @for file in `find . -type f -name '*.go'`; \ - do goimports-reviser -list-diff -set-exit-status -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/"$(PROJECTNAME)"" -output stdout $$file \ + do goimports-reviser -list-diff -set-exit-status -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/celestia-node" -output stdout $$file \ || exit 1; \ done; .PHONY: lint-imports ## sort-imports: Sort Go imports. sort-imports: - @goimports-reviser -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/"$(PROJECTNAME)"" -output stdout . + @goimports-reviser -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/celestia-node" -output stdout . .PHONY: sort-imports ## adr-gen: Generate ADR from template. Must set NUM and TITLE parameters. diff --git a/share/eds/store.go b/share/eds/store.go index 33a3520927..9fb76e6fb8 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -252,11 +252,11 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to register shard: %w", result.Error) } - //accessor returned in result will be nil, so shard needs to be acquired first, to become + // accessor returned in result will be nil, so shard needs to be acquired first, to become // available in cache. It might take some time and result should not affect put operation, so do it // in goroutine //TODO: Ideally only recent blocks should be put in cache, but there is no way right now to check - //such condition. + // such condition. go func() { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 0dfcb9b980..37941a018c 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -6,9 +6,8 @@ import ( "testing" "time" - "github.com/ipfs/go-cid" - "github.com/filecoin-project/dagstore/shard" + "github.com/ipfs/go-cid" "github.com/ipfs/go-datastore" ds_sync "github.com/ipfs/go-datastore/sync" "github.com/ipld/go-car" From 0a2354f30820795edda844c507c77781fe877a3a Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 31 Aug 2023 13:47:32 +0300 Subject: [PATCH 06/14] refactor chache into pkg --- Makefile | 4 +- nodebuilder/blob/mocks/api.go | 4 +- nodebuilder/fraud/mocks/api.go | 4 +- nodebuilder/store_test.go | 1 + share/eds/accessor_cache.go | 304 ------------------------- share/eds/blockstore.go | 16 +- share/eds/blockstore_test.go | 3 + share/eds/cache/accessor_cache.go | 170 ++++++++++++++ share/eds/cache/accessor_cache_test.go | 239 +++++++++++++++++++ share/eds/cache/cache.go | 87 +++++++ share/eds/cache/metrics.go | 61 +++++ share/eds/cache/multicache.go | 60 +++++ share/eds/metrics.go | 2 +- share/eds/ods_test.go | 6 + share/eds/store.go | 68 ++++-- share/eds/store_test.go | 24 +- share/getter.go | 3 +- share/getters/store.go | 10 + share/p2p/shrexeds/server.go | 10 +- 19 files changed, 726 insertions(+), 350 deletions(-) delete mode 100644 share/eds/accessor_cache.go create mode 100644 share/eds/cache/accessor_cache.go create mode 100644 share/eds/cache/accessor_cache_test.go create mode 100644 share/eds/cache/cache.go create mode 100644 share/eds/cache/metrics.go create mode 100644 share/eds/cache/multicache.go diff --git a/Makefile b/Makefile index 741cf5c1bf..feef6172aa 100644 --- a/Makefile +++ b/Makefile @@ -161,14 +161,14 @@ openrpc-gen: lint-imports: @echo "--> Running imports linter" @for file in `find . -type f -name '*.go'`; \ - do goimports-reviser -list-diff -set-exit-status -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/celestia-node" -output stdout $$file \ + do goimports-reviser -list-diff -set-exit-status -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/"$(PROJECTNAME)"" -output stdout $$file \ || exit 1; \ done; .PHONY: lint-imports ## sort-imports: Sort Go imports. sort-imports: - @goimports-reviser -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/celestia-node" -output stdout . + @goimports-reviser -company-prefixes "github.com/celestiaorg" -project-name "github.com/celestiaorg/"$(PROJECTNAME)"" -output stdout . .PHONY: sort-imports ## adr-gen: Generate ADR from template. Must set NUM and TITLE parameters. diff --git a/nodebuilder/blob/mocks/api.go b/nodebuilder/blob/mocks/api.go index e8d9f322b5..5cd34b74b6 100644 --- a/nodebuilder/blob/mocks/api.go +++ b/nodebuilder/blob/mocks/api.go @@ -40,7 +40,7 @@ func (m *MockModule) EXPECT() *MockModuleMockRecorder { // Get mocks base method. func (m *MockModule) Get(arg0 context.Context, arg1 uint64, arg2 share.Namespace, arg3 blob.Commitment) (*blob.Blob, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "get", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*blob.Blob) ret1, _ := ret[1].(error) return ret0, ret1 @@ -49,7 +49,7 @@ func (m *MockModule) Get(arg0 context.Context, arg1 uint64, arg2 share.Namespace // Get indicates an expected call of Get. func (mr *MockModuleMockRecorder) Get(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1, arg2, arg3) } // GetAll mocks base method. diff --git a/nodebuilder/fraud/mocks/api.go b/nodebuilder/fraud/mocks/api.go index 0936a2abb7..399f8746e1 100644 --- a/nodebuilder/fraud/mocks/api.go +++ b/nodebuilder/fraud/mocks/api.go @@ -40,7 +40,7 @@ func (m *MockModule) EXPECT() *MockModuleMockRecorder { // Get mocks base method. func (m *MockModule) Get(arg0 context.Context, arg1 fraud0.ProofType) ([]fraud.Proof, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "get", arg0, arg1) + ret := m.ctrl.Call(m, "Get", arg0, arg1) ret0, _ := ret[0].([]fraud.Proof) ret1, _ := ret[1].(error) return ret0, ret1 @@ -49,7 +49,7 @@ func (m *MockModule) Get(arg0 context.Context, arg1 fraud0.ProofType) ([]fraud.P // Get indicates an expected call of Get. func (mr *MockModuleMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockModule)(nil).Get), arg0, arg1) } // Subscribe mocks base method. diff --git a/nodebuilder/store_test.go b/nodebuilder/store_test.go index 8a39849060..8ea56a280f 100644 --- a/nodebuilder/store_test.go +++ b/nodebuilder/store_test.go @@ -154,6 +154,7 @@ func TestStoreRestart(t *testing.T) { require.NoError(t, err) _, err = eds.ReadEDS(ctx, odsReader, h) require.NoError(t, err) + require.NoError(t, edsReader.Close()) } } diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go deleted file mode 100644 index bfaba46335..0000000000 --- a/share/eds/accessor_cache.go +++ /dev/null @@ -1,304 +0,0 @@ -package eds - -import ( - "context" - "errors" - "fmt" - "reflect" - "sync" - - "github.com/filecoin-project/dagstore" - "github.com/filecoin-project/dagstore/shard" - lru "github.com/hashicorp/golang-lru" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/metric" -) - -const cacheFoundKey = "found" - -var ( - errCacheMiss = errors.New("accessor not found in blockstore cache") -) - -// cache is an interface that defines the basic cache operations. -type cache interface { - // get retrieves an item from the cache. - get(shard.Key) (*accessorWithBlockstore, error) - - // getOrLoad attempts to get an item from the cache and, if not found, invokes - // the provided loader function to load it into the cache. - getOrLoad( - ctx context.Context, - key shard.Key, - loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), - ) (*accessorWithBlockstore, error) - - // remove removes an item from cache. - remove(shard.Key) error - - // enableMetrics enables metrics in cache - enableMetrics() error -} - -// multiCache represents a cache that looks into multiple caches one by one. -type multiCache struct { - recentBlocks cache - ipldRequestedBlocks cache -} - -// newMultiCache creates a new multiCache with the provided caches. -func newMultiCache(recentBlocks, ipldRequestedBlocks cache) *multiCache { - return &multiCache{ - recentBlocks: recentBlocks, - ipldRequestedBlocks: ipldRequestedBlocks, - } -} - -// get looks for an item in all the caches one by one and returns the first found item. -func (mc *multiCache) get(key shard.Key) (*accessorWithBlockstore, error) { - ac, err := mc.recentBlocks.get(key) - if err == nil { - return ac, nil - } - return mc.ipldRequestedBlocks.get(key) -} - -// getOrLoad attempts to get an item from all caches, and if not found, invokes -// the provided loader function to load it into one of the caches. -func (mc *multiCache) getOrLoad( - ctx context.Context, - key shard.Key, - loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), -) (*accessorWithBlockstore, error) { - ac, err := mc.recentBlocks.getOrLoad(ctx, key, loader) - if err == nil { - return ac, nil - } - return mc.ipldRequestedBlocks.getOrLoad(ctx, key, loader) -} - -// remove removes an item from all underlying caches -func (mc *multiCache) remove(key shard.Key) error { - err1 := mc.recentBlocks.remove(key) - err2 := mc.ipldRequestedBlocks.remove(key) - return errors.Join(err1, err2) -} - -func (mc *multiCache) enableMetrics() error { - if err := mc.recentBlocks.enableMetrics(); err != nil { - return err - } - return mc.ipldRequestedBlocks.enableMetrics() -} - -// accessorWithBlockstore is the value that we store in the blockstore cache -type accessorWithBlockstore struct { - sa *dagstore.ShardAccessor - // blockstore is stored separately because each access to the blockstore over the shard accessor - // reopens the underlying CAR. - bs dagstore.ReadBlockstore -} - -type blockstoreCache struct { - // name is a prefix, that will be used for cache metrics if it is enabled - name string - // stripedLocks prevents simultaneous RW access to the blockstore cache for a shard. Instead - // of using only one lock or one lock per key, we stripe the shard keys across 256 locks. 256 is - // chosen because it 0-255 is the range of values we get looking at the last byte of the key. - stripedLocks [256]sync.Mutex - // caches the blockstore for a given shard for shard read affinity i.e. - // further reads will likely be from the same shard. Maps (shard key -> blockstore). - cache *lru.Cache - - metrics *cacheMetrics -} - -func newBlockstoreCache(name string, cacheSize int) (*blockstoreCache, error) { - bc := &blockstoreCache{ - name: name, - } - // instantiate the blockstore cache - bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) - if err != nil { - return nil, fmt.Errorf("failed to instantiate blockstore cache: %w", err) - } - bc.cache = bslru - return bc, nil -} - -func (bc *blockstoreCache) evictFn() func(_ interface{}, val interface{}) { - return func(_ interface{}, val interface{}) { - // ensure we close the blockstore for a shard when it's evicted so dagstore can gc it. - abs, ok := val.(*accessorWithBlockstore) - if !ok { - panic(fmt.Sprintf( - "casting value from cache to accessorWithBlockstore: %s", - reflect.TypeOf(val), - )) - } - - err := abs.sa.Close() - if err != nil { - log.Errorf("couldn't close accessor after cache eviction: %s", err) - } - bc.metrics.observeEvicted(err != nil) - } -} - -// Get retrieves the blockstore for a given shard key from the cache. If the blockstore is not in -// the cache, it returns an errCacheMiss -func (bc *blockstoreCache) get(key shard.Key) (*accessorWithBlockstore, error) { - lk := &bc.stripedLocks[shardKeyToStriped(key)] - lk.Lock() - defer lk.Unlock() - - return bc.unsafeGet(key) -} - -func (bc *blockstoreCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, error) { - // We've already ensured that the given shard has the cid/multihash we are looking for. - val, ok := bc.cache.Get(key) - if !ok { - bc.metrics.observeGet(false) - return nil, errCacheMiss - } - - accessor, ok := val.(*accessorWithBlockstore) - if !ok { - panic(fmt.Sprintf( - "casting value from cache to accessorWithBlockstore: %s", - reflect.TypeOf(val), - )) - } - bc.metrics.observeGet(true) - return accessor, nil -} - -// getOrLoad attempts to get an item from all caches, and if not found, invokes -// the provided loader function to load it into one of the caches. -func (bc *blockstoreCache) getOrLoad( - ctx context.Context, - key shard.Key, - loader func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), -) (*accessorWithBlockstore, error) { - lk := &bc.stripedLocks[shardKeyToStriped(key)] - lk.Lock() - defer lk.Unlock() - - if accessor, err := bc.unsafeGet(key); err == nil { - return accessor, nil - } - - accessor, err := loader(ctx, key) - if err != nil { - return nil, fmt.Errorf("unable to get accessor: %w", err) - } - - blockStore, err := accessor.Blockstore() - if err != nil { - return nil, fmt.Errorf("failed to get blockstore from accessor: %w", err) - } - - newAccessor := &accessorWithBlockstore{ - bs: blockStore, - sa: accessor, - } - bc.cache.Add(key, newAccessor) - return newAccessor, nil -} - -func (bc *blockstoreCache) remove(key shard.Key) error { - lk := &bc.stripedLocks[shardKeyToStriped(key)] - lk.Lock() - defer lk.Unlock() - - ac, err := bc.unsafeGet(key) - if err != nil { - // nothing to remove - return nil - } - - return ac.sa.Close() -} - -// shardKeyToStriped returns the index of the lock to use for a given shard key. We use the last -// byte of the shard key as the pseudo-random index. -func shardKeyToStriped(sk shard.Key) byte { - return sk.String()[len(sk.String())-1] -} - -type cacheMetrics struct { - getCounter metric.Int64Counter - evictedCounter metric.Int64Counter -} - -func (bc *blockstoreCache) enableMetrics() error { - evictedCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_evicted_counter", - metric.WithDescription("eds blockstore cache evicted event counter")) - if err != nil { - return err - } - - getCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_get_counter", - metric.WithDescription("eds blockstore cache evicted event counter")) - if err != nil { - return err - } - - cacheSize, err := meter.Int64ObservableGauge("eds_blockstore_cache_"+bc.name+"_size", - metric.WithDescription("total amount of items in blockstore cache"), - ) - if err != nil { - return err - } - - callback := func(ctx context.Context, observer metric.Observer) error { - observer.ObserveInt64(cacheSize, int64(bc.cache.Len())) - return nil - } - _, err = meter.RegisterCallback(callback, cacheSize) - if err != nil { - return err - } - bc.metrics = &cacheMetrics{ - getCounter: getCounter, - evictedCounter: evictedCounter} - return nil -} - -func (m *cacheMetrics) observeEvicted(failed bool) { - if m == nil { - return - } - m.evictedCounter.Add(context.Background(), 1, metric.WithAttributes( - attribute.Bool(failedKey, failed))) -} - -func (m *cacheMetrics) observeGet(found bool) { - if m == nil { - return - } - m.getCounter.Add(context.Background(), 1, metric.WithAttributes( - attribute.Bool(cacheFoundKey, found))) -} - -type noopCache struct{} - -func (n noopCache) get(shard.Key) (*accessorWithBlockstore, error) { - return nil, errCacheMiss -} - -func (n noopCache) getOrLoad( - context.Context, shard.Key, - func(context.Context, shard.Key) (*dagstore.ShardAccessor, error), -) (*accessorWithBlockstore, error) { - return nil, nil -} - -func (n noopCache) remove(shard.Key) error { - return nil -} - -func (n noopCache) enableMetrics() error { - return nil -} diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 3f4cb569e3..f4da4d5774 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -13,6 +13,8 @@ import ( "github.com/ipfs/go-datastore/namespace" dshelp "github.com/ipfs/go-ipfs-ds-help" ipld "github.com/ipfs/go-ipld-format" + + "github.com/celestiaorg/celestia-node/share/eds/cache" ) var _ bstore.Blockstore = (*blockstore)(nil) @@ -32,12 +34,14 @@ var ( // implementation to allow for the blockstore operations to be routed to the underlying stores. type blockstore struct { store *Store + cache cache.Cache ds datastore.Batching } -func newBlockstore(store *Store, ds datastore.Batching) *blockstore { +func newBlockstore(store *Store, cache cache.Cache, ds datastore.Batching) *blockstore { return &blockstore{ store: store, + cache: cache, ds: namespace.Wrap(ds, blockstoreCacheKey), } } @@ -146,18 +150,18 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d return nil, fmt.Errorf("failed to find shards containing multihash: %w", err) } - // check hash for any of keys + // a share can exist in multiple EDSes, check cache to contain any of accessors containing shard for _, k := range keys { - if accessor, err := bs.store.cache.get(k); err == nil { - return accessor.bs, nil + if accessor, err := bs.store.cache.Get(k); err == nil { + return accessor.Blockstore() } } // a share can exist in multiple EDSes, so just take the first one. shardKey := keys[0] - accessor, err := bs.store.cache.ipldRequestedBlocks.getOrLoad(ctx, shardKey, bs.store.getAccessor) + accessor, err := bs.cache.GetOrLoad(ctx, shardKey, bs.store.getAccessor) if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) } - return accessor.bs, nil + return accessor.Blockstore() } diff --git a/share/eds/blockstore_test.go b/share/eds/blockstore_test.go index 745797bf42..d9dbf7ed30 100644 --- a/share/eds/blockstore_test.go +++ b/share/eds/blockstore_test.go @@ -37,6 +37,9 @@ func TestBlockstore_Operations(t *testing.T) { topLevelBS := edsStore.Blockstore() carBS, err := edsStore.CARBlockstore(ctx, dah.Hash()) require.NoError(t, err) + defer func() { + require.NoError(t, carBS.Close()) + }() root, err := edsStore.GetDAH(ctx, dah.Hash()) require.NoError(t, err) diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go new file mode 100644 index 0000000000..f8fb36080b --- /dev/null +++ b/share/eds/cache/accessor_cache.go @@ -0,0 +1,170 @@ +package cache + +import ( + "context" + "fmt" + "io" + "reflect" + "sync" + + "github.com/filecoin-project/dagstore" + "github.com/filecoin-project/dagstore/shard" + lru "github.com/hashicorp/golang-lru" +) + +var _ Cache = (*AccessorCache)(nil) + +type AccessorCache struct { + // name is a prefix, that will be used for cache metrics if it is enabled + name string + // stripedLocks prevents simultaneous RW access to the blockstore cache for a shard. Instead + // of using only one lock or one lock per key, we stripe the shard keys across 256 locks. 256 is + // chosen because it 0-255 is the range of values we get looking at the last byte of the key. + stripedLocks [256]sync.Mutex + // caches the blockstore for a given shard for shard read affinity i.e. + // further reads will likely be from the same shard. Maps (shard key -> blockstore). + cache *lru.Cache + + metrics *metrics +} + +// accessorWithBlockstore is the value that we store in the blockstore Cache. Implements Accessor +// interface +type accessorWithBlockstore struct { + sync.Mutex + sa AccessorProvider + // blockstore is stored separately because each access to the blockstore over the shard accessor + // reopens the underlying CAR. + bs dagstore.ReadBlockstore +} + +func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { + bc := &AccessorCache{ + name: name, + } + // instantiate the blockstore Cache + bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) + if err != nil { + return nil, fmt.Errorf("failed to instantiate blockstore cache: %w", err) + } + bc.cache = bslru + return bc, nil +} + +func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { + return func(_ interface{}, val interface{}) { + // ensure we close the blockstore for a shard when it's evicted so dagstore can gc it. + abs, ok := val.(*accessorWithBlockstore) + if !ok { + panic(fmt.Sprintf( + "casting value from cache to accessorWithBlockstore: %s", + reflect.TypeOf(val), + )) + } + + err := abs.sa.Close() + if err != nil { + log.Errorf("couldn't close accessor after cache eviction: %s", err) + } + bc.metrics.observeEvicted(err != nil) + } +} + +// Get retrieves the blockstore for a given shard key from the Cache. If the blockstore is not in +// the Cache, it returns an errCacheMiss +func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { + lk := &bc.stripedLocks[shardKeyToStriped(key)] + lk.Lock() + defer lk.Unlock() + + return bc.unsafeGet(key) +} + +func (bc *AccessorCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, error) { + // We've already ensured that the given shard has the cid/multihash we are looking for. + val, ok := bc.cache.Get(key) + if !ok { + bc.metrics.observeGet(false) + return nil, ErrCacheMiss + } + + accessor, ok := val.(*accessorWithBlockstore) + if !ok { + panic(fmt.Sprintf( + "casting value from cache to accessorWithBlockstore: %s", + reflect.TypeOf(val), + )) + } + bc.metrics.observeGet(true) + return accessor, nil +} + +// GetOrLoad attempts to get an item from all caches, and if not found, invokes +// the provided loader function to load it into one of the caches. +func (bc *AccessorCache) GetOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (AccessorProvider, error), +) (Accessor, error) { + lk := &bc.stripedLocks[shardKeyToStriped(key)] + lk.Lock() + defer lk.Unlock() + + if accessor, err := bc.unsafeGet(key); err == nil { + return accessor, nil + } + + accessor, err := loader(ctx, key) + if err != nil { + return nil, fmt.Errorf("unable to get accessor: %w", err) + } + + newAccessor := &accessorWithBlockstore{ + sa: accessor, + } + bc.cache.Add(key, newAccessor) + return newAccessor, nil +} + +func (bc *AccessorCache) Remove(key shard.Key) error { + lk := &bc.stripedLocks[shardKeyToStriped(key)] + lk.Lock() + defer lk.Unlock() + + bc.cache.Remove(key) + return nil +} + +func (bc *AccessorCache) EnableMetrics() error { + var err error + bc.metrics, err = newMetrics(bc) + return err +} + +// ReadCloser implements ReadCloser of the Accessor interface, using noop Closer to disallow user +// from manually closing. Close will be performed on accessor on cache eviction. +func (s *accessorWithBlockstore) ReadCloser() io.ReadCloser { + return io.NopCloser(s.sa.Reader()) +} + +// Blockstore implements Blockstore of the Accessor interface. It creates blockstore on first +// request and reuses created instance for all next requests. +func (s *accessorWithBlockstore) Blockstore() (*BlockstoreCloser, error) { + s.Lock() + defer s.Unlock() + var err error + if s.bs == nil { + s.bs, err = s.sa.Blockstore() + } + + return &BlockstoreCloser{ + ReadBlockstore: s.bs, + Closer: io.NopCloser(nil), + }, err +} + +// shardKeyToStriped returns the index of the lock to use for a given shard key. We use the last +// byte of the shard key as the pseudo-random index. +func shardKeyToStriped(sk shard.Key) byte { + return sk.String()[len(sk.String())-1] +} diff --git a/share/eds/cache/accessor_cache_test.go b/share/eds/cache/accessor_cache_test.go new file mode 100644 index 0000000000..bffa4d7588 --- /dev/null +++ b/share/eds/cache/accessor_cache_test.go @@ -0,0 +1,239 @@ +package cache + +import ( + "bytes" + "context" + "errors" + "io" + "testing" + "time" + + "github.com/filecoin-project/dagstore" + "github.com/filecoin-project/dagstore/shard" + blocks "github.com/ipfs/go-block-format" + "github.com/ipfs/go-cid" + "github.com/stretchr/testify/require" +) + +func TestAccessorCache(t *testing.T) { + t.Run("add / get item from cache", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{} + loaded, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + // check if item exists + got, err := cache.Get(key) + require.NoError(t, err) + require.True(t, loaded == got) + }) + + t.Run("get blockstore from accessor", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{} + accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + // check if item exists + _, err = cache.Get(key) + require.NoError(t, err) + + // blockstore should be created only after first request + require.Equal(t, 0, mock.returnedBs) + + // try to get blockstore + _, err = accessor.Blockstore() + require.NoError(t, err) + + // second call to blockstore should return same blockstore + bs, err := accessor.Blockstore() + require.Equal(t, 1, mock.returnedBs) + require.NoError(t, err) + + // check that closed blockstore don't close accessor + err = bs.Close() + require.NoError(t, err) + require.False(t, mock.isClosed) + }) + + t.Run("remove an item", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{} + _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + err = cache.Remove(key) + require.NoError(t, err) + + // accessor should be closed on removal + require.True(t, mock.isClosed) + + // check if item exists + _, err = cache.Get(key) + require.ErrorIs(t, err, ErrCacheMiss) + }) + + t.Run("successive reads should read the same data", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{data: []byte("test")} + accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + loaded, err := io.ReadAll(accessor.ReadCloser()) + require.NoError(t, err) + require.Equal(t, mock.data, loaded) + + for i := 0; i < 2; i++ { + accessor, err = cache.Get(key) + require.NoError(t, err) + got, err := io.ReadAll(accessor.ReadCloser()) + require.NoError(t, err) + require.Equal(t, mock.data, got) + } + }) + + t.Run("removed by eviction", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{} + _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + // add second item + key2 := shard.KeyFromString("key2") + _, err = cache.GetOrLoad(ctx, key2, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + // give cache time to evict an item + time.Sleep(time.Millisecond * 100) + // accessor should be closed on removal by eviction + require.True(t, mock.isClosed) + + // check if item evicted + _, err = cache.Get(key) + require.ErrorIs(t, err, ErrCacheMiss) + }) + + t.Run("close on accessor is noop", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + cache, err := NewAccessorCache("test", 1) + require.NoError(t, err) + + // add accessor to the cache + key := shard.KeyFromString("key") + mock := &mockAccessorProvider{} + _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + return mock, nil + }) + require.NoError(t, err) + + // check if item exists + accessor, err := cache.Get(key) + require.NoError(t, err) + require.NotNil(t, accessor) + + // close on reader should be noop + err = accessor.ReadCloser().Close() + require.NoError(t, err) + + // close on blockstore should be noop + bs, err := accessor.Blockstore() + require.NoError(t, err) + err = bs.Close() + require.NoError(t, err) + + // check that close was not performed on accessor + require.False(t, mock.isClosed) + }) +} + +type mockAccessorProvider struct { + data []byte + isClosed bool + returnedBs int +} + +func (m *mockAccessorProvider) Reader() io.Reader { + return bytes.NewBuffer(m.data) +} + +func (m *mockAccessorProvider) Blockstore() (dagstore.ReadBlockstore, error) { + if m.returnedBs > 0 { + return nil, errors.New("blockstore already returned") + } + m.returnedBs++ + return rbsMock{}, nil +} + +func (m *mockAccessorProvider) Close() error { + if m.isClosed { + return errors.New("already closed") + } + m.isClosed = true + return nil +} + +// rbsMock is a dagstore.ReadBlockstore mock +type rbsMock struct{} + +func (r rbsMock) Has(context.Context, cid.Cid) (bool, error) { + panic("implement me") +} + +func (r rbsMock) Get(_ context.Context, _ cid.Cid) (blocks.Block, error) { + panic("implement me") +} + +func (r rbsMock) GetSize(context.Context, cid.Cid) (int, error) { + panic("implement me") +} + +func (r rbsMock) AllKeysChan(context.Context) (<-chan cid.Cid, error) { + panic("implement me") +} + +func (r rbsMock) HashOnRead(bool) { + panic("implement me") +} diff --git a/share/eds/cache/cache.go b/share/eds/cache/cache.go new file mode 100644 index 0000000000..e0fe2ef4ef --- /dev/null +++ b/share/eds/cache/cache.go @@ -0,0 +1,87 @@ +package cache + +import ( + "context" + "errors" + "io" + + "github.com/filecoin-project/dagstore" + "github.com/filecoin-project/dagstore/shard" + logging "github.com/ipfs/go-log/v2" + "go.opentelemetry.io/otel" +) + +var ( + log = logging.Logger("share/eds/cache") + meter = otel.Meter("eds_store_cache") +) + +const ( + cacheFoundKey = "found" + failedKey = "failed" +) + +var ( + ErrCacheMiss = errors.New("accessor not found in blockstore cache") +) + +// Cache is an interface that defines the basic Cache operations. +type Cache interface { + // Get retrieves an item from the Cache. + Get(shard.Key) (Accessor, error) + + // GetOrLoad attempts to get an item from the Cache and, if not found, invokes + // the provided loader function to load it into the Cache. + GetOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (AccessorProvider, error), + ) (Accessor, error) + + // Remove removes an item from Cache. + Remove(shard.Key) error + + // EnableMetrics enables metrics in Cache + EnableMetrics() error +} + +type Accessor interface { + ReadCloser() io.ReadCloser + Blockstore() (*BlockstoreCloser, error) +} + +// AccessorProvider defines interface used from dagstore.ShardAccessor to allow easy testing +type AccessorProvider interface { + Reader() io.Reader + Blockstore() (dagstore.ReadBlockstore, error) + io.Closer +} + +type BlockstoreCloser struct { + dagstore.ReadBlockstore + io.Closer +} + +var _ Cache = (*NoopCache)(nil) + +// NoopCache implements noop version of Cache interface +type NoopCache struct{} + +func (n NoopCache) Get(shard.Key) (Accessor, error) { + return nil, ErrCacheMiss +} + +func (n NoopCache) GetOrLoad( + context.Context, shard.Key, + func(context.Context, shard.Key) (AccessorProvider, error), +) (Accessor, error) { + return nil, nil +} + +func (n NoopCache) Remove(shard.Key) error { + return nil +} + +func (n NoopCache) EnableMetrics() error { + return nil +} diff --git a/share/eds/cache/metrics.go b/share/eds/cache/metrics.go new file mode 100644 index 0000000000..0d0fa47a13 --- /dev/null +++ b/share/eds/cache/metrics.go @@ -0,0 +1,61 @@ +package cache + +import ( + "context" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" +) + +type metrics struct { + getCounter metric.Int64Counter + evictedCounter metric.Int64Counter +} + +func newMetrics(bc *AccessorCache) (*metrics, error) { + evictedCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_evicted_counter", + metric.WithDescription("eds blockstore cache evicted event counter")) + if err != nil { + return nil, err + } + + getCounter, err := meter.Int64Counter("eds_blockstore_cache_"+bc.name+"_get_counter", + metric.WithDescription("eds blockstore cache evicted event counter")) + if err != nil { + return nil, err + } + + cacheSize, err := meter.Int64ObservableGauge("eds_blockstore_cache_"+bc.name+"_size", + metric.WithDescription("total amount of items in blockstore cache"), + ) + if err != nil { + return nil, err + } + + callback := func(ctx context.Context, observer metric.Observer) error { + observer.ObserveInt64(cacheSize, int64(bc.cache.Len())) + return nil + } + _, err = meter.RegisterCallback(callback, cacheSize) + + return &metrics{ + getCounter: getCounter, + evictedCounter: evictedCounter, + }, err +} + +func (m *metrics) observeEvicted(failed bool) { + if m == nil { + return + } + m.evictedCounter.Add(context.Background(), 1, metric.WithAttributes( + attribute.Bool(failedKey, failed))) +} + +func (m *metrics) observeGet(found bool) { + if m == nil { + return + } + m.getCounter.Add(context.Background(), 1, metric.WithAttributes( + attribute.Bool(cacheFoundKey, found))) +} diff --git a/share/eds/cache/multicache.go b/share/eds/cache/multicache.go new file mode 100644 index 0000000000..42cc672a1f --- /dev/null +++ b/share/eds/cache/multicache.go @@ -0,0 +1,60 @@ +package cache + +import ( + "context" + "errors" + + "github.com/filecoin-project/dagstore/shard" +) + +var _ Cache = (*MultiCache)(nil) + +// MultiCache represents a Cache that looks into multiple caches one by one. +type MultiCache struct { + first, second Cache +} + +// NewMultiCache creates a new MultiCache with the provided caches. +func NewMultiCache(first, second Cache) *MultiCache { + return &MultiCache{ + first: first, + second: second, + } +} + +// Get looks for an item in all the caches one by one and returns the Cache found item. +func (mc *MultiCache) Get(key shard.Key) (Accessor, error) { + ac, err := mc.first.Get(key) + if err == nil { + return ac, nil + } + return mc.second.Get(key) +} + +// GetOrLoad attempts to get an item from all caches, and if not found, invokes +// the provided loader function to load it into one of the caches. +func (mc *MultiCache) GetOrLoad( + ctx context.Context, + key shard.Key, + loader func(context.Context, shard.Key) (AccessorProvider, error), +) (Accessor, error) { + ac, err := mc.first.GetOrLoad(ctx, key, loader) + if err == nil { + return ac, nil + } + return mc.second.GetOrLoad(ctx, key, loader) +} + +// Remove removes an item from all underlying caches +func (mc *MultiCache) Remove(key shard.Key) error { + err1 := mc.first.Remove(key) + err2 := mc.second.Remove(key) + return errors.Join(err1, err2) +} + +func (mc *MultiCache) EnableMetrics() error { + if err := mc.first.EnableMetrics(); err != nil { + return err + } + return mc.second.EnableMetrics() +} diff --git a/share/eds/metrics.go b/share/eds/metrics.go index f9186c5379..d661f4ad41 100644 --- a/share/eds/metrics.go +++ b/share/eds/metrics.go @@ -108,7 +108,7 @@ func (s *Store) WithMetrics() error { return err } - if err = s.cache.enableMetrics(); err != nil { + if err = s.cache.EnableMetrics(); err != nil { return err } s.metrics = &metrics{ diff --git a/share/eds/ods_test.go b/share/eds/ods_test.go index 5b6ed5568b..0f7c69e708 100644 --- a/share/eds/ods_test.go +++ b/share/eds/ods_test.go @@ -32,6 +32,9 @@ func TestODSReader(t *testing.T) { // get CAR reader from store r, err := edsStore.GetCAR(ctx, dah.Hash()) assert.NoError(t, err) + defer func() { + require.NoError(t, r.Close()) + }() // create ODSReader wrapper based on car reader to limit reads to ODS only odsR, err := ODSReader(r) @@ -81,6 +84,9 @@ func TestODSReaderReconstruction(t *testing.T) { // get CAR reader from store r, err := edsStore.GetCAR(ctx, dah.Hash()) assert.NoError(t, err) + defer func() { + require.NoError(t, r.Close()) + }() // create ODSReader wrapper based on car reader to limit reads to ODS only odsR, err := ODSReader(r) diff --git a/share/eds/store.go b/share/eds/store.go index 9fb76e6fb8..5c438a20de 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -27,6 +27,7 @@ import ( "github.com/celestiaorg/celestia-node/header" "github.com/celestiaorg/celestia-node/libs/utils" "github.com/celestiaorg/celestia-node/share" + "github.com/celestiaorg/celestia-node/share/eds/cache" "github.com/celestiaorg/celestia-node/share/ipld" ) @@ -57,7 +58,7 @@ type Store struct { mounts *mount.Registry bs *blockstore - cache *multiCache + cache cache.Cache carIdx index.FullIndexRepo invertedIdx *simpleInvertedIndex @@ -108,12 +109,12 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { return nil, fmt.Errorf("failed to create DAGStore: %w", err) } - recentBlocksCache, err := newBlockstoreCache("recent", defaultRecentBlocksCacheSize) + recentBlocksCache, err := cache.NewAccessorCache("recent", defaultRecentBlocksCacheSize) if err != nil { return nil, fmt.Errorf("failed to create recent blocks cache: %w", err) } - blockstoreCache, err := newBlockstoreCache("blockstore", defaultBlockstoreCacheSize) + blockstoreCache, err := cache.NewAccessorCache("blockstore", defaultBlockstoreCacheSize) if err != nil { return nil, fmt.Errorf("failed to create blockstore cache: %w", err) } @@ -125,9 +126,9 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { invertedIdx: invertedIdx, gcInterval: defaultGCInterval, mounts: r, - cache: newMultiCache(recentBlocksCache, blockstoreCache), + cache: cache.NewMultiCache(recentBlocksCache, blockstoreCache), } - store.bs = newBlockstore(store, ds) + store.bs = newBlockstore(store, blockstoreCache, ds) return store, nil } @@ -260,7 +261,7 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext go func() { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() - if _, err := s.cache.getOrLoad(ctx, result.Key, s.getAccessor); err != nil { + if _, err := s.cache.GetOrLoad(ctx, result.Key, s.getAccessor); err != nil { log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) } }() @@ -286,7 +287,7 @@ func trackLateResult(opName string, res <-chan dagstore.ShardResult, metrics *me } if result.Error != nil { metrics.observeLongOp(context.Background(), opName, time.Since(tnow), longOpFailed) - log.Errorf("failed to register shard after context expired: %v ago, err: %w", time.Since(tnow), result.Error) + log.Errorf("failed to register shard after context expired: %v ago, err: %s", time.Since(tnow), result.Error) return } metrics.observeLongOp(context.Background(), opName, time.Since(tnow), longOpOK) @@ -303,7 +304,7 @@ func trackLateResult(opName string, res <-chan dagstore.ShardResult, metrics *me // // The shard is cached in the Store, so subsequent calls to GetCAR with the same root will use the // same reader. The cache is responsible for closing the underlying reader. -func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.Reader, error) { +func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, error) { ctx, span := tracer.Start(ctx, "store/get-car") tnow := time.Now() r, err := s.getCAR(ctx, root) @@ -312,11 +313,11 @@ func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.Reader, err return r, err } -func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, error) { +func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.get(key) + accessor, err := s.cache.Get(key) if err == nil { - return accessor.sa.Reader(), nil + return accessor.ReadCloser(), nil } // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor // to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared @@ -325,7 +326,11 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.Reader, err if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) } - return shardAccessor.Reader(), nil + + return readCloser{ + Reader: shardAccessor.Reader(), + Closer: shardAccessor, + }, nil } // Blockstore returns an IPFS blockstore providing access to individual shares/nodes of all EDS @@ -343,7 +348,7 @@ func (s *Store) Blockstore() bstore.Blockstore { func (s *Store) CARBlockstore( ctx context.Context, root share.DataHash, -) (dagstore.ReadBlockstore, error) { +) (*cache.BlockstoreCloser, error) { ctx, span := tracer.Start(ctx, "store/car-blockstore") tnow := time.Now() r, err := s.carBlockstore(ctx, root) @@ -355,11 +360,11 @@ func (s *Store) CARBlockstore( func (s *Store) carBlockstore( ctx context.Context, root share.DataHash, -) (dagstore.ReadBlockstore, error) { +) (*cache.BlockstoreCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.get(key) + accessor, err := s.cache.Get(key) if err == nil { - return accessor.bs, nil + return accessor.Blockstore() } // if accessor not found in cache, create new one from dagstore sa, err := s.getAccessor(ctx, key) @@ -371,7 +376,10 @@ func (s *Store) carBlockstore( if err != nil { return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) } - return bs, nil + return &cache.BlockstoreCloser{ + ReadBlockstore: bs, + Closer: sa, + }, nil } // GetDAH returns the DataAvailabilityHeader for the EDS identified by DataHash. @@ -389,6 +397,11 @@ func (s *Store) getDAH(ctx context.Context, root share.DataHash) (*share.Root, e if err != nil { return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) } + defer func() { + if err := r.Close(); err != nil { + log.Warnw("closing car reader", "err", err) + } + }() carHeader, err := carv1.ReadHeader(bufio.NewReader(r)) if err != nil { @@ -415,7 +428,7 @@ func dahFromCARHeader(carHeader *carv1.CarHeader) *header.DataAvailabilityHeader } } -func (s *Store) getAccessor(ctx context.Context, key shard.Key) (*dagstore.ShardAccessor, error) { +func (s *Store) getAccessor(ctx context.Context, key shard.Key) (cache.AccessorProvider, error) { ch := make(chan dagstore.ShardResult, 1) err := s.dgstr.AcquireShard(ctx, key, ch, dagstore.AcquireOpts{}) if err != nil { @@ -450,7 +463,8 @@ func (s *Store) Remove(ctx context.Context, root share.DataHash) error { func (s *Store) remove(ctx context.Context, root share.DataHash) (err error) { key := shard.KeyFromString(root.String()) - if err := s.cache.remove(key); err != nil { + // remove open links to accessor from cache + if err := s.cache.Remove(key); err != nil { log.Warnw("remove accessor from cache", "err", err) } ch := make(chan dagstore.ShardResult, 1) @@ -503,11 +517,17 @@ func (s *Store) get(ctx context.Context, root share.DataHash) (eds *rsmt2d.Exten utils.SetStatusAndEnd(span, err) }() - f, err := s.GetCAR(ctx, root) + r, err := s.GetCAR(ctx, root) if err != nil { return nil, fmt.Errorf("failed to get CAR file: %w", err) } - eds, err = ReadEDS(ctx, f, root) + defer func() { + if err := r.Close(); err != nil { + log.Warnw("closing car reader", "err", err) + } + }() + + eds, err = ReadEDS(ctx, r, root) if err != nil { return nil, fmt.Errorf("failed to read EDS from CAR file: %w", err) } @@ -611,3 +631,9 @@ type inMemoryReader struct { func (r *inMemoryReader) Close() error { return nil } + +// readCloser combines io.Reader and io.Closer +type readCloser struct { + io.Reader + io.Closer +} diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 37941a018c..c08330e0a6 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -18,6 +18,7 @@ import ( "github.com/celestiaorg/rsmt2d" "github.com/celestiaorg/celestia-node/share" + "github.com/celestiaorg/celestia-node/share/eds/cache" "github.com/celestiaorg/celestia-node/share/eds/edstest" ) @@ -71,6 +72,9 @@ func TestEDSStore(t *testing.T) { r, err := edsStore.GetCAR(ctx, dah.Hash()) assert.NoError(t, err) + defer func() { + require.NoError(t, r.Close()) + }() carReader, err := car.NewCarReader(r) assert.NoError(t, err) @@ -145,7 +149,7 @@ func TestEDSStore(t *testing.T) { // check, that the key is in the cache after put shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.get(shardKey) + _, err = edsStore.cache.Get(shardKey) assert.NoError(t, err) }) @@ -188,7 +192,7 @@ func TestEDSStore_GC(t *testing.T) { // remove links to the shard from cache key := shard.KeyFromString(share.DataHash(dah.Hash()).String()) - err = edsStore.cache.remove(key) + err = edsStore.cache.Remove(key) require.NoError(t, err) // doesn't exist yet @@ -218,7 +222,7 @@ func Test_BlockstoreCache(t *testing.T) { // store eds to the store with noopCache to allow clean cache after put swap := edsStore.cache - edsStore.cache = newMultiCache(noopCache{}, noopCache{}) + edsStore.cache = cache.NewMultiCache(cache.NoopCache{}, cache.NoopCache{}) eds, dah := randomEDS(t) err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) @@ -240,13 +244,13 @@ func Test_BlockstoreCache(t *testing.T) { // key isn't in cache yet, so get returns errCacheMiss shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.get(shardKey) - require.ErrorIs(t, err, errCacheMiss) + _, err = edsStore.cache.Get(shardKey) + require.ErrorIs(t, err, cache.ErrCacheMiss) // now get it, so that the key is in the cache _, err = edsStore.Blockstore().Get(ctx, key) require.NoError(t, err) - _, err = edsStore.cache.get(shardKey) + _, err = edsStore.cache.Get(shardKey) require.NoError(t, err) } @@ -267,19 +271,19 @@ func Test_CachedAccessor(t *testing.T) { shardKey := shard.KeyFromString(dah.String()) // accessor is expected to be in cache - cachedAccessor, err := edsStore.cache.get(shardKey) + cachedAccessor, err := edsStore.cache.Get(shardKey) assert.NoError(t, err) // first read - carReader, err := car.NewCarReader(cachedAccessor.sa.Reader()) + carReader, err := car.NewCarReader(cachedAccessor.ReadCloser()) assert.NoError(t, err) firstBlock, err := carReader.Next() assert.NoError(t, err) // second read - cachedAccessor, err = edsStore.cache.get(shardKey) + cachedAccessor, err = edsStore.cache.Get(shardKey) assert.NoError(t, err) - carReader, err = car.NewCarReader(cachedAccessor.sa.Reader()) + carReader, err = car.NewCarReader(cachedAccessor.ReadCloser()) assert.NoError(t, err) secondBlock, err := carReader.Next() assert.NoError(t, err) diff --git a/share/getter.go b/share/getter.go index 19e4498458..d9805ee911 100644 --- a/share/getter.go +++ b/share/getter.go @@ -13,7 +13,8 @@ import ( var ( // ErrNotFound is used to indicate that requested data could not be found. ErrNotFound = errors.New("share: data not found") - // ErrOutOfBounds is used to indicate that a passed row or column index is out of bounds of the square size. + // ErrOutOfBounds is used to indicate that a passed row or column index is out of bounds of the + // square size. ErrOutOfBounds = errors.New("share: row or column index is larger than square size") ) diff --git a/share/getters/store.go b/share/getters/store.go index 989649f795..9327cd5581 100644 --- a/share/getters/store.go +++ b/share/getters/store.go @@ -58,6 +58,11 @@ func (sg *StoreGetter) GetShare(ctx context.Context, dah *share.Root, row, col i if err != nil { return nil, fmt.Errorf("getter/store: failed to retrieve blockstore: %w", err) } + defer func() { + if err := bs.Close(); err != nil { + log.Warnw("closing blockstore", "err", err) + } + }() // wrap the read-only CAR blockstore in a getter blockGetter := eds.NewBlockGetter(bs) @@ -117,6 +122,11 @@ func (sg *StoreGetter) GetSharesByNamespace( if err != nil { return nil, fmt.Errorf("getter/store: failed to retrieve blockstore: %w", err) } + defer func() { + if err := bs.Close(); err != nil { + log.Warnw("closing blockstore", "err", err) + } + }() // wrap the read-only CAR blockstore in a getter blockGetter := eds.NewBlockGetter(bs) diff --git a/share/p2p/shrexeds/server.go b/share/p2p/shrexeds/server.go index fffa0e8152..8cb5385446 100644 --- a/share/p2p/shrexeds/server.go +++ b/share/p2p/shrexeds/server.go @@ -100,8 +100,16 @@ func (s *Server) handleStream(stream network.Stream) { // we do not close the reader, so that other requests will not need to re-open the file. // closing is handled by the LRU cache. edsReader, err := s.store.GetCAR(ctx, hash) - status := p2p_pb.Status_OK + var status p2p_pb.Status switch { + case err == nil: + defer func() { + if err := edsReader.Close(); err != nil { + log.Warnw("closing car reader", "err", err) + } + }() + edsReader.Close() + status = p2p_pb.Status_OK case errors.Is(err, eds.ErrNotFound): logger.Warnw("server: request hash not found") s.metrics.ObserveRequests(ctx, 1, p2p.StatusNotFound) From e520b4ff56bffc9bd3fb139c5c2ac0222fba93a5 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 5 Sep 2023 14:08:20 +0300 Subject: [PATCH 07/14] extract cache split into separate PR --- share/eds/blockstore.go | 8 +--- share/eds/cache/multicache.go | 60 --------------------------- share/eds/store.go | 76 +++++------------------------------ share/eds/store_test.go | 52 ++++++------------------ 4 files changed, 26 insertions(+), 170 deletions(-) delete mode 100644 share/eds/cache/multicache.go diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index f4da4d5774..1e2dd7fd8d 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -13,8 +13,6 @@ import ( "github.com/ipfs/go-datastore/namespace" dshelp "github.com/ipfs/go-ipfs-ds-help" ipld "github.com/ipfs/go-ipld-format" - - "github.com/celestiaorg/celestia-node/share/eds/cache" ) var _ bstore.Blockstore = (*blockstore)(nil) @@ -34,14 +32,12 @@ var ( // implementation to allow for the blockstore operations to be routed to the underlying stores. type blockstore struct { store *Store - cache cache.Cache ds datastore.Batching } -func newBlockstore(store *Store, cache cache.Cache, ds datastore.Batching) *blockstore { +func newBlockstore(store *Store, ds datastore.Batching) *blockstore { return &blockstore{ store: store, - cache: cache, ds: namespace.Wrap(ds, blockstoreCacheKey), } } @@ -159,7 +155,7 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d // a share can exist in multiple EDSes, so just take the first one. shardKey := keys[0] - accessor, err := bs.cache.GetOrLoad(ctx, shardKey, bs.store.getAccessor) + accessor, err := bs.store.cache.GetOrLoad(ctx, shardKey, bs.store.getAccessor) if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) } diff --git a/share/eds/cache/multicache.go b/share/eds/cache/multicache.go deleted file mode 100644 index 42cc672a1f..0000000000 --- a/share/eds/cache/multicache.go +++ /dev/null @@ -1,60 +0,0 @@ -package cache - -import ( - "context" - "errors" - - "github.com/filecoin-project/dagstore/shard" -) - -var _ Cache = (*MultiCache)(nil) - -// MultiCache represents a Cache that looks into multiple caches one by one. -type MultiCache struct { - first, second Cache -} - -// NewMultiCache creates a new MultiCache with the provided caches. -func NewMultiCache(first, second Cache) *MultiCache { - return &MultiCache{ - first: first, - second: second, - } -} - -// Get looks for an item in all the caches one by one and returns the Cache found item. -func (mc *MultiCache) Get(key shard.Key) (Accessor, error) { - ac, err := mc.first.Get(key) - if err == nil { - return ac, nil - } - return mc.second.Get(key) -} - -// GetOrLoad attempts to get an item from all caches, and if not found, invokes -// the provided loader function to load it into one of the caches. -func (mc *MultiCache) GetOrLoad( - ctx context.Context, - key shard.Key, - loader func(context.Context, shard.Key) (AccessorProvider, error), -) (Accessor, error) { - ac, err := mc.first.GetOrLoad(ctx, key, loader) - if err == nil { - return ac, nil - } - return mc.second.GetOrLoad(ctx, key, loader) -} - -// Remove removes an item from all underlying caches -func (mc *MultiCache) Remove(key shard.Key) error { - err1 := mc.first.Remove(key) - err2 := mc.second.Remove(key) - return errors.Join(err1, err2) -} - -func (mc *MultiCache) EnableMetrics() error { - if err := mc.first.EnableMetrics(); err != nil { - return err - } - return mc.second.EnableMetrics() -} diff --git a/share/eds/store.go b/share/eds/store.go index 5c438a20de..0a82bcb4c9 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -41,8 +41,7 @@ const ( // We don't use transient files right now, so GC is turned off by default. defaultGCInterval = 0 - defaultRecentBlocksCacheSize = 10 - defaultBlockstoreCacheSize = 128 + defaultCacheSize = 128 ) var ErrNotFound = errors.New("eds not found in store") @@ -109,16 +108,11 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { return nil, fmt.Errorf("failed to create DAGStore: %w", err) } - recentBlocksCache, err := cache.NewAccessorCache("recent", defaultRecentBlocksCacheSize) + accessorCache, err := cache.NewAccessorCache("cache", defaultCacheSize) if err != nil { return nil, fmt.Errorf("failed to create recent blocks cache: %w", err) } - blockstoreCache, err := cache.NewAccessorCache("blockstore", defaultBlockstoreCacheSize) - if err != nil { - return nil, fmt.Errorf("failed to create blockstore cache: %w", err) - } - store := &Store{ basepath: basepath, dgstr: dagStore, @@ -126,9 +120,9 @@ func NewStore(basepath string, ds datastore.Batching) (*Store, error) { invertedIdx: invertedIdx, gcInterval: defaultGCInterval, mounts: r, - cache: cache.NewMultiCache(recentBlocksCache, blockstoreCache), + cache: accessorCache, } - store.bs = newBlockstore(store, blockstoreCache, ds) + store.bs = newBlockstore(store, ds) return store, nil } @@ -252,20 +246,6 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext if result.Error != nil { return fmt.Errorf("failed to register shard: %w", result.Error) } - - // accessor returned in result will be nil, so shard needs to be acquired first, to become - // available in cache. It might take some time and result should not affect put operation, so do it - // in goroutine - //TODO: Ideally only recent blocks should be put in cache, but there is no way right now to check - // such condition. - go func() { - ctx, cancel := context.WithTimeout(context.Background(), time.Minute) - defer cancel() - if _, err := s.cache.GetOrLoad(ctx, result.Key, s.getAccessor); err != nil { - log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) - } - }() - return nil } @@ -315,22 +295,11 @@ func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.Get(key) - if err == nil { - return accessor.ReadCloser(), nil - } - // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor - // to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared - // to other cache put triggers, that the same block to be requested again. - shardAccessor, err := s.getAccessor(ctx, key) + accessor, err := s.cache.GetOrLoad(ctx, key, s.getAccessor) if err != nil { - return nil, fmt.Errorf("failed to get accessor: %w", err) + return nil, err } - - return readCloser{ - Reader: shardAccessor.Reader(), - Closer: shardAccessor, - }, nil + return accessor.ReadCloser(), nil } // Blockstore returns an IPFS blockstore providing access to individual shares/nodes of all EDS @@ -362,24 +331,11 @@ func (s *Store) carBlockstore( root share.DataHash, ) (*cache.BlockstoreCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.Get(key) - if err == nil { - return accessor.Blockstore() - } - // if accessor not found in cache, create new one from dagstore - sa, err := s.getAccessor(ctx, key) + accessor, err := s.cache.GetOrLoad(ctx, key, s.getAccessor) if err != nil { - return nil, fmt.Errorf("failed to get accessor: %w", err) + return nil, err } - - bs, err := sa.Blockstore() - if err != nil { - return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) - } - return &cache.BlockstoreCloser{ - ReadBlockstore: bs, - Closer: sa, - }, nil + return accessor.Blockstore() } // GetDAH returns the DataAvailabilityHeader for the EDS identified by DataHash. @@ -527,11 +483,7 @@ func (s *Store) get(ctx context.Context, root share.DataHash) (eds *rsmt2d.Exten } }() - eds, err = ReadEDS(ctx, r, root) - if err != nil { - return nil, fmt.Errorf("failed to read EDS from CAR file: %w", err) - } - return eds, nil + return ReadEDS(ctx, r, root) } // Has checks if EDS exists by the given share.Root hash. @@ -631,9 +583,3 @@ type inMemoryReader struct { func (r *inMemoryReader) Close() error { return nil } - -// readCloser combines io.Reader and io.Closer -type readCloser struct { - io.Reader - io.Closer -} diff --git a/share/eds/store_test.go b/share/eds/store_test.go index c08330e0a6..cb868f3201 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -2,6 +2,7 @@ package eds import ( "context" + "io" "os" "testing" "time" @@ -18,7 +19,6 @@ import ( "github.com/celestiaorg/rsmt2d" "github.com/celestiaorg/celestia-node/share" - "github.com/celestiaorg/celestia-node/share/eds/cache" "github.com/celestiaorg/celestia-node/share/eds/edstest" ) @@ -142,17 +142,6 @@ func TestEDSStore(t *testing.T) { assert.True(t, ok) }) - t.Run("RecentBlocksCache", func(t *testing.T) { - eds, dah := randomEDS(t) - err = edsStore.Put(ctx, dah.Hash(), eds) - require.NoError(t, err) - - // check, that the key is in the cache after put - shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.Get(shardKey) - assert.NoError(t, err) - }) - t.Run("List", func(t *testing.T) { const amount = 10 hashes := make([]share.DataHash, 0, amount) @@ -221,8 +210,6 @@ func Test_BlockstoreCache(t *testing.T) { require.NoError(t, err) // store eds to the store with noopCache to allow clean cache after put - swap := edsStore.cache - edsStore.cache = cache.NewMultiCache(cache.NoopCache{}, cache.NoopCache{}) eds, dah := randomEDS(t) err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) @@ -239,17 +226,11 @@ func Test_BlockstoreCache(t *testing.T) { t.Fatal("context timeout") } - // swap back original cache - edsStore.cache = swap - - // key isn't in cache yet, so get returns errCacheMiss - shardKey := shard.KeyFromString(dah.String()) - _, err = edsStore.cache.Get(shardKey) - require.ErrorIs(t, err, cache.ErrCacheMiss) - // now get it, so that the key is in the cache + shardKey := shard.KeyFromString(dah.String()) _, err = edsStore.Blockstore().Get(ctx, key) require.NoError(t, err) + // check that blockstore is in the cache _, err = edsStore.cache.Get(shardKey) require.NoError(t, err) } @@ -269,26 +250,19 @@ func Test_CachedAccessor(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - shardKey := shard.KeyFromString(dah.String()) - // accessor is expected to be in cache - cachedAccessor, err := edsStore.cache.Get(shardKey) - assert.NoError(t, err) - // first read - carReader, err := car.NewCarReader(cachedAccessor.ReadCloser()) - assert.NoError(t, err) - firstBlock, err := carReader.Next() - assert.NoError(t, err) + car, err := edsStore.GetCAR(ctx, dah.Hash()) + require.NoError(t, err) + first, err := io.ReadAll(car) + require.NoError(t, err) // second read - cachedAccessor, err = edsStore.cache.Get(shardKey) - assert.NoError(t, err) - carReader, err = car.NewCarReader(cachedAccessor.ReadCloser()) - assert.NoError(t, err) - secondBlock, err := carReader.Next() - assert.NoError(t, err) - - assert.Equal(t, firstBlock, secondBlock) + car, err = edsStore.GetCAR(ctx, dah.Hash()) + require.NoError(t, err) + second, err := io.ReadAll(car) + require.NoError(t, err) + + assert.Equal(t, first, second) } func BenchmarkStore(b *testing.B) { From c7308c56a5a7ed4f0ca7b77909105954191f4f3b Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 6 Sep 2023 11:37:58 +0300 Subject: [PATCH 08/14] refactoring --- share/eds/accessor_cache.go | 0 share/eds/blockstore.go | 49 +++++++++---- share/eds/cache/accessor_cache.go | 92 ++++++++++++++---------- share/eds/cache/accessor_cache_test.go | 89 ++++++++++++----------- share/eds/cache/cache.go | 20 ++---- share/eds/cache/metrics.go | 5 +- share/eds/store.go | 87 ++++++++++++++++------- share/eds/store_test.go | 98 +++++++++++++++++++++++--- share/eds/utils.go | 9 +++ 9 files changed, 307 insertions(+), 142 deletions(-) delete mode 100644 share/eds/accessor_cache.go create mode 100644 share/eds/utils.go diff --git a/share/eds/accessor_cache.go b/share/eds/accessor_cache.go deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 400f408513..8eec05f4b0 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "github.com/filecoin-project/dagstore" bstore "github.com/ipfs/boxo/blockstore" @@ -13,6 +14,8 @@ import ( "github.com/ipfs/go-datastore" "github.com/ipfs/go-datastore/namespace" ipld "github.com/ipfs/go-ipld-format" + + "github.com/celestiaorg/celestia-node/share/eds/cache" ) var _ bstore.Blockstore = (*blockstore)(nil) @@ -35,6 +38,11 @@ type blockstore struct { ds datastore.Batching } +type BlockstoreCloser struct { + dagstore.ReadBlockstore + io.Closer +} + func newBlockstore(store *Store, ds datastore.Batching) *blockstore { return &blockstore{ store: store, @@ -61,6 +69,11 @@ func (bs *blockstore) Has(ctx context.Context, cid cid.Cid) (bool, error) { func (bs *blockstore) Get(ctx context.Context, cid cid.Cid) (blocks.Block, error) { blockstr, err := bs.getReadOnlyBlockstore(ctx, cid) + if err == nil { + defer closeAndLog("blockstore", blockstr) + return blockstr.Get(ctx, cid) + } + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrNotFoundInIndex) { k := dshelp.MultihashToDsKey(cid.Hash()) blockData, err := bs.ds.Get(ctx, k) @@ -70,15 +83,17 @@ func (bs *blockstore) Get(ctx context.Context, cid cid.Cid) (blocks.Block, error // nmt's GetNode expects an ipld.ErrNotFound when a cid is not found. return nil, ipld.ErrNotFound{Cid: cid} } - if err != nil { - log.Debugf("failed to get blockstore for cid %s: %s", cid, err) - return nil, err - } - return blockstr.Get(ctx, cid) + + log.Debugf("failed to get blockstore for cid %s: %s", cid, err) + return nil, err } func (bs *blockstore) GetSize(ctx context.Context, cid cid.Cid) (int, error) { blockstr, err := bs.getReadOnlyBlockstore(ctx, cid) + if err == nil { + defer closeAndLog("blockstore", blockstr) + return blockstr.GetSize(ctx, cid) + } if errors.Is(err, ErrNotFound) || errors.Is(err, ErrNotFoundInIndex) { k := dshelp.MultihashToDsKey(cid.Hash()) size, err := bs.ds.GetSize(ctx, k) @@ -88,10 +103,9 @@ func (bs *blockstore) GetSize(ctx context.Context, cid cid.Cid) (int, error) { // nmt's GetSize expects an ipld.ErrNotFound when a cid is not found. return 0, ipld.ErrNotFound{Cid: cid} } - if err != nil { - return 0, err - } - return blockstr.GetSize(ctx, cid) + + log.Debugf("failed to get size for cid %s: %s", cid, err) + return 0, err } func (bs *blockstore) DeleteBlock(ctx context.Context, cid cid.Cid) error { @@ -137,7 +151,7 @@ func (bs *blockstore) HashOnRead(bool) { } // getReadOnlyBlockstore finds the underlying blockstore of the shard that contains the given CID. -func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (dagstore.ReadBlockstore, error) { +func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (*BlockstoreCloser, error) { keys, err := bs.store.dgstr.ShardsContainingMultihash(ctx, cid.Hash()) if errors.Is(err, datastore.ErrNotFound) || errors.Is(err, ErrNotFoundInIndex) { return nil, ErrNotFound @@ -149,7 +163,7 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d // a share can exist in multiple EDSes, check cache to contain any of accessors containing shard for _, k := range keys { if accessor, err := bs.store.cache.Get(k); err == nil { - return accessor.Blockstore() + return blockstoreCloser(accessor) } } @@ -159,5 +173,16 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (d if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) } - return accessor.Blockstore() + return blockstoreCloser(accessor) +} + +func blockstoreCloser(ac cache.Accessor) (*BlockstoreCloser, error) { + bs, err := ac.Blockstore() + if err != nil { + return nil, fmt.Errorf("eds/store: failed to get blockstore: %w", err) + } + return &BlockstoreCloser{ + ReadBlockstore: bs, + Closer: ac, + }, nil } diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index f8fb36080b..94eb5da6ff 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -3,13 +3,12 @@ package cache import ( "context" "fmt" - "io" - "reflect" - "sync" - "github.com/filecoin-project/dagstore" "github.com/filecoin-project/dagstore/shard" lru "github.com/hashicorp/golang-lru" + "io" + "reflect" + "sync" ) var _ Cache = (*AccessorCache)(nil) @@ -31,8 +30,8 @@ type AccessorCache struct { // accessorWithBlockstore is the value that we store in the blockstore Cache. Implements Accessor // interface type accessorWithBlockstore struct { - sync.Mutex - sa AccessorProvider + sync.RWMutex + shardAccessor Accessor // blockstore is stored separately because each access to the blockstore over the shard accessor // reopens the underlying CAR. bs dagstore.ReadBlockstore @@ -62,11 +61,11 @@ func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { )) } - err := abs.sa.Close() + err := abs.shardAccessor.Close() if err != nil { log.Errorf("couldn't close accessor after cache eviction: %s", err) } - bc.metrics.observeEvicted(err != nil) + bc.metrics.observeEvicted() } } @@ -77,26 +76,30 @@ func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { lk.Lock() defer lk.Unlock() - return bc.unsafeGet(key) + accessor, err := bc.get(key) + if err != nil { + bc.metrics.observeGet(false) + return nil, err + } + bc.metrics.observeGet(true) + return newCloser(accessor) } -func (bc *AccessorCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, error) { +func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { // We've already ensured that the given shard has the cid/multihash we are looking for. val, ok := bc.cache.Get(key) if !ok { - bc.metrics.observeGet(false) return nil, ErrCacheMiss } - accessor, ok := val.(*accessorWithBlockstore) + abs, ok := val.(*accessorWithBlockstore) if !ok { panic(fmt.Sprintf( "casting value from cache to accessorWithBlockstore: %s", reflect.TypeOf(val), )) } - bc.metrics.observeGet(true) - return accessor, nil + return abs, nil } // GetOrLoad attempts to get an item from all caches, and if not found, invokes @@ -104,33 +107,37 @@ func (bc *AccessorCache) unsafeGet(key shard.Key) (*accessorWithBlockstore, erro func (bc *AccessorCache) GetOrLoad( ctx context.Context, key shard.Key, - loader func(context.Context, shard.Key) (AccessorProvider, error), + loader func(context.Context, shard.Key) (Accessor, error), ) (Accessor, error) { lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() defer lk.Unlock() - if accessor, err := bc.unsafeGet(key); err == nil { - return accessor, nil + if accessor, err := bc.get(key); err == nil { + return newCloser(accessor) } - accessor, err := loader(ctx, key) + provider, err := loader(ctx, key) if err != nil { return nil, fmt.Errorf("unable to get accessor: %w", err) } - newAccessor := &accessorWithBlockstore{ - sa: accessor, + abs := &accessorWithBlockstore{ + shardAccessor: provider, + } + + // create new accessor first to inc ref count in it, so it could not get evicted from inner cache + // before it is used + accessor, err := newCloser(abs) + if err != nil { + return nil, err } - bc.cache.Add(key, newAccessor) - return newAccessor, nil + bc.cache.Add(key, abs) + return accessor, nil } func (bc *AccessorCache) Remove(key shard.Key) error { - lk := &bc.stripedLocks[shardKeyToStriped(key)] - lk.Lock() - defer lk.Unlock() - + // cache will call evictFn on removal, where accessor close will be called bc.cache.Remove(key) return nil } @@ -141,26 +148,35 @@ func (bc *AccessorCache) EnableMetrics() error { return err } -// ReadCloser implements ReadCloser of the Accessor interface, using noop Closer to disallow user -// from manually closing. Close will be performed on accessor on cache eviction. -func (s *accessorWithBlockstore) ReadCloser() io.ReadCloser { - return io.NopCloser(s.sa.Reader()) -} - // Blockstore implements Blockstore of the Accessor interface. It creates blockstore on first // request and reuses created instance for all next requests. -func (s *accessorWithBlockstore) Blockstore() (*BlockstoreCloser, error) { +func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { s.Lock() defer s.Unlock() var err error if s.bs == nil { - s.bs, err = s.sa.Blockstore() + s.bs, err = s.shardAccessor.Blockstore() } - return &BlockstoreCloser{ - ReadBlockstore: s.bs, - Closer: io.NopCloser(nil), - }, err + return s.bs, err +} + +// Reader returns new copy of reader to read data +func (s *accessorWithBlockstore) Reader() io.Reader { + return s.shardAccessor.Reader() +} + +// temporarily object before refs count is implemented +type accessorCloser struct { + *accessorWithBlockstore + io.Closer +} + +func newCloser(abs *accessorWithBlockstore) (*accessorCloser, error) { + return &accessorCloser{ + accessorWithBlockstore: abs, + Closer: io.NopCloser(nil), + }, nil } // shardKeyToStriped returns the index of the lock to use for a given shard key. We use the last diff --git a/share/eds/cache/accessor_cache_test.go b/share/eds/cache/accessor_cache_test.go index bffa4d7588..1e4beaf4d1 100644 --- a/share/eds/cache/accessor_cache_test.go +++ b/share/eds/cache/accessor_cache_test.go @@ -24,8 +24,10 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{} - loaded, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{ + data: []byte("test_data"), + } + loaded, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) @@ -33,7 +35,13 @@ func TestAccessorCache(t *testing.T) { // check if item exists got, err := cache.Get(key) require.NoError(t, err) - require.True(t, loaded == got) + + l, err := io.ReadAll(loaded.Reader()) + require.NoError(t, err) + require.Equal(t, mock.data, l) + g, err := io.ReadAll(got.Reader()) + require.NoError(t, err) + require.Equal(t, mock.data, g) }) t.Run("get blockstore from accessor", func(t *testing.T) { @@ -44,8 +52,8 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{} - accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{} + accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) @@ -62,14 +70,9 @@ func TestAccessorCache(t *testing.T) { require.NoError(t, err) // second call to blockstore should return same blockstore - bs, err := accessor.Blockstore() - require.Equal(t, 1, mock.returnedBs) - require.NoError(t, err) - - // check that closed blockstore don't close accessor - err = bs.Close() + _, err = accessor.Blockstore() require.NoError(t, err) - require.False(t, mock.isClosed) + require.Equal(t, 1, mock.returnedBs) }) t.Run("remove an item", func(t *testing.T) { @@ -80,17 +83,19 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{} - _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{} + ac, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) + err = ac.Close() + require.NoError(t, err) err = cache.Remove(key) require.NoError(t, err) // accessor should be closed on removal - require.True(t, mock.isClosed) + mock.checkClosed(t, true) // check if item exists _, err = cache.Get(key) @@ -105,20 +110,20 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{data: []byte("test")} - accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{data: []byte("test")} + accessor, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) - loaded, err := io.ReadAll(accessor.ReadCloser()) + loaded, err := io.ReadAll(accessor.Reader()) require.NoError(t, err) require.Equal(t, mock.data, loaded) for i := 0; i < 2; i++ { accessor, err = cache.Get(key) require.NoError(t, err) - got, err := io.ReadAll(accessor.ReadCloser()) + got, err := io.ReadAll(accessor.Reader()) require.NoError(t, err) require.Equal(t, mock.data, got) } @@ -132,23 +137,25 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{} - _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{} + ac1, err := cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) + err = ac1.Close() + require.NoError(t, err) // add second item key2 := shard.KeyFromString("key2") - _, err = cache.GetOrLoad(ctx, key2, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + ac2, err := cache.GetOrLoad(ctx, key2, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) + err = ac2.Close() + require.NoError(t, err) - // give cache time to evict an item - time.Sleep(time.Millisecond * 100) // accessor should be closed on removal by eviction - require.True(t, mock.isClosed) + mock.checkClosed(t, true) // check if item evicted _, err = cache.Get(key) @@ -163,8 +170,8 @@ func TestAccessorCache(t *testing.T) { // add accessor to the cache key := shard.KeyFromString("key") - mock := &mockAccessorProvider{} - _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (AccessorProvider, error) { + mock := &mockAccessor{} + _, err = cache.GetOrLoad(ctx, key, func(ctx context.Context, key shard.Key) (Accessor, error) { return mock, nil }) require.NoError(t, err) @@ -174,32 +181,26 @@ func TestAccessorCache(t *testing.T) { require.NoError(t, err) require.NotNil(t, accessor) - // close on reader should be noop - err = accessor.ReadCloser().Close() - require.NoError(t, err) - - // close on blockstore should be noop - bs, err := accessor.Blockstore() - require.NoError(t, err) - err = bs.Close() + // close on returned accessor should not close inner reader + err = accessor.Close() require.NoError(t, err) - // check that close was not performed on accessor - require.False(t, mock.isClosed) + // check that close was not performed on inner accessor + mock.checkClosed(t, false) }) } -type mockAccessorProvider struct { +type mockAccessor struct { data []byte isClosed bool returnedBs int } -func (m *mockAccessorProvider) Reader() io.Reader { +func (m *mockAccessor) Reader() io.Reader { return bytes.NewBuffer(m.data) } -func (m *mockAccessorProvider) Blockstore() (dagstore.ReadBlockstore, error) { +func (m *mockAccessor) Blockstore() (dagstore.ReadBlockstore, error) { if m.returnedBs > 0 { return nil, errors.New("blockstore already returned") } @@ -207,7 +208,7 @@ func (m *mockAccessorProvider) Blockstore() (dagstore.ReadBlockstore, error) { return rbsMock{}, nil } -func (m *mockAccessorProvider) Close() error { +func (m *mockAccessor) Close() error { if m.isClosed { return errors.New("already closed") } @@ -215,6 +216,12 @@ func (m *mockAccessorProvider) Close() error { return nil } +func (m *mockAccessor) checkClosed(t *testing.T, expected bool) { + // item will be removed in background, so give it some time to settle + time.Sleep(time.Millisecond * 100) + require.Equal(t, expected, m.isClosed) +} + // rbsMock is a dagstore.ReadBlockstore mock type rbsMock struct{} diff --git a/share/eds/cache/cache.go b/share/eds/cache/cache.go index e0fe2ef4ef..41fb30c194 100644 --- a/share/eds/cache/cache.go +++ b/share/eds/cache/cache.go @@ -18,7 +18,6 @@ var ( const ( cacheFoundKey = "found" - failedKey = "failed" ) var ( @@ -35,7 +34,7 @@ type Cache interface { GetOrLoad( ctx context.Context, key shard.Key, - loader func(context.Context, shard.Key) (AccessorProvider, error), + loader func(context.Context, shard.Key) (Accessor, error), ) (Accessor, error) // Remove removes an item from Cache. @@ -45,20 +44,11 @@ type Cache interface { EnableMetrics() error } +// Accessor is a interface type returned by cache, that allows to read raw data by reader or create +// readblockstore type Accessor interface { - ReadCloser() io.ReadCloser - Blockstore() (*BlockstoreCloser, error) -} - -// AccessorProvider defines interface used from dagstore.ShardAccessor to allow easy testing -type AccessorProvider interface { - Reader() io.Reader Blockstore() (dagstore.ReadBlockstore, error) - io.Closer -} - -type BlockstoreCloser struct { - dagstore.ReadBlockstore + Reader() io.Reader io.Closer } @@ -73,7 +63,7 @@ func (n NoopCache) Get(shard.Key) (Accessor, error) { func (n NoopCache) GetOrLoad( context.Context, shard.Key, - func(context.Context, shard.Key) (AccessorProvider, error), + func(context.Context, shard.Key) (Accessor, error), ) (Accessor, error) { return nil, nil } diff --git a/share/eds/cache/metrics.go b/share/eds/cache/metrics.go index 0d0fa47a13..482f3e2ec2 100644 --- a/share/eds/cache/metrics.go +++ b/share/eds/cache/metrics.go @@ -44,12 +44,11 @@ func newMetrics(bc *AccessorCache) (*metrics, error) { }, err } -func (m *metrics) observeEvicted(failed bool) { +func (m *metrics) observeEvicted() { if m == nil { return } - m.evictedCounter.Add(context.Background(), 1, metric.WithAttributes( - attribute.Bool(failedKey, failed))) + m.evictedCounter.Add(context.Background(), 1) } func (m *metrics) observeGet(found bool) { diff --git a/share/eds/store.go b/share/eds/store.go index bed54ed0ea..11ebe7911f 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -234,7 +234,7 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext if err != nil { return err } - defer f.Close() + defer closeAndLog("car file", f) // save encoded eds into buffer mount := &inMemoryOnceMount{ @@ -270,6 +270,22 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext if result.Error != nil { return fmt.Errorf("failed to register shard: %w", result.Error) } + + // accessor returned in result will be nil, so shard needs to be acquired first, to become + // available in cache. It might take some time and result should not affect put operation, so do it + // in goroutine + //TODO: Ideally only recent blocks should be put in cache, but there is no way right now to check + // such condition. + go func() { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + _, err := s.cache.GetOrLoad(ctx, result.Key, s.getAccessor) + if err != nil { + log.Warnw("unable to put accessor to recent blocks accessors cache", "err", err) + return + } + }() + return nil } @@ -319,11 +335,19 @@ func (s *Store) GetCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.GetOrLoad(ctx, key, s.getAccessor) + accessor, err := s.cache.Get(key) + if err == nil { + return newReadCloser(accessor), nil + } + // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor + // to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared + // to other cache put triggers, that the same block to be requested again. + shardAccessor, err := s.getAccessor(ctx, key) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get accessor: %w", err) } - return accessor.ReadCloser(), nil + + return newReadCloser(shardAccessor), nil } // Blockstore returns an IPFS blockstore providing access to individual shares/nodes of all EDS @@ -341,25 +365,31 @@ func (s *Store) Blockstore() bstore.Blockstore { func (s *Store) CARBlockstore( ctx context.Context, root share.DataHash, -) (*cache.BlockstoreCloser, error) { +) (*BlockstoreCloser, error) { ctx, span := tracer.Start(ctx, "store/car-blockstore") tnow := time.Now() - r, err := s.carBlockstore(ctx, root) + cbs, err := s.carBlockstore(ctx, root) s.metrics.observeCARBlockstore(ctx, time.Since(tnow), err != nil) utils.SetStatusAndEnd(span, err) - return r, err + return cbs, err } func (s *Store) carBlockstore( ctx context.Context, root share.DataHash, -) (*cache.BlockstoreCloser, error) { +) (*BlockstoreCloser, error) { key := shard.KeyFromString(root.String()) - accessor, err := s.cache.GetOrLoad(ctx, key, s.getAccessor) + accessor, err := s.cache.Get(key) + if err == nil { + return blockstoreCloser(accessor) + } + + // if accessor not found in cache, create new one from dagstore + sa, err := s.getAccessor(ctx, key) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get accessor: %w", err) } - return accessor.Blockstore() + return blockstoreCloser(sa) } // GetDAH returns the DataAvailabilityHeader for the EDS identified by DataHash. @@ -377,11 +407,7 @@ func (s *Store) getDAH(ctx context.Context, root share.DataHash) (*share.Root, e if err != nil { return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) } - defer func() { - if err := r.Close(); err != nil { - log.Warnw("closing car reader", "err", err) - } - }() + defer closeAndLog("car reader", r) carHeader, err := carv1.ReadHeader(bufio.NewReader(r)) if err != nil { @@ -408,7 +434,7 @@ func dahFromCARHeader(carHeader *carv1.CarHeader) *header.DataAvailabilityHeader } } -func (s *Store) getAccessor(ctx context.Context, key shard.Key) (cache.AccessorProvider, error) { +func (s *Store) getAccessor(ctx context.Context, key shard.Key) (cache.Accessor, error) { ch := make(chan dagstore.ShardResult, 1) err := s.dgstr.AcquireShard(ctx, key, ch, dagstore.AcquireOpts{}) if err != nil { @@ -497,17 +523,17 @@ func (s *Store) get(ctx context.Context, root share.DataHash) (eds *rsmt2d.Exten utils.SetStatusAndEnd(span, err) }() - r, err := s.GetCAR(ctx, root) + r, err := s.getCAR(ctx, root) if err != nil { return nil, fmt.Errorf("failed to get CAR file: %w", err) } - defer func() { - if err := r.Close(); err != nil { - log.Warnw("closing car reader", "err", err) - } - }() + defer closeAndLog("car reader", r) - return ReadEDS(ctx, r, root) + eds, err = ReadEDS(ctx, r, root) + if err != nil { + return nil, fmt.Errorf("failed to read EDS from CAR file: %w", err) + } + return eds, nil } // Has checks if EDS exists by the given share.Root hash. @@ -604,3 +630,16 @@ type inMemoryReader struct { func (r *inMemoryReader) Close() error { return nil } + +// readCloser is a helper struct, that combines io.Reader and io.Closer +type readCloser struct { + io.Reader + io.Closer +} + +func newReadCloser(ac cache.Accessor) io.ReadCloser { + return readCloser{ + ac.Reader(), + ac, + } +} diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 171d605ec0..fe67d0e2b9 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -19,6 +19,7 @@ import ( "github.com/celestiaorg/rsmt2d" "github.com/celestiaorg/celestia-node/share" + "github.com/celestiaorg/celestia-node/share/eds/cache" "github.com/celestiaorg/celestia-node/share/eds/edstest" ) @@ -110,6 +111,7 @@ func TestEDSStore(t *testing.T) { _, err = os.Stat(edsStore.basepath + blocksPath + dah.String()) assert.NoError(t, err) + time.Sleep(time.Millisecond * 100) err = edsStore.Remove(ctx, dah.Hash()) assert.NoError(t, err) @@ -146,6 +148,11 @@ func TestEDSStore(t *testing.T) { err = os.Remove(path) assert.NoError(t, err) + // remove non-failed accessor from cache + time.Sleep(time.Millisecond * 100) + err = edsStore.cache.Remove(shard.KeyFromString(dah.String())) + assert.NoError(t, err) + _, err = edsStore.GetCAR(ctx, dah.Hash()) assert.Error(t, err) @@ -180,6 +187,17 @@ func TestEDSStore(t *testing.T) { assert.True(t, ok) }) + t.Run("RecentBlocksCache", func(t *testing.T) { + eds, dah := randomEDS(t) + err = edsStore.Put(ctx, dah.Hash(), eds) + require.NoError(t, err) + + // check, that the key is in the cache after put + shardKey := shard.KeyFromString(dah.String()) + _, err = edsStore.cache.Get(shardKey) + assert.NoError(t, err) + }) + t.Run("List", func(t *testing.T) { const amount = 10 hashes := make([]share.DataHash, 0, amount) @@ -218,6 +236,7 @@ func TestEDSStore_GC(t *testing.T) { require.NoError(t, err) // remove links to the shard from cache + time.Sleep(time.Millisecond * 100) key := shard.KeyFromString(share.DataHash(dah.Hash()).String()) err = edsStore.cache.Remove(key) require.NoError(t, err) @@ -248,6 +267,8 @@ func Test_BlockstoreCache(t *testing.T) { require.NoError(t, err) // store eds to the store with noopCache to allow clean cache after put + swap := edsStore.cache + edsStore.cache = cache.NoopCache{} eds, dah := randomEDS(t) err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) @@ -255,6 +276,9 @@ func Test_BlockstoreCache(t *testing.T) { // get any key from saved eds bs, err := edsStore.carBlockstore(ctx, dah.Hash()) require.NoError(t, err) + defer func() { + require.NoError(t, bs.Close()) + }() keys, err := bs.AllKeysChan(ctx) require.NoError(t, err) var key cid.Cid @@ -264,11 +288,19 @@ func Test_BlockstoreCache(t *testing.T) { t.Fatal("context timeout") } - // now get it, so that the key is in the cache + // swap back original cache + edsStore.cache = swap + + // key shouldn't be in cache yet, check for returned errCacheMiss shardKey := shard.KeyFromString(dah.String()) + _, err = edsStore.cache.Get(shardKey) + require.ErrorIs(t, err, cache.ErrCacheMiss) + + // now get it from blockstore, to trigger storing to cache _, err = edsStore.Blockstore().Get(ctx, key) require.NoError(t, err) - // check that blockstore is in the cache + + // should be no errCacheMiss anymore _, err = edsStore.cache.Get(shardKey) require.NoError(t, err) } @@ -288,19 +320,67 @@ func Test_CachedAccessor(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // first read - car, err := edsStore.GetCAR(ctx, dah.Hash()) + // give some time to let cache to get settled in background + time.Sleep(time.Millisecond * 50) + + // accessor should be in cache + cachedAccessor, err := edsStore.cache.Get(shard.KeyFromString(dah.String())) + require.NoError(t, err) + + // first read from cached accessor + firstBlock, err := io.ReadAll(cachedAccessor.Reader()) + require.NoError(t, err) + require.NoError(t, cachedAccessor.Close()) + + // second read from cached accessor + cachedAccessor, err = edsStore.cache.Get(shard.KeyFromString(dah.String())) + require.NoError(t, err) + secondBlock, err := io.ReadAll(cachedAccessor.Reader()) + require.NoError(t, err) + require.NoError(t, cachedAccessor.Close()) + + require.Equal(t, firstBlock, secondBlock) +} + +// Test_CachedAccessor verifies that the reader represented by a accessor obtained directly from +// dagstore can be read from multiple times, without exhausting the underlying reader. +func Test_NotCachedAccessor(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + edsStore, err := newStore(t) + require.NoError(t, err) + err = edsStore.Start(ctx) + require.NoError(t, err) + // replace cache with noopCache to + edsStore.cache = cache.NoopCache{} + + eds, dah := randomEDS(t) + err = edsStore.Put(ctx, dah.Hash(), eds) + require.NoError(t, err) + + // give some time to let cache to get settled in background + time.Sleep(time.Millisecond * 50) + + // accessor should be in cache + _, err = edsStore.cache.Get(shard.KeyFromString(dah.String())) + require.ErrorIs(t, err, cache.ErrCacheMiss) + + // first read from direct accessor + carReader, err := edsStore.getCAR(ctx, dah.Hash()) require.NoError(t, err) - first, err := io.ReadAll(car) + firstBlock, err := io.ReadAll(carReader) require.NoError(t, err) + require.NoError(t, carReader.Close()) - // second read - car, err = edsStore.GetCAR(ctx, dah.Hash()) + // second read from direct accessor + carReader, err = edsStore.getCAR(ctx, dah.Hash()) require.NoError(t, err) - second, err := io.ReadAll(car) + secondBlock, err := io.ReadAll(carReader) require.NoError(t, err) + require.NoError(t, carReader.Close()) - assert.Equal(t, first, second) + require.Equal(t, firstBlock, secondBlock) } func BenchmarkStore(b *testing.B) { diff --git a/share/eds/utils.go b/share/eds/utils.go new file mode 100644 index 0000000000..26d77ae3ee --- /dev/null +++ b/share/eds/utils.go @@ -0,0 +1,9 @@ +package eds + +import "io" + +func closeAndLog(name string, closer io.Closer) { + if err := closer.Close(); err != nil { + log.Warnw("closing "+name, "err", err) + } +} From 535a418fb6ed771f52d5ab7cd4f8df996317897e Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 6 Sep 2023 12:37:57 +0300 Subject: [PATCH 09/14] fix lint --- share/eds/blockstore.go | 1 + share/eds/cache/accessor_cache.go | 21 +++++++++++++-------- share/eds/cache/cache.go | 4 ---- share/eds/cache/metrics.go | 4 ++++ share/eds/store_test.go | 1 + 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 8eec05f4b0..9bad2f5d71 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -176,6 +176,7 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (* return blockstoreCloser(accessor) } +// blockstoreCloser constructs new BlockstoreCloser from cache.Accessor func blockstoreCloser(ac cache.Accessor) (*BlockstoreCloser, error) { bs, err := ac.Blockstore() if err != nil { diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index 94eb5da6ff..70f9ba39da 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -3,16 +3,18 @@ package cache import ( "context" "fmt" - "github.com/filecoin-project/dagstore" - "github.com/filecoin-project/dagstore/shard" - lru "github.com/hashicorp/golang-lru" "io" "reflect" "sync" + + "github.com/filecoin-project/dagstore" + "github.com/filecoin-project/dagstore/shard" + lru "github.com/hashicorp/golang-lru" ) var _ Cache = (*AccessorCache)(nil) +// AccessorCache implements Cache interface using LRU cache backend type AccessorCache struct { // name is a prefix, that will be used for cache metrics if it is enabled name string @@ -50,6 +52,7 @@ func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { return bc, nil } +// evictFn will be invoked when item is evicted from the cache func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { return func(_ interface{}, val interface{}) { // ensure we close the blockstore for a shard when it's evicted so dagstore can gc it. @@ -69,8 +72,8 @@ func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { } } -// Get retrieves the blockstore for a given shard key from the Cache. If the blockstore is not in -// the Cache, it returns an errCacheMiss +// Get retrieves the Accessor for a given shard key from the Cache. If the Accessor is not in +// the Cache, it returns an ErrCacheMiss func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() @@ -102,8 +105,8 @@ func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { return abs, nil } -// GetOrLoad attempts to get an item from all caches, and if not found, invokes -// the provided loader function to load it into one of the caches. +// GetOrLoad attempts to get an item from cache, and if not found, invokes +// the provided loader function to load it. func (bc *AccessorCache) GetOrLoad( ctx context.Context, key shard.Key, @@ -136,12 +139,14 @@ func (bc *AccessorCache) GetOrLoad( return accessor, nil } +// Remove removes Accessor for given key from the cache func (bc *AccessorCache) Remove(key shard.Key) error { // cache will call evictFn on removal, where accessor close will be called bc.cache.Remove(key) return nil } +// EnableMetrics enables metrics for the cache func (bc *AccessorCache) EnableMetrics() error { var err error bc.metrics, err = newMetrics(bc) @@ -166,7 +171,7 @@ func (s *accessorWithBlockstore) Reader() io.Reader { return s.shardAccessor.Reader() } -// temporarily object before refs count is implemented +// accessorCloser is a temporal object before refs count is implemented type accessorCloser struct { *accessorWithBlockstore io.Closer diff --git a/share/eds/cache/cache.go b/share/eds/cache/cache.go index 41fb30c194..5fa2cc9ab6 100644 --- a/share/eds/cache/cache.go +++ b/share/eds/cache/cache.go @@ -16,10 +16,6 @@ var ( meter = otel.Meter("eds_store_cache") ) -const ( - cacheFoundKey = "found" -) - var ( ErrCacheMiss = errors.New("accessor not found in blockstore cache") ) diff --git a/share/eds/cache/metrics.go b/share/eds/cache/metrics.go index 482f3e2ec2..2aef5dc36a 100644 --- a/share/eds/cache/metrics.go +++ b/share/eds/cache/metrics.go @@ -7,6 +7,10 @@ import ( "go.opentelemetry.io/otel/metric" ) +const ( + cacheFoundKey = "found" +) + type metrics struct { getCounter metric.Int64Counter evictedCounter metric.Int64Counter diff --git a/share/eds/store_test.go b/share/eds/store_test.go index fe67d0e2b9..69b12277d7 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -193,6 +193,7 @@ func TestEDSStore(t *testing.T) { require.NoError(t, err) // check, that the key is in the cache after put + time.Sleep(time.Millisecond * 100) shardKey := shard.KeyFromString(dah.String()) _, err = edsStore.cache.Get(shardKey) assert.NoError(t, err) From 79b72d7d4ed0eb6e211a51de8a8054db8b6d8455 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 7 Sep 2023 08:59:54 +0300 Subject: [PATCH 10/14] fix grammar --- share/eds/blockstore.go | 3 ++ share/eds/cache/accessor_cache.go | 46 +++++++++++++++---------------- share/eds/store.go | 23 ++++++++-------- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 9bad2f5d71..bde71ffee6 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -38,6 +38,8 @@ type blockstore struct { ds datastore.Batching } +// BlockstoreCloser represents a blockstore that can also be closed. It combines the functionality +// of a dagstore.ReadBlockstore with that of an io.Closer. type BlockstoreCloser struct { dagstore.ReadBlockstore io.Closer @@ -94,6 +96,7 @@ func (bs *blockstore) GetSize(ctx context.Context, cid cid.Cid) (int, error) { defer closeAndLog("blockstore", blockstr) return blockstr.GetSize(ctx, cid) } + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrNotFoundInIndex) { k := dshelp.MultihashToDsKey(cid.Hash()) size, err := bs.ds.GetSize(ctx, k) diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index 70f9ba39da..b92b818261 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -14,28 +14,26 @@ import ( var _ Cache = (*AccessorCache)(nil) -// AccessorCache implements Cache interface using LRU cache backend +// AccessorCache implements the Cache interface using an LRU cache backend. type AccessorCache struct { - // name is a prefix, that will be used for cache metrics if it is enabled + // The name is a prefix that will be used for cache metrics if they are enabled. name string // stripedLocks prevents simultaneous RW access to the blockstore cache for a shard. Instead // of using only one lock or one lock per key, we stripe the shard keys across 256 locks. 256 is // chosen because it 0-255 is the range of values we get looking at the last byte of the key. stripedLocks [256]sync.Mutex - // caches the blockstore for a given shard for shard read affinity i.e. - // further reads will likely be from the same shard. Maps (shard key -> blockstore). + // Caches the blockstore for a given shard for shard read affinity, i.e., further reads will likely be from the same shard. + // Maps (shard key -> blockstore). cache *lru.Cache metrics *metrics } -// accessorWithBlockstore is the value that we store in the blockstore Cache. Implements Accessor -// interface +// accessorWithBlockstore is the value that we store in the blockstore Cache. It implements the Accessor interface. type accessorWithBlockstore struct { sync.RWMutex shardAccessor Accessor - // blockstore is stored separately because each access to the blockstore over the shard accessor - // reopens the underlying CAR. + // The blockstore is stored separately because each access to the blockstore over the shard accessor reopens the underlying CAR. bs dagstore.ReadBlockstore } @@ -43,7 +41,7 @@ func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { bc := &AccessorCache{ name: name, } - // instantiate the blockstore Cache + // Instantiate the blockstore Cache. bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) if err != nil { return nil, fmt.Errorf("failed to instantiate blockstore cache: %w", err) @@ -52,10 +50,10 @@ func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { return bc, nil } -// evictFn will be invoked when item is evicted from the cache +// evictFn will be invoked when an item is evicted from the cache. func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { return func(_ interface{}, val interface{}) { - // ensure we close the blockstore for a shard when it's evicted so dagstore can gc it. + // Ensure we close the blockstore for a shard when it's evicted so dagstore can garbage collect it. abs, ok := val.(*accessorWithBlockstore) if !ok { panic(fmt.Sprintf( @@ -73,7 +71,7 @@ func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { } // Get retrieves the Accessor for a given shard key from the Cache. If the Accessor is not in -// the Cache, it returns an ErrCacheMiss +// the Cache, it returns an ErrCacheMiss. func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() @@ -89,7 +87,7 @@ func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { } func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { - // We've already ensured that the given shard has the cid/multihash we are looking for. + // We've already ensured that the given shard has the CID/multihash we are looking for. val, ok := bc.cache.Get(key) if !ok { return nil, ErrCacheMiss @@ -105,7 +103,7 @@ func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { return abs, nil } -// GetOrLoad attempts to get an item from cache, and if not found, invokes +// GetOrLoad attempts to get an item from the cache, and if not found, invokes // the provided loader function to load it. func (bc *AccessorCache) GetOrLoad( ctx context.Context, @@ -122,15 +120,15 @@ func (bc *AccessorCache) GetOrLoad( provider, err := loader(ctx, key) if err != nil { - return nil, fmt.Errorf("unable to get accessor: %w", err) + return nil, fmt.Errorf("unable to load accessor: %w", err) } abs := &accessorWithBlockstore{ shardAccessor: provider, } - // create new accessor first to inc ref count in it, so it could not get evicted from inner cache - // before it is used + // Create a new accessor first to increment the reference count in it, so it cannot get evicted from the inner lru cache + // before it is used. accessor, err := newCloser(abs) if err != nil { return nil, err @@ -139,22 +137,22 @@ func (bc *AccessorCache) GetOrLoad( return accessor, nil } -// Remove removes Accessor for given key from the cache +// Remove removes the Accessor for a given key from the cache. func (bc *AccessorCache) Remove(key shard.Key) error { - // cache will call evictFn on removal, where accessor close will be called + // The cache will call evictFn on removal, where accessor close will be called. bc.cache.Remove(key) return nil } -// EnableMetrics enables metrics for the cache +// EnableMetrics enables metrics for the cache. func (bc *AccessorCache) EnableMetrics() error { var err error bc.metrics, err = newMetrics(bc) return err } -// Blockstore implements Blockstore of the Accessor interface. It creates blockstore on first -// request and reuses created instance for all next requests. +// Blockstore implements the Blockstore of the Accessor interface. It creates the blockstore on the first +// request and reuses the created instance for all subsequent requests. func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { s.Lock() defer s.Unlock() @@ -166,12 +164,12 @@ func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { return s.bs, err } -// Reader returns new copy of reader to read data +// Reader returns a new copy of the reader to read data. func (s *accessorWithBlockstore) Reader() io.Reader { return s.shardAccessor.Reader() } -// accessorCloser is a temporal object before refs count is implemented +// accessorCloser is a temporary object before reference counting is implemented. type accessorCloser struct { *accessorWithBlockstore io.Closer diff --git a/share/eds/store.go b/share/eds/store.go index 11ebe7911f..b5c4e1934b 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -178,7 +178,6 @@ func (s *Store) gc(ctx context.Context) { } s.lastGCResult.Store(res) } - } } @@ -262,7 +261,7 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext select { case result = <-ch: case <-ctx.Done(): - // if context finished before result was received, track result in separate goroutine + // if the context finished before the result was received, track the result in a separate goroutine go trackLateResult("put", ch, s.metrics, time.Minute*5) return ctx.Err() } @@ -271,11 +270,11 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to register shard: %w", result.Error) } - // accessor returned in result will be nil, so shard needs to be acquired first, to become - // available in cache. It might take some time and result should not affect put operation, so do it - // in goroutine - //TODO: Ideally only recent blocks should be put in cache, but there is no way right now to check - // such condition. + // the accessor returned in the result will be nil, so the shard needs to be acquired first to become + // available in the cache. It might take some time, and the result should not affect the put operation, so do it + // in a goroutine + // TODO: Ideally, only recent blocks should be put in the cache, but there is no way right now to check + // such a condition. go func() { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() @@ -339,9 +338,9 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, if err == nil { return newReadCloser(accessor), nil } - // If the accessor is not found in the cache, create a new one from dagstore. We don't put accessor - // to the cache here, because getCAR is used by shrex.eds. There is a lower probability, compared - // to other cache put triggers, that the same block to be requested again. + // If the accessor is not found in the cache, create a new one from dagstore. We don't put the accessor + // in the cache here because getCAR is used by shrex-eds. There is a lower probability, compared + // to other cache put triggers, that the same block will be requested again soon. shardAccessor, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) @@ -384,7 +383,7 @@ func (s *Store) carBlockstore( return blockstoreCloser(accessor) } - // if accessor not found in cache, create new one from dagstore + // if the accessor is not found in the cache, create a new one from dagstore sa, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) @@ -405,7 +404,7 @@ func (s *Store) GetDAH(ctx context.Context, root share.DataHash) (*share.Root, e func (s *Store) getDAH(ctx context.Context, root share.DataHash) (*share.Root, error) { r, err := s.getCAR(ctx, root) if err != nil { - return nil, fmt.Errorf("eds/store: failed to get accessor: %w", err) + return nil, fmt.Errorf("eds/store: failed to get CAR file: %w", err) } defer closeAndLog("car reader", r) From 718a80a3653baa4d3fab5a59b14765291887ecfa Mon Sep 17 00:00:00 2001 From: Vlad Date: Fri, 8 Sep 2023 19:10:17 +0300 Subject: [PATCH 11/14] fix typo --- share/p2p/shrexeds/server.go | 1 - 1 file changed, 1 deletion(-) diff --git a/share/p2p/shrexeds/server.go b/share/p2p/shrexeds/server.go index 8cb5385446..11b99a3438 100644 --- a/share/p2p/shrexeds/server.go +++ b/share/p2p/shrexeds/server.go @@ -108,7 +108,6 @@ func (s *Server) handleStream(stream network.Stream) { log.Warnw("closing car reader", "err", err) } }() - edsReader.Close() status = p2p_pb.Status_OK case errors.Is(err, eds.ErrNotFound): logger.Warnw("server: request hash not found") From 795735af45a784aa754c2304f1669c5b4c7ecd0c Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 13 Sep 2023 14:11:55 +0300 Subject: [PATCH 12/14] - don't check multiple shard from inverted index keys in cache - minor renames --- share/eds/blockstore.go | 12 +++++------ share/eds/cache/accessor_cache.go | 35 ++++++++++++++++++------------- share/eds/store.go | 16 +++++++------- 3 files changed, 33 insertions(+), 30 deletions(-) diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index bde71ffee6..8b8655ce71 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -163,15 +163,13 @@ func (bs *blockstore) getReadOnlyBlockstore(ctx context.Context, cid cid.Cid) (* return nil, fmt.Errorf("failed to find shards containing multihash: %w", err) } - // a share can exist in multiple EDSes, check cache to contain any of accessors containing shard - for _, k := range keys { - if accessor, err := bs.store.cache.Get(k); err == nil { - return blockstoreCloser(accessor) - } + // check if cache contains any of accessors + shardKey := keys[0] + if accessor, err := bs.store.cache.Get(shardKey); err == nil { + return blockstoreCloser(accessor) } - // a share can exist in multiple EDSes, so just take the first one. - shardKey := keys[0] + // load accessor to the cache and use it as blockstoreCloser accessor, err := bs.store.cache.GetOrLoad(ctx, shardKey, bs.store.getAccessor) if err != nil { return nil, fmt.Errorf("failed to get accessor for shard %s: %w", shardKey, err) diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index b92b818261..22412abfb4 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -22,18 +22,20 @@ type AccessorCache struct { // of using only one lock or one lock per key, we stripe the shard keys across 256 locks. 256 is // chosen because it 0-255 is the range of values we get looking at the last byte of the key. stripedLocks [256]sync.Mutex - // Caches the blockstore for a given shard for shard read affinity, i.e., further reads will likely be from the same shard. - // Maps (shard key -> blockstore). + // Caches the blockstore for a given shard for shard read affinity, i.e., further reads will likely + // be from the same shard. Maps (shard key -> blockstore). cache *lru.Cache metrics *metrics } -// accessorWithBlockstore is the value that we store in the blockstore Cache. It implements the Accessor interface. +// accessorWithBlockstore is the value that we store in the blockstore Cache. It implements the +// Accessor interface. type accessorWithBlockstore struct { sync.RWMutex shardAccessor Accessor - // The blockstore is stored separately because each access to the blockstore over the shard accessor reopens the underlying CAR. + // The blockstore is stored separately because each access to the blockstore over the shard + // accessor reopens the underlying CAR. bs dagstore.ReadBlockstore } @@ -114,27 +116,30 @@ func (bc *AccessorCache) GetOrLoad( lk.Lock() defer lk.Unlock() - if accessor, err := bc.get(key); err == nil { - return newCloser(accessor) + abs, err := bc.get(key) + if err == nil { + bc.metrics.observeGet(true) + return newCloser(abs) } - provider, err := loader(ctx, key) + // accessor not found in cache, so load new one using loader + accessor, err := loader(ctx, key) if err != nil { return nil, fmt.Errorf("unable to load accessor: %w", err) } - abs := &accessorWithBlockstore{ - shardAccessor: provider, + abs = &accessorWithBlockstore{ + shardAccessor: accessor, } - // Create a new accessor first to increment the reference count in it, so it cannot get evicted from the inner lru cache - // before it is used. - accessor, err := newCloser(abs) + // Create a new accessor first to increment the reference count in it, so it cannot get evicted + // from the inner lru cache before it is used. + ac, err := newCloser(abs) if err != nil { return nil, err } bc.cache.Add(key, abs) - return accessor, nil + return ac, nil } // Remove removes the Accessor for a given key from the cache. @@ -151,8 +156,8 @@ func (bc *AccessorCache) EnableMetrics() error { return err } -// Blockstore implements the Blockstore of the Accessor interface. It creates the blockstore on the first -// request and reuses the created instance for all subsequent requests. +// Blockstore implements the Blockstore of the Accessor interface. It creates the blockstore on the +// first request and reuses the created instance for all subsequent requests. func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { s.Lock() defer s.Unlock() diff --git a/share/eds/store.go b/share/eds/store.go index 9884da72b3..5c103cb340 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -277,11 +277,11 @@ func (s *Store) put(ctx context.Context, root share.DataHash, square *rsmt2d.Ext return fmt.Errorf("failed to register shard: %w", result.Error) } - // the accessor returned in the result will be nil, so the shard needs to be acquired first to become - // available in the cache. It might take some time, and the result should not affect the put operation, so do it - // in a goroutine - // TODO: Ideally, only recent blocks should be put in the cache, but there is no way right now to check - // such a condition. + // the accessor returned in the result will be nil, so the shard needs to be acquired first to + // become available in the cache. It might take some time, and the result should not affect the put + // operation, so do it in a goroutine + // TODO: Ideally, only recent blocks should be put in the cache, but there is no way right now to + // check such a condition. go func() { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() @@ -345,9 +345,9 @@ func (s *Store) getCAR(ctx context.Context, root share.DataHash) (io.ReadCloser, if err == nil { return newReadCloser(accessor), nil } - // If the accessor is not found in the cache, create a new one from dagstore. We don't put the accessor - // in the cache here because getCAR is used by shrex-eds. There is a lower probability, compared - // to other cache put triggers, that the same block will be requested again soon. + // If the accessor is not found in the cache, create a new one from dagstore. We don't put the + // accessor in the cache here because getCAR is used by shrex-eds. There is a lower probability, + // compared to other cache put triggers, that the same block will be requested again soon. shardAccessor, err := s.getAccessor(ctx, key) if err != nil { return nil, fmt.Errorf("failed to get accessor: %w", err) From a1184016d666421529fce8a45dc2ea4ace37a03e Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 13 Sep 2023 19:38:14 +0300 Subject: [PATCH 13/14] - upg to golang-lru/v2 - track close errors in metrics - move BlockstoreCloser and readCloser to utils - add comment for sleep in test --- go.mod | 5 ++-- share/eds/blockstore.go | 17 ----------- share/eds/cache/accessor_cache.go | 49 +++++++++---------------------- share/eds/cache/metrics.go | 6 ++-- share/eds/store.go | 13 -------- share/eds/store_test.go | 22 +++++++++----- share/eds/utils.go | 37 ++++++++++++++++++++++- 7 files changed, 72 insertions(+), 77 deletions(-) diff --git a/go.mod b/go.mod index 0890bf64f1..cb0eec3cf7 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,6 @@ require ( github.com/golang/mock v1.6.0 github.com/gorilla/mux v1.8.0 github.com/hashicorp/go-retryablehttp v0.7.4 - github.com/hashicorp/golang-lru v1.0.2 github.com/imdario/mergo v0.3.16 github.com/ipfs/boxo v0.12.0 github.com/ipfs/go-block-format v0.1.2 @@ -76,6 +75,8 @@ require ( google.golang.org/protobuf v1.31.0 ) +require github.com/hashicorp/golang-lru v1.0.2 // indirect + require ( cloud.google.com/go v0.110.6 // indirect cloud.google.com/go/compute v1.23.0 // indirect @@ -186,7 +187,7 @@ require ( github.com/hashicorp/go-safetemp v1.0.0 // indirect github.com/hashicorp/go-version v1.6.0 // indirect github.com/hashicorp/golang-lru/arc/v2 v2.0.5 // indirect - github.com/hashicorp/golang-lru/v2 v2.0.5 // indirect + github.com/hashicorp/golang-lru/v2 v2.0.5 github.com/hashicorp/hcl v1.0.0 // indirect github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3 // indirect github.com/holiman/uint256 v1.2.2-0.20230321075855-87b91420868c // indirect diff --git a/share/eds/blockstore.go b/share/eds/blockstore.go index 8b8655ce71..349d6f58ba 100644 --- a/share/eds/blockstore.go +++ b/share/eds/blockstore.go @@ -4,15 +4,12 @@ import ( "context" "errors" "fmt" - "io" - "github.com/filecoin-project/dagstore" bstore "github.com/ipfs/boxo/blockstore" dshelp "github.com/ipfs/boxo/datastore/dshelp" blocks "github.com/ipfs/go-block-format" "github.com/ipfs/go-cid" "github.com/ipfs/go-datastore" - "github.com/ipfs/go-datastore/namespace" ipld "github.com/ipfs/go-ipld-format" "github.com/celestiaorg/celestia-node/share/eds/cache" @@ -38,20 +35,6 @@ type blockstore struct { ds datastore.Batching } -// BlockstoreCloser represents a blockstore that can also be closed. It combines the functionality -// of a dagstore.ReadBlockstore with that of an io.Closer. -type BlockstoreCloser struct { - dagstore.ReadBlockstore - io.Closer -} - -func newBlockstore(store *Store, ds datastore.Batching) *blockstore { - return &blockstore{ - store: store, - ds: namespace.Wrap(ds, blockstoreCacheKey), - } -} - func (bs *blockstore) Has(ctx context.Context, cid cid.Cid) (bool, error) { keys, err := bs.store.dgstr.ShardsContainingMultihash(ctx, cid.Hash()) if errors.Is(err, ErrNotFound) || errors.Is(err, ErrNotFoundInIndex) { diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index 22412abfb4..5de795ac8e 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -4,12 +4,11 @@ import ( "context" "fmt" "io" - "reflect" "sync" "github.com/filecoin-project/dagstore" "github.com/filecoin-project/dagstore/shard" - lru "github.com/hashicorp/golang-lru" + lru "github.com/hashicorp/golang-lru/v2" ) var _ Cache = (*AccessorCache)(nil) @@ -24,7 +23,7 @@ type AccessorCache struct { stripedLocks [256]sync.Mutex // Caches the blockstore for a given shard for shard read affinity, i.e., further reads will likely // be from the same shard. Maps (shard key -> blockstore). - cache *lru.Cache + cache *lru.Cache[shard.Key, *accessorWithBlockstore] metrics *metrics } @@ -44,7 +43,7 @@ func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { name: name, } // Instantiate the blockstore Cache. - bslru, err := lru.NewWithEvict(cacheSize, bc.evictFn()) + bslru, err := lru.NewWithEvict[shard.Key, *accessorWithBlockstore](cacheSize, bc.evictFn()) if err != nil { return nil, fmt.Errorf("failed to instantiate blockstore cache: %w", err) } @@ -53,22 +52,15 @@ func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { } // evictFn will be invoked when an item is evicted from the cache. -func (bc *AccessorCache) evictFn() func(_ interface{}, val interface{}) { - return func(_ interface{}, val interface{}) { - // Ensure we close the blockstore for a shard when it's evicted so dagstore can garbage collect it. - abs, ok := val.(*accessorWithBlockstore) - if !ok { - panic(fmt.Sprintf( - "casting value from cache to accessorWithBlockstore: %s", - reflect.TypeOf(val), - )) - } - +func (bc *AccessorCache) evictFn() func(shard.Key, *accessorWithBlockstore) { + return func(_ shard.Key, abs *accessorWithBlockstore) { err := abs.shardAccessor.Close() if err != nil { + bc.metrics.observeEvicted(true) log.Errorf("couldn't close accessor after cache eviction: %s", err) + return } - bc.metrics.observeEvicted() + bc.metrics.observeEvicted(false) } } @@ -85,23 +77,14 @@ func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { return nil, err } bc.metrics.observeGet(true) - return newCloser(accessor) + return newCloser(accessor), nil } func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { - // We've already ensured that the given shard has the CID/multihash we are looking for. - val, ok := bc.cache.Get(key) + abs, ok := bc.cache.Get(key) if !ok { return nil, ErrCacheMiss } - - abs, ok := val.(*accessorWithBlockstore) - if !ok { - panic(fmt.Sprintf( - "casting value from cache to accessorWithBlockstore: %s", - reflect.TypeOf(val), - )) - } return abs, nil } @@ -119,7 +102,7 @@ func (bc *AccessorCache) GetOrLoad( abs, err := bc.get(key) if err == nil { bc.metrics.observeGet(true) - return newCloser(abs) + return newCloser(abs), nil } // accessor not found in cache, so load new one using loader @@ -134,10 +117,7 @@ func (bc *AccessorCache) GetOrLoad( // Create a new accessor first to increment the reference count in it, so it cannot get evicted // from the inner lru cache before it is used. - ac, err := newCloser(abs) - if err != nil { - return nil, err - } + ac := newCloser(abs) bc.cache.Add(key, abs) return ac, nil } @@ -165,7 +145,6 @@ func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { if s.bs == nil { s.bs, err = s.shardAccessor.Blockstore() } - return s.bs, err } @@ -180,11 +159,11 @@ type accessorCloser struct { io.Closer } -func newCloser(abs *accessorWithBlockstore) (*accessorCloser, error) { +func newCloser(abs *accessorWithBlockstore) *accessorCloser { return &accessorCloser{ accessorWithBlockstore: abs, Closer: io.NopCloser(nil), - }, nil + } } // shardKeyToStriped returns the index of the lock to use for a given shard key. We use the last diff --git a/share/eds/cache/metrics.go b/share/eds/cache/metrics.go index 2aef5dc36a..21e52fec10 100644 --- a/share/eds/cache/metrics.go +++ b/share/eds/cache/metrics.go @@ -9,6 +9,7 @@ import ( const ( cacheFoundKey = "found" + failedKey = "failed" ) type metrics struct { @@ -48,11 +49,12 @@ func newMetrics(bc *AccessorCache) (*metrics, error) { }, err } -func (m *metrics) observeEvicted() { +func (m *metrics) observeEvicted(failed bool) { if m == nil { return } - m.evictedCounter.Add(context.Background(), 1) + m.evictedCounter.Add(context.Background(), 1, metric.WithAttributes( + attribute.Bool(failedKey, failed))) } func (m *metrics) observeGet(found bool) { diff --git a/share/eds/store.go b/share/eds/store.go index 5c103cb340..b58298e940 100644 --- a/share/eds/store.go +++ b/share/eds/store.go @@ -636,16 +636,3 @@ type inMemoryReader struct { func (r *inMemoryReader) Close() error { return nil } - -// readCloser is a helper struct, that combines io.Reader and io.Closer -type readCloser struct { - io.Reader - io.Closer -} - -func newReadCloser(ac cache.Accessor) io.ReadCloser { - return readCloser{ - ac.Reader(), - ac, - } -} diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 1d5a07a04e..1c724082ca 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -113,7 +113,9 @@ func TestEDSStore(t *testing.T) { _, err = os.Stat(edsStore.basepath + blocksPath + dah.String()) assert.NoError(t, err) + // accessor will be registered in cache async on put, so give it some time to settle time.Sleep(time.Millisecond * 100) + err = edsStore.Remove(ctx, dah.Hash()) assert.NoError(t, err) @@ -150,8 +152,10 @@ func TestEDSStore(t *testing.T) { err = os.Remove(path) assert.NoError(t, err) - // remove non-failed accessor from cache + // accessor will be registered in cache async on put, so give it some time to settle time.Sleep(time.Millisecond * 100) + + // remove non-failed accessor from cache err = edsStore.cache.Remove(shard.KeyFromString(dah.String())) assert.NoError(t, err) @@ -194,8 +198,10 @@ func TestEDSStore(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // check, that the key is in the cache after put + // accessor will be registered in cache async on put, so give it some time to settle time.Sleep(time.Millisecond * 100) + + // check, that the key is in the cache after put shardKey := shard.KeyFromString(dah.String()) _, err = edsStore.cache.Get(shardKey) assert.NoError(t, err) @@ -262,8 +268,10 @@ func TestEDSStore_GC(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // remove links to the shard from cache + // accessor will be registered in cache async on put, so give it some time to settle time.Sleep(time.Millisecond * 100) + + // remove links to the shard from cache key := shard.KeyFromString(share.DataHash(dah.Hash()).String()) err = edsStore.cache.Remove(key) require.NoError(t, err) @@ -347,8 +355,8 @@ func Test_CachedAccessor(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // give some time to let cache to get settled in background - time.Sleep(time.Millisecond * 50) + // accessor will be registered in cache async on put, so give it some time to settle + time.Sleep(time.Millisecond * 100) // accessor should be in cache cachedAccessor, err := edsStore.cache.Get(shard.KeyFromString(dah.String())) @@ -386,8 +394,8 @@ func Test_NotCachedAccessor(t *testing.T) { err = edsStore.Put(ctx, dah.Hash(), eds) require.NoError(t, err) - // give some time to let cache to get settled in background - time.Sleep(time.Millisecond * 50) + // accessor will be registered in cache async on put, so give it some time to settle + time.Sleep(time.Millisecond * 100) // accessor should be in cache _, err = edsStore.cache.Get(shard.KeyFromString(dah.String())) diff --git a/share/eds/utils.go b/share/eds/utils.go index 26d77ae3ee..e7b24a9aee 100644 --- a/share/eds/utils.go +++ b/share/eds/utils.go @@ -1,6 +1,41 @@ package eds -import "io" +import ( + "io" + + "github.com/filecoin-project/dagstore" + "github.com/ipfs/go-datastore" + "github.com/ipfs/go-datastore/namespace" + + "github.com/celestiaorg/celestia-node/share/eds/cache" +) + +// readCloser is a helper struct, that combines io.Reader and io.Closer +type readCloser struct { + io.Reader + io.Closer +} + +// BlockstoreCloser represents a blockstore that can also be closed. It combines the functionality +// of a dagstore.ReadBlockstore with that of an io.Closer. +type BlockstoreCloser struct { + dagstore.ReadBlockstore + io.Closer +} + +func newReadCloser(ac cache.Accessor) io.ReadCloser { + return readCloser{ + ac.Reader(), + ac, + } +} + +func newBlockstore(store *Store, ds datastore.Batching) *blockstore { + return &blockstore{ + store: store, + ds: namespace.Wrap(ds, blockstoreCacheKey), + } +} func closeAndLog(name string, closer io.Closer) { if err := closer.Close(); err != nil { From 3880b30fe90513e5f951bd58fbcaf151723ab2c7 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 14 Sep 2023 19:17:59 +0300 Subject: [PATCH 14/14] - reoreder methods in accessor_cache.go - make errCacheMiss private - fix go.mod --- go.mod | 5 ++-- share/eds/cache/accessor_cache.go | 38 +++++++++++++------------- share/eds/cache/accessor_cache_test.go | 4 +-- share/eds/cache/cache.go | 2 +- share/eds/cache/noop.go | 2 +- share/eds/store_test.go | 4 +-- 6 files changed, 27 insertions(+), 28 deletions(-) diff --git a/go.mod b/go.mod index cb0eec3cf7..6937b91883 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/golang/mock v1.6.0 github.com/gorilla/mux v1.8.0 github.com/hashicorp/go-retryablehttp v0.7.4 + github.com/hashicorp/golang-lru/v2 v2.0.5 github.com/imdario/mergo v0.3.16 github.com/ipfs/boxo v0.12.0 github.com/ipfs/go-block-format v0.1.2 @@ -75,8 +76,6 @@ require ( google.golang.org/protobuf v1.31.0 ) -require github.com/hashicorp/golang-lru v1.0.2 // indirect - require ( cloud.google.com/go v0.110.6 // indirect cloud.google.com/go/compute v1.23.0 // indirect @@ -186,8 +185,8 @@ require ( github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/hashicorp/go-safetemp v1.0.0 // indirect github.com/hashicorp/go-version v1.6.0 // indirect + github.com/hashicorp/golang-lru v1.0.2 // indirect github.com/hashicorp/golang-lru/arc/v2 v2.0.5 // indirect - github.com/hashicorp/golang-lru/v2 v2.0.5 github.com/hashicorp/hcl v1.0.0 // indirect github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3 // indirect github.com/holiman/uint256 v1.2.2-0.20230321075855-87b91420868c // indirect diff --git a/share/eds/cache/accessor_cache.go b/share/eds/cache/accessor_cache.go index 5de795ac8e..ff955f8c45 100644 --- a/share/eds/cache/accessor_cache.go +++ b/share/eds/cache/accessor_cache.go @@ -38,6 +38,23 @@ type accessorWithBlockstore struct { bs dagstore.ReadBlockstore } +// Blockstore implements the Blockstore of the Accessor interface. It creates the blockstore on the +// first request and reuses the created instance for all subsequent requests. +func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { + s.Lock() + defer s.Unlock() + var err error + if s.bs == nil { + s.bs, err = s.shardAccessor.Blockstore() + } + return s.bs, err +} + +// Reader returns a new copy of the reader to read data. +func (s *accessorWithBlockstore) Reader() io.Reader { + return s.shardAccessor.Reader() +} + func NewAccessorCache(name string, cacheSize int) (*AccessorCache, error) { bc := &AccessorCache{ name: name, @@ -65,7 +82,7 @@ func (bc *AccessorCache) evictFn() func(shard.Key, *accessorWithBlockstore) { } // Get retrieves the Accessor for a given shard key from the Cache. If the Accessor is not in -// the Cache, it returns an ErrCacheMiss. +// the Cache, it returns an errCacheMiss. func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { lk := &bc.stripedLocks[shardKeyToStriped(key)] lk.Lock() @@ -83,7 +100,7 @@ func (bc *AccessorCache) Get(key shard.Key) (Accessor, error) { func (bc *AccessorCache) get(key shard.Key) (*accessorWithBlockstore, error) { abs, ok := bc.cache.Get(key) if !ok { - return nil, ErrCacheMiss + return nil, errCacheMiss } return abs, nil } @@ -136,23 +153,6 @@ func (bc *AccessorCache) EnableMetrics() error { return err } -// Blockstore implements the Blockstore of the Accessor interface. It creates the blockstore on the -// first request and reuses the created instance for all subsequent requests. -func (s *accessorWithBlockstore) Blockstore() (dagstore.ReadBlockstore, error) { - s.Lock() - defer s.Unlock() - var err error - if s.bs == nil { - s.bs, err = s.shardAccessor.Blockstore() - } - return s.bs, err -} - -// Reader returns a new copy of the reader to read data. -func (s *accessorWithBlockstore) Reader() io.Reader { - return s.shardAccessor.Reader() -} - // accessorCloser is a temporary object before reference counting is implemented. type accessorCloser struct { *accessorWithBlockstore diff --git a/share/eds/cache/accessor_cache_test.go b/share/eds/cache/accessor_cache_test.go index 1e4beaf4d1..5e928e85cc 100644 --- a/share/eds/cache/accessor_cache_test.go +++ b/share/eds/cache/accessor_cache_test.go @@ -99,7 +99,7 @@ func TestAccessorCache(t *testing.T) { // check if item exists _, err = cache.Get(key) - require.ErrorIs(t, err, ErrCacheMiss) + require.ErrorIs(t, err, errCacheMiss) }) t.Run("successive reads should read the same data", func(t *testing.T) { @@ -159,7 +159,7 @@ func TestAccessorCache(t *testing.T) { // check if item evicted _, err = cache.Get(key) - require.ErrorIs(t, err, ErrCacheMiss) + require.ErrorIs(t, err, errCacheMiss) }) t.Run("close on accessor is noop", func(t *testing.T) { diff --git a/share/eds/cache/cache.go b/share/eds/cache/cache.go index 79bf87d0ae..13e207d7c0 100644 --- a/share/eds/cache/cache.go +++ b/share/eds/cache/cache.go @@ -17,7 +17,7 @@ var ( ) var ( - ErrCacheMiss = errors.New("accessor not found in blockstore cache") + errCacheMiss = errors.New("accessor not found in blockstore cache") ) // Cache is an interface that defines the basic Cache operations. diff --git a/share/eds/cache/noop.go b/share/eds/cache/noop.go index 42b6a2accb..2af94feb1b 100644 --- a/share/eds/cache/noop.go +++ b/share/eds/cache/noop.go @@ -12,7 +12,7 @@ var _ Cache = (*NoopCache)(nil) type NoopCache struct{} func (n NoopCache) Get(shard.Key) (Accessor, error) { - return nil, ErrCacheMiss + return nil, errCacheMiss } func (n NoopCache) GetOrLoad( diff --git a/share/eds/store_test.go b/share/eds/store_test.go index 1c724082ca..620ad077bf 100644 --- a/share/eds/store_test.go +++ b/share/eds/store_test.go @@ -329,7 +329,7 @@ func Test_BlockstoreCache(t *testing.T) { // key shouldn't be in cache yet, check for returned errCacheMiss shardKey := shard.KeyFromString(dah.String()) _, err = edsStore.cache.Get(shardKey) - require.ErrorIs(t, err, cache.ErrCacheMiss) + require.Error(t, err) // now get it from blockstore, to trigger storing to cache _, err = edsStore.Blockstore().Get(ctx, key) @@ -399,7 +399,7 @@ func Test_NotCachedAccessor(t *testing.T) { // accessor should be in cache _, err = edsStore.cache.Get(shard.KeyFromString(dah.String())) - require.ErrorIs(t, err, cache.ErrCacheMiss) + require.Error(t, err) // first read from direct accessor carReader, err := edsStore.getCAR(ctx, dah.Hash())