Skip to content

Commit

Permalink
Lock dirty segs before incrementing ref counts (#12275)
Browse files Browse the repository at this point in the history
This PR removes a race condition from dirty segments processing by:

* ref counting finalized blocks - so they get ignored during open/close
* lock the dirty segments on begin transaction

The lock prevents the race condition where the retire process reads the
refcount and starts processing the reopen of a file, then the view
starts a transaction.

The view iteration generally happens faster than the file operations -
causing a race as the refcount is incremented while the operations is
happening.

I've tested this by running with race from scratch for amoy - and it now
produces no data races, and panics.
  • Loading branch information
mh0lt authored Oct 11, 2024
1 parent 6c82d93 commit 174116c
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 6 deletions.
27 changes: 21 additions & 6 deletions turbo/snapshotsync/freezeblocks/block_snapshots.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,9 +429,7 @@ func (s *segments) Segment(blockNum uint64, f func(*VisibleSegment) error) (foun

func (s *segments) BeginRotx() *segmentsRotx {
for _, seg := range s.VisibleSegments {
if !seg.src.frozen {
seg.src.refcount.Add(1)
}
seg.src.refcount.Add(1)
}
return &segmentsRotx{segments: s, VisibleSegments: s.VisibleSegments}
}
Expand All @@ -445,10 +443,13 @@ func (s *segmentsRotx) Close() {

for i := range VisibleSegments {
src := VisibleSegments[i].src
if src == nil || src.frozen {
if src == nil {
continue
}
refCnt := src.refcount.Add(-1)
if src.frozen {
continue
}
if refCnt == 0 && src.canDelete.Load() {
src.closeAndRemoveFiles()
}
Expand Down Expand Up @@ -893,11 +894,13 @@ func (s *RoSnapshots) Close() {
if s == nil {
return
}

// defer to preserve lock order
defer s.recalcVisibleFiles()
s.dirtySegmentsLock.Lock()
defer s.dirtySegmentsLock.Unlock()

s.closeWhatNotInList(nil)
s.recalcVisibleFiles()
}

func (s *RoSnapshots) closeWhatNotInList(l []string) {
Expand Down Expand Up @@ -2460,6 +2463,9 @@ func (s *RoSnapshots) View() *View {

var sgs btree.Map[snaptype.Enum, *segmentsRotx]
s.segments.Scan(func(segtype snaptype.Enum, value *segments) bool {
// BeginRo increments refcount - which is contended
s.dirtySegmentsLock.RLock()
defer s.dirtySegmentsLock.RUnlock()
sgs.Set(segtype, value.BeginRotx())
return true
})
Expand Down Expand Up @@ -2488,6 +2494,9 @@ func (s *RoSnapshots) ViewType(t snaptype.Type) *segmentsRotx {
if !ok {
return nil
}
// BeginRo increments refcount - which is contended
s.dirtySegmentsLock.RLock()
defer s.dirtySegmentsLock.RUnlock()
return seg.BeginRotx()
}

Expand All @@ -2504,7 +2513,13 @@ func (s *RoSnapshots) ViewSingleFile(t snaptype.Type, blockNum uint64) (segment
return nil, false, noop
}

segmentRotx := segs.BeginRotx()
segmentRotx := func() *segmentsRotx {
// BeginRo increments refcount - which is contended
s.dirtySegmentsLock.RLock()
defer s.dirtySegmentsLock.RUnlock()
return segs.BeginRotx()
}()

for _, seg := range segmentRotx.VisibleSegments {
if !(blockNum >= seg.from && blockNum < seg.to) {
continue
Expand Down
3 changes: 3 additions & 0 deletions turbo/snapshotsync/freezeblocks/caplin_snapshots.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@ func (s *CaplinSnapshots) View() *CaplinView {
defer s.visibleSegmentsLock.RUnlock()

v := &CaplinView{s: s}
// BeginRo increments refcount - which is contended
s.dirtySegmentsLock.RLock()
defer s.dirtySegmentsLock.RUnlock()
if s.BeaconBlocks != nil {
v.BeaconBlockRotx = s.BeaconBlocks.BeginRotx()
}
Expand Down

0 comments on commit 174116c

Please sign in to comment.