[pve-devel] [PATCH-SERIES qemu-server 0/3] image convert: fix handling qcow2 on snapshot-as-volume-chain dir target

[pve-devel] [PATCH-SERIES qemu-server 0/3] image convert: fix handling qcow2 on snapshot-as-volume-chain dir target

[Fiona Ebner]

As reported by Friedrich, having qcow2 on a snapshot-as-volume-chain
directory storage be the target would fail with:
> Block format 'qcow2' does not support the option 'zeroinit:driver'

First patch is to work around this by just not specifying zeroinit
which leads to a performance penalty.

Following patches would fix the issue by actually using the zeroinit
driver, and we meet and old friend again with 2/3 :P (patch was
proposed once before already).

Fiona Ebner (3):
  image convert: avoid combining target image options and zeroinit
    filter
  image convert: re-use generate_drive_blockdev()
  image convert: make using zeroinit with target-image-opts work

 src/PVE/QemuServer/QemuImage.pm        | 77 ++++++++++++++++++++++----
 src/test/run_qemu_img_convert_tests.pl | 47 +++++++++++++---
 2 files changed, 107 insertions(+), 17 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/3] image convert: avoid combining target image options and zeroinit filter

[Fiona Ebner]

Would fail with an error
> Block format 'qcow2' does not support the option 'zeroinit:driver'
for a qcow2 target on a directory storage with
'snapshot-as-volume-chain'.

Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/QemuImage.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index bbfb4fd6..8fe75e92 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -109,11 +109,12 @@ sub convert {
         push @$cmd, '-f', $src_format;
     }
 
+    my $dst_uses_target_image_opts = $dst_is_iscsi || $dst_needs_discard_no_unref;
+    push @$cmd, '--target-image-opts' if $dst_uses_target_image_opts;
+
     if ($dst_is_iscsi) {
-        push @$cmd, '--target-image-opts';
         $dst_path = convert_iscsi_path($dst_path);
     } elsif ($dst_needs_discard_no_unref) {
-        push @$cmd, '--target-image-opts';
         $dst_path = qcow2_target_image_opts($dst_path, 'discard-no-unref=true');
     } else {
         push @$cmd, '-O', $dst_format;
@@ -121,7 +122,7 @@ sub convert {
 
     push @$cmd, $src_path;
 
-    if (!$dst_is_iscsi && $opts->{'is-zero-initialized'}) {
+    if (!$dst_uses_target_image_opts && $opts->{'is-zero-initialized'}) {
         push @$cmd, "zeroinit:$dst_path";
     } else {
         push @$cmd, $dst_path;
-- 
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 2/3] image convert: re-use generate_drive_blockdev()

[Fiona Ebner]

This avoids having the handling for 'discard-no-unref' in two places.

In the tests, rename the relevant target images with a '-target'
suffix to test for them in the mocked volume_snapshot_info() helper.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes since last iteration:
* rebase + make tidy

 src/PVE/QemuServer/QemuImage.pm        | 65 +++++++++++++++++++++++---
 src/test/run_qemu_img_convert_tests.pl | 24 +++++++---
 2 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index 8fe75e92..f2cadd69 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -5,11 +5,13 @@ use warnings;
 
 use Fcntl qw(S_ISBLK);
 use File::stat;
+use JSON;
 
 use PVE::Format qw(render_bytes);
 use PVE::Storage;
 use PVE::Tools;
 
+use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::Drive qw(checked_volume_format);
 use PVE::QemuServer::Helpers;
 
@@ -31,16 +33,62 @@ sub convert_iscsi_path {
 }
 
 my sub qcow2_target_image_opts {
-    my ($path, @qcow2_opts) = @_;
+    my ($storecfg, $drive, @qcow2_opts) = @_;
 
-    # FIXME this duplicates logic from qemu_blockdev_options
-    my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
+    # There is no machine version, the qemu-img binary version is what's important.
+    my $version = PVE::QemuServer::Helpers::kvm_user_version();
 
-    my $driver = S_ISBLK($st->mode) ? 'host_device' : 'file';
+    my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
+        $storecfg,
+        $drive,
+        $version,
+        { 'no-throttle' => 1 },
+    );
 
-    my $qcow2_opts_str = ',' . join(',', @qcow2_opts);
+    my $opts = [];
+    my $opt_prefix = '';
+    my $next_child = $blockdev;
+    while ($next_child) {
+        my $current = $next_child;
+        $next_child = delete($current->{file});
 
-    return "driver=qcow2$qcow2_opts_str,file.driver=$driver,file.filename=$path";
+        # TODO should cache settings be configured here (via appropriate drive configuration) rather
+        # than via dedicated qemu-img options?
+        delete($current->{cache});
+        # TODO e.g. can't use aio 'native' without cache.direct, just use QEMU default like for
+        # other targets for now
+        delete($current->{aio});
+
+        # no need for node names
+        delete($current->{'node-name'});
+
+        # it's the write target, while the flag should be 'false' anyways, remove to be sure
+        delete($current->{'read-only'});
+
+        # TODO should those be set (via appropriate drive configuration)?
+        delete($current->{'detect-zeroes'});
+        delete($current->{'discard'});
+
+        for my $key (sort keys $current->%*) {
+            my $value;
+            if (ref($current->{$key})) {
+                if ($current->{$key} eq JSON::false) {
+                    $value = 'false';
+                } elsif ($current->{$key} eq JSON::true) {
+                    $value = 'true';
+                } else {
+                    die "target image options: unhandled structured key: $key\n";
+                }
+            } else {
+                $value = $current->{$key};
+            }
+            push $opts->@*, "$opt_prefix$key=$value";
+        }
+
+        $opt_prefix .= 'file.';
+    }
+
+    return join(',', $opts->@*);
 }
 
 # The possible options are:
@@ -115,7 +163,10 @@ sub convert {
     if ($dst_is_iscsi) {
         $dst_path = convert_iscsi_path($dst_path);
     } elsif ($dst_needs_discard_no_unref) {
-        $dst_path = qcow2_target_image_opts($dst_path, 'discard-no-unref=true');
+        # don't use any other drive options, those are intended for use with a running VM and just
+        # use scsi0 as a dummy interface+index for now
+        my $dst_drive = { file => $dst_volid, interface => 'scsi', index => 0 };
+        $dst_path = qcow2_target_image_opts($storecfg, $dst_drive, 'discard-no-unref=true');
     } else {
         push @$cmd, '-O', $dst_format;
     }
diff --git a/src/test/run_qemu_img_convert_tests.pl b/src/test/run_qemu_img_convert_tests.pl
index 3c8f09f0..393fc4a8 100755
--- a/src/test/run_qemu_img_convert_tests.pl
+++ b/src/test/run_qemu_img_convert_tests.pl
@@ -532,7 +532,7 @@ my $tests = [
         name => "qcow2_external_snapshot_target",
         parameters => [
             "local:$vmid/vm-$vmid-disk-0.raw",
-            "localsnapext:$vmid/vm-$vmid-disk-0.qcow2",
+            "localsnapext:$vmid/vm-$vmid-disk-target.qcow2",
             1024 * 10,
         ],
         expected => [
@@ -544,14 +544,15 @@ my $tests = [
             "raw",
             "--target-image-opts",
             "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
-            "driver=qcow2,discard-no-unref=true,file.driver=file,"
-                . "file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-0.qcow2",
+            "discard-no-unref=true,driver=qcow2,file.driver=file"
+                . ",file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-target.qcow2",
         ],
     },
     {
         name => "lvmqcow2_external_snapshot_target",
         parameters => [
-            "local:$vmid/vm-$vmid-disk-0.raw", "lvm-store:vm-$vmid-disk-0.qcow2", 1024 * 10,
+            "local:$vmid/vm-$vmid-disk-0.raw", "lvm-store:vm-$vmid-disk-target.qcow2",
+            1024 * 10,
         ],
         expected => [
             "/usr/bin/qemu-img",
@@ -562,8 +563,8 @@ my $tests = [
             "raw",
             "--target-image-opts",
             "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
-            "driver=qcow2,discard-no-unref=true,file.driver=host_device,"
-                . "file.filename=/dev/pve/vm-$vmid-disk-0.qcow2",
+            "discard-no-unref=true,driver=qcow2,file.driver=host_device"
+                . ",file.filename=/dev/pve/vm-$vmid-disk-target.qcow2",
         ],
     },
 ];
@@ -588,6 +589,17 @@ $storage_module->mock(
     activate_volumes => sub {
         return 1;
     },
+    volume_snapshot_info => sub {
+        my ($cfg, $volid) = @_;
+        if (
+            $volid eq "lvm-store:vm-$vmid-disk-target.qcow2"
+            || $volid eq "localsnapext:$vmid/vm-$vmid-disk-target.qcow2"
+        ) {
+            # target volumes don't have snapshots
+            return {};
+        }
+        die "mocked volume_snapshot_info called with unexpected volid $volid\n";
+    },
 );
 
 my $lio_module = Test::MockModule->new("PVE::Storage::LunCmd::LIO");
-- 
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 3/3] image convert: make using zeroinit with target-image-opts work

[Fiona Ebner]

Also add a test to witness this combination.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/QemuImage.pm        | 17 +++++++++++------
 src/test/run_qemu_img_convert_tests.pl | 21 +++++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index f2cadd69..71be3abb 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -33,16 +33,16 @@ sub convert_iscsi_path {
 }
 
 my sub qcow2_target_image_opts {
-    my ($storecfg, $drive, @qcow2_opts) = @_;
+    my ($storecfg, $drive, $qcow2_opts, $zeroinit) = @_;
 
     # There is no machine version, the qemu-img binary version is what's important.
     my $version = PVE::QemuServer::Helpers::kvm_user_version();
 
+    my $blockdev_opts = { 'no-throttle' => 1 };
+    $blockdev_opts->{'zero-initialized'} = 1 if $zeroinit;
+
     my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
-        $storecfg,
-        $drive,
-        $version,
-        { 'no-throttle' => 1 },
+        $storecfg, $drive, $version, $blockdev_opts,
     );
 
     my $opts = [];
@@ -166,7 +166,12 @@ sub convert {
         # don't use any other drive options, those are intended for use with a running VM and just
         # use scsi0 as a dummy interface+index for now
         my $dst_drive = { file => $dst_volid, interface => 'scsi', index => 0 };
-        $dst_path = qcow2_target_image_opts($storecfg, $dst_drive, 'discard-no-unref=true');
+        $dst_path = qcow2_target_image_opts(
+            $storecfg,
+            $dst_drive,
+            ['discard-no-unref=true'],
+            $opts->{'is-zero-initialized'},
+        );
     } else {
         push @$cmd, '-O', $dst_format;
     }
diff --git a/src/test/run_qemu_img_convert_tests.pl b/src/test/run_qemu_img_convert_tests.pl
index 393fc4a8..8a0ad283 100755
--- a/src/test/run_qemu_img_convert_tests.pl
+++ b/src/test/run_qemu_img_convert_tests.pl
@@ -548,6 +548,27 @@ my $tests = [
                 . ",file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-target.qcow2",
         ],
     },
+    {
+        name => "qcow2_external_snapshot_target_zeroinit",
+        parameters => [
+            "local:$vmid/vm-$vmid-disk-0.raw",
+            "localsnapext:$vmid/vm-$vmid-disk-target.qcow2",
+            1024 * 10,
+            { 'is-zero-initialized' => 1 },
+        ],
+        expected => [
+            "/usr/bin/qemu-img",
+            "convert",
+            "-p",
+            "-n",
+            "-f",
+            "raw",
+            "--target-image-opts",
+            "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
+            "driver=zeroinit,file.discard-no-unref=true,file.driver=qcow2,file.file.driver=file"
+                . ",file.file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-target.qcow2",
+        ],
+    },
     {
         name => "lvmqcow2_external_snapshot_target",
         parameters => [
-- 
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/3] image convert: fix handling qcow2 on snapshot-as-volume-chain dir target

[Thomas Lamprecht]

On Wed, 30 Jul 2025 17:03:14 +0200, Fiona Ebner wrote:
> As reported by Friedrich, having qcow2 on a snapshot-as-volume-chain
> directory storage be the target would fail with:
> > Block format 'qcow2' does not support the option 'zeroinit:driver'
> 
> First patch is to work around this by just not specifying zeroinit
> which leads to a performance penalty.
> 
> [...]

Applied, thanks!

[1/3] image convert: avoid combining target image options and zeroinit filter
      commit: 4866264c595c119dbad4ab742cbbd3f6f67f2253
[2/3] image convert: re-use generate_drive_blockdev()
      commit: 659b0716cee24e6374b7a35c7204ddc1a3d8a7fc
[3/3] image convert: make using zeroinit with target-image-opts work
      commit: f27115cede9b16f83213dafe49a32cc8f2facc2c


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