From 81cdaf0771956204ecbbc4865ee004cfdf872e4d Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 28 Jul 2022 15:08:16 -0700 Subject: [PATCH 1/7] govmm: Correct documentation for Linux aio. The comments for "native" aio are incorrect. Correct these. Signed-off-by: Archana Shinde --- src/runtime/pkg/govmm/qemu/qemu.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/pkg/govmm/qemu/qemu.go b/src/runtime/pkg/govmm/qemu/qemu.go index 100316dd9e..5fb284eb11 100644 --- a/src/runtime/pkg/govmm/qemu/qemu.go +++ b/src/runtime/pkg/govmm/qemu/qemu.go @@ -1140,7 +1140,7 @@ const ( // Threads is the pthread asynchronous I/O implementation. Threads BlockDeviceAIO = "threads" - // Native is the pthread asynchronous I/O implementation. + // Native is the native Linux AIO implementation. Native BlockDeviceAIO = "native" ) From b6cd2348f51541b994ceee26e1394be6db19e2e5 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 28 Jul 2022 15:37:00 -0700 Subject: [PATCH 2/7] govmm: Add io_uring as AIO type io_uring was introduced as a new kernel IO interface in kernel 5.1. It is designed for higher performance than the older Linux AIO API. This feature was added in qemu 5.0. Fixes #4645 Signed-off-by: Archana Shinde --- src/runtime/pkg/govmm/qemu/qemu.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/runtime/pkg/govmm/qemu/qemu.go b/src/runtime/pkg/govmm/qemu/qemu.go index 5fb284eb11..4b36df2ea3 100644 --- a/src/runtime/pkg/govmm/qemu/qemu.go +++ b/src/runtime/pkg/govmm/qemu/qemu.go @@ -1142,6 +1142,9 @@ const ( // Native is the native Linux AIO implementation. Native BlockDeviceAIO = "native" + + // IOUring is the Linux io_uring I/O implementation. + IOUring BlockDeviceAIO = "io_uring" ) const ( From ed0f1d0b323b5e13410163f7b3380ace33bc26ae Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Mon, 1 Aug 2022 13:35:29 -0700 Subject: [PATCH 3/7] config: Add "block_device_aio" as a config option for qemu This configuration will allow users to choose between different I/O backends for qemu, with the default being io_uring. This will allow users to fallback to a different I/O mechanism while running on kernels olders than 5.1. Signed-off-by: Archana Shinde --- src/runtime/Makefile | 2 ++ src/runtime/config/configuration-qemu.toml.in | 14 +++++++++++ src/runtime/pkg/device/config/config.go | 11 +++++++++ src/runtime/pkg/katatestutils/utils.go | 2 ++ .../pkg/katautils/config-settings.go.in | 1 + src/runtime/pkg/katautils/config.go | 24 +++++++++++++++++++ src/runtime/pkg/katautils/config_test.go | 11 +++++++++ src/runtime/virtcontainers/hypervisor.go | 3 +++ 8 files changed, 68 insertions(+) diff --git a/src/runtime/Makefile b/src/runtime/Makefile index fa07b87deb..b05106318b 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -253,6 +253,7 @@ ifneq (,$(QEMUCMD)) # qemu-specific options (all should be suffixed by "_QEMU") DEFBLOCKSTORAGEDRIVER_QEMU := virtio-scsi + DEFBLOCKDEVICEAIO_QEMU := io_uring DEFNETWORKMODEL_QEMU := tcfilter KERNELTYPE = uncompressed KERNELNAME = $(call MAKE_KERNEL_NAME,$(KERNELTYPE)) @@ -458,6 +459,7 @@ USER_VARS += DEFDISABLEBLOCK USER_VARS += DEFBLOCKSTORAGEDRIVER_ACRN USER_VARS += DEFBLOCKSTORAGEDRIVER_FC USER_VARS += DEFBLOCKSTORAGEDRIVER_QEMU +USER_VARS += DEFBLOCKDEVICEAIO_QEMU USER_VARS += DEFSHAREDFS_CLH_VIRTIOFS USER_VARS += DEFSHAREDFS_QEMU_VIRTIOFS USER_VARS += DEFVIRTIOFSDAEMON diff --git a/src/runtime/config/configuration-qemu.toml.in b/src/runtime/config/configuration-qemu.toml.in index 3ec44c8b6e..d0a711dcfe 100644 --- a/src/runtime/config/configuration-qemu.toml.in +++ b/src/runtime/config/configuration-qemu.toml.in @@ -208,6 +208,20 @@ virtio_fs_cache = "@DEFVIRTIOFSCACHE@" # or nvdimm. block_device_driver = "@DEFBLOCKSTORAGEDRIVER_QEMU@" +# aio is the I/O mechanism used by qemu +# Options: +# +# - threads +# Pthread based disk I/O. +# +# - native +# Native Linux I/O. +# +# - io_uring +# Linux io_uring API. This provides the fastest I/O operations on Linux, requires kernel>5.1 and +# qemu >=5.0. +block_device_aio = "@DEFBLOCKDEVICEAIO_QEMU@" + # Specifies cache-related options will be set to block devices or not. # Default false #block_device_cache_set = true diff --git a/src/runtime/pkg/device/config/config.go b/src/runtime/pkg/device/config/config.go index 61f9236b9c..a11c52f75c 100644 --- a/src/runtime/pkg/device/config/config.go +++ b/src/runtime/pkg/device/config/config.go @@ -61,6 +61,17 @@ const ( Nvdimm = "nvdimm" ) +const ( + // AIOThreads is the pthread asynchronous I/O implementation. + AIOThreads = "threads" + + // AIONative is the native Linux AIO implementation + AIONative = "native" + + // AIOUring is the Linux io_uring I/O implementation + AIOIOUring = "io_uring" +) + const ( // Virtio9P means use virtio-9p for the shared file system Virtio9P = "virtio-9p" diff --git a/src/runtime/pkg/katatestutils/utils.go b/src/runtime/pkg/katatestutils/utils.go index 5676f4451c..2aa0754cac 100644 --- a/src/runtime/pkg/katatestutils/utils.go +++ b/src/runtime/pkg/katatestutils/utils.go @@ -216,6 +216,7 @@ type RuntimeConfigOptions struct { ShimPath string LogPath string BlockDeviceDriver string + BlockDeviceAIO string SharedFS string VirtioFSDaemon string JaegerEndpoint string @@ -305,6 +306,7 @@ func MakeRuntimeConfigFileData(config RuntimeConfigOptions) string { path = "` + config.HypervisorPath + `" kernel = "` + config.KernelPath + `" block_device_driver = "` + config.BlockDeviceDriver + `" + block_device_aio = "` + config.BlockDeviceAIO + `" kernel_params = "` + config.KernelParams + `" image = "` + config.ImagePath + `" machine_type = "` + config.MachineType + `" diff --git a/src/runtime/pkg/katautils/config-settings.go.in b/src/runtime/pkg/katautils/config-settings.go.in index 2aad22bd85..37dbfee45a 100644 --- a/src/runtime/pkg/katautils/config-settings.go.in +++ b/src/runtime/pkg/katautils/config-settings.go.in @@ -63,6 +63,7 @@ const defaultBridgesCount uint32 = 1 const defaultInterNetworkingModel = "tcfilter" const defaultDisableBlockDeviceUse bool = false const defaultBlockDeviceDriver = "virtio-scsi" +const defaultBlockDeviceAIO string = "io_uring" const defaultBlockDeviceCacheSet bool = false const defaultBlockDeviceCacheDirect bool = false const defaultBlockDeviceCacheNoflush bool = false diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 0903c8ea9e..6147e2738c 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -99,6 +99,7 @@ type hypervisor struct { GuestHookPath string `toml:"guest_hook_path"` GuestMemoryDumpPath string `toml:"guest_memory_dump_path"` SeccompSandbox string `toml:"seccompsandbox"` + BlockDeviceAIO string `toml:"block_device_aio"` HypervisorPathList []string `toml:"valid_hypervisor_paths"` JailerPathList []string `toml:"valid_jailer_paths"` CtlPathList []string `toml:"valid_ctlpaths"` @@ -468,6 +469,22 @@ func (h hypervisor) blockDeviceDriver() (string, error) { return "", fmt.Errorf("Invalid hypervisor block storage driver %v specified (supported drivers: %v)", h.BlockDeviceDriver, supportedBlockDrivers) } +func (h hypervisor) blockDeviceAIO() (string, error) { + supportedBlockAIO := []string{config.AIOIOUring, config.AIONative, config.AIOThreads} + + if h.BlockDeviceAIO == "" { + return defaultBlockDeviceAIO, nil + } + + for _, b := range supportedBlockAIO { + if b == h.BlockDeviceAIO { + return h.BlockDeviceAIO, nil + } + } + + return "", fmt.Errorf("Invalid hypervisor block storage I/O mechanism %v specified (supported AIO: %v)", h.BlockDeviceAIO, supportedBlockAIO) +} + func (h hypervisor) sharedFS() (string, error) { supportedSharedFS := []string{config.Virtio9P, config.VirtioFS, config.VirtioFSNydus} @@ -727,6 +744,11 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{}, err } + blockAIO, err := h.blockDeviceAIO() + if err != nil { + return vc.HypervisorConfig{}, err + } + sharedFS, err := h.sharedFS() if err != nil { return vc.HypervisorConfig{}, err @@ -783,6 +805,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { Debug: h.Debug, DisableNestingChecks: h.DisableNestingChecks, BlockDeviceDriver: blockDriver, + BlockDeviceAIO: blockAIO, BlockDeviceCacheSet: h.BlockDeviceCacheSet, BlockDeviceCacheDirect: h.BlockDeviceCacheDirect, BlockDeviceCacheNoflush: h.BlockDeviceCacheNoflush, @@ -1154,6 +1177,7 @@ func GetDefaultHypervisorConfig() vc.HypervisorConfig { Debug: defaultEnableDebug, DisableNestingChecks: defaultDisableNestingChecks, BlockDeviceDriver: defaultBlockDeviceDriver, + BlockDeviceAIO: defaultBlockDeviceAIO, BlockDeviceCacheSet: defaultBlockDeviceCacheSet, BlockDeviceCacheDirect: defaultBlockDeviceCacheDirect, BlockDeviceCacheNoflush: defaultBlockDeviceCacheNoflush, diff --git a/src/runtime/pkg/katautils/config_test.go b/src/runtime/pkg/katautils/config_test.go index 86619ab13d..5e493b40e3 100644 --- a/src/runtime/pkg/katautils/config_test.go +++ b/src/runtime/pkg/katautils/config_test.go @@ -79,6 +79,7 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf machineType := "machineType" disableBlockDevice := true blockDeviceDriver := "virtio-scsi" + blockDeviceAIO := "io_uring" enableIOThreads := true hotplugVFIOOnRootBus := true pcieRootPort := uint32(2) @@ -99,6 +100,7 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf DefaultGuestHookPath: defaultGuestHookPath, DisableBlock: disableBlockDevice, BlockDeviceDriver: blockDeviceDriver, + BlockDeviceAIO: blockDeviceAIO, EnableIOThreads: enableIOThreads, HotplugVFIOOnRootBus: hotplugVFIOOnRootBus, PCIeRootPort: pcieRootPort, @@ -159,6 +161,7 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf DefaultMaxMemorySize: maxMemory, DisableBlockDeviceUse: disableBlockDevice, BlockDeviceDriver: defaultBlockDeviceDriver, + BlockDeviceAIO: defaultBlockDeviceAIO, DefaultBridges: defaultBridgesCount, EnableIOThreads: enableIOThreads, HotplugVFIOOnRootBus: hotplugVFIOOnRootBus, @@ -550,6 +553,7 @@ func TestMinimalRuntimeConfig(t *testing.T) { GuestHookPath: defaultGuestHookPath, VhostUserStorePath: defaultVhostUserStorePath, VirtioFSCache: defaultVirtioFSCacheMode, + BlockDeviceAIO: defaultBlockDeviceAIO, } expectedAgentConfig := vc.KataAgentConfig{ @@ -593,6 +597,7 @@ func TestNewQemuHypervisorConfig(t *testing.T) { hotplugVFIOOnRootBus := true pcieRootPort := uint32(2) orgVHostVSockDevicePath := utils.VHostVSockDevicePath + blockDeviceAIO := "io_uring" defer func() { utils.VHostVSockDevicePath = orgVHostVSockDevicePath }() @@ -614,6 +619,7 @@ func TestNewQemuHypervisorConfig(t *testing.T) { TxRateLimiterMaxRate: txRateLimiterMaxRate, SharedFS: "virtio-fs", VirtioFSDaemon: filepath.Join(dir, "virtiofsd"), + BlockDeviceAIO: blockDeviceAIO, } files := []string{hypervisorPath, kernelPath, imagePath} @@ -674,6 +680,11 @@ func TestNewQemuHypervisorConfig(t *testing.T) { if config.TxRateLimiterMaxRate != txRateLimiterMaxRate { t.Errorf("Expected value for tx rate limiter %v, got %v", txRateLimiterMaxRate, config.TxRateLimiterMaxRate) } + + if config.BlockDeviceAIO != blockDeviceAIO { + t.Errorf("Expected value for BlockDeviceAIO %v, got %v", blockDeviceAIO, config.BlockDeviceAIO) + } + } func TestNewFirecrackerHypervisorConfig(t *testing.T) { diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 48eda09778..6f549454ed 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -371,6 +371,9 @@ type HypervisorConfig struct { // SeccompSandbox is the qemu function which enables the seccomp feature SeccompSandbox string + // BlockiDeviceAIO specifies the I/O API to be used. + BlockDeviceAIO string + // KernelParams are additional guest kernel parameters. KernelParams []Param From e1b49d75864b37182d50752fb41fc5f735adae41 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 4 Aug 2022 15:23:32 -0700 Subject: [PATCH 4/7] config: Add block aio as a supported annotation Allow Block AIO to be passed as a per pod annotation. Signed-off-by: Archana Shinde --- src/runtime/pkg/oci/utils.go | 16 ++++++++++++++++ src/runtime/pkg/oci/utils_test.go | 2 ++ .../pkg/annotations/annotations.go | 3 +++ 3 files changed, 21 insertions(+) diff --git a/src/runtime/pkg/oci/utils.go b/src/runtime/pkg/oci/utils.go index 055acb9b09..5b6746ec97 100644 --- a/src/runtime/pkg/oci/utils.go +++ b/src/runtime/pkg/oci/utils.go @@ -683,6 +683,22 @@ func addHypervisorBlockOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) } } + if value, ok := ocispec.Annotations[vcAnnotations.BlockDeviceAIO]; ok { + supportedAIO := []string{config.AIONative, config.AIOThreads, config.AIOIOUring} + + valid := false + for _, b := range supportedAIO { + if b == value { + sbConfig.HypervisorConfig.BlockDeviceAIO = value + valid = true + } + } + + if !valid { + return fmt.Errorf("Invalid AIO mechanism %v specified in annotation (supported IO mechanism : %v)", value, supportedAIO) + } + } + if err := newAnnotationConfiguration(ocispec, vcAnnotations.DisableBlockDeviceUse).setBool(func(disableBlockDeviceUse bool) { sbConfig.HypervisorConfig.DisableBlockDeviceUse = disableBlockDeviceUse }); err != nil { diff --git a/src/runtime/pkg/oci/utils_test.go b/src/runtime/pkg/oci/utils_test.go index b5e7440b0c..e7ed37fb33 100644 --- a/src/runtime/pkg/oci/utils_test.go +++ b/src/runtime/pkg/oci/utils_test.go @@ -642,6 +642,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { ocispec.Annotations[vcAnnotations.HugePages] = "true" ocispec.Annotations[vcAnnotations.IOMMU] = "true" ocispec.Annotations[vcAnnotations.BlockDeviceDriver] = "virtio-scsi" + ocispec.Annotations[vcAnnotations.BlockDeviceAIO] = "io_uring" ocispec.Annotations[vcAnnotations.DisableBlockDeviceUse] = "true" ocispec.Annotations[vcAnnotations.EnableIOThreads] = "true" ocispec.Annotations[vcAnnotations.BlockDeviceCacheSet] = "true" @@ -679,6 +680,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { assert.Equal(config.HypervisorConfig.HugePages, true) assert.Equal(config.HypervisorConfig.IOMMU, true) assert.Equal(config.HypervisorConfig.BlockDeviceDriver, "virtio-scsi") + assert.Equal(config.HypervisorConfig.BlockDeviceAIO, "io_uring") assert.Equal(config.HypervisorConfig.DisableBlockDeviceUse, true) assert.Equal(config.HypervisorConfig.EnableIOThreads, true) assert.Equal(config.HypervisorConfig.BlockDeviceCacheSet, true) diff --git a/src/runtime/virtcontainers/pkg/annotations/annotations.go b/src/runtime/virtcontainers/pkg/annotations/annotations.go index 6618d0acdf..d7919a0c8a 100644 --- a/src/runtime/virtcontainers/pkg/annotations/annotations.go +++ b/src/runtime/virtcontainers/pkg/annotations/annotations.go @@ -203,6 +203,9 @@ const ( // BlockDeviceDriver specifies the driver to be used for block device either VirtioSCSI or VirtioBlock BlockDeviceDriver = kataAnnotHypervisorPrefix + "block_device_driver" + // BlockDeviceAIO specifies I/O mechanism to be used with VirtioBlock for qemu + BlockDeviceAIO = kataAnnotHypervisorPrefix + "block_device_aio" + // DisableBlockDeviceUse is a sandbox annotation that disallows a block device from being used. DisableBlockDeviceUse = kataAnnotHypervisorPrefix + "disable_block_device_use" From 00860a7e43ebf3ff0e9b802985be58e4fa49288b Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Mon, 1 Aug 2022 14:30:15 -0700 Subject: [PATCH 5/7] qmp: Pass aio backend while adding block device Allow govmm to pass aio backend while adding block device. Signed-off-by: Archana Shinde --- src/runtime/pkg/govmm/qemu/qmp.go | 15 ++++++++------- src/runtime/virtcontainers/qemu.go | 7 ++++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/runtime/pkg/govmm/qemu/qmp.go b/src/runtime/pkg/govmm/qemu/qmp.go index 9bf091af84..b9b755573d 100644 --- a/src/runtime/pkg/govmm/qemu/qmp.go +++ b/src/runtime/pkg/govmm/qemu/qmp.go @@ -771,7 +771,7 @@ func (q *QMP) ExecuteQuit(ctx context.Context) error { return q.executeCommand(ctx, "quit", nil, nil) } -func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID string, ro bool) (map[string]interface{}, map[string]interface{}) { +func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro bool) (map[string]interface{}, map[string]interface{}) { var args map[string]interface{} blockdevArgs := map[string]interface{}{ @@ -780,6 +780,7 @@ func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID string, ro bool) (m "file": map[string]interface{}{ "driver": driver, "filename": device, + "aio": aio, }, } @@ -793,8 +794,8 @@ func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID string, ro bool) (m // path of the device to add, e.g., /dev/rdb0, and blockdevID is an identifier // used to name the device. As this identifier will be passed directly to QMP, // it must obey QMP's naming rules, e,g., it must start with a letter. -func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID string, ro bool) error { - args, _ := q.blockdevAddBaseArgs("host_device", device, blockdevID, ro) +func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID, aio string, ro bool) error { + args, _ := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) return q.executeCommand(ctx, "blockdev-add", args, nil) } @@ -806,8 +807,8 @@ func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID string, // direct denotes whether use of O_DIRECT (bypass the host page cache) // is enabled. noFlush denotes whether flush requests for the device are // ignored. -func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevID string, direct, noFlush, ro bool) error { - args, blockdevArgs := q.blockdevAddBaseArgs("host_device", device, blockdevID, ro) +func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevID, aio string, direct, noFlush, ro bool) error { + args, blockdevArgs := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, @@ -820,8 +821,8 @@ func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevI // ExecuteBlockdevAddWithDriverCache has three one parameter driver // than ExecuteBlockdevAddWithCache. // Parameter driver can set the driver of block device. -func (q *QMP) ExecuteBlockdevAddWithDriverCache(ctx context.Context, driver, device, blockdevID string, direct, noFlush, ro bool) error { - args, blockdevArgs := q.blockdevAddBaseArgs(driver, device, blockdevID, ro) +func (q *QMP) ExecuteBlockdevAddWithDriverCache(ctx context.Context, driver, device, blockdevID, aio string, direct, noFlush, ro bool) error { + args, blockdevArgs := q.blockdevAddBaseArgs(driver, device, blockdevID, aio, ro) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 115ff5e7d4..7584b4728a 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -1292,12 +1292,13 @@ func (q *qemu) hotplugAddBlockDevice(ctx context.Context, drive *config.BlockDri return nil } + aio := q.config.BlockDeviceAIO if drive.Swap { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithDriverCache(q.qmpMonitorCh.ctx, "file", drive.File, drive.ID, false, false, false) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithDriverCache(q.qmpMonitorCh.ctx, "file", drive.File, drive.ID, aio, false, false, false) } else if q.config.BlockDeviceCacheSet { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithCache(q.qmpMonitorCh.ctx, drive.File, drive.ID, q.config.BlockDeviceCacheDirect, q.config.BlockDeviceCacheNoflush, drive.ReadOnly) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithCache(q.qmpMonitorCh.ctx, drive.File, drive.ID, aio, q.config.BlockDeviceCacheDirect, q.config.BlockDeviceCacheNoflush, drive.ReadOnly) } else { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAdd(q.qmpMonitorCh.ctx, drive.File, drive.ID, drive.ReadOnly) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAdd(q.qmpMonitorCh.ctx, drive.File, drive.ID, aio, drive.ReadOnly) } if err != nil { return err From 598884f3744b40723168e66051f91639833c1c12 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Tue, 2 Aug 2022 22:53:40 -0700 Subject: [PATCH 6/7] govmm: Refactor code to get rid of redundant code Get rid of redundant return values from function. args and blockdevArgs used to return different values to maintain compatilibity between qemu versions. These are exactly the same now. Signed-off-by: Archana Shinde --- src/runtime/pkg/govmm/qemu/qmp.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/runtime/pkg/govmm/qemu/qmp.go b/src/runtime/pkg/govmm/qemu/qmp.go index b9b755573d..480d38c7fa 100644 --- a/src/runtime/pkg/govmm/qemu/qmp.go +++ b/src/runtime/pkg/govmm/qemu/qmp.go @@ -771,9 +771,7 @@ func (q *QMP) ExecuteQuit(ctx context.Context) error { return q.executeCommand(ctx, "quit", nil, nil) } -func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro bool) (map[string]interface{}, map[string]interface{}) { - var args map[string]interface{} - +func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro bool) map[string]interface{} { blockdevArgs := map[string]interface{}{ "driver": "raw", "read-only": ro, @@ -785,9 +783,8 @@ func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro boo } blockdevArgs["node-name"] = blockdevID - args = blockdevArgs - return args, blockdevArgs + return blockdevArgs } // ExecuteBlockdevAdd sends a blockdev-add to the QEMU instance. device is the @@ -795,7 +792,7 @@ func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro boo // used to name the device. As this identifier will be passed directly to QMP, // it must obey QMP's naming rules, e,g., it must start with a letter. func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID, aio string, ro bool) error { - args, _ := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) + args := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) return q.executeCommand(ctx, "blockdev-add", args, nil) } @@ -808,28 +805,28 @@ func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID, aio st // is enabled. noFlush denotes whether flush requests for the device are // ignored. func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevID, aio string, direct, noFlush, ro bool) error { - args, blockdevArgs := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) + blockdevArgs := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, "no-flush": noFlush, } - return q.executeCommand(ctx, "blockdev-add", args, nil) + return q.executeCommand(ctx, "blockdev-add", blockdevArgs, nil) } // ExecuteBlockdevAddWithDriverCache has three one parameter driver // than ExecuteBlockdevAddWithCache. // Parameter driver can set the driver of block device. func (q *QMP) ExecuteBlockdevAddWithDriverCache(ctx context.Context, driver, device, blockdevID, aio string, direct, noFlush, ro bool) error { - args, blockdevArgs := q.blockdevAddBaseArgs(driver, device, blockdevID, aio, ro) + blockdevArgs := q.blockdevAddBaseArgs(driver, device, blockdevID, aio, ro) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, "no-flush": noFlush, } - return q.executeCommand(ctx, "blockdev-add", args, nil) + return q.executeCommand(ctx, "blockdev-add", blockdevArgs, nil) } // ExecuteDeviceAdd adds the guest portion of a device to a QEMU instance From c1e3b8f40f494c2b7e617a85450793207cb45aa3 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Tue, 2 Aug 2022 23:51:34 -0700 Subject: [PATCH 7/7] govmm: Refactor qmp functions for adding block device Instead of passing a bunch of arguments to qmp functions for adding block devices, use govmm BlockDevice structure to reduce these. Signed-off-by: Archana Shinde --- src/runtime/pkg/govmm/qemu/qmp.go | 22 +++++++++++----------- src/runtime/pkg/govmm/qemu/qmp_test.go | 18 ++++++++++++++---- src/runtime/virtcontainers/qemu.go | 14 ++++++++++---- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/runtime/pkg/govmm/qemu/qmp.go b/src/runtime/pkg/govmm/qemu/qmp.go index 480d38c7fa..24c92b9b20 100644 --- a/src/runtime/pkg/govmm/qemu/qmp.go +++ b/src/runtime/pkg/govmm/qemu/qmp.go @@ -771,18 +771,18 @@ func (q *QMP) ExecuteQuit(ctx context.Context) error { return q.executeCommand(ctx, "quit", nil, nil) } -func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro bool) map[string]interface{} { +func (q *QMP) blockdevAddBaseArgs(driver string, blockDevice *BlockDevice) map[string]interface{} { blockdevArgs := map[string]interface{}{ "driver": "raw", - "read-only": ro, + "read-only": blockDevice.ReadOnly, "file": map[string]interface{}{ "driver": driver, - "filename": device, - "aio": aio, + "filename": blockDevice.File, + "aio": string(blockDevice.AIO), }, } - blockdevArgs["node-name"] = blockdevID + blockdevArgs["node-name"] = blockDevice.ID return blockdevArgs } @@ -791,8 +791,8 @@ func (q *QMP) blockdevAddBaseArgs(driver, device, blockdevID, aio string, ro boo // path of the device to add, e.g., /dev/rdb0, and blockdevID is an identifier // used to name the device. As this identifier will be passed directly to QMP, // it must obey QMP's naming rules, e,g., it must start with a letter. -func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID, aio string, ro bool) error { - args := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) +func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, blockDevice *BlockDevice) error { + args := q.blockdevAddBaseArgs("host_device", blockDevice) return q.executeCommand(ctx, "blockdev-add", args, nil) } @@ -804,8 +804,8 @@ func (q *QMP) ExecuteBlockdevAdd(ctx context.Context, device, blockdevID, aio st // direct denotes whether use of O_DIRECT (bypass the host page cache) // is enabled. noFlush denotes whether flush requests for the device are // ignored. -func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevID, aio string, direct, noFlush, ro bool) error { - blockdevArgs := q.blockdevAddBaseArgs("host_device", device, blockdevID, aio, ro) +func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, blockDevice *BlockDevice, direct, noFlush bool) error { + blockdevArgs := q.blockdevAddBaseArgs("host_device", blockDevice) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, @@ -818,8 +818,8 @@ func (q *QMP) ExecuteBlockdevAddWithCache(ctx context.Context, device, blockdevI // ExecuteBlockdevAddWithDriverCache has three one parameter driver // than ExecuteBlockdevAddWithCache. // Parameter driver can set the driver of block device. -func (q *QMP) ExecuteBlockdevAddWithDriverCache(ctx context.Context, driver, device, blockdevID, aio string, direct, noFlush, ro bool) error { - blockdevArgs := q.blockdevAddBaseArgs(driver, device, blockdevID, aio, ro) +func (q *QMP) ExecuteBlockdevAddWithDriverCache(ctx context.Context, driver string, blockDevice *BlockDevice, direct, noFlush bool) error { + blockdevArgs := q.blockdevAddBaseArgs(driver, blockDevice) blockdevArgs["cache"] = map[string]interface{}{ "direct": direct, diff --git a/src/runtime/pkg/govmm/qemu/qmp_test.go b/src/runtime/pkg/govmm/qemu/qmp_test.go index 23114a0d72..3f82fa54f9 100644 --- a/src/runtime/pkg/govmm/qemu/qmp_test.go +++ b/src/runtime/pkg/govmm/qemu/qmp_test.go @@ -400,8 +400,13 @@ func TestQMPBlockdevAdd(t *testing.T) { cfg := QMPConfig{Logger: qmpTestLogger{}} q := startQMPLoop(buf, cfg, connectedCh, disconnectedCh) q.version = checkVersion(t, connectedCh) - err := q.ExecuteBlockdevAdd(context.Background(), "/dev/rbd0", - fmt.Sprintf("drive_%s", volumeUUID), false) + dev := BlockDevice{ + ID: fmt.Sprintf("drive_%s", volumeUUID), + File: "/dev/rbd0", + ReadOnly: false, + AIO: Native, + } + err := q.ExecuteBlockdevAdd(context.Background(), &dev) if err != nil { t.Fatalf("Unexpected error %v", err) } @@ -424,8 +429,13 @@ func TestQMPBlockdevAddWithCache(t *testing.T) { cfg := QMPConfig{Logger: qmpTestLogger{}} q := startQMPLoop(buf, cfg, connectedCh, disconnectedCh) q.version = checkVersion(t, connectedCh) - err := q.ExecuteBlockdevAddWithCache(context.Background(), "/dev/rbd0", - fmt.Sprintf("drive_%s", volumeUUID), true, true, false) + dev := BlockDevice{ + ID: fmt.Sprintf("drive_%s", volumeUUID), + File: "/dev/rbd0", + ReadOnly: false, + AIO: Native, + } + err := q.ExecuteBlockdevAddWithCache(context.Background(), &dev, true, true) if err != nil { t.Fatalf("Unexpected error %v", err) } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 7584b4728a..6fddd985f6 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -1292,13 +1292,19 @@ func (q *qemu) hotplugAddBlockDevice(ctx context.Context, drive *config.BlockDri return nil } - aio := q.config.BlockDeviceAIO + qblkDevice := govmmQemu.BlockDevice{ + ID: drive.ID, + File: drive.File, + ReadOnly: drive.ReadOnly, + AIO: govmmQemu.BlockDeviceAIO(q.config.BlockDeviceAIO), + } + if drive.Swap { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithDriverCache(q.qmpMonitorCh.ctx, "file", drive.File, drive.ID, aio, false, false, false) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithDriverCache(q.qmpMonitorCh.ctx, "file", &qblkDevice, false, false) } else if q.config.BlockDeviceCacheSet { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithCache(q.qmpMonitorCh.ctx, drive.File, drive.ID, aio, q.config.BlockDeviceCacheDirect, q.config.BlockDeviceCacheNoflush, drive.ReadOnly) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAddWithCache(q.qmpMonitorCh.ctx, &qblkDevice, q.config.BlockDeviceCacheDirect, q.config.BlockDeviceCacheNoflush) } else { - err = q.qmpMonitorCh.qmp.ExecuteBlockdevAdd(q.qmpMonitorCh.ctx, drive.File, drive.ID, aio, drive.ReadOnly) + err = q.qmpMonitorCh.qmp.ExecuteBlockdevAdd(q.qmpMonitorCh.ctx, &qblkDevice) } if err != nil { return err