[pve-devel] [PATCH-SERIES qemu-server 0/2] fix scsi-{block, generic} with blockdev regressions

[pve-devel] [PATCH-SERIES qemu-server 0/2] fix scsi-{block, generic} with blockdev regressions

[Fiona Ebner]

Fiona Ebner (2):
  fix #6680: avoid setting nonexistent 'write-cache' option for
    scsi-{block,generic}
  drive device: scsi: avoid setting nonexistent 'device_id' for
    scsi-{block,generic}

 src/PVE/QemuServer.pm | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 1/2] fix #6680: avoid setting nonexistent 'write-cache' option for scsi-{block, generic}

[Fiona Ebner]

Only scsi-cd and scsi-hd have a 'write-cache' option, scsi-block and
scsi-generic do not.

Setting the 'cache' setting on such a drive in the VM configuration is
still valid and should not be prohibited, because it affects the
blockdev settings.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d40b14de..99d4da4f 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1203,6 +1203,7 @@ sub print_drivedevice_full {
     my $maxdev = 0;
 
     my $machine_version = extract_version($machine_type, kvm_user_version());
+    my $has_write_cache = 1; # whether the device has a 'write-cache' option
 
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
     if ($drive->{interface} eq 'virtio') {
@@ -1244,7 +1245,8 @@ sub print_drivedevice_full {
         }
         $device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
 
-        # only scsi-hd and scsi-cd support passing vendor and product information
+        # only scsi-hd and scsi-cd support passing vendor and product information and have a
+        # 'write-cache' option
         if ($device_type eq 'hd' || $device_type eq 'cd') {
             if (my $vendor = $drive->{vendor}) {
                 $device .= ",vendor=$vendor";
@@ -1252,6 +1254,10 @@ sub print_drivedevice_full {
             if (my $product = $drive->{product}) {
                 $device .= ",product=$product";
             }
+
+            $has_write_cache = 1;
+        } else {
+            $has_write_cache = 0;
         }
 
     } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
@@ -1316,7 +1322,7 @@ sub print_drivedevice_full {
     }
 
     if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
-        if (!drive_is_cdrom($drive)) {
+        if (!drive_is_cdrom($drive) && $has_write_cache) {
             my $write_cache = 'on';
             if (my $cache = $drive->{cache}) {
                 $write_cache = 'off' if $cache eq 'writethrough' || $cache eq 'directsync';
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 1/2] fix #6680: avoid setting nonexistent 'write-cache' option for scsi-{block, generic}

[Fabian Grünbichler]

On August 13, 2025 10:58 am, Fiona Ebner wrote:
> Only scsi-cd and scsi-hd have a 'write-cache' option, scsi-block and
> scsi-generic do not.
> 
> Setting the 'cache' setting on such a drive in the VM configuration is
> still valid and should not be prohibited, because it affects the
> blockdev settings.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index d40b14de..99d4da4f 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1203,6 +1203,7 @@ sub print_drivedevice_full {
>      my $maxdev = 0;
>  
>      my $machine_version = extract_version($machine_type, kvm_user_version());
> +    my $has_write_cache = 1; # whether the device has a 'write-cache' option
>  
>      my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
>      if ($drive->{interface} eq 'virtio') {
> @@ -1244,7 +1245,8 @@ sub print_drivedevice_full {
>          }
>          $device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
>  
> -        # only scsi-hd and scsi-cd support passing vendor and product information
> +        # only scsi-hd and scsi-cd support passing vendor and product information and have a
> +        # 'write-cache' option
>          if ($device_type eq 'hd' || $device_type eq 'cd') {
>              if (my $vendor = $drive->{vendor}) {
>                  $device .= ",vendor=$vendor";
> @@ -1252,6 +1254,10 @@ sub print_drivedevice_full {
>              if (my $product = $drive->{product}) {
>                  $device .= ",product=$product";
>              }
> +
> +            $has_write_cache = 1;

nit: this already is the default initial value above, and we don't loop
over devices here? so we could drop this and the comment above

> +        } else {

and instead add a comment here that -block and -generic don't support
this?

> +            $has_write_cache = 0;
>          }
>  
>      } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
> @@ -1316,7 +1322,7 @@ sub print_drivedevice_full {
>      }
>  
>      if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
> -        if (!drive_is_cdrom($drive)) {
> +        if (!drive_is_cdrom($drive) && $has_write_cache) {
>              my $write_cache = 'on';
>              if (my $cache = $drive->{cache}) {
>                  $write_cache = 'off' if $cache eq 'writethrough' || $cache eq 'directsync';
> -- 
> 2.47.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 1/2] fix #6680: avoid setting nonexistent 'write-cache' option for scsi-{block, generic}

[Fiona Ebner]

Am 13.08.25 um 12:08 PM schrieb Fabian Grünbichler:
> On August 13, 2025 10:58 am, Fiona Ebner wrote:
>> @@ -1252,6 +1254,10 @@ sub print_drivedevice_full {
>>              if (my $product = $drive->{product}) {
>>                  $device .= ",product=$product";
>>              }
>> +
>> +            $has_write_cache = 1;
> 
> nit: this already is the default initial value above, and we don't loop
> over devices here? so we could drop this and the comment above

Yes, but since the SCSI branch does modify it in one place, I though it
is more readable and future-proof to be explicit covering both
sub-branches for the device types.

> 
>> +        } else {
> 
> and instead add a comment here that -block and -generic don't support
> this?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 2/2] drive device: scsi: avoid setting nonexistent 'device_id' for scsi-{block, generic}

[Fiona Ebner]

Fixes: 46d1225e ("drive device: fix regression with missing '/dev/disk/by-id' paths")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 99d4da4f..78cfad62 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1235,8 +1235,11 @@ sub print_drivedevice_full {
         }
         $device .= ",id=$drive_id";
 
-        # for the switch to -blockdev, the SCSI device ID needs to be explicitly specified
-        if (min_version($machine_version, 10, 0)) {
+        # For the switch to -blockdev, the SCSI device ID needs to be explicitly specified. Note
+        # that only ide-cd and ide-hd have a 'device_id' option.
+        if (
+            min_version($machine_version, 10, 0) && ($device_type eq 'cd' || $device_type eq 'hd')
+        ) {
             $device .= ",device_id=drive-${drive_id}";
         }
 
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH-SERIES qemu-server 0/2] fix scsi-{block, generic} with blockdev regressions

[Fabian Grünbichler]

On Wed, 13 Aug 2025 10:58:35 +0200, Fiona Ebner wrote:
> Fiona Ebner (2):
>   fix #6680: avoid setting nonexistent 'write-cache' option for
>     scsi-{block,generic}
>   drive device: scsi: avoid setting nonexistent 'device_id' for
>     scsi-{block,generic}
> 
> src/PVE/QemuServer.pm | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> [...]

You convinced me ;) - applied, thanks!

[1/2] fix #6680: avoid setting nonexistent 'write-cache' option for scsi-{block, generic}
      (no commit info)
[2/2] drive device: scsi: avoid setting nonexistent 'device_id' for scsi-{block, generic}
      (no commit info)

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel