From 60f9dea60a8b6d892d35ee94a77bfbc19c0fe402 Mon Sep 17 00:00:00 2001
From: chrislu <chris.lu@gmail.com>
Date: Thu, 30 Oct 2025 20:41:30 -0700
Subject: [PATCH] Optimize vidMap locking for better concurrency

Improved locking strategy based on the understanding that:
- vidMapLock protects the vidMap pointer from concurrent swaps
- vidMap has internal locks that protect its data structures

Changes:
1. Read operations: Grab pointer with RLock, release immediately, then operate
   - Reduces lock hold time
   - Allows resetVidMap to proceed sooner
   - Methods: GetLocations, GetLocationsClone, GetVidLocations,
     LookupVolumeServerUrl, GetDataCenter

2. Write operations: Changed from Lock() to RLock()
   - RLock prevents pointer swap during operation
   - Allows concurrent readers and other writers (serialized by vidMap's lock)
   - Methods: addLocation, deleteLocation, addEcLocation, deleteEcLocation

Benefits:
- Significantly reduced lock contention
- Better concurrent performance under load
- Still prevents all race conditions

Verified with: go test -race ./weed/wdclient/... (passes)
---
 weed/wdclient/masterclient.go | 46 ++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/weed/wdclient/masterclient.go b/weed/wdclient/masterclient.go
index 44e24297a..74828eb2a 100644
--- a/weed/wdclient/masterclient.go
+++ b/weed/wdclient/masterclient.go
@@ -504,22 +504,25 @@ func (mc *MasterClient) WithClientCustomGetMaster(getMasterF func() pb.ServerAdd
 // GetLocations safely retrieves volume locations
 func (mc *MasterClient) GetLocations(vid uint32) (locations []Location, found bool) {
 	mc.vidMapLock.RLock()
-	defer mc.vidMapLock.RUnlock()
-	return mc.vidMap.GetLocations(vid)
+	vm := mc.vidMap
+	mc.vidMapLock.RUnlock()
+	return vm.GetLocations(vid)
 }
 
 // GetLocationsClone safely retrieves a clone of volume locations
 func (mc *MasterClient) GetLocationsClone(vid uint32) (locations []Location, found bool) {
 	mc.vidMapLock.RLock()
-	defer mc.vidMapLock.RUnlock()
-	return mc.vidMap.GetLocationsClone(vid)
+	vm := mc.vidMap
+	mc.vidMapLock.RUnlock()
+	return vm.GetLocationsClone(vid)
 }
 
 // GetVidLocations safely retrieves volume locations by string ID
 func (mc *MasterClient) GetVidLocations(vid string) (locations []Location, err error) {
 	mc.vidMapLock.RLock()
-	defer mc.vidMapLock.RUnlock()
-	return mc.vidMap.GetVidLocations(vid)
+	vm := mc.vidMap
+	mc.vidMapLock.RUnlock()
+	return vm.GetVidLocations(vid)
 }
 
 // LookupFileId safely looks up URLs for a file ID
@@ -533,42 +536,45 @@ func (mc *MasterClient) LookupFileId(ctx context.Context, fileId string) (fullUr
 // LookupVolumeServerUrl safely looks up volume server URLs
 func (mc *MasterClient) LookupVolumeServerUrl(vid string) (serverUrls []string, err error) {
 	mc.vidMapLock.RLock()
-	defer mc.vidMapLock.RUnlock()
-	return mc.vidMap.LookupVolumeServerUrl(vid)
+	vm := mc.vidMap
+	mc.vidMapLock.RUnlock()
+	return vm.LookupVolumeServerUrl(vid)
 }
 
 // GetDataCenter safely retrieves the data center
 func (mc *MasterClient) GetDataCenter() string {
 	mc.vidMapLock.RLock()
-	defer mc.vidMapLock.RUnlock()
-	return mc.vidMap.DataCenter
+	vm := mc.vidMap
+	mc.vidMapLock.RUnlock()
+	return vm.DataCenter
 }
 
 // Thread-safe helpers for vidMap operations
-// These methods acquire exclusive locks to protect both the vidMap pointer
-// and the underlying map mutations from concurrent access
+// These methods use RLock to get a stable pointer to vidMap, preventing resetVidMap
+// from swapping it during the operation. The actual map mutations are protected by
+// vidMap's internal mutex, so RLock is sufficient here for better concurrency.
 
 func (mc *MasterClient) addLocation(vid uint32, location Location) {
-	mc.vidMapLock.Lock()
-	defer mc.vidMapLock.Unlock()
+	mc.vidMapLock.RLock()
+	defer mc.vidMapLock.RUnlock()
 	mc.vidMap.addLocation(vid, location)
 }
 
 func (mc *MasterClient) deleteLocation(vid uint32, location Location) {
-	mc.vidMapLock.Lock()
-	defer mc.vidMapLock.Unlock()
+	mc.vidMapLock.RLock()
+	defer mc.vidMapLock.RUnlock()
 	mc.vidMap.deleteLocation(vid, location)
 }
 
 func (mc *MasterClient) addEcLocation(vid uint32, location Location) {
-	mc.vidMapLock.Lock()
-	defer mc.vidMapLock.Unlock()
+	mc.vidMapLock.RLock()
+	defer mc.vidMapLock.RUnlock()
 	mc.vidMap.addEcLocation(vid, location)
 }
 
 func (mc *MasterClient) deleteEcLocation(vid uint32, location Location) {
-	mc.vidMapLock.Lock()
-	defer mc.vidMapLock.Unlock()
+	mc.vidMapLock.RLock()
+	defer mc.vidMapLock.RUnlock()
 	mc.vidMap.deleteEcLocation(vid, location)
 }