From a743afb8733133857a4c1c78dd268390022fd0b7 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Thu, 8 Jun 2023 23:38:10 -0400 Subject: [PATCH] admission: squash datarace in (*StoreWorkqueue).gcSequencers() 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 --- pkg/util/admission/sequencer.go | 6 +++++- pkg/util/admission/work_queue.go | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/util/admission/sequencer.go b/pkg/util/admission/sequencer.go index 05b389cc9310..fe836080b2d8 100644 --- a/pkg/util/admission/sequencer.go +++ b/pkg/util/admission/sequencer.go @@ -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 @@ -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 } diff --git a/pkg/util/admission/work_queue.go b/pkg/util/admission/work_queue.go index 3613a412cd9c..3afd5d7bc458 100644 --- a/pkg/util/admission/work_queue.go +++ b/pkg/util/admission/work_queue.go @@ -17,6 +17,7 @@ import ( "math" "sort" "sync" + "sync/atomic" "time" "github.com/cockroachdb/cockroach/pkg/base" @@ -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) }