Browse Source

Further reduce lock contention in LookupVolumeIdsWithFallback

Optimized two loops that were holding RLock for extended periods:

Before:
- Held RLock during entire loop iteration
- Included string parsing and cache lookups
- Could block resetVidMap for significant time with large batches

After:
- Grab vidMap pointer with brief RLock
- Release lock immediately
- Perform all loop operations on local pointer

Impact:
- First loop: Cache check on initial volumeIds
- Second loop: Double-check after singleflight wait

Benefits:
- Minimal lock hold time (just pointer copy)
- resetVidMap no longer blocked by long loops
- Better concurrent performance with large volume ID lists
- Still thread-safe (vidMap methods have internal locks)

Verified with: go test -race ./weed/wdclient/... (passes)
pr-7412
chrislu 2 months ago
parent
commit
c4c4d227a1
  1. 16
      weed/wdclient/masterclient.go

16
weed/wdclient/masterclient.go

@ -128,23 +128,26 @@ func (mc *MasterClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI
// Check cache first and parse volume IDs once // Check cache first and parse volume IDs once
vidStringToUint := make(map[string]uint32, len(volumeIds)) vidStringToUint := make(map[string]uint32, len(volumeIds))
// Get stable pointer to vidMap with minimal lock hold time
mc.vidMapLock.RLock() mc.vidMapLock.RLock()
vm := mc.vidMap
mc.vidMapLock.RUnlock()
for _, vidString := range volumeIds { for _, vidString := range volumeIds {
vid, err := strconv.ParseUint(vidString, 10, 32) vid, err := strconv.ParseUint(vidString, 10, 32)
if err != nil { if err != nil {
mc.vidMapLock.RUnlock()
return nil, fmt.Errorf("invalid volume id %s: %v", vidString, err) return nil, fmt.Errorf("invalid volume id %s: %v", vidString, err)
} }
vidStringToUint[vidString] = uint32(vid) vidStringToUint[vidString] = uint32(vid)
locations, found := mc.vidMap.GetLocations(uint32(vid))
locations, found := vm.GetLocations(uint32(vid))
if found && len(locations) > 0 { if found && len(locations) > 0 {
result[vidString] = locations result[vidString] = locations
} else { } else {
needsLookup = append(needsLookup, vidString) needsLookup = append(needsLookup, vidString)
} }
} }
mc.vidMapLock.RUnlock()
if len(needsLookup) == 0 { if len(needsLookup) == 0 {
return result, nil return result, nil
@ -160,16 +163,19 @@ func (mc *MasterClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI
stillNeedLookup := make([]string, 0, len(needsLookup)) stillNeedLookup := make([]string, 0, len(needsLookup))
batchResult := make(map[string][]Location) batchResult := make(map[string][]Location)
// Get stable pointer with minimal lock hold time
mc.vidMapLock.RLock() mc.vidMapLock.RLock()
vm := mc.vidMap
mc.vidMapLock.RUnlock()
for _, vidString := range needsLookup { for _, vidString := range needsLookup {
vid := vidStringToUint[vidString] // Use pre-parsed value vid := vidStringToUint[vidString] // Use pre-parsed value
if locations, found := mc.vidMap.GetLocations(vid); found && len(locations) > 0 {
if locations, found := vm.GetLocations(vid); found && len(locations) > 0 {
batchResult[vidString] = locations batchResult[vidString] = locations
} else { } else {
stillNeedLookup = append(stillNeedLookup, vidString) stillNeedLookup = append(stillNeedLookup, vidString)
} }
} }
mc.vidMapLock.RUnlock()
if len(stillNeedLookup) == 0 { if len(stillNeedLookup) == 0 {
return batchResult, nil return batchResult, nil

Loading…
Cancel
Save