From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B2A10487A5 for ; Fri, 11 Jul 2025 12:54:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752238442; cv=none; b=UuckCDzea9C6whCel6I1gFuBblcdSNCGA/CfVucjALRpNhT5hRenhEPnwT5u4Vg3YUsSOjQU/DGLKmFgtJGGdfJ1Ztj6yphw9dGmmDD5Xuapo8CXvM1sKGK2c4fTf+OwF9kPRh5Cw6omDFw52Vw1w31Zp1CnI3XCUE+ahpnBGLc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752238442; c=relaxed/simple; bh=S8Q1XOplXiXO67Pn/UKesjg+ZnqE/RDBMFvoiHO4LJ4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=p5d8LWmmlIjz7X12DQ6eyrGTZBy3AVOcWPZhmhBqgf6sIy3s2gE4FX4QuZXft6QVIZqXlvBCoZYFj0Q0FY6ezFsUAmhqYbA+oJS1+llHrUkSVSYGcRHvCSGOM3BwTF+RVRIRXc6wlX5n9hAvXOUxdS9UtcCQUuMDL3XbPheD/zI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ZOgC17KR; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ZOgC17KR" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 73D5FC4CEF0; Fri, 11 Jul 2025 12:54:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1752238442; bh=S8Q1XOplXiXO67Pn/UKesjg+ZnqE/RDBMFvoiHO4LJ4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ZOgC17KRP7lBqBNTH6q4qfVJnYZ5YDJVHflyuu6/llXoO9NgZbCSQO8RFxE+PXQ88 YeWgaL2osUIEcKpUrqKEIX7/n43hWcYDZ8FmGl9rnyT9Parf8D6wWEYtmFp3bBWi7F t5IL1hh4NQcUhemlXxPLNvpUNMZFDngURCocnCD8dQv9yCMjEX3qRol8Ej3BHcxrb/ vKa2FrCtDNDwXT6sIOWU7gpDdrmK57Wy4t0vMfXRTPVZtL3JLz+4glG+nvxA0C60Xt 2Ol/7TbnIyHFGjXKzLc81hQ9vsyc22+NRCMI69Sn2pIdW1O7ehbBetqUGb8C6PmyfY YX4RDEw4hPZeg== Message-ID: <2dece31a-6551-4f6a-8684-9d7b13bc4f12@kernel.org> Date: Fri, 11 Jul 2025 08:53:59 -0400 Precedence: bulk X-Mailing-List: kdevops@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] guestfs: add scsi extra storage option To: Luis Chamberlain Cc: Daniel Gomez , Swarna Prabhu , swarnassp6@gmail.com, kdevops@lists.linux.dev, Luis Chamberlain References: <20250710173517.3003494-1-mcgrof@kernel.org> Content-Language: en-US From: Chuck Lever Organization: kernel.org In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 7/10/25 8:45 PM, Luis Chamberlain wrote: > On Thu, Jul 10, 2025 at 06:53:37PM -0400, Chuck Lever wrote: >> On 7/10/25 1:35 PM, Luis Chamberlain wrote: >>> From: Luis Chamberlain >>> >>> Add scsi device support for virtualization. >>> >>> Generated-by: ChatGPT Codex >>> Signed-off-by: Luis Chamberlain >>> --- >>> defconfigs/blktests_scsi | 2 +- >>> kconfigs/Kconfig.libvirt | 25 ++++++++++++------- >>> playbooks/roles/gen_nodes/defaults/main.yml | 1 + >>> .../roles/gen_nodes/templates/Vagrantfile.j2 | 11 ++++++-- >>> playbooks/roles/gen_nodes/templates/drives.j2 | 24 ++++++++++++++++-- >>> .../roles/gen_nodes/templates/gen_drives.j2 | 21 ++++++++++------ >>> .../kdevops_nodes_split_start.j2.yaml | 10 ++++++++ >>> workflows/steady_state/Kconfig | 1 + >>> 8 files changed, 74 insertions(+), 21 deletions(-) >>> >>> diff --git a/defconfigs/blktests_scsi b/defconfigs/blktests_scsi >>> index d49e72893690..28462939e11e 100644 >>> --- a/defconfigs/blktests_scsi >>> +++ b/defconfigs/blktests_scsi >>> @@ -9,7 +9,7 @@ CONFIG_GUESTFS_BRINGUP_DEBUG_1=y >>> CONFIG_BOOTLINUX=y >>> CONFIG_BOOTLINUX_9P=y >>> >>> -CONFIG_LIBVIRT_EXTRA_STORAGE_DRIVE_NVME=y >>> +CONFIG_LIBVIRT_EXTRA_STORAGE_DRIVE_SCSI=y >>> >>> CONFIG_WORKFLOWS_TESTS=y >>> CONFIG_WORKFLOWS_LINUX_TESTS=y >>> diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt >>> index 8654eb9b1a06..f737b79faea4 100644 >>> --- a/kconfigs/Kconfig.libvirt >>> +++ b/kconfigs/Kconfig.libvirt >>> @@ -487,7 +487,7 @@ config LIBVIRT_MACHINE_TYPE_DEFAULT >>> is the old i440x, so you will not get PCIe support. >>> >>> We only want to support PCI-E capable guests with libguest so the >>> - deafult is not allowed on libguest. >>> + default is not allowed on libguest. >>> >>> config LIBVIRT_MACHINE_TYPE_Q35 >>> bool "q35" >>> @@ -548,11 +548,18 @@ config LIBVIRT_EXTRA_STORAGE_DRIVE_VIRTIO >>> you won't be able to test ZNS. >>> >>> config LIBVIRT_EXTRA_STORAGE_DRIVE_IDE >>> - bool "ide" >>> - output yaml >>> - help >>> - Use the QEMU ide driver for extra storage drives. This is useful for >>> - really old Linux distributions that lack the virtio backend driver. >>> + bool "ide" >>> + output yaml >>> + help >>> + Use the QEMU ide driver for extra storage drives. This is useful for >>> + really old Linux distributions that lack the virtio backend driver. >>> + >>> +config LIBVIRT_EXTRA_STORAGE_DRIVE_SCSI >>> + bool "scsi" >>> + output yaml >>> + help >>> + Use the QEMU SCSI driver for extra storage drives. This relies on a >>> + virtio-scsi controller with scsi-hd devices attached. >>> >>> endchoice >>> >>> @@ -817,7 +824,7 @@ config LIBVIRT_AIO_MODE_NATIVE >>> help >>> Use the aio=native mode. For some older kernels it is known that >>> native will cause corruption if used on ext4 or xfs filesystem if >>> - you also use cace=none. This corruption is documented for RHEL: >>> + you also use cache=none. This corruption is documented for RHEL: >>> >>> https://access.redhat.com/articles/41313 >>> https://bugzilla.redhat.com/show_bug.cgi?id=615309 >>> @@ -1104,7 +1111,7 @@ config LIBVIRT_ENABLE_GDB >>> output yaml >>> help >>> Select this option if you want to enable debugging support for GDB. >>> - By default , it is assumed that gdb is disabled since we dont want >>> + By default , it is assumed that gdb is disabled since we don't want >>> to complicate this for the CI runs. If enabled then libvirt guest >>> xml for each guest will be configured to use gdb on a specific >>> tcp port. >>> @@ -1116,7 +1123,7 @@ config LIBVIRT_GDB_BASEPORT >>> depends on LIBVIRT_ENABLE_GDB >>> help >>> This option defines the base port to be used for the GDB. >>> - Esentially we need to make QEMU listen for an incoming connection from >>> + Essentially we need to make QEMU listen for an incoming connection from >>> gdb on a TCP port. The default port is chosen to be 1234. However we >>> introduce variability for assigning the port to each guest by defining >>> a base port and adding an index to it based on the number of libvrt guest >>> diff --git a/playbooks/roles/gen_nodes/defaults/main.yml b/playbooks/roles/gen_nodes/defaults/main.yml >>> index 6a899be45128..2cf450b84281 100644 >>> --- a/playbooks/roles/gen_nodes/defaults/main.yml >>> +++ b/playbooks/roles/gen_nodes/defaults/main.yml >>> @@ -70,6 +70,7 @@ libvirt_extra_drive_id_prefix: 'drv' >>> libvirt_extra_storage_drive_nvme: False >>> libvirt_extra_storage_drive_virtio: False >>> libvirt_extra_storage_drive_ide: False >>> +libvirt_extra_storage_drive_scsi: False >>> libvirt_extra_storage_aio_mode: "native" >>> libvirt_extra_storage_aio_cache_mode: "none" >>> # Note that NVMe on qemu does not allow the physical block size >>> diff --git a/playbooks/roles/gen_nodes/templates/Vagrantfile.j2 b/playbooks/roles/gen_nodes/templates/Vagrantfile.j2 >>> index 2298c3455251..a52f5566280e 100644 >>> --- a/playbooks/roles/gen_nodes/templates/Vagrantfile.j2 >>> +++ b/playbooks/roles/gen_nodes/templates/Vagrantfile.j2 >>> @@ -244,7 +244,7 @@ Vagrant.configure("2") do |config| >>> # >>> # Just create a PCI or PCI-E root bus dedicated for extra drives. We >>> # use 0x08 to place this PCI / PCI-E root bus as we know this is >>> - # avilable on modern x86-64 systems. Eventually we may need to bump >>> + # available on modern x86-64 systems. Eventually we may need to bump >>> # this to 0x9, but it would be better instead to have vagant-libvirt >>> # speak "add a new PCI or PCI-E root bus" and "add extra drives" whether >>> # that is nvme or virtio. >>> @@ -252,7 +252,7 @@ Vagrant.configure("2") do |config| >>> {% if not libvirt_override_machine_type %} >>> # For i440x on x86_64 (default on libvirt as of today) we use PCI, not >>> # PCI-E. Below assumes i440x. i440x cannot support CXL as it does not >>> - # suport PCI-E. >>> + # support PCI-E. >>> libvirt.qemuargs :value => "-device" >>> libvirt.qemuargs :value => "pci-bridge,id=custom-pci-for-{{ extra_disk_driver }},chassis_nr=1,bus=pci.0,addr=0x8" >>> {% else %} >>> @@ -417,6 +417,13 @@ Vagrant.configure("2") do |config| >>> libvirt.qemuargs :value => "file=#{extra_disk},format={{ libvirt_extra_drive_format }},if=none,aio={{ libvirt_extra_storage_aio_mode }},cache={{ libvirt_extra_storage_aio_cache_mode }},id=#{disk_id}" >>> libvirt.qemuargs :value => "-device" >>> libvirt.qemuargs :value => "virtio-blk-pci,drive=#{disk_id},id=virtio-#{disk_id},serial=#{serial_id},bus=#{bus_for_extra_drives},addr=#{pci_function},iothread=kdevops-virtio-iothread-#{port}#{extra_drive_largio_args},logical_block_size=#{virtio_lbs},physical_block_size=#{virtio_pbs}" >>> +{% elif libvirt_extra_storage_drive_scsi %} >>> + libvirt.qemuargs :value => "-device" >>> + libvirt.qemuargs :value => "virtio-scsi-pci,id=scsi#{port},bus=#{bus_for_extra_drives},addr=#{pci_function}" >>> + libvirt.qemuargs :value => "-drive" >>> + libvirt.qemuargs :value => "file=#{extra_disk},format={{ libvirt_extra_drive_format }},if=none,aio={{ libvirt_extra_storage_aio_mode }},cache={{ libvirt_extra_storage_aio_cache_mode }},id=#{disk_id}" >>> + libvirt.qemuargs :value => "-device" >>> + libvirt.qemuargs :value => "scsi-hd,drive=#{disk_id},bus=scsi#{port}.0" >>> {% elif libvirt_extra_storage_drive_nvme %} >>> if ! largio >>> nvme_lbs = "{{ libvirt_extra_storage_nvme_logical_block_size }}" >>> diff --git a/playbooks/roles/gen_nodes/templates/drives.j2 b/playbooks/roles/gen_nodes/templates/drives.j2 >>> index 8327cd430981..1241b88a9379 100644 >>> --- a/playbooks/roles/gen_nodes/templates/drives.j2 >>> +++ b/playbooks/roles/gen_nodes/templates/drives.j2 >>> @@ -40,6 +40,26 @@ the drives can vary by type, so we have one macro by type of drive. >>> {% endfor %} >>> >>> {%- endmacro -%} >>> +{%- macro gen_drive_scsi(num_drives, >>> + kdevops_storage_pool_path, >>> + hostname, >>> + libvirt_extra_drive_format, >>> + libvirt_extra_storage_aio_mode, >>> + libvirt_extra_storage_aio_cache_mode) -%} >>> + >>> +{% for n in range(0,num_drives) %} >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> +{% endfor %} >>> + >>> +{%- endmacro -%} >>> {%- macro gen_drive_large_io_virtio(libvirt_largeio_logical_compat, >>> libvirt_largeio_logical_compat_size, >>> libvirt_largeio_pow_limit, >>> @@ -49,7 +69,7 @@ the drives can vary by type, so we have one macro by type of drive. >>> libvirt_extra_storage_aio_mode, >>> libvirt_extra_storage_aio_cache_mode, >>> kdevops_storage_pool_path) -%} >>> - >>> + >>> {% set ns = namespace(lbs_idx=0) %} >>> {% set max_pbs = libvirt_largeio_logical_compat_size * (2 ** libvirt_largeio_pow_limit) %} >>> {% for n in range(0,libvirt_largeio_pow_limit+1) %} >>> @@ -106,7 +126,7 @@ the drives can vary by type, so we have one macro by type of drive. >>> libvirt_extra_storage_aio_mode, >>> libvirt_extra_storage_aio_cache_mode, >>> kdevops_storage_pool_path) -%} >>> - >>> + >>> {% set ns = namespace(lbs_idx=0) %} >>> {% set max_pbs = libvirt_largeio_logical_compat_size * (2 ** libvirt_largeio_pow_limit) %} >>> {% for n in range(0,libvirt_largeio_pow_limit+1) %} >>> diff --git a/playbooks/roles/gen_nodes/templates/gen_drives.j2 b/playbooks/roles/gen_nodes/templates/gen_drives.j2 >>> index 874e5b0623b9..2de13da4ab8e 100644 >>> --- a/playbooks/roles/gen_nodes/templates/gen_drives.j2 >>> +++ b/playbooks/roles/gen_nodes/templates/gen_drives.j2 >>> @@ -19,14 +19,21 @@ >>> kdevops_storage_pool_path) }} >>> {% else %} >>> {{ drives.gen_drive_virtio(4, >>> - kdevops_storage_pool_path, >>> - hostname, >>> - libvirt_extra_drive_format, >>> - libvirt_extra_storage_aio_mode, >>> - libvirt_extra_storage_aio_cache_mode, >>> - libvirt_extra_storage_virtio_logical_block_size, >>> - libvirt_extra_storage_virtio_physical_block_size) }} >>> + kdevops_storage_pool_path, >>> + hostname, >>> + libvirt_extra_drive_format, >>> + libvirt_extra_storage_aio_mode, >>> + libvirt_extra_storage_aio_cache_mode, >>> + libvirt_extra_storage_virtio_logical_block_size, >>> + libvirt_extra_storage_virtio_physical_block_size) }} >>> {% endif %} >>> +{% elif libvirt_extra_storage_drive_scsi %} >>> +{{ drives.gen_drive_scsi(4, >>> + kdevops_storage_pool_path, >>> + hostname, >>> + libvirt_extra_drive_format, >>> + libvirt_extra_storage_aio_mode, >>> + libvirt_extra_storage_aio_cache_mode) }} >>> {% elif libvirt_extra_storage_drive_nvme %} >>> {% if libvirt_largeio_enable %} >>> {{ drives.gen_drive_large_io_nvme(libvirt_largeio_logical_compat, >>> diff --git a/playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml b/playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml >>> index e5b88efefc63..91e105cd3bff 100644 >>> --- a/playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml >>> +++ b/playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml >>> @@ -76,6 +76,16 @@ vagrant_global: >>> physical_block_size: {{ libvirt_extra_storage_virtio_physical_block_size }} >>> logical_block_size: {{ libvirt_extra_storage_virtio_logical_block_size }} >>> {% endif %} >>> +{% elif libvirt_extra_storage_drive_scsi %} >>> + extra_disks: >>> + data: >>> + size: 102400 >>> + scratch: >>> + size: 102400 >>> + extra1: >>> + size: 102400 >>> + extra2: >>> + size: 102400 >>> {% elif libvirt_extra_storage_drive_nvme %} >>> extra_disks: >>> data: >>> diff --git a/workflows/steady_state/Kconfig b/workflows/steady_state/Kconfig >>> index 335b833cfbcc..f7064d078c0a 100644 >>> --- a/workflows/steady_state/Kconfig >>> +++ b/workflows/steady_state/Kconfig >>> @@ -6,6 +6,7 @@ config SSD_STEADY_STATE_DEVICE >>> default "/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_kdevops1" if LIBVIRT && LIBVIRT_EXTRA_STORAGE_DRIVE_NVME >>> default "/dev/disk/by-id/virtio-kdevops1" if LIBVIRT && LIBVIRT_EXTRA_STORAGE_DRIVE_VIRTIO >>> default "/dev/disk/by-id/ata-QEMU_HARDDISK_kdevops1" if LIBVIRT && LIBVIRT_EXTRA_STORAGE_DRIVE_IDE >>> + default "/dev/sdc" if LIBVIRT && LIBVIRT_EXTRA_STORAGE_DRIVE_SCSI >>> default "/dev/nvme2n1" if TERRAFORM_AWS_INSTANCE_M5AD_4XLARGE >>> default "/dev/nvme1n1" if TERRAFORM_GCE >>> default "/dev/sdd" if TERRAFORM_AZURE >> >> I would split the spelling corrections into a separate patch. That might >> even be helpful for LLM training. > > Sure I can do that. > >> This patch looks like it is missing a change to >> >> playbooks/roles/volume_group/tasks/guestfs.yml >> >> Add a task specific to guestfs SCSI drives after the IDE task. You can >> test by running the pynfs workflow, which will try to set up a single >> guest with NFSD and an LVM group made from these disk devices. > > Are you OK if I add this after this patch, I'd like to test codex to see > if I can just input your requirement to do that. It used to be AI for > kdevops was gettind D grading, I am now grading it at B with Codex. I don't think there's a strong concern about bisect-ability here, so it should be OK as a separate patch. -- Chuck Lever