fix(block): read mountinfo once for all the binds

pkg/unikontainers/block.go

@@ -209,11 +209,27 @@ func handleExplicitBlockImage(blockImg string, mountPoint string) (types.BlockDe
 // Search the bind mounts passed to the guest, as selected by onlyBindMounts,
 // and find the ones that come from a block.
 func getBlockVolumes(mounts []specs.Mount, ukernel types.Unikernel) ([]types.BlockDevParams, error) {
+	return blockVolumesFrom(mounts, ukernel, func() ([]*mountinfo.Info, error) {
+		return mountinfo.GetMounts(nil)
+	})
+}
+
+// blockVolumesFrom resolves every bind mount against one snapshot of the mount
+// table instead of reading /proc/thread-self/mountinfo once per bind. The
+// snapshot is refreshed after an unmount, because unmounting a source changes
+// the table that the following lookups must see.
+func blockVolumesFrom(mounts []specs.Mount, ukernel types.Unikernel, readMountInfo func() ([]*mountinfo.Info, error)) ([]types.BlockDevParams, error) {
 	blkImgs := []types.BlockDevParams{}
+
+	mountInfo, err := readMountInfo()
+	if err != nil {
+		return nil, fmt.Errorf("failed to read mountinfo: %w", err)
+	}
+
 	for i, m := range mounts {
 		// Get the information of the source path
-		// from /proc/thread-self/mountinfo
-		mInfo, err := getMountInfo(m.Source)
+		// from the mount table snapshot
+		mInfo, err := findMountInfo(mountInfo, m.Source)
 		if errors.Is(err, ErrMountpoint) {
 			// ErrMountpoint means we did not find any
 			// such mount and hence we can skip it.
@@ -246,6 +262,12 @@ func getBlockVolumes(mounts []specs.Mount, ukernel types.Unikernel) ([]types.Blo
 			if err != nil {
 				return nil, err
 			}
+			// The unmount removed the source from the mount table, so refresh
+			// the snapshot before resolving the next bind.
+			mountInfo, err = readMountInfo()
+			if err != nil {
+				return nil, fmt.Errorf("failed to read mountinfo: %w", err)
+			}
 			mInfo.ID = fmt.Sprintf("vol%d", i)
 			mInfo.HostMountPoint = mInfo.MountPoint
 			mInfo.MountPoint = m.Destination

pkg/unikontainers/block_test.go

@@ -15,10 +15,12 @@
 package unikontainers
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/moby/sys/mountinfo"
+	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/urunc-dev/urunc/pkg/unikontainers/types"
 )
@@ -70,3 +72,58 @@ func TestFindMountInfoEscapedPath(t *testing.T) {
 		})
 	}
 }
+
+// stubUnikernel only implements the parts of types.Unikernel that
+// blockVolumesFrom can reach in the tests below.
+type stubUnikernel struct {
+	types.Unikernel
+	fsType string
+}
+
+func (s stubUnikernel) SupportsFS(fsType string) bool {
+	return fsType == s.fsType
+}
+
+func testBinds(n int) []specs.Mount {
+	mounts := make([]specs.Mount, n)
+	for i := range mounts {
+		mounts[i] = specs.Mount{
+			Source:      fmt.Sprintf("/mnt/vol%d", i),
+			Destination: fmt.Sprintf("/data/vol%d", i),
+			Type:        "bind",
+		}
+	}
+	return mounts
+}
+
+// TestBlockVolumesShareMountInfoSnapshot makes sure the mount table is read
+// once for all the bind mounts instead of once per bind. None of the binds
+// below is mounted, so every lookup ends in ErrMountpoint and no unmount
+// (which would need a real mount) is triggered.
+func TestBlockVolumesShareMountInfoSnapshot(t *testing.T) {
+	reads := 0
+	readMountInfo := func() ([]*mountinfo.Info, error) {
+		reads++
+		return nil, nil
+	}
+
+	vols, err := blockVolumesFrom(testBinds(8), stubUnikernel{}, readMountInfo)
+	assert.NoError(t, err)
+	assert.Empty(t, vols)
+	assert.Equal(t, 1, reads, "mountinfo must be read once for all binds")
+}
+
+func BenchmarkBlockVolumesShareMountInfoSnapshot(b *testing.B) {
+	line := "36 35 8:1 / /mnt/data rw,relatime shared:1 - ext4 /dev/sdb1 rw\n"
+	readMountInfo := func() ([]*mountinfo.Info, error) {
+		return mountinfo.GetMountsFromReader(strings.NewReader(strings.Repeat(line, 500)), nil)
+	}
+	mounts := testBinds(32)
+
+	b.ResetTimer()
+	for b.Loop() {
+		if _, err := blockVolumesFrom(mounts, stubUnikernel{}, readMountInfo); err != nil {
+			b.Fatal(err)
+		}
+	}
+}