diff --git a/weed/topology/crowded_writable_test.go b/weed/topology/crowded_writable_test.go index ba87bcb6d..32e0a072f 100644 --- a/weed/topology/crowded_writable_test.go +++ b/weed/topology/crowded_writable_test.go @@ -1,6 +1,7 @@ package topology import ( + "sync" "testing" "time" @@ -73,3 +74,52 @@ func TestCrowdedCountStillMatchesWritables(t *testing.T) { t.Errorf("writable=%d crowded=%d, want 1 and 1", writable, crowded) } } + +// SetVolumeCrowded mutates the crowded map while GetWritableVolumeCount reads +// it on the Assign hot path. Under RLock both could run at once and the Go +// runtime aborts with "concurrent map read and map write". Run with -race to +// reproduce the original bug; the write lock makes this pass. +func TestSetVolumeCrowdedNoRaceWithGetWritableVolumeCount(t *testing.T) { + rp, _ := super_block.NewReplicaPlacementFromString("000") + vl := NewVolumeLayout(rp, needle.EMPTY_TTL, types.HardDriveType, 10000, false) + // GetWritableVolumeCount iterates vl.writables, so give it something to + // read while the crowded map is being mutated. + vl.writables = []needle.VolumeId{1, 2, 3} + + var wg sync.WaitGroup + stop := make(chan struct{}) + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + vl.SetVolumeCrowded(needle.VolumeId(2)) + } + } + }() + } + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + vl.GetWritableVolumeCount() + } + } + }() + } + + // The race detector needs only a brief window to flag a concurrent + // map read/write; let the goroutines hammer the lock briefly. + time.Sleep(50 * time.Millisecond) + close(stop) + wg.Wait() +} diff --git a/weed/topology/volume_layout.go b/weed/topology/volume_layout.go index 17897cbbf..8a36f79cc 100644 --- a/weed/topology/volume_layout.go +++ b/weed/topology/volume_layout.go @@ -1066,11 +1066,14 @@ func (vl *VolumeLayout) setVolumeCrowded(vid needle.VolumeId) { } func (vl *VolumeLayout) SetVolumeCrowded(vid needle.VolumeId) { - // since delete is guarded by accessLock.Lock(), - // and is always called in sequential order, - // RLock() should be safe enough - vl.accessLock.RLock() - defer vl.accessLock.RUnlock() + // setVolumeCrowded mutates the crowded map, and GetWritableVolumeCount + // reads it under RLock on the Assign hot path. Two concurrent RLock + // holders with one writing the map triggers a fatal + // "concurrent map read and map write". Take the write lock: this path + // is a low-frequency single consumer driven by the crowded-volume + // event loop, and every other mutation of crowded already holds Lock(). + vl.accessLock.Lock() + defer vl.accessLock.Unlock() vl.setVolumeCrowded(vid) }