From 7d6c55dedba53f895a7744bd2596b97ddb6d16ba Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 9 Aug 2026 09:38:57 -0700 Subject: [PATCH] topology: build the volume list without copying the volume map first (#10668) * topology: build the volume list without copying the volume map first ToDiskInfo copied every VolumeInfo on the disk into a fresh slice, walked it to build a protobuf message for each, and threw the copy away. The copy was as large as the messages it produced. Building them straight from the map holds the disk's read lock for the walk rather than just the copy, so a heartbeat updating that disk waits for it. It is a read lock on a call that is now infrequent, against an allocation of the same size as the response. ToTopologyInfo over 550k volumes 193617502 B/op -> 110011017 B/op, and faster for not making the copy. * trim the comments on this change to the parts that are not evident --- weed/topology/disk.go | 23 ++++++----- weed/topology/disk_info_test.go | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 weed/topology/disk_info_test.go diff --git a/weed/topology/disk.go b/weed/topology/disk.go index 5dd250fc0..527d655b6 100644 --- a/weed/topology/disk.go +++ b/weed/topology/disk.go @@ -371,13 +371,21 @@ func (d *Disk) FreeSpace() int64 { func (d *Disk) ToDiskInfo() *master_pb.DiskInfo { diskUsage := d.diskUsages.getOrCreateDisk(types.ToDiskType(string(d.Id()))) - // Get disk ID from first volume or EC shard + // Built under the read lock rather than from a copy as large as the + // messages it fed. Nothing here re-enters the topology, so the hold is safe. + d.RLock() + volumeInfos := make([]*master_pb.VolumeInformationMessage, 0, len(d.volumes)) var diskId uint32 - volumes := d.GetVolumes() + for _, v := range d.volumes { + if len(volumeInfos) == 0 { + diskId = v.DiskId + } + volumeInfos = append(volumeInfos, v.ToVolumeInformationMessage()) + } + d.RUnlock() + ecShards := d.GetEcShards() - if len(volumes) > 0 { - diskId = volumes[0].DiskId - } else if len(ecShards) > 0 { + if len(volumeInfos) == 0 && len(ecShards) > 0 { diskId = ecShards[0].DiskId } @@ -392,10 +400,7 @@ func (d *Disk) ToDiskInfo() *master_pb.DiskInfo { DiskTotalBytes: uint64(max(0, diskUsage.diskTotalBytes)), DiskFreeBytes: uint64(max(0, diskUsage.diskFreeBytes)), } - m.VolumeInfos = make([]*master_pb.VolumeInformationMessage, 0, len(volumes)) - for _, v := range volumes { - m.VolumeInfos = append(m.VolumeInfos, v.ToVolumeInformationMessage()) - } + m.VolumeInfos = volumeInfos m.EcShardInfos = make([]*master_pb.VolumeEcShardInformationMessage, 0, len(ecShards)) for _, ecv := range ecShards { m.EcShardInfos = append(m.EcShardInfos, ecv.ToVolumeEcShardInformationMessage()) diff --git a/weed/topology/disk_info_test.go b/weed/topology/disk_info_test.go new file mode 100644 index 000000000..53f2296f5 --- /dev/null +++ b/weed/topology/disk_info_test.go @@ -0,0 +1,69 @@ +package topology + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" +) + +func TestDiskInfoReportsEveryVolume(t *testing.T) { + topo := NewTopology("diskinfo", nil, 32*1024*1024*1024, 5, false) + dn := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1"). + GetOrCreateDataNode("10.0.0.1", 8080, 18080, "", "", map[string]uint32{"": 100}) + + volumes := []*master_pb.VolumeInformationMessage{ + {Id: 1, Collection: "c", Size: 1000, FileCount: 10, Version: 3, DiskId: 2}, + {Id: 2, Collection: "c", Size: 2000, FileCount: 20, Version: 3, DiskId: 2}, + {Id: 3, Collection: "other", Size: 3000, FileCount: 30, Version: 3, DiskId: 2}, + } + topo.SyncDataNodeRegistration(volumes, dn) + topo.SyncDataNodeEcShards([]*master_pb.VolumeEcShardInformationMessage{ + {Id: 9, Collection: "c", EcIndexBits: 0x3fff, DiskId: 2}, + }, dn) + + var info *master_pb.DiskInfo + for _, c := range dn.Children() { + info = c.(*Disk).ToDiskInfo() + } + if info == nil { + t.Fatal("the node reported no disk") + } + if len(info.VolumeInfos) != len(volumes) { + t.Fatalf("reported %d volumes, want %d", len(info.VolumeInfos), len(volumes)) + } + if len(info.EcShardInfos) != 1 { + t.Fatalf("reported %d ec volumes, want 1", len(info.EcShardInfos)) + } + if info.DiskId != 2 { + t.Errorf("reported disk id %d, want the one its volumes are on", info.DiskId) + } + + bySize := map[uint32]uint64{} + for _, v := range info.VolumeInfos { + bySize[v.Id] = v.Size + } + for _, want := range volumes { + if got := bySize[want.Id]; got != want.Size { + t.Errorf("volume %d reported size %d, want %d", want.Id, got, want.Size) + } + } +} + +func TestDiskInfoReportsTheDiskIdOfEcOnlyDisks(t *testing.T) { + topo := NewTopology("diskinfo", nil, 32*1024*1024*1024, 5, false) + dn := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1"). + GetOrCreateDataNode("10.0.0.1", 8080, 18080, "", "", map[string]uint32{"": 100}) + topo.SyncDataNodeEcShards([]*master_pb.VolumeEcShardInformationMessage{ + {Id: 9, Collection: "c", EcIndexBits: 0x3fff, DiskId: 5}, + }, dn) + + for _, c := range dn.Children() { + info := c.(*Disk).ToDiskInfo() + if len(info.VolumeInfos) != 0 { + t.Fatalf("expected no regular volumes, got %d", len(info.VolumeInfos)) + } + if info.DiskId != 5 { + t.Errorf("reported disk id %d, want the one its shards are on", info.DiskId) + } + } +}