Re: [PATCH] guestfs: add scsi extra storage option

[Chuck Lever <cel@kernel.org>]

On 7/10/25 1:35 PM, Luis Chamberlain wrote:
> From: Luis Chamberlain <mcgrof@gmail.com>
> 
> Add scsi device support for virtualization.
> 
> Generated-by: ChatGPT Codex
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  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 %}
>  <!-- End of virtio drives-->
>  {%- 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) -%}
> +<!-- We generated {{ num_drives }} scsi dives -->
> +{% for n in range(0,num_drives) %}
> +    <!-- This is scsi drive # {{ n + 1 }} -->
> +    <qemu:arg value='-device'/>
> +    <qemu:arg value='pcie-root-port,id=pcie-port-for-scsi-{{ n }},multifunction=on,bus=pcie.1,addr=0x{{ "%0x" | format( n | int) }},chassis=5{{ n }}'/>
> +    <qemu:arg value='-device'/>
> +    <qemu:arg value='virtio-scsi-pci,id=scsi{{ n }},bus=pcie-port-for-scsi-{{ n }},addr=0x0'/>
> +    <qemu:arg value='-drive'/>
> +    <qemu:arg value='file={{ kdevops_storage_pool_path }}/guestfs/{{ hostname }}/extra{{ n }}.{{ libvirt_extra_drive_format }},format={{ libvirt_extra_drive_format }},if=none,aio={{ libvirt_extra_storage_aio_mode }},cache={{ libvirt_extra_storage_aio_cache_mode }},id=drv{{ n }}'/>
> +    <qemu:arg value='-device'/>
> +    <qemu:arg value='scsi-hd,drive=drv{{ n }},bus=scsi{{ n }}.0'/>
> +{% endfor %}
> +<!-- End of scsi drives-->
> +{%- 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) -%}
> -<!-- These are virtio drives used for large IO experimentaiton, with LBS support -->
> +<!-- These are virtio drives used for large IO experimentation, with LBS support -->
>  {% 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) -%}
> -<!-- These are NVMe drives used for large IO experimentaiton, with LBS support -->
> +<!-- These are NVMe drives used for large IO experimentation, with LBS support -->
>  {% 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.

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.


-- 
Chuck Lever