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

@@ -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())

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)
+		}
+	}
+}