From c1d649299c57828e494dc743c99e32a53d22a0b1 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Fri, 27 Sep 2024 11:23:53 -0700 Subject: [PATCH] scsi: Take optional guest path for mount Currently the SCSI mount manager will generate a new path for each new mount, based on a format string that it is instantiated with. However, it turns out some code in the GCS (e.g. sandbox mounts) assumes that the container scratch is mounted at a certain path. The long-term best solution here is probably to pass what paths to use explicitly to the GCS, but that would be more impactful. We need a more contained fix. This commit addresses the issue by allowing an optional guest path to be given for a SCSI mount. The mount manager has been changed as follows: - If a guest path is not supplied: The mount can re-use (refcount++) any existing mount with the same controller/lun/options. If a new mount is created, the mount manager will generate a path for it. - If a guest path is supplied: The mount can re-use (refcount++) any existing mount with the same controller/lun/guestpath/options. If a new mount is created, the mount manager will use the supplied path for it. Accordingly, code calling into the mount manager has been updated to pass an empty string for the guest path. The exception to this is the LCOW layer mounting code, which will pass an explicit guest path for the scratch disk. As far as I know, WCOW does not depend on a specific path for its scratch disk. Signed-off-by: Kevin Parsons --- internal/devices/drivers.go | 1 + internal/hcsoci/resources_lcow.go | 2 ++ internal/hcsoci/resources_wcow.go | 3 +++ internal/layers/lcow.go | 2 ++ internal/layers/wcow_mount.go | 2 +- internal/lcow/disk.go | 2 +- internal/lcow/scratch.go | 1 + internal/tools/uvmboot/mounts.go | 1 + internal/uvm/scsi/manager.go | 11 ++++++++--- internal/uvm/scsi/mount.go | 23 ++++++++++++++++------- 10 files changed, 36 insertions(+), 12 deletions(-) diff --git a/internal/devices/drivers.go b/internal/devices/drivers.go index f5dc588e02..eb908583b1 100644 --- a/internal/devices/drivers.go +++ b/internal/devices/drivers.go @@ -60,6 +60,7 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string) (close share, true, vm.ID(), + "", &scsi.MountConfig{}, ) if err != nil { diff --git a/internal/hcsoci/resources_lcow.go b/internal/hcsoci/resources_lcow.go index b98493ce4c..633bde02eb 100644 --- a/internal/hcsoci/resources_lcow.go +++ b/internal/hcsoci/resources_lcow.go @@ -92,6 +92,7 @@ func allocateLinuxResources(ctx context.Context, coi *createOptionsInternal, r * hostPath, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{Options: mount.Options, BlockDev: isBlockDev}, ) if err != nil { @@ -114,6 +115,7 @@ func allocateLinuxResources(ctx context.Context, coi *createOptionsInternal, r * hostPath, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{Options: mount.Options, BlockDev: isBlockDev}, ) if err != nil { diff --git a/internal/hcsoci/resources_wcow.go b/internal/hcsoci/resources_wcow.go index 0503b371f6..604ba8b550 100644 --- a/internal/hcsoci/resources_wcow.go +++ b/internal/hcsoci/resources_wcow.go @@ -144,6 +144,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R mount.Source, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{}, ) case MountTypeVirtualDisk: @@ -153,6 +154,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R mount.Source, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{}, ) case MountTypeExtensibleVirtualDisk: @@ -161,6 +163,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R ctx, mount.Source, readOnly, + "", &scsi.MountConfig{}, ) } diff --git a/internal/layers/lcow.go b/internal/layers/lcow.go index cc934121b4..2a6da2a622 100644 --- a/internal/layers/lcow.go +++ b/internal/layers/lcow.go @@ -136,6 +136,7 @@ func MountLCOWLayers(ctx context.Context, containerID string, layers *LCOWLayers hostPath, false, vm.ID(), + guestRoot, mConfig, ) if err != nil { @@ -194,6 +195,7 @@ func addLCOWLayer(ctx context.Context, vm *uvm.UtilityVM, layer *LCOWLayer) (uvm layer.VHDPath, true, "", + "", &scsi.MountConfig{ Partition: layer.Partition, Options: []string{"ro"}, diff --git a/internal/layers/wcow_mount.go b/internal/layers/wcow_mount.go index d12593d179..a5f706f940 100644 --- a/internal/layers/wcow_mount.go +++ b/internal/layers/wcow_mount.go @@ -331,7 +331,7 @@ func mountHypervIsolatedWCIFSLayers(ctx context.Context, l *wcowWCIFSLayers, vm hostPath := filepath.Join(l.scratchLayerPath, "sandbox.vhdx") log.G(ctx).WithField("hostPath", hostPath).Debug("mounting scratch VHD") - scsiMount, err := vm.SCSIManager.AddVirtualDisk(ctx, hostPath, false, vm.ID(), &scsi.MountConfig{}) + scsiMount, err := vm.SCSIManager.AddVirtualDisk(ctx, hostPath, false, vm.ID(), "", &scsi.MountConfig{}) if err != nil { return nil, nil, fmt.Errorf("failed to add SCSI scratch VHD: %w", err) } diff --git a/internal/lcow/disk.go b/internal/lcow/disk.go index 4e68f26258..08fad8d2ea 100644 --- a/internal/lcow/disk.go +++ b/internal/lcow/disk.go @@ -31,7 +31,7 @@ func FormatDisk(ctx context.Context, lcowUVM *uvm.UtilityVM, destPath string) er }).Debug("lcow::FormatDisk opts") // Attach without mounting. - scsi, err := lcowUVM.SCSIManager.AddPhysicalDisk(ctx, destPath, false, lcowUVM.ID(), nil) + scsi, err := lcowUVM.SCSIManager.AddPhysicalDisk(ctx, destPath, false, lcowUVM.ID(), "", nil) if err != nil { return err } diff --git a/internal/lcow/scratch.go b/internal/lcow/scratch.go index 3c424dcbad..e44a6b388a 100644 --- a/internal/lcow/scratch.go +++ b/internal/lcow/scratch.go @@ -75,6 +75,7 @@ func CreateScratch(ctx context.Context, lcowUVM *uvm.UtilityVM, destFile string, destFile, false, lcowUVM.ID(), + "", &scsi.MountConfig{ BlockDev: true, }, diff --git a/internal/tools/uvmboot/mounts.go b/internal/tools/uvmboot/mounts.go index 2f160c01ca..0f546b9f86 100644 --- a/internal/tools/uvmboot/mounts.go +++ b/internal/tools/uvmboot/mounts.go @@ -25,6 +25,7 @@ func mountSCSI(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error { m.host, !m.writable, vm.ID(), + "", &scsi.MountConfig{}, ) if err != nil { diff --git a/internal/uvm/scsi/manager.go b/internal/uvm/scsi/manager.go index fd7173b5b1..50aad34699 100644 --- a/internal/uvm/scsi/manager.go +++ b/internal/uvm/scsi/manager.go @@ -141,6 +141,7 @@ func (m *Manager) AddVirtualDisk( hostPath string, readOnly bool, vmID string, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -169,6 +170,7 @@ func (m *Manager) AddVirtualDisk( readOnly: readOnly, typ: "VirtualDisk", }, + guestPath, mcInternal) } @@ -187,6 +189,7 @@ func (m *Manager) AddPhysicalDisk( hostPath string, readOnly bool, vmID string, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -215,6 +218,7 @@ func (m *Manager) AddPhysicalDisk( readOnly: readOnly, typ: "PassThru", }, + guestPath, mcInternal) } @@ -233,6 +237,7 @@ func (m *Manager) AddExtensibleVirtualDisk( ctx context.Context, hostPath string, readOnly bool, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -260,10 +265,11 @@ func (m *Manager) AddExtensibleVirtualDisk( typ: "ExtensibleVirtualDisk", evdType: evdType, }, + guestPath, mcInternal) } -func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, mountConfig *mountConfig) (_ *Mount, err error) { +func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, guestPath string, mountConfig *mountConfig) (_ *Mount, err error) { controller, lun, err := m.attachManager.attach(ctx, attachConfig) if err != nil { return nil, err @@ -274,9 +280,8 @@ func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, mountConf } }() - var guestPath string if mountConfig != nil { - guestPath, err = m.mountManager.mount(ctx, controller, lun, mountConfig) + guestPath, err = m.mountManager.mount(ctx, controller, lun, guestPath, mountConfig) if err != nil { return nil, err } diff --git a/internal/uvm/scsi/mount.go b/internal/uvm/scsi/mount.go index ed77fa9991..df5651e008 100644 --- a/internal/uvm/scsi/mount.go +++ b/internal/uvm/scsi/mount.go @@ -47,14 +47,17 @@ type mountConfig struct { filesystem string } -func (mm *mountManager) mount(ctx context.Context, controller, lun uint, c *mountConfig) (_ string, err error) { +func (mm *mountManager) mount(ctx context.Context, controller, lun uint, path string, c *mountConfig) (_ string, err error) { // Normalize the mount config for comparison. // Config equality relies on the options slices being compared element-wise. Sort the options // slice first so that two slices with different ordering compare as equal. We assume that // order will never matter for mount options. sort.Strings(c.options) - mount, existed := mm.trackMount(controller, lun, c) + mount, existed, err := mm.trackMount(controller, lun, path, c) + if err != nil { + return "", err + } if existed { select { case <-ctx.Done(): @@ -108,7 +111,7 @@ func (mm *mountManager) unmount(ctx context.Context, path string) (bool, error) return true, nil } -func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount, bool) { +func (mm *mountManager) trackMount(controller, lun uint, path string, c *mountConfig) (*mount, bool, error) { mm.m.Lock() defer mm.m.Unlock() @@ -120,15 +123,19 @@ func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount } } else if controller == mount.controller && lun == mount.lun && + (path == "" || path == mount.path) && reflect.DeepEqual(c, mount.config) { mount.refCount++ - return mount, true + return mount, true, nil + } else if path != "" && path == mount.path { + return nil, false, fmt.Errorf("cannot mount over an existing mountpoint: %s", path) } } // New mount. mount := &mount{ + path: path, // If path is empty, this will be replaced with a generated path below. controller: controller, lun: lun, config: c, @@ -142,9 +149,11 @@ func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount mount.index = freeIndex mm.mounts[freeIndex] = mount } - // Use the mount index to produce a unique guest path. - mount.path = fmt.Sprintf(mm.mountFmt, mount.index) - return mount, false + if mount.path == "" { + // Use the mount index to produce a unique guest path. + mount.path = fmt.Sprintf(mm.mountFmt, mount.index) + } + return mount, false, nil } // Caller must be holding mm.m.