Skip to content

Commit

Permalink
admission: squash datarace in (*StoreWorkqueue).gcSequencers()
Browse files Browse the repository at this point in the history
We see this (benign) data race in under --stress --race. Squash it by
targeted use of atomics.

  Read at 0x00c0039c06a8 by goroutine 31161:
    github.com/cockroachdb/cockroach/pkg/util/admission.(*StoreWorkQueue).gcSequencers()
        github.com/cockroachdb/cockroach/pkg/util/admission/work_queue.go:2176 +0x196
    github.com/cockroachdb/cockroach/pkg/util/admission.makeStoreWorkQueue.func1()
        github.com/cockroachdb/cockroach/pkg/util/admission/work_queue.go:2162 +0x5b

  Previous write at 0x00c0039c06a8 by goroutine 31105:
    github.com/cockroachdb/cockroach/pkg/util/admission.(*sequencer).sequence()
        github.com/cockroachdb/cockroach/pkg/util/admission/sequencer.go:61 +0x190
    github.com/cockroachdb/cockroach/pkg/util/admission.(*StoreWorkQueue).sequenceReplicatedWork()
        github.com/cockroachdb/cockroach/pkg/util/admission/work_queue.go:2193 +0x145
    github.com/cockroachdb/cockroach/pkg/util/admission.(*StoreWorkQueue).Admit()

Release note: None
  • Loading branch information
irfansharif committed Jun 9, 2023
1 parent 769c096 commit a743afb
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 2 deletions.
6 changes: 5 additions & 1 deletion pkg/util/admission/sequencer.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

package admission

import "sync/atomic"

// sequencer issues monotonic sequencing timestamps derived from observed
// CreateTimes. This is a purpose-built data structure for replication admission
// control where we want to assign each AC-queued work below-raft a "sequence
Expand Down Expand Up @@ -58,6 +60,8 @@ func (s *sequencer) sequence(createTime int64) int64 {
if createTime <= s.maxCreateTime {
createTime = s.maxCreateTime + 1
}
s.maxCreateTime = createTime
// This counter is read concurrently in gcSequencers. We use an atomic store
// here and an atomic load there, to avoid tripping up the race detector.
atomic.StoreInt64(&s.maxCreateTime, createTime)
return createTime
}
3 changes: 2 additions & 1 deletion pkg/util/admission/work_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"math"
"sort"
"sync"
"sync/atomic"
"time"

"github.com/cockroachdb/cockroach/pkg/base"
Expand Down Expand Up @@ -2173,7 +2174,7 @@ func (q *StoreWorkQueue) gcSequencers() {
defer q.sequencersMu.Unlock()

for rangeID, seq := range q.sequencersMu.s {
maxCreateTime := timeutil.FromUnixNanos(seq.maxCreateTime)
maxCreateTime := timeutil.FromUnixNanos(atomic.LoadInt64(&seq.maxCreateTime))
if q.timeSource.Now().Sub(maxCreateTime) > rangeSequencerGCThreshold.Get(&q.settings.SV) {
delete(q.sequencersMu.s, rangeID)
}
Expand Down

0 comments on commit a743afb

Please sign in to comment.