-
Notifications
You must be signed in to change notification settings - Fork 380
fix: pin eviction guard #5222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: pin eviction guard #5222
Changes from all commits
07ce3a5
d8dfcd9
b22264a
328e37e
cbae5b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import ( | |
| "github.com/ethersphere/bee/v2/pkg/postage" | ||
| "github.com/ethersphere/bee/v2/pkg/storage" | ||
| "github.com/ethersphere/bee/v2/pkg/storer/internal/chunkstamp" | ||
| pinstore "github.com/ethersphere/bee/v2/pkg/storer/internal/pinning" | ||
| "github.com/ethersphere/bee/v2/pkg/storer/internal/stampindex" | ||
| "github.com/ethersphere/bee/v2/pkg/storer/internal/transaction" | ||
| "github.com/ethersphere/bee/v2/pkg/swarm" | ||
|
|
@@ -327,6 +328,7 @@ func (r *Reserve) Get(ctx context.Context, addr swarm.Address, batchID []byte, s | |
| } | ||
|
|
||
| // EvictBatchBin evicts all chunks from bins upto the bin provided. | ||
| // Pinned chunks are protected from eviction to maintain data integrity. | ||
| func (r *Reserve) EvictBatchBin( | ||
| ctx context.Context, | ||
| batchID []byte, | ||
|
|
@@ -336,21 +338,46 @@ func (r *Reserve) EvictBatchBin( | |
| r.multx.Lock(string(batchID)) | ||
| defer r.multx.Unlock(string(batchID)) | ||
|
|
||
| var evicteditems []*BatchRadiusItem | ||
| var ( | ||
| evictedItems []*BatchRadiusItem | ||
| pinnedEvictedItems []*BatchRadiusItem | ||
| ) | ||
|
|
||
| if count <= 0 { | ||
| return 0, nil | ||
| } | ||
|
|
||
| err := r.st.IndexStore().Iterate(storage.Query{ | ||
| pinUuids, err := pinstore.GetCollectionUUIDs(r.st.IndexStore()) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| err = r.st.IndexStore().Iterate(storage.Query{ | ||
| Factory: func() storage.Item { return &BatchRadiusItem{} }, | ||
| Prefix: string(batchID), | ||
| }, func(res storage.Result) (bool, error) { | ||
| batchRadius := res.Entry.(*BatchRadiusItem) | ||
| if batchRadius.Bin >= bin { | ||
| return true, nil | ||
| } | ||
| evicteditems = append(evicteditems, batchRadius) | ||
|
|
||
| // Check if the chunk is pinned in any collection | ||
| pinned := false | ||
| for _, uuid := range pinUuids { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will slow down batch eviction linearly with the number of collections. Is there a possibility to have collection uuid as a field in BatchRadiusItem so that all of these lookups (Has method calls) are not done? From what I see, it would need some complexity to manage the synchronisations between BatchRadiusItem and pinChunkItem keys, but if the performance degradation is significant, it may be necessary. It is ok to try with this simpler solution.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is information redundant approach but faster one for sure. |
||
| has, err := pinstore.IsChunkPinnedInCollection(r.st.IndexStore(), batchRadius.Address, uuid) | ||
| if err != nil { | ||
| return true, err | ||
| } | ||
| if has { | ||
| pinned = true | ||
| pinnedEvictedItems = append(pinnedEvictedItems, batchRadius) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !pinned { | ||
| evictedItems = append(evictedItems, batchRadius) | ||
| } | ||
| count-- | ||
| if count == 0 { | ||
| return true, nil | ||
|
|
@@ -366,7 +393,7 @@ func (r *Reserve) EvictBatchBin( | |
|
|
||
| var evicted atomic.Int64 | ||
|
|
||
| for _, item := range evicteditems { | ||
| for _, item := range evictedItems { | ||
| func(item *BatchRadiusItem) { | ||
| eg.Go(func() error { | ||
| err := r.st.Run(ctx, func(s transaction.Store) error { | ||
|
|
@@ -381,6 +408,21 @@ func (r *Reserve) EvictBatchBin( | |
| }(item) | ||
| } | ||
|
|
||
| for _, item := range pinnedEvictedItems { | ||
| func(item *BatchRadiusItem) { | ||
| eg.Go(func() error { | ||
| err := r.st.Run(ctx, func(s transaction.Store) error { | ||
| return RemoveChunkMetaData(ctx, s, item) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want to remove the Pinned Chunks metadata, would this affect retrieval of those pinned chunks if storage pressure eviction or manual eviction? |
||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| evicted.Add(1) | ||
| return nil | ||
| }) | ||
| }(item) | ||
| } | ||
|
|
||
| err = eg.Wait() | ||
|
|
||
| r.size.Add(-evicted.Load()) | ||
|
|
@@ -430,6 +472,29 @@ func RemoveChunkWithItem( | |
| ) | ||
| } | ||
|
|
||
| // RemoveChunkMetaData removes chunk reserve metadata from reserve indexes but keeps the cunks in the chunkstore. | ||
| // used at pinned data eviction | ||
| func RemoveChunkMetaData( | ||
| ctx context.Context, | ||
| trx transaction.Store, | ||
| item *BatchRadiusItem, | ||
| ) error { | ||
| var errs error | ||
|
|
||
| stamp, _ := chunkstamp.LoadWithStampHash(trx.IndexStore(), reserveScope, item.Address, item.StampHash) | ||
| if stamp != nil { | ||
| errs = errors.Join( | ||
| stampindex.Delete(trx.IndexStore(), reserveScope, stamp), | ||
| chunkstamp.DeleteWithStamp(trx.IndexStore(), reserveScope, item.Address, stamp), | ||
| ) | ||
| } | ||
|
|
||
| return errors.Join(errs, | ||
| trx.IndexStore().Delete(item), | ||
| trx.IndexStore().Delete(&ChunkBinItem{Bin: item.Bin, BinID: item.BinID}), | ||
| ) | ||
| } | ||
|
|
||
| func (r *Reserve) IterateBin(bin uint8, startBinID uint64, cb func(swarm.Address, uint64, []byte, []byte) (bool, error)) error { | ||
| err := r.st.IndexStore().Iterate(storage.Query{ | ||
| Factory: func() storage.Item { return &ChunkBinItem{} }, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think to change names of this 2 collections and add some comment, something like: