From 0cc68af975b5a701b38a35422a9ec708b367e927 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 20 Mar 2026 22:11:50 +0530 Subject: [PATCH 1/6] [shimV2] add SCSI disk controller Added the SCSI controller which manages the lifecycle of SCSI disk attachments on a Hyper-V VM. It abstracts host-side slot allocation, reference counting, and two-phase teardown (guest unplug followed by host removal) behind the Controller interface. Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/doc.go | 73 ++++ internal/controller/device/scsi/interface.go | 78 +++++ internal/controller/device/scsi/scsi.go | 314 ++++++++++++++++++ .../controller/device/scsi/scsi_guest_lcow.go | 30 ++ .../controller/device/scsi/scsi_guest_wcow.go | 11 + internal/controller/device/scsi/state.go | 60 ++++ internal/logfields/fields.go | 6 + 7 files changed, 572 insertions(+) create mode 100644 internal/controller/device/scsi/doc.go create mode 100644 internal/controller/device/scsi/interface.go create mode 100644 internal/controller/device/scsi/scsi.go create mode 100644 internal/controller/device/scsi/scsi_guest_lcow.go create mode 100644 internal/controller/device/scsi/scsi_guest_wcow.go create mode 100644 internal/controller/device/scsi/state.go diff --git a/internal/controller/device/scsi/doc.go b/internal/controller/device/scsi/doc.go new file mode 100644 index 0000000000..5d70f4eba3 --- /dev/null +++ b/internal/controller/device/scsi/doc.go @@ -0,0 +1,73 @@ +//go:build windows + +// Package scsi manages the lifecycle of SCSI disk attachments on a Hyper-V VM. +// +// It abstracts host-side slot allocation, reference counting, and two-phase +// teardown (guest unplug followed by host removal) behind the [Controller] +// interface, with [Manager] as the primary implementation. +// +// # Lifecycle +// +// Each disk attachment progresses through the states below: +// +// ┌──────────────────────┐ +// │ attachmentAttached │◄── [Manager.AttachDiskToVM] succeeds +// └──────────┬───────────┘ +// │ unplugFromGuest succeeds +// ▼ +// ┌──────────────────────┐ +// │ attachmentUnplugged │ +// └──────────┬───────────┘ +// │ RemoveSCSIDisk succeeds +// ▼ +// ┌──────────────────────┐ +// │ attachmentDetached │ (terminal — slot freed) +// └──────────────────────┘ +// +// ┌──────────────────────┐ +// │ attachmentReserved │ (no transitions — pre-reserved at construction) +// └──────────────────────┘ +// +// State descriptions: +// +// - [attachmentAttached]: entered once [AddSCSIDisk] succeeds; +// the disk is on the SCSI bus and available for guest mounts. +// - [attachmentUnplugged]: entered once the guest-side unplug completes; +// the guest has released the device but the host has not yet removed it. +// - [attachmentDetached]: terminal state entered once [RemoveSCSIDisk] succeeds. +// - [attachmentReserved]: special state for slots pre-reserved via [New]; +// these are never allocated to new disks and never torn down. +// +// # Reference Counting +// +// Multiple callers may request the same disk (identical host path, type, and +// read-only flag). [Manager.AttachDiskToVM] detects duplicates and increments a +// reference count instead of issuing a second HCS call; the slot is shared. +// [Manager.DetachFromVM] decrements the count and only tears down the attachment +// when it reaches zero. +// +// # Platform Variants +// +// The guest-side unplug step differs between LCOW and WCOW guests and is +// selected via build tags (default for the LCOW shim; "wcow" tag for the WCOW shim): +// +// - LCOW: sends a SCSIDevice removal request to the Guest Compute Service (GCS), +// which hot-unplugs the device from the Linux kernel before the host removes the disk. +// - WCOW: unplugFromGuest is a no-op; Windows handles SCSI hot-unplug +// automatically when the host removes the disk from the VM. +// +// # Usage +// +// mgr := scsi.New(vmScsiManager, linuxGuestMgr, numControllers, reservedSlots) +// +// slot, err := mgr.AttachDiskToVM(ctx, "/path/to/disk.vhdx", scsi.DiskTypeVirtualDisk, false) +// if err != nil { +// // handle error +// } +// +// // ... use slot for guest mounts ... +// +// if err := mgr.DetachFromVM(ctx, slot); err != nil { +// // handle error +// } +package scsi diff --git a/internal/controller/device/scsi/interface.go b/internal/controller/device/scsi/interface.go new file mode 100644 index 0000000000..42ce448185 --- /dev/null +++ b/internal/controller/device/scsi/interface.go @@ -0,0 +1,78 @@ +//go:build windows + +package scsi + +import "context" + +// DiskType identifies the attachment protocol used when adding a disk to the VM's SCSI bus. +type DiskType string + +const ( + // DiskTypeVirtualDisk attaches the disk as a virtual hard disk (VHD/VHDX). + DiskTypeVirtualDisk DiskType = "VirtualDisk" + + // DiskTypePassThru attaches a physical disk directly to the VM with pass-through access. + DiskTypePassThru DiskType = "PassThru" + + // DiskTypeExtensibleVirtualDisk attaches a disk via an extensible virtual disk (EVD) provider. + // The hostPath must be in the form evd:///. + DiskTypeExtensibleVirtualDisk DiskType = "ExtensibleVirtualDisk" +) + +// Controller is the primary interface for attaching and detaching SCSI disks on a VM. +type Controller interface { + // AttachDiskToVM attaches the disk at hostPath to the VM and returns the allocated [VMSlot]. + // If the same disk is already attached, the existing slot is reused. + AttachDiskToVM(ctx context.Context, hostPath string, diskType DiskType, readOnly bool) (VMSlot, error) + + // DetachFromVM unplugs and detaches the disk from the VM. + DetachFromVM(ctx context.Context, slot VMSlot) error +} + +// VMSlot identifies a disk's hardware address on the VM's SCSI bus. +type VMSlot struct { + // Controller is the zero-based SCSI controller index. + Controller uint + // LUN is the logical unit number within the controller. + LUN uint +} + +// ============================================================================== +// INTERNAL DATA STRUCTURES +// Types and constants below this line are unexported and used for state tracking. +// ============================================================================== + +// numLUNsPerController is the maximum number of LUNs per controller, fixed by Hyper-V. +const numLUNsPerController = 64 + +// diskConfig holds the immutable parameters that uniquely identify a disk attachment request. +type diskConfig struct { + hostPath string + readOnly bool + typ DiskType + // evdType is the EVD provider name; only populated when typ is [DiskTypeExtensibleVirtualDisk]. + evdType string +} + +// vmAttachment records one disk's full attachment state and reference count. +type vmAttachment struct { + // config is the immutable disk parameters used to match duplicate attach requests. + config *diskConfig + + // controller and lun are the allocated hardware address on the SCSI bus. + controller uint + lun uint + + // refCount is the number of active callers sharing this attachment. + // Access must be guarded by [Manager.mu]. + refCount uint + + // state tracks the forward-only lifecycle position of this attachment. + // Access must be guarded by [Manager.mu]. + state attachmentState + + // waitCh is closed (with waitErr set) once the HCS attach call for this + // attachment has finished. + waitCh chan struct{} + waitErr error +} diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go new file mode 100644 index 0000000000..1dae5816dc --- /dev/null +++ b/internal/controller/device/scsi/scsi.go @@ -0,0 +1,314 @@ +//go:build windows + +package scsi + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + "github.com/Microsoft/hcsshim/internal/log" + "github.com/Microsoft/hcsshim/internal/logfields" + "github.com/Microsoft/hcsshim/internal/vm/guestmanager" + "github.com/Microsoft/hcsshim/internal/vm/vmmanager" + + "github.com/sirupsen/logrus" +) + +// Manager implements [Controller] and manages SCSI disk attachment across +// one or more controllers on a Hyper-V VM. +type Manager struct { + // mu guards attachments and all mutable fields of every [vmAttachment] in the map. + mu sync.Mutex + + // numControllers is the number of SCSI controllers available on the VM. + // It bounds the (controller, lun) search space when allocating a free slot. + numControllers int + + // attachments tracks every disk currently being attached or already attached + // to the VM. Key = (controller, lun) hardware address. + // Access must be guarded by mu. + attachments map[VMSlot]*vmAttachment + + // vmScsiManager is the host-side SCSI manager used to add and remove disks from the VM. + vmScsiManager vmmanager.SCSIManager + + // linuxGuestMgr is used to perform the guest-side unplug on LCOW prior to detach. + linuxGuestMgr guestmanager.LCOWScsiManager +} + +var _ Controller = (*Manager)(nil) + +// New creates a new [Manager] instance conforming to [Controller] interface. +// ReservedSlots are never allocated to new disks. +func New( + vmScsiManager vmmanager.SCSIManager, + linuxGuest guestmanager.LCOWScsiManager, + numControllers int, + reservedSlots []VMSlot, +) *Manager { + m := &Manager{ + numControllers: numControllers, + attachments: make(map[VMSlot]*vmAttachment, len(reservedSlots)), + vmScsiManager: vmScsiManager, + linuxGuestMgr: linuxGuest, + } + + // Pre-populate attachments with reserved slots so they are never allocated to new disks. + for _, s := range reservedSlots { + m.attachments[s] = &vmAttachment{ + controller: s.Controller, + lun: s.LUN, + refCount: 1, + state: attachmentReserved, + } + } + + return m +} + +// AttachDiskToVM attaches the disk at hostPath to the VM and returns the allocated [VMSlot]. +// If the same disk is already in flight or attached, AttachDiskToVM blocks until the +// original operation completes and then returns the shared slot. +func (m *Manager) AttachDiskToVM( + ctx context.Context, + hostPath string, + diskType DiskType, + readOnly bool, +) (VMSlot, error) { + ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Operation, "AttachDiskToVM")) + + log.G(ctx).WithFields(logrus.Fields{ + logfields.HostPath: hostPath, + logfields.DiskType: diskType, + logfields.ReadOnly: readOnly, + }).Debug("Attaching disk to VM") + + // Create the disk config for the VM. + config := &diskConfig{ + hostPath: hostPath, + readOnly: readOnly, + typ: diskType, + } + + // Parse EVD-specific fields out of hostPath before forwarding to attachDiskToVM, + if diskType == DiskTypeExtensibleVirtualDisk { + evdType, evdMountPath, err := parseEVDPath(hostPath) + if err != nil { + return VMSlot{}, err + } + config.hostPath = evdMountPath + config.evdType = evdType + } + + return m.attachDiskToVM(ctx, config) +} + +// attachDiskToVM is the internal implementation of [Manager.AttachDiskToVM]. +// It calls trackAttachment to either reuse an in-flight attachment or +// claim a new slot, then drives the HCS add-disk call. +func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlot, error) { + // Track the attachment and get the slot to attach to. + att, existed, err := m.trackAttachment(ctx, config) + if err != nil { + return VMSlot{}, err + } + + // ============================================================================== + // Found an existing attachment. + // ============================================================================== + if existed { + // Another goroutine is already attaching (or has attached) the same disk. + // Wait for it to finish, honoring context cancellation. + select { + case <-ctx.Done(): + // Undo the refCount bump from trackAttachment so the + // attachment can eventually reach zero and be torn down. + m.mu.Lock() + att.refCount-- + m.mu.Unlock() + return VMSlot{}, ctx.Err() + case <-att.waitCh: + if att.waitErr != nil { + // The original attach failed. + // The attachment will be removed from the map. + return VMSlot{}, att.waitErr + } + } + + log.G(ctx).WithFields(logrus.Fields{ + logfields.Controller: att.controller, + logfields.LUN: att.lun, + }).Debug("reusing existing SCSI VM attachment") + + return VMSlot{Controller: att.controller, LUN: att.lun}, nil + } + + // ============================================================================== + // New attachment — we own the slot. + // ============================================================================== + + // Perform the host-side HCS call to add the disk at the allocated (controller, lun) slot. + log.G(ctx).WithFields(logrus.Fields{ + logfields.Controller: att.controller, + logfields.LUN: att.lun, + }).Debug("performing AddSCSIDisk call to add disk to VM") + + err = m.vmScsiManager.AddSCSIDisk(ctx, hcsschema.Attachment{ + Path: config.hostPath, + Type_: string(config.typ), + ReadOnly: config.readOnly, + ExtensibleVirtualDiskType: config.evdType, + }, att.controller, att.lun) + + // Signal completion to any goroutines waiting on the same disk. + att.waitErr = err + close(att.waitCh) + + // Clean up on failure. + if err != nil { + m.mu.Lock() + delete(m.attachments, VMSlot{att.controller, att.lun}) + m.mu.Unlock() + + return VMSlot{}, fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", + config.hostPath, att.controller, att.lun, err) + } + + log.G(ctx).WithFields(logrus.Fields{ + logfields.Controller: att.controller, + logfields.LUN: att.lun, + }).Debug("SCSI disk attached to VM") + + return VMSlot{Controller: att.controller, LUN: att.lun}, nil +} + +// trackAttachment either reuses an existing [vmAttachment] for the same disk config, +// incrementing its reference count, or allocates the first free (controller, lun) slot +// and registers a new [vmAttachment] in the internal map. +func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmAttachment, bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + + // Reuse an existing attachment for the same disk. + for _, existing := range m.attachments { + if existing.config != nil && *existing.config == *config { + existing.refCount++ + return existing, true, nil + } + } + + log.G(ctx).Debug("no existing attachment found for disk, allocating new slot") + + // Find the first free (controller, lun) pair. + for ctrl := uint(0); ctrl < uint(m.numControllers); ctrl++ { + for lun := uint(0); lun < numLUNsPerController; lun++ { + key := VMSlot{ctrl, lun} + // if the slot is occupied, then continue to next slot. + if _, occupied := m.attachments[key]; occupied { + continue + } + + // Found a slot, return it. + log.G(ctx).WithFields(logrus.Fields{ + logfields.Controller: ctrl, + logfields.LUN: lun, + }).Debug("allocating new attachment") + + att := &vmAttachment{ + config: config, + controller: ctrl, + lun: lun, + refCount: 1, + state: attachmentAttached, + waitCh: make(chan struct{}), + } + m.attachments[key] = att + return att, false, nil + } + } + + return nil, false, errors.New("no available scsi slot") +} + +// DetachFromVM detaches the disk at slot from the VM, unplugging it from the guest first. +// If the disk is shared with other callers, DetachFromVM returns without removing it +// until the last caller detaches. +func (m *Manager) DetachFromVM( + ctx context.Context, + slot VMSlot, +) error { + ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{ + logfields.Operation: "DetachFromVM", + logfields.Controller: slot.Controller, + logfields.LUN: slot.LUN, + })) + + log.G(ctx).Debug("Detaching from VM") + + m.mu.Lock() + defer m.mu.Unlock() + + existing, ok := m.attachments[slot] + if !ok { + return fmt.Errorf("no existing attachment found for controller=%d lun=%d", slot.Controller, slot.LUN) + } + + if existing.state == attachmentReserved { + return fmt.Errorf("cannot release reserved attachment at controller=%d lun=%d", slot.Controller, slot.LUN) + } + + if existing.refCount > 0 { + existing.refCount-- + } + if existing.refCount > 0 { + // Other callers still hold a reference to this disk. + log.G(ctx).Debug("disk still in use by other callers, not detaching from VM") + return nil + } + + // Unplug the device from the guest before removing it from the VM. + // Skip if already unplugged from a previous attempt where RemoveSCSIDisk + // failed after a successful unplug. + if existing.state == attachmentAttached { + if err := m.unplugFromGuest(ctx, slot.Controller, slot.LUN); err != nil { + return fmt.Errorf("unplug scsi disk at controller=%d lun=%d from guest: %w", + slot.Controller, slot.LUN, err) + } + existing.state = attachmentUnplugged + + log.G(ctx).Debug("disk unplugged from guest") + } + + if existing.state == attachmentUnplugged { + if err := m.vmScsiManager.RemoveSCSIDisk(ctx, slot.Controller, slot.LUN); err != nil { + return fmt.Errorf("remove scsi disk at controller=%d lun=%d from vm: %w", + slot.Controller, slot.LUN, err) + } + existing.state = attachmentDetached + } + + delete(m.attachments, slot) + + log.G(ctx).WithFields(logrus.Fields{ + logfields.Controller: slot.Controller, + logfields.LUN: slot.LUN, + }).Debug("SCSI disk detached from VM") + + return nil +} + +// parseEVDPath splits an EVD host path of the form "evd:///" into +// its EVD provider type and the underlying mount path. +// Returns an error if the path does not conform to this scheme. +func parseEVDPath(hostPath string) (evdType, mountPath string, err error) { + trimmedPath := strings.TrimPrefix(hostPath, "evd://") + separatorIndex := strings.Index(trimmedPath, "/") + if separatorIndex <= 0 { + return "", "", fmt.Errorf("invalid extensible vhd path: %q", hostPath) + } + return trimmedPath[:separatorIndex], trimmedPath[separatorIndex+1:], nil +} diff --git a/internal/controller/device/scsi/scsi_guest_lcow.go b/internal/controller/device/scsi/scsi_guest_lcow.go new file mode 100644 index 0000000000..fcac68b04d --- /dev/null +++ b/internal/controller/device/scsi/scsi_guest_lcow.go @@ -0,0 +1,30 @@ +//go:build windows && !wcow + +package scsi + +import ( + "context" + "fmt" + + "github.com/Microsoft/hcsshim/internal/protocol/guestresource" +) + +// unplugFromGuest ejects the SCSI device at (controller, lun) from the Linux guest +// before the host removes it from the VM. +func (m *Manager) unplugFromGuest(ctx context.Context, controller, lun uint) error { + settings := guestresource.SCSIDevice{ + Controller: uint8(controller), + Lun: uint8(lun), + } + + // RemoveSCSIDevice sends a guest modification request that the GCS handles + // by first remapping the logical controller number to the actual kernel-visible + // controller index (HCS and the Linux kernel assign them independently), then + // writing "1" to /sys/bus/scsi/devices//delete. That sysfs write is a + // guest-initiated hot-unplug: the kernel removes the device from its bus and + // flushes any in-flight I/O before the host removes the disk from the VM. + if err := m.linuxGuestMgr.RemoveSCSIDevice(ctx, settings); err != nil { + return fmt.Errorf("remove scsi device at controller=%d lun=%d from lcow guest: %w", controller, lun, err) + } + return nil +} diff --git a/internal/controller/device/scsi/scsi_guest_wcow.go b/internal/controller/device/scsi/scsi_guest_wcow.go new file mode 100644 index 0000000000..1b1bde8a31 --- /dev/null +++ b/internal/controller/device/scsi/scsi_guest_wcow.go @@ -0,0 +1,11 @@ +//go:build windows && wcow + +package scsi + +import "context" + +// unplugFromGuest is a no-op on Windows guests because Windows handles +// SCSI hot-unplug automatically when the host removes the disk from the VM. +func (m *Manager) unplugFromGuest(_ context.Context, _, _ uint) error { + return nil +} diff --git a/internal/controller/device/scsi/state.go b/internal/controller/device/scsi/state.go new file mode 100644 index 0000000000..40fb46d6d1 --- /dev/null +++ b/internal/controller/device/scsi/state.go @@ -0,0 +1,60 @@ +//go:build windows + +package scsi + +// attachmentState represents the current state of a SCSI disk attachment lifecycle. +// +// The normal progression is: +// +// attachmentAttached → attachmentUnplugged → attachmentDetached +// +// attachmentReserved is a special state for pre-reserved slots that never +// transition to any other state. +// +// Full state-transition table: +// +// Current State │ Trigger │ Next State +// ────────────────────────┼────────────────────────────────────┼──────────────────── +// attachmentAttached │ unplugFromGuest succeeds │ attachmentUnplugged +// attachmentUnplugged │ RemoveSCSIDisk succeeds │ attachmentDetached +// attachmentDetached │ (terminal — no further transitions)│ — +// attachmentReserved │ (never transitions) │ — +type attachmentState int + +const ( + // attachmentAttached means AddSCSIDisk succeeded; the disk is on the SCSI + // bus and available for guest mounts. + // Valid transitions: + // - attachmentAttached → attachmentUnplugged (via unplugFromGuest, on success) + attachmentAttached attachmentState = iota + + // attachmentUnplugged means unplugFromGuest succeeded; the guest has + // released the device but RemoveSCSIDisk has not yet been called. + // Valid transitions: + // - attachmentUnplugged → attachmentDetached (via RemoveSCSIDisk, on success) + attachmentUnplugged + + // attachmentDetached means RemoveSCSIDisk succeeded; the disk has been + // fully removed from the VM. This is a terminal state. + attachmentDetached + + // attachmentReserved is used for slots pre-reserved at Manager construction + // time. These must never be handed out or torn down — no transitions are valid. + attachmentReserved +) + +// String returns a human-readable name for the [attachmentState]. +func (s attachmentState) String() string { + switch s { + case attachmentAttached: + return "Attached" + case attachmentUnplugged: + return "Unplugged" + case attachmentDetached: + return "Detached" + case attachmentReserved: + return "Reserved" + default: + return "Unknown" + } +} diff --git a/internal/logfields/fields.go b/internal/logfields/fields.go index dac5a708e5..6cd9d615b7 100644 --- a/internal/logfields/fields.go +++ b/internal/logfields/fields.go @@ -27,6 +27,12 @@ const ( Attempt = "attemptNo" JSON = "json" + // SCSI Constants + + Controller = "controller" + LUN = "lun" + DiskType = "disk-type" + // Time StartTime = "startTime" From 18a40edd14cdccc6cd6277647efb3ec84a42c42a Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Sat, 21 Mar 2026 02:34:49 +0530 Subject: [PATCH 2/6] review comments : 1 Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/doc.go | 50 ++-- internal/controller/device/scsi/interface.go | 38 ++- internal/controller/device/scsi/scsi.go | 272 ++++++++++-------- .../controller/device/scsi/scsi_guest_lcow.go | 2 +- internal/controller/device/scsi/state.go | 28 +- internal/vm/guestmanager/scsi_lcow.go | 12 - internal/vm/vmmanager/scsi.go | 11 - 7 files changed, 245 insertions(+), 168 deletions(-) diff --git a/internal/controller/device/scsi/doc.go b/internal/controller/device/scsi/doc.go index 5d70f4eba3..05dafd14ae 100644 --- a/internal/controller/device/scsi/doc.go +++ b/internal/controller/device/scsi/doc.go @@ -8,30 +8,46 @@ // // # Lifecycle // -// Each disk attachment progresses through the states below: -// -// ┌──────────────────────┐ -// │ attachmentAttached │◄── [Manager.AttachDiskToVM] succeeds -// └──────────┬───────────┘ -// │ unplugFromGuest succeeds -// ▼ -// ┌──────────────────────┐ -// │ attachmentUnplugged │ -// └──────────┬───────────┘ +// Each disk attachment progresses through the states below. +// The happy path runs down the left column; the error path is on the right. +// +// Allocate slot for the disk +// │ +// ▼ +// ┌─────────────────────┐ +// │ attachmentPending │ +// └──────────┬──────────┘ +// │ +// ┌───────┴────────────────────────────────┐ +// │ AddSCSIDisk succeeds │ AddSCSIDisk fails +// ▼ ▼ +// ┌─────────────────────┐ ┌──────────────────────┐ +// │ attachmentAttached │ │ attachmentInvalid │ +// └──────────┬──────────┘ └──────────┬───────────┘ +// │ unplugFromGuest │ DetachFromVM +// │ succeeds │ (refCount → 0) +// ▼ ▼ +// ┌─────────────────────┐ (removed from map) +// │ attachmentUnplugged │ +// └──────────┬──────────┘ // │ RemoveSCSIDisk succeeds // ▼ -// ┌──────────────────────┐ -// │ attachmentDetached │ (terminal — slot freed) -// └──────────────────────┘ +// ┌─────────────────────┐ +// │ attachmentDetached │ ← terminal; entry removed from map +// └─────────────────────┘ // -// ┌──────────────────────┐ -// │ attachmentReserved │ (no transitions — pre-reserved at construction) -// └──────────────────────┘ +// ┌─────────────────────┐ +// │ attachmentReserved │ ← no transitions; pre-reserved at construction +// └─────────────────────┘ // // State descriptions: // +// - [attachmentPending]: entered when a new slot is allocated. +// The disk has not yet been added to the SCSI bus. // - [attachmentAttached]: entered once [AddSCSIDisk] succeeds; // the disk is on the SCSI bus and available for guest mounts. +// - [attachmentInvalid]: entered when [AddSCSIDisk] fails; +// the caller must call [Manager.DetachFromVM] to free the slot. // - [attachmentUnplugged]: entered once the guest-side unplug completes; // the guest has released the device but the host has not yet removed it. // - [attachmentDetached]: terminal state entered once [RemoveSCSIDisk] succeeds. @@ -58,7 +74,7 @@ // // # Usage // -// mgr := scsi.New(vmScsiManager, linuxGuestMgr, numControllers, reservedSlots) +// mgr := scsi.New(vmSCSI, linuxGuestSCSI, numControllers, reservedSlots) // // slot, err := mgr.AttachDiskToVM(ctx, "/path/to/disk.vhdx", scsi.DiskTypeVirtualDisk, false) // if err != nil { diff --git a/internal/controller/device/scsi/interface.go b/internal/controller/device/scsi/interface.go index 42ce448185..eb458966b1 100644 --- a/internal/controller/device/scsi/interface.go +++ b/internal/controller/device/scsi/interface.go @@ -2,7 +2,13 @@ package scsi -import "context" +import ( + "context" + "sync" + + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + "github.com/Microsoft/hcsshim/internal/protocol/guestresource" +) // DiskType identifies the attachment protocol used when adding a disk to the VM's SCSI bus. type DiskType string @@ -37,6 +43,25 @@ type VMSlot struct { LUN uint } +// vmSCSI manages adding and removing SCSI devices for a Utility VM. +type vmSCSI interface { + // AddSCSIDisk hot adds a SCSI disk to the Utility VM. + AddSCSIDisk(ctx context.Context, disk hcsschema.Attachment, controller uint, lun uint) error + + // RemoveSCSIDisk removes a SCSI disk from a Utility VM. + RemoveSCSIDisk(ctx context.Context, controller uint, lun uint) error +} + +// linuxGuestSCSI exposes mapped virtual disk and SCSI device operations in the LCOW guest. +type linuxGuestSCSI interface { + // AddLCOWMappedVirtualDisk maps a virtual disk into the LCOW guest. + AddLCOWMappedVirtualDisk(ctx context.Context, settings guestresource.LCOWMappedVirtualDisk) error + // RemoveLCOWMappedVirtualDisk unmaps a virtual disk from the LCOW guest. + RemoveLCOWMappedVirtualDisk(ctx context.Context, settings guestresource.LCOWMappedVirtualDisk) error + // RemoveSCSIDevice removes a SCSI device from the guest. + RemoveSCSIDevice(ctx context.Context, settings guestresource.SCSIDevice) error +} + // ============================================================================== // INTERNAL DATA STRUCTURES // Types and constants below this line are unexported and used for state tracking. @@ -56,6 +81,9 @@ type diskConfig struct { // vmAttachment records one disk's full attachment state and reference count. type vmAttachment struct { + // mu serializes state transitions and broadcasts completion to concurrent waiters. + mu sync.Mutex + // config is the immutable disk parameters used to match duplicate attach requests. config *diskConfig @@ -71,8 +99,8 @@ type vmAttachment struct { // Access must be guarded by [Manager.mu]. state attachmentState - // waitCh is closed (with waitErr set) once the HCS attach call for this - // attachment has finished. - waitCh chan struct{} - waitErr error + // stateErr records the error that caused a transition to [attachmentInvalid]. + // Waiters that find the attachment in the invalid state return this error so + // that every caller sees the original failure reason. + stateErr error } diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go index 1dae5816dc..353f423e88 100644 --- a/internal/controller/device/scsi/scsi.go +++ b/internal/controller/device/scsi/scsi.go @@ -12,8 +12,6 @@ import ( hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/logfields" - "github.com/Microsoft/hcsshim/internal/vm/guestmanager" - "github.com/Microsoft/hcsshim/internal/vm/vmmanager" "github.com/sirupsen/logrus" ) @@ -21,23 +19,23 @@ import ( // Manager implements [Controller] and manages SCSI disk attachment across // one or more controllers on a Hyper-V VM. type Manager struct { - // mu guards attachments and all mutable fields of every [vmAttachment] in the map. - mu sync.Mutex + // globalMu protects the attachments map and serializes slot allocation across concurrent callers. + globalMu sync.Mutex // numControllers is the number of SCSI controllers available on the VM. // It bounds the (controller, lun) search space when allocating a free slot. numControllers int // attachments tracks every disk currently being attached or already attached - // to the VM. Key = (controller, lun) hardware address. - // Access must be guarded by mu. - attachments map[VMSlot]*vmAttachment + // to the VM. Indexed as attachments[controller][lun]. + // A nil entry means the slot is free. Access must be guarded by globalMu. + attachments [][]*vmAttachment - // vmScsiManager is the host-side SCSI manager used to add and remove disks from the VM. - vmScsiManager vmmanager.SCSIManager + // vmSCSI is the host-side SCSI manager used to add and remove disks from the VM. + vmSCSI vmSCSI - // linuxGuestMgr is used to perform the guest-side unplug on LCOW prior to detach. - linuxGuestMgr guestmanager.LCOWScsiManager + // linuxGuestSCSI is used to perform the guest-side unplug on LCOW prior to detach. + linuxGuestSCSI linuxGuestSCSI } var _ Controller = (*Manager)(nil) @@ -45,21 +43,26 @@ var _ Controller = (*Manager)(nil) // New creates a new [Manager] instance conforming to [Controller] interface. // ReservedSlots are never allocated to new disks. func New( - vmScsiManager vmmanager.SCSIManager, - linuxGuest guestmanager.LCOWScsiManager, + vmScsi vmSCSI, + linuxGuestScsi linuxGuestSCSI, numControllers int, reservedSlots []VMSlot, ) *Manager { + attachments := make([][]*vmAttachment, numControllers) + for i := range attachments { + attachments[i] = make([]*vmAttachment, numLUNsPerController) + } + m := &Manager{ numControllers: numControllers, - attachments: make(map[VMSlot]*vmAttachment, len(reservedSlots)), - vmScsiManager: vmScsiManager, - linuxGuestMgr: linuxGuest, + attachments: attachments, + vmSCSI: vmScsi, + linuxGuestSCSI: linuxGuestScsi, } // Pre-populate attachments with reserved slots so they are never allocated to new disks. for _, s := range reservedSlots { - m.attachments[s] = &vmAttachment{ + m.attachments[s.Controller][s.LUN] = &vmAttachment{ controller: s.Controller, lun: s.LUN, refCount: 1, @@ -79,7 +82,6 @@ func (m *Manager) AttachDiskToVM( diskType DiskType, readOnly bool, ) (VMSlot, error) { - ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Operation, "AttachDiskToVM")) log.G(ctx).WithFields(logrus.Fields{ logfields.HostPath: hostPath, @@ -108,96 +110,104 @@ func (m *Manager) AttachDiskToVM( } // attachDiskToVM is the internal implementation of [Manager.AttachDiskToVM]. -// It calls trackAttachment to either reuse an in-flight attachment or -// claim a new slot, then drives the HCS add-disk call. +// It calls [Manager.trackAttachment] to reuse an existing slot or allocate a new one, +// then drives the HCS add-disk call. On failure the attachment is marked invalid and +// the caller must invoke [Manager.DetachFromVM] to clean up the entry. func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlot, error) { - // Track the attachment and get the slot to attach to. - att, existed, err := m.trackAttachment(ctx, config) + // Track the attachment and get the slot for attachment. + // The attachment may be Pending, Attached, or Invalid. + m.globalMu.Lock() + att, err := m.trackAttachment(ctx, config) if err != nil { + m.globalMu.Unlock() return VMSlot{}, err } - // ============================================================================== - // Found an existing attachment. - // ============================================================================== - if existed { - // Another goroutine is already attaching (or has attached) the same disk. - // Wait for it to finish, honoring context cancellation. - select { - case <-ctx.Done(): - // Undo the refCount bump from trackAttachment so the - // attachment can eventually reach zero and be torn down. - m.mu.Lock() - att.refCount-- - m.mu.Unlock() - return VMSlot{}, ctx.Err() - case <-att.waitCh: - if att.waitErr != nil { - // The original attach failed. - // The attachment will be removed from the map. - return VMSlot{}, att.waitErr - } - } - - log.G(ctx).WithFields(logrus.Fields{ - logfields.Controller: att.controller, - logfields.LUN: att.lun, - }).Debug("reusing existing SCSI VM attachment") - - return VMSlot{Controller: att.controller, LUN: att.lun}, nil - } - - // ============================================================================== - // New attachment — we own the slot. - // ============================================================================== + // Acquire the attachment mutex to check the state and potentially drive the attach operation. + att.mu.Lock() + // Release the global lock. + m.globalMu.Unlock() + defer att.mu.Unlock() - // Perform the host-side HCS call to add the disk at the allocated (controller, lun) slot. - log.G(ctx).WithFields(logrus.Fields{ + ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{ logfields.Controller: att.controller, logfields.LUN: att.lun, - }).Debug("performing AddSCSIDisk call to add disk to VM") + })) - err = m.vmScsiManager.AddSCSIDisk(ctx, hcsschema.Attachment{ - Path: config.hostPath, - Type_: string(config.typ), - ReadOnly: config.readOnly, - ExtensibleVirtualDiskType: config.evdType, - }, att.controller, att.lun) + log.G(ctx).Debug("received attachment for disk, checking state") + + switch att.state { + case attachmentAttached: + // ============================================================================== + // Found an existing attachment. + // ============================================================================== + att.refCount++ + slot := VMSlot{Controller: att.controller, LUN: att.lun} + + log.G(ctx).Debug("disk already attached to VM, reusing existing slot") + + return slot, nil + case attachmentPending: + // ============================================================================== + // New attachment — we own the slot. + // Other callers requesting this attachment will block on + // att.mu until we transition the state out of Pending. + // ============================================================================== + + log.G(ctx).Debug("performing AddSCSIDisk call to add disk to VM") + + // Perform the host-side HCS call to add the disk at the allocated (controller, lun) slot. + if err = m.vmSCSI.AddSCSIDisk(ctx, hcsschema.Attachment{ + Path: config.hostPath, + Type_: string(config.typ), + ReadOnly: config.readOnly, + ExtensibleVirtualDiskType: config.evdType, + }, att.controller, att.lun); err != nil { + + // Move the state to Invalid so that other goroutines waiting on + // the same attachment see the real failure reason via stateErr. + // The caller must call DetachFromVM to remove the map entry. + att.state = attachmentInvalid + att.stateErr = err + + return VMSlot{Controller: att.controller, LUN: att.lun}, + fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", + config.hostPath, att.controller, att.lun, err) + } - // Signal completion to any goroutines waiting on the same disk. - att.waitErr = err - close(att.waitCh) + // Mark the attachment as attached so that future callers can reuse it. + att.state = attachmentAttached + att.refCount++ - // Clean up on failure. - if err != nil { - m.mu.Lock() - delete(m.attachments, VMSlot{att.controller, att.lun}) - m.mu.Unlock() + log.G(ctx).Debug("SCSI disk attached to VM") - return VMSlot{}, fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", - config.hostPath, att.controller, att.lun, err) + return VMSlot{Controller: att.controller, LUN: att.lun}, nil + case attachmentInvalid: + // ============================================================================== + // Found an attachment which failed during HCS operation. + // ============================================================================== + + // Return the original error along with the slot so the caller can + // call DetachFromVM to clean up the entry. + return VMSlot{Controller: att.controller, LUN: att.lun}, + fmt.Errorf("previous attempt to attach disk to VM at controller=%d lun=%d failed: %w", + att.controller, att.lun, att.stateErr) + default: + // Unlikely state that should never be observed here. + return VMSlot{}, fmt.Errorf("disk in unexpected state %s during attach", att.state) } - - log.G(ctx).WithFields(logrus.Fields{ - logfields.Controller: att.controller, - logfields.LUN: att.lun, - }).Debug("SCSI disk attached to VM") - - return VMSlot{Controller: att.controller, LUN: att.lun}, nil } // trackAttachment either reuses an existing [vmAttachment] for the same disk config, // incrementing its reference count, or allocates the first free (controller, lun) slot // and registers a new [vmAttachment] in the internal map. -func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmAttachment, bool, error) { - m.mu.Lock() - defer m.mu.Unlock() - +func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmAttachment, error) { // Reuse an existing attachment for the same disk. - for _, existing := range m.attachments { - if existing.config != nil && *existing.config == *config { - existing.refCount++ - return existing, true, nil + for _, row := range m.attachments { + for _, existing := range row { + if existing != nil && existing.config != nil && *existing.config == *config { + return existing, nil + } } } @@ -206,9 +216,8 @@ func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmA // Find the first free (controller, lun) pair. for ctrl := uint(0); ctrl < uint(m.numControllers); ctrl++ { for lun := uint(0); lun < numLUNsPerController; lun++ { - key := VMSlot{ctrl, lun} // if the slot is occupied, then continue to next slot. - if _, occupied := m.attachments[key]; occupied { + if m.attachments[ctrl][lun] != nil { continue } @@ -222,16 +231,16 @@ func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmA config: config, controller: ctrl, lun: lun, - refCount: 1, - state: attachmentAttached, - waitCh: make(chan struct{}), + // Refcount is 0 here since the attachment has not been claimed yet. + refCount: 0, + state: attachmentPending, } - m.attachments[key] = att - return att, false, nil + m.attachments[ctrl][lun] = att + return att, nil } } - return nil, false, errors.New("no available scsi slot") + return nil, errors.New("no available scsi slot") } // DetachFromVM detaches the disk at slot from the VM, unplugging it from the guest first. @@ -242,61 +251,88 @@ func (m *Manager) DetachFromVM( slot VMSlot, ) error { ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{ - logfields.Operation: "DetachFromVM", logfields.Controller: slot.Controller, logfields.LUN: slot.LUN, })) log.G(ctx).Debug("Detaching from VM") - m.mu.Lock() - defer m.mu.Unlock() + // Under global lock, find the attachment and lock it before releasing + // globalMu. This prevents a concurrent AttachDiskToVM from observing the + // attachment and incrementing its refCount between our globalMu.Unlock and + // att.mu.Lock. + m.globalMu.Lock() - existing, ok := m.attachments[slot] - if !ok { + // Ensure the slot is valid. + if slot.Controller >= uint(m.numControllers) || slot.LUN >= numLUNsPerController { + m.globalMu.Unlock() + return fmt.Errorf("invalid slot: controller=%d lun=%d", slot.Controller, slot.LUN) + } + + att := m.attachments[slot.Controller][slot.LUN] + if att == nil { + m.globalMu.Unlock() return fmt.Errorf("no existing attachment found for controller=%d lun=%d", slot.Controller, slot.LUN) } - if existing.state == attachmentReserved { + if att.state == attachmentReserved { + m.globalMu.Unlock() return fmt.Errorf("cannot release reserved attachment at controller=%d lun=%d", slot.Controller, slot.LUN) } - if existing.refCount > 0 { - existing.refCount-- + // Lock the attachment while still holding globalMu to close the race window. + att.mu.Lock() + m.globalMu.Unlock() + defer att.mu.Unlock() + + if att.refCount > 0 { + att.refCount-- } - if existing.refCount > 0 { + if att.refCount > 0 { // Other callers still hold a reference to this disk. log.G(ctx).Debug("disk still in use by other callers, not detaching from VM") return nil } + // If the disk attach failed (AddSCSIDisk never succeeded), just remove the map + // entry — there is nothing to unplug or detach on the host. + if att.state == attachmentInvalid { + log.G(ctx).WithError(att.stateErr).Error("previous attach attempt failed, cleaning up invalid attachment") + + m.globalMu.Lock() + m.attachments[slot.Controller][slot.LUN] = nil + m.globalMu.Unlock() + + return nil + } + // Unplug the device from the guest before removing it from the VM. // Skip if already unplugged from a previous attempt where RemoveSCSIDisk // failed after a successful unplug. - if existing.state == attachmentAttached { + if att.state == attachmentAttached { if err := m.unplugFromGuest(ctx, slot.Controller, slot.LUN); err != nil { return fmt.Errorf("unplug scsi disk at controller=%d lun=%d from guest: %w", slot.Controller, slot.LUN, err) } - existing.state = attachmentUnplugged + att.state = attachmentUnplugged log.G(ctx).Debug("disk unplugged from guest") } - if existing.state == attachmentUnplugged { - if err := m.vmScsiManager.RemoveSCSIDisk(ctx, slot.Controller, slot.LUN); err != nil { + if att.state == attachmentUnplugged { + if err := m.vmSCSI.RemoveSCSIDisk(ctx, slot.Controller, slot.LUN); err != nil { return fmt.Errorf("remove scsi disk at controller=%d lun=%d from vm: %w", slot.Controller, slot.LUN, err) } - existing.state = attachmentDetached + att.state = attachmentDetached } - delete(m.attachments, slot) + // Re-acquire globalMu to safely remove the entry from the map. + m.globalMu.Lock() + m.attachments[slot.Controller][slot.LUN] = nil + m.globalMu.Unlock() - log.G(ctx).WithFields(logrus.Fields{ - logfields.Controller: slot.Controller, - logfields.LUN: slot.LUN, - }).Debug("SCSI disk detached from VM") + log.G(ctx).Debug("SCSI disk detached from VM") return nil } @@ -305,6 +341,10 @@ func (m *Manager) DetachFromVM( // its EVD provider type and the underlying mount path. // Returns an error if the path does not conform to this scheme. func parseEVDPath(hostPath string) (evdType, mountPath string, err error) { + if !strings.HasPrefix(hostPath, "evd://") { + return "", "", fmt.Errorf("invalid extensible vhd path: %q", hostPath) + } + trimmedPath := strings.TrimPrefix(hostPath, "evd://") separatorIndex := strings.Index(trimmedPath, "/") if separatorIndex <= 0 { diff --git a/internal/controller/device/scsi/scsi_guest_lcow.go b/internal/controller/device/scsi/scsi_guest_lcow.go index fcac68b04d..7bc8fd3433 100644 --- a/internal/controller/device/scsi/scsi_guest_lcow.go +++ b/internal/controller/device/scsi/scsi_guest_lcow.go @@ -23,7 +23,7 @@ func (m *Manager) unplugFromGuest(ctx context.Context, controller, lun uint) err // writing "1" to /sys/bus/scsi/devices//delete. That sysfs write is a // guest-initiated hot-unplug: the kernel removes the device from its bus and // flushes any in-flight I/O before the host removes the disk from the VM. - if err := m.linuxGuestMgr.RemoveSCSIDevice(ctx, settings); err != nil { + if err := m.linuxGuestSCSI.RemoveSCSIDevice(ctx, settings); err != nil { return fmt.Errorf("remove scsi device at controller=%d lun=%d from lcow guest: %w", controller, lun, err) } return nil diff --git a/internal/controller/device/scsi/state.go b/internal/controller/device/scsi/state.go index 40fb46d6d1..14d294a7d1 100644 --- a/internal/controller/device/scsi/state.go +++ b/internal/controller/device/scsi/state.go @@ -6,7 +6,12 @@ package scsi // // The normal progression is: // -// attachmentAttached → attachmentUnplugged → attachmentDetached +// attachmentPending → attachmentAttached → attachmentUnplugged → attachmentDetached +// +// If AddSCSIDisk fails, the owning goroutine moves the attachment to +// attachmentInvalid and records the error. Other goroutines waiting on +// the same attachment observe the invalid state and receive the original +// error. The caller must call DetachFromVM to remove the map entry. // // attachmentReserved is a special state for pre-reserved slots that never // transition to any other state. @@ -15,29 +20,36 @@ package scsi // // Current State │ Trigger │ Next State // ────────────────────────┼────────────────────────────────────┼──────────────────── +// attachmentPending │ AddSCSIDisk succeeds │ attachmentAttached +// attachmentPending │ AddSCSIDisk fails │ attachmentInvalid // attachmentAttached │ unplugFromGuest succeeds │ attachmentUnplugged // attachmentUnplugged │ RemoveSCSIDisk succeeds │ attachmentDetached // attachmentDetached │ (terminal — no further transitions)│ — +// attachmentInvalid │ DetachFromVM removes entry │ — // attachmentReserved │ (never transitions) │ — type attachmentState int const ( + // attachmentPending is the initial state; AddSCSIDisk has been called but + // has not yet completed. + attachmentPending attachmentState = iota + // attachmentAttached means AddSCSIDisk succeeded; the disk is on the SCSI // bus and available for guest mounts. - // Valid transitions: - // - attachmentAttached → attachmentUnplugged (via unplugFromGuest, on success) - attachmentAttached attachmentState = iota + attachmentAttached // attachmentUnplugged means unplugFromGuest succeeded; the guest has // released the device but RemoveSCSIDisk has not yet been called. - // Valid transitions: - // - attachmentUnplugged → attachmentDetached (via RemoveSCSIDisk, on success) attachmentUnplugged // attachmentDetached means RemoveSCSIDisk succeeded; the disk has been // fully removed from the VM. This is a terminal state. attachmentDetached + // attachmentInvalid means AddSCSIDisk failed. The caller must call + // [Manager.DetachFromVM] to remove the map entry and free the slot. + attachmentInvalid + // attachmentReserved is used for slots pre-reserved at Manager construction // time. These must never be handed out or torn down — no transitions are valid. attachmentReserved @@ -46,12 +58,16 @@ const ( // String returns a human-readable name for the [attachmentState]. func (s attachmentState) String() string { switch s { + case attachmentPending: + return "Pending" case attachmentAttached: return "Attached" case attachmentUnplugged: return "Unplugged" case attachmentDetached: return "Detached" + case attachmentInvalid: + return "Invalid" case attachmentReserved: return "Reserved" default: diff --git a/internal/vm/guestmanager/scsi_lcow.go b/internal/vm/guestmanager/scsi_lcow.go index 65d1b5b24e..528912275d 100644 --- a/internal/vm/guestmanager/scsi_lcow.go +++ b/internal/vm/guestmanager/scsi_lcow.go @@ -11,18 +11,6 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) -// LCOWScsiManager exposes mapped virtual disk and SCSI device operations in the LCOW guest. -type LCOWScsiManager interface { - // AddLCOWMappedVirtualDisk maps a virtual disk into the LCOW guest. - AddLCOWMappedVirtualDisk(ctx context.Context, settings guestresource.LCOWMappedVirtualDisk) error - // RemoveLCOWMappedVirtualDisk unmaps a virtual disk from the LCOW guest. - RemoveLCOWMappedVirtualDisk(ctx context.Context, settings guestresource.LCOWMappedVirtualDisk) error - // RemoveSCSIDevice removes a SCSI device from the guest. - RemoveSCSIDevice(ctx context.Context, settings guestresource.SCSIDevice) error -} - -var _ LCOWScsiManager = (*Guest)(nil) - // AddLCOWMappedVirtualDisk maps a virtual disk into a LCOW guest. func (gm *Guest) AddLCOWMappedVirtualDisk(ctx context.Context, settings guestresource.LCOWMappedVirtualDisk) error { request := &hcsschema.ModifySettingRequest{ diff --git a/internal/vm/vmmanager/scsi.go b/internal/vm/vmmanager/scsi.go index 6af4e8cc96..038ac66a5e 100644 --- a/internal/vm/vmmanager/scsi.go +++ b/internal/vm/vmmanager/scsi.go @@ -11,17 +11,6 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" ) -// SCSIManager manages adding and removing SCSI devices for a Utility VM. -type SCSIManager interface { - // AddSCSIDisk hot adds a SCSI disk to the Utility VM. - AddSCSIDisk(ctx context.Context, disk hcsschema.Attachment, controller uint, lun uint) error - - // RemoveSCSIDisk removes a SCSI disk from a Utility VM. - RemoveSCSIDisk(ctx context.Context, controller uint, lun uint) error -} - -var _ SCSIManager = (*UtilityVM)(nil) - func (uvm *UtilityVM) AddSCSIDisk(ctx context.Context, disk hcsschema.Attachment, controller uint, lun uint) error { request := &hcsschema.ModifySettingRequest{ RequestType: guestrequest.RequestTypeAdd, From ca0f1260b5ca5949cd696faf489e2f43d99fcce0 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Sat, 21 Mar 2026 04:24:20 +0530 Subject: [PATCH 3/6] review comments : 2 Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/scsi.go | 86 +++++++++---------------- 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go index 353f423e88..80b96752cd 100644 --- a/internal/controller/device/scsi/scsi.go +++ b/internal/controller/device/scsi/scsi.go @@ -27,9 +27,9 @@ type Manager struct { numControllers int // attachments tracks every disk currently being attached or already attached - // to the VM. Indexed as attachments[controller][lun]. - // A nil entry means the slot is free. Access must be guarded by globalMu. - attachments [][]*vmAttachment + // to the VM. Keyed by VMSlot{Controller, LUN}. + // An absent entry means the slot is free. Access must be guarded by globalMu. + attachments map[VMSlot]*vmAttachment // vmSCSI is the host-side SCSI manager used to add and remove disks from the VM. vmSCSI vmSCSI @@ -48,21 +48,16 @@ func New( numControllers int, reservedSlots []VMSlot, ) *Manager { - attachments := make([][]*vmAttachment, numControllers) - for i := range attachments { - attachments[i] = make([]*vmAttachment, numLUNsPerController) - } - m := &Manager{ numControllers: numControllers, - attachments: attachments, + attachments: make(map[VMSlot]*vmAttachment), vmSCSI: vmScsi, linuxGuestSCSI: linuxGuestScsi, } // Pre-populate attachments with reserved slots so they are never allocated to new disks. for _, s := range reservedSlots { - m.attachments[s.Controller][s.LUN] = &vmAttachment{ + m.attachments[s] = &vmAttachment{ controller: s.Controller, lun: s.LUN, refCount: 1, @@ -110,23 +105,19 @@ func (m *Manager) AttachDiskToVM( } // attachDiskToVM is the internal implementation of [Manager.AttachDiskToVM]. -// It calls [Manager.trackAttachment] to reuse an existing slot or allocate a new one, +// It calls [Manager.getOrAllocateSlot] to reuse an existing slot or allocate a new one, // then drives the HCS add-disk call. On failure the attachment is marked invalid and // the caller must invoke [Manager.DetachFromVM] to clean up the entry. func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlot, error) { // Track the attachment and get the slot for attachment. // The attachment may be Pending, Attached, or Invalid. - m.globalMu.Lock() - att, err := m.trackAttachment(ctx, config) + att, err := m.getOrAllocateSlot(ctx, config) if err != nil { - m.globalMu.Unlock() return VMSlot{}, err } // Acquire the attachment mutex to check the state and potentially drive the attach operation. att.mu.Lock() - // Release the global lock. - m.globalMu.Unlock() defer att.mu.Unlock() ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{ @@ -198,26 +189,29 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo } } -// trackAttachment either reuses an existing [vmAttachment] for the same disk config, +// getOrAllocateSlot either reuses an existing [vmAttachment] for the same disk config, // incrementing its reference count, or allocates the first free (controller, lun) slot // and registers a new [vmAttachment] in the internal map. -func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmAttachment, error) { +func (m *Manager) getOrAllocateSlot(ctx context.Context, config *diskConfig) (*vmAttachment, error) { + m.globalMu.Lock() + defer m.globalMu.Unlock() + // Reuse an existing attachment for the same disk. - for _, row := range m.attachments { - for _, existing := range row { - if existing != nil && existing.config != nil && *existing.config == *config { - return existing, nil - } + for _, existing := range m.attachments { + if existing != nil && existing.config != nil && existing.config.hostPath == config.hostPath { + return existing, nil } } log.G(ctx).Debug("no existing attachment found for disk, allocating new slot") // Find the first free (controller, lun) pair. - for ctrl := uint(0); ctrl < uint(m.numControllers); ctrl++ { - for lun := uint(0); lun < numLUNsPerController; lun++ { + for ctrl := range m.numControllers { + for lun := range numLUNsPerController { + slot := VMSlot{Controller: uint(ctrl), LUN: uint(lun)} + // if the slot is occupied, then continue to next slot. - if m.attachments[ctrl][lun] != nil { + if _, occupied := m.attachments[slot]; occupied { continue } @@ -229,13 +223,13 @@ func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmA att := &vmAttachment{ config: config, - controller: ctrl, - lun: lun, + controller: uint(ctrl), + lun: uint(lun), // Refcount is 0 here since the attachment has not been claimed yet. refCount: 0, state: attachmentPending, } - m.attachments[ctrl][lun] = att + m.attachments[slot] = att return att, nil } } @@ -249,7 +243,8 @@ func (m *Manager) trackAttachment(ctx context.Context, config *diskConfig) (*vmA func (m *Manager) DetachFromVM( ctx context.Context, slot VMSlot, -) error { +) (err error) { + ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{ logfields.Controller: slot.Controller, logfields.LUN: slot.LUN, @@ -257,32 +252,21 @@ func (m *Manager) DetachFromVM( log.G(ctx).Debug("Detaching from VM") - // Under global lock, find the attachment and lock it before releasing - // globalMu. This prevents a concurrent AttachDiskToVM from observing the - // attachment and incrementing its refCount between our globalMu.Unlock and - // att.mu.Lock. + // Under global lock, find the attachment. m.globalMu.Lock() + // Get the attachment for this slot and unlock global lock. + att := m.attachments[slot] + m.globalMu.Unlock() - // Ensure the slot is valid. - if slot.Controller >= uint(m.numControllers) || slot.LUN >= numLUNsPerController { - m.globalMu.Unlock() - return fmt.Errorf("invalid slot: controller=%d lun=%d", slot.Controller, slot.LUN) - } - - att := m.attachments[slot.Controller][slot.LUN] if att == nil { - m.globalMu.Unlock() return fmt.Errorf("no existing attachment found for controller=%d lun=%d", slot.Controller, slot.LUN) } if att.state == attachmentReserved { - m.globalMu.Unlock() return fmt.Errorf("cannot release reserved attachment at controller=%d lun=%d", slot.Controller, slot.LUN) } - // Lock the attachment while still holding globalMu to close the race window. att.mu.Lock() - m.globalMu.Unlock() defer att.mu.Unlock() if att.refCount > 0 { @@ -297,12 +281,8 @@ func (m *Manager) DetachFromVM( // If the disk attach failed (AddSCSIDisk never succeeded), just remove the map // entry — there is nothing to unplug or detach on the host. if att.state == attachmentInvalid { + delete(m.attachments, slot) log.G(ctx).WithError(att.stateErr).Error("previous attach attempt failed, cleaning up invalid attachment") - - m.globalMu.Lock() - m.attachments[slot.Controller][slot.LUN] = nil - m.globalMu.Unlock() - return nil } @@ -327,10 +307,8 @@ func (m *Manager) DetachFromVM( att.state = attachmentDetached } - // Re-acquire globalMu to safely remove the entry from the map. - m.globalMu.Lock() - m.attachments[slot.Controller][slot.LUN] = nil - m.globalMu.Unlock() + // Cleanup from the map. + delete(m.attachments, slot) log.G(ctx).Debug("SCSI disk detached from VM") From 31cef2309ae44b5f6a27bbe4173d312f4a6f148a Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Sat, 21 Mar 2026 05:04:36 +0530 Subject: [PATCH 4/6] review comments : 3 Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/doc.go | 8 ++--- internal/controller/device/scsi/scsi.go | 42 +++++++++++++----------- internal/controller/device/scsi/state.go | 7 ++-- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/internal/controller/device/scsi/doc.go b/internal/controller/device/scsi/doc.go index 05dafd14ae..c6140e30af 100644 --- a/internal/controller/device/scsi/doc.go +++ b/internal/controller/device/scsi/doc.go @@ -24,9 +24,9 @@ // ┌─────────────────────┐ ┌──────────────────────┐ // │ attachmentAttached │ │ attachmentInvalid │ // └──────────┬──────────┘ └──────────┬───────────┘ -// │ unplugFromGuest │ DetachFromVM -// │ succeeds │ (refCount → 0) -// ▼ ▼ +// │ unplugFromGuest │ (auto-removed +// │ succeeds │ from map) +// ▼ ▼ // ┌─────────────────────┐ (removed from map) // │ attachmentUnplugged │ // └──────────┬──────────┘ @@ -47,7 +47,7 @@ // - [attachmentAttached]: entered once [AddSCSIDisk] succeeds; // the disk is on the SCSI bus and available for guest mounts. // - [attachmentInvalid]: entered when [AddSCSIDisk] fails; -// the caller must call [Manager.DetachFromVM] to free the slot. +// the map entry is removed. // - [attachmentUnplugged]: entered once the guest-side unplug completes; // the guest has released the device but the host has not yet removed it. // - [attachmentDetached]: terminal state entered once [RemoveSCSIDisk] succeeds. diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go index 80b96752cd..70bd866a43 100644 --- a/internal/controller/device/scsi/scsi.go +++ b/internal/controller/device/scsi/scsi.go @@ -106,8 +106,8 @@ func (m *Manager) AttachDiskToVM( // attachDiskToVM is the internal implementation of [Manager.AttachDiskToVM]. // It calls [Manager.getOrAllocateSlot] to reuse an existing slot or allocate a new one, -// then drives the HCS add-disk call. On failure the attachment is marked invalid and -// the caller must invoke [Manager.DetachFromVM] to clean up the entry. +// then drives the HCS add-disk call. On failure the attachment is removed from +// the internal map and the error is returned. func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlot, error) { // Track the attachment and get the slot for attachment. // The attachment may be Pending, Attached, or Invalid. @@ -157,13 +157,17 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo // Move the state to Invalid so that other goroutines waiting on // the same attachment see the real failure reason via stateErr. - // The caller must call DetachFromVM to remove the map entry. att.state = attachmentInvalid att.stateErr = err - return VMSlot{Controller: att.controller, LUN: att.lun}, - fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", - config.hostPath, att.controller, att.lun, err) + // Delete from the map. Any callers waiting on this attachment + // will see the invalid state and receive the original error. + m.globalMu.Lock() + delete(m.attachments, VMSlot{Controller: att.controller, LUN: att.lun}) + m.globalMu.Unlock() + + return VMSlot{}, fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", + config.hostPath, att.controller, att.lun, err) } // Mark the attachment as attached so that future callers can reuse it. @@ -178,11 +182,10 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo // Found an attachment which failed during HCS operation. // ============================================================================== - // Return the original error along with the slot so the caller can - // call DetachFromVM to clean up the entry. - return VMSlot{Controller: att.controller, LUN: att.lun}, - fmt.Errorf("previous attempt to attach disk to VM at controller=%d lun=%d failed: %w", - att.controller, att.lun, att.stateErr) + // Return the original error. The map entry has already been removed + // by the goroutine that drove the failed attach. + return VMSlot{}, fmt.Errorf("previous attempt to attach disk to VM at controller=%d lun=%d failed: %w", + att.controller, att.lun, att.stateErr) default: // Unlikely state that should never be observed here. return VMSlot{}, fmt.Errorf("disk in unexpected state %s during attach", att.state) @@ -258,8 +261,9 @@ func (m *Manager) DetachFromVM( att := m.attachments[slot] m.globalMu.Unlock() + // If there is no attachment, then the slot is already free and there is nothing to detach. if att == nil { - return fmt.Errorf("no existing attachment found for controller=%d lun=%d", slot.Controller, slot.LUN) + return nil } if att.state == attachmentReserved { @@ -269,20 +273,16 @@ func (m *Manager) DetachFromVM( att.mu.Lock() defer att.mu.Unlock() - if att.refCount > 0 { + if att.refCount > 1 { att.refCount-- - } - if att.refCount > 0 { // Other callers still hold a reference to this disk. log.G(ctx).Debug("disk still in use by other callers, not detaching from VM") - return nil + return } - // If the disk attach failed (AddSCSIDisk never succeeded), just remove the map - // entry — there is nothing to unplug or detach on the host. + // If the disk attach failed (AddSCSIDisk never succeeded), but we got the + // entry just prior to removal from map, then state would be invalid. if att.state == attachmentInvalid { - delete(m.attachments, slot) - log.G(ctx).WithError(att.stateErr).Error("previous attach attempt failed, cleaning up invalid attachment") return nil } @@ -308,7 +308,9 @@ func (m *Manager) DetachFromVM( } // Cleanup from the map. + m.globalMu.Lock() delete(m.attachments, slot) + m.globalMu.Unlock() log.G(ctx).Debug("SCSI disk detached from VM") diff --git a/internal/controller/device/scsi/state.go b/internal/controller/device/scsi/state.go index 14d294a7d1..1e20e815c7 100644 --- a/internal/controller/device/scsi/state.go +++ b/internal/controller/device/scsi/state.go @@ -11,7 +11,7 @@ package scsi // If AddSCSIDisk fails, the owning goroutine moves the attachment to // attachmentInvalid and records the error. Other goroutines waiting on // the same attachment observe the invalid state and receive the original -// error. The caller must call DetachFromVM to remove the map entry. +// error. The entry is removed from the map immediately. // // attachmentReserved is a special state for pre-reserved slots that never // transition to any other state. @@ -25,7 +25,7 @@ package scsi // attachmentAttached │ unplugFromGuest succeeds │ attachmentUnplugged // attachmentUnplugged │ RemoveSCSIDisk succeeds │ attachmentDetached // attachmentDetached │ (terminal — no further transitions)│ — -// attachmentInvalid │ DetachFromVM removes entry │ — +// attachmentInvalid │ entry removed from map │ — // attachmentReserved │ (never transitions) │ — type attachmentState int @@ -46,8 +46,7 @@ const ( // fully removed from the VM. This is a terminal state. attachmentDetached - // attachmentInvalid means AddSCSIDisk failed. The caller must call - // [Manager.DetachFromVM] to remove the map entry and free the slot. + // attachmentInvalid means AddSCSIDisk failed. attachmentInvalid // attachmentReserved is used for slots pre-reserved at Manager construction From 8dce756a783b5276858155a33a72abcb3ff6f27b Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 25 Mar 2026 03:54:16 +0530 Subject: [PATCH 5/6] review comments: 4 Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/doc.go | 4 ++-- internal/controller/device/scsi/scsi.go | 21 +++++++++++++++---- .../device/scsi/{interface.go => types.go} | 14 ++++--------- 3 files changed, 23 insertions(+), 16 deletions(-) rename internal/controller/device/scsi/{interface.go => types.go} (87%) diff --git a/internal/controller/device/scsi/doc.go b/internal/controller/device/scsi/doc.go index c6140e30af..a3cbd9177b 100644 --- a/internal/controller/device/scsi/doc.go +++ b/internal/controller/device/scsi/doc.go @@ -3,8 +3,8 @@ // Package scsi manages the lifecycle of SCSI disk attachments on a Hyper-V VM. // // It abstracts host-side slot allocation, reference counting, and two-phase -// teardown (guest unplug followed by host removal) behind the [Controller] -// interface, with [Manager] as the primary implementation. +// teardown (guest unplug followed by host removal) with [Manager] as +// the primary implementation. // // # Lifecycle // diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go index 70bd866a43..f46c39bd3f 100644 --- a/internal/controller/device/scsi/scsi.go +++ b/internal/controller/device/scsi/scsi.go @@ -12,16 +12,20 @@ import ( hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/logfields" + "github.com/Microsoft/hcsshim/internal/wclayer" "github.com/sirupsen/logrus" ) -// Manager implements [Controller] and manages SCSI disk attachment across +// Manager implements the methods to manage SCSI disk attachment across // one or more controllers on a Hyper-V VM. type Manager struct { // globalMu protects the attachments map and serializes slot allocation across concurrent callers. globalMu sync.Mutex + // vmID is the ID for the HCS compute system to which we are attaching disks. + vmID string + // numControllers is the number of SCSI controllers available on the VM. // It bounds the (controller, lun) search space when allocating a free slot. numControllers int @@ -38,17 +42,17 @@ type Manager struct { linuxGuestSCSI linuxGuestSCSI } -var _ Controller = (*Manager)(nil) - -// New creates a new [Manager] instance conforming to [Controller] interface. +// New creates a new [Manager] instance for managing disk attachments. // ReservedSlots are never allocated to new disks. func New( + vmID string, vmScsi vmSCSI, linuxGuestScsi linuxGuestSCSI, numControllers int, reservedSlots []VMSlot, ) *Manager { m := &Manager{ + vmID: vmID, numControllers: numControllers, attachments: make(map[VMSlot]*vmAttachment), vmSCSI: vmScsi, @@ -91,6 +95,15 @@ func (m *Manager) AttachDiskToVM( typ: diskType, } + // For Virtual and Physical disks, we need to grant VM access to the VHD. + if diskType == DiskTypeVirtualDisk || diskType == DiskTypePassThru { + log.G(ctx).WithField(logfields.HostPath, hostPath).Debug("Granting VM access to disk") + + if err := wclayer.GrantVmAccess(ctx, m.vmID, hostPath); err != nil { + return VMSlot{}, err + } + } + // Parse EVD-specific fields out of hostPath before forwarding to attachDiskToVM, if diskType == DiskTypeExtensibleVirtualDisk { evdType, evdMountPath, err := parseEVDPath(hostPath) diff --git a/internal/controller/device/scsi/interface.go b/internal/controller/device/scsi/types.go similarity index 87% rename from internal/controller/device/scsi/interface.go rename to internal/controller/device/scsi/types.go index eb458966b1..c52e5e7e43 100644 --- a/internal/controller/device/scsi/interface.go +++ b/internal/controller/device/scsi/types.go @@ -25,16 +25,6 @@ const ( DiskTypeExtensibleVirtualDisk DiskType = "ExtensibleVirtualDisk" ) -// Controller is the primary interface for attaching and detaching SCSI disks on a VM. -type Controller interface { - // AttachDiskToVM attaches the disk at hostPath to the VM and returns the allocated [VMSlot]. - // If the same disk is already attached, the existing slot is reused. - AttachDiskToVM(ctx context.Context, hostPath string, diskType DiskType, readOnly bool) (VMSlot, error) - - // DetachFromVM unplugs and detaches the disk from the VM. - DetachFromVM(ctx context.Context, slot VMSlot) error -} - // VMSlot identifies a disk's hardware address on the VM's SCSI bus. type VMSlot struct { // Controller is the zero-based SCSI controller index. @@ -43,6 +33,10 @@ type VMSlot struct { LUN uint } +// ============================================================================== +// Interfaces used by Manager to perform actions on VM and Guest. +// ============================================================================== + // vmSCSI manages adding and removing SCSI devices for a Utility VM. type vmSCSI interface { // AddSCSIDisk hot adds a SCSI disk to the Utility VM. From cf1b9717cbb34b10e3f752d6eb514ac926df22a0 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 25 Mar 2026 18:27:51 +0530 Subject: [PATCH 6/6] review comments: 5 Signed-off-by: Harsh Rawat --- internal/controller/device/scsi/doc.go | 13 +++- internal/controller/device/scsi/scsi.go | 78 +++++++++++++++--------- internal/controller/device/scsi/types.go | 23 ++++--- 3 files changed, 74 insertions(+), 40 deletions(-) diff --git a/internal/controller/device/scsi/doc.go b/internal/controller/device/scsi/doc.go index a3cbd9177b..6fc6ec80d6 100644 --- a/internal/controller/device/scsi/doc.go +++ b/internal/controller/device/scsi/doc.go @@ -76,7 +76,18 @@ // // mgr := scsi.New(vmSCSI, linuxGuestSCSI, numControllers, reservedSlots) // -// slot, err := mgr.AttachDiskToVM(ctx, "/path/to/disk.vhdx", scsi.DiskTypeVirtualDisk, false) +// cfg := scsi.DiskConfig{HostPath: "/path/to/disk.vhdx", Type: scsi.DiskTypeVirtualDisk} +// +// // Optionally pre-allocate the slot before the actual attach, e.g. to resolve +// // a guest mount path ahead of time. +// slot, err := mgr.ResolveDiskSlot(ctx, cfg) +// if err != nil { +// // handle error +// } +// +// // ... use slot to resolve downstream resources (e.g. guest mount path) ... +// +// slot, err = mgr.AttachDiskToVM(ctx, cfg) // if err != nil { // // handle error // } diff --git a/internal/controller/device/scsi/scsi.go b/internal/controller/device/scsi/scsi.go index f46c39bd3f..de06f4fb4a 100644 --- a/internal/controller/device/scsi/scsi.go +++ b/internal/controller/device/scsi/scsi.go @@ -72,56 +72,76 @@ func New( return m } +// ResolveDiskSlot pre-emptively allocates a SCSI slot for the given [DiskConfig] and +// returns the assigned [VMSlot]. If a slot is already allocated for this disk, +// we return the existing slot without allocating a new one. +// The returned [VMSlot] can be used to resolve downstream resources (e.g., guest mount paths). +func (m *Manager) ResolveDiskSlot(ctx context.Context, opts DiskConfig) (VMSlot, error) { + log.G(ctx).WithFields(logrus.Fields{ + logfields.HostPath: opts.HostPath, + logfields.DiskType: opts.Type, + }).Debug("Resolving disk slot") + + // Parse EVD-specific fields out of hostPath so the HostPath key used in + // getOrAllocateSlot is consistent with the one used by AttachDiskToVM. + if opts.Type == DiskTypeExtensibleVirtualDisk { + evdType, evdMountPath, err := parseEVDPath(opts.HostPath) + if err != nil { + return VMSlot{}, err + } + opts.HostPath = evdMountPath + opts.EVDType = evdType + } + + att, err := m.getOrAllocateSlot(ctx, &opts) + if err != nil { + return VMSlot{}, err + } + + return VMSlot{Controller: att.controller, LUN: att.lun}, nil +} + // AttachDiskToVM attaches the disk at hostPath to the VM and returns the allocated [VMSlot]. // If the same disk is already in flight or attached, AttachDiskToVM blocks until the // original operation completes and then returns the shared slot. func (m *Manager) AttachDiskToVM( ctx context.Context, - hostPath string, - diskType DiskType, - readOnly bool, + opts DiskConfig, ) (VMSlot, error) { log.G(ctx).WithFields(logrus.Fields{ - logfields.HostPath: hostPath, - logfields.DiskType: diskType, - logfields.ReadOnly: readOnly, + logfields.HostPath: opts.HostPath, + logfields.DiskType: opts.Type, + logfields.ReadOnly: opts.ReadOnly, }).Debug("Attaching disk to VM") - // Create the disk config for the VM. - config := &diskConfig{ - hostPath: hostPath, - readOnly: readOnly, - typ: diskType, - } - // For Virtual and Physical disks, we need to grant VM access to the VHD. - if diskType == DiskTypeVirtualDisk || diskType == DiskTypePassThru { - log.G(ctx).WithField(logfields.HostPath, hostPath).Debug("Granting VM access to disk") + if opts.Type == DiskTypeVirtualDisk || opts.Type == DiskTypePassThru { + log.G(ctx).WithField(logfields.HostPath, opts.HostPath).Debug("Granting VM access to disk") - if err := wclayer.GrantVmAccess(ctx, m.vmID, hostPath); err != nil { + if err := wclayer.GrantVmAccess(ctx, m.vmID, opts.HostPath); err != nil { return VMSlot{}, err } } // Parse EVD-specific fields out of hostPath before forwarding to attachDiskToVM, - if diskType == DiskTypeExtensibleVirtualDisk { - evdType, evdMountPath, err := parseEVDPath(hostPath) + if opts.Type == DiskTypeExtensibleVirtualDisk { + evdType, evdMountPath, err := parseEVDPath(opts.HostPath) if err != nil { return VMSlot{}, err } - config.hostPath = evdMountPath - config.evdType = evdType + opts.HostPath = evdMountPath + opts.EVDType = evdType } - return m.attachDiskToVM(ctx, config) + return m.attachDiskToVM(ctx, &opts) } // attachDiskToVM is the internal implementation of [Manager.AttachDiskToVM]. // It calls [Manager.getOrAllocateSlot] to reuse an existing slot or allocate a new one, // then drives the HCS add-disk call. On failure the attachment is removed from // the internal map and the error is returned. -func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlot, error) { +func (m *Manager) attachDiskToVM(ctx context.Context, config *DiskConfig) (VMSlot, error) { // Track the attachment and get the slot for attachment. // The attachment may be Pending, Attached, or Invalid. att, err := m.getOrAllocateSlot(ctx, config) @@ -162,10 +182,10 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo // Perform the host-side HCS call to add the disk at the allocated (controller, lun) slot. if err = m.vmSCSI.AddSCSIDisk(ctx, hcsschema.Attachment{ - Path: config.hostPath, - Type_: string(config.typ), - ReadOnly: config.readOnly, - ExtensibleVirtualDiskType: config.evdType, + Path: config.HostPath, + Type_: string(config.Type), + ReadOnly: config.ReadOnly, + ExtensibleVirtualDiskType: config.EVDType, }, att.controller, att.lun); err != nil { // Move the state to Invalid so that other goroutines waiting on @@ -180,7 +200,7 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo m.globalMu.Unlock() return VMSlot{}, fmt.Errorf("add scsi disk %q to vm at controller=%d lun=%d: %w", - config.hostPath, att.controller, att.lun, err) + config.HostPath, att.controller, att.lun, err) } // Mark the attachment as attached so that future callers can reuse it. @@ -208,13 +228,13 @@ func (m *Manager) attachDiskToVM(ctx context.Context, config *diskConfig) (VMSlo // getOrAllocateSlot either reuses an existing [vmAttachment] for the same disk config, // incrementing its reference count, or allocates the first free (controller, lun) slot // and registers a new [vmAttachment] in the internal map. -func (m *Manager) getOrAllocateSlot(ctx context.Context, config *diskConfig) (*vmAttachment, error) { +func (m *Manager) getOrAllocateSlot(ctx context.Context, config *DiskConfig) (*vmAttachment, error) { m.globalMu.Lock() defer m.globalMu.Unlock() // Reuse an existing attachment for the same disk. for _, existing := range m.attachments { - if existing != nil && existing.config != nil && existing.config.hostPath == config.hostPath { + if existing != nil && existing.config != nil && existing.config.HostPath == config.HostPath { return existing, nil } } diff --git a/internal/controller/device/scsi/types.go b/internal/controller/device/scsi/types.go index c52e5e7e43..7223511321 100644 --- a/internal/controller/device/scsi/types.go +++ b/internal/controller/device/scsi/types.go @@ -25,6 +25,18 @@ const ( DiskTypeExtensibleVirtualDisk DiskType = "ExtensibleVirtualDisk" ) +type DiskConfig struct { + // HostPath is the path on the host to the disk to be attached. + HostPath string + // ReadOnly specifies whether the disk should be attached with read-only access. + ReadOnly bool + // Type specifies the attachment protocol to use when attaching the disk. + Type DiskType + // EVDType is the EVD provider name. + // Only populated when Type is [DiskTypeExtensibleVirtualDisk]. + EVDType string +} + // VMSlot identifies a disk's hardware address on the VM's SCSI bus. type VMSlot struct { // Controller is the zero-based SCSI controller index. @@ -64,22 +76,13 @@ type linuxGuestSCSI interface { // numLUNsPerController is the maximum number of LUNs per controller, fixed by Hyper-V. const numLUNsPerController = 64 -// diskConfig holds the immutable parameters that uniquely identify a disk attachment request. -type diskConfig struct { - hostPath string - readOnly bool - typ DiskType - // evdType is the EVD provider name; only populated when typ is [DiskTypeExtensibleVirtualDisk]. - evdType string -} - // vmAttachment records one disk's full attachment state and reference count. type vmAttachment struct { // mu serializes state transitions and broadcasts completion to concurrent waiters. mu sync.Mutex // config is the immutable disk parameters used to match duplicate attach requests. - config *diskConfig + config *DiskConfig // controller and lun are the allocated hardware address on the SCSI bus. controller uint