[pve-devel] [RFC qemu-server/storage 0/3] fix #5779: introduce guest hints to pass rxbounce flag to KRBD

[pve-devel] [RFC qemu-server/storage 0/3] fix #5779: introduce guest hints to pass rxbounce flag to KRBD

[Friedrich Weber]

When using Windows VMs with KRBD, it can be necessary to pass the rxbounce
option to `rbd map` to avoid "bad crc/signature" warnings in the journal,
retransmits and degraded performance. A first attempt to allow this [0] added
an rxbounce storage option, but Thomas pointed out that this might nudge users
towards creating two storages pointing to the same Ceph pool (one for Linux VMs
that do not need rxbounce, one for Windows VMs that need rxbounce), which we
should discourage.

The cleanest solution from the user POV would for us to pass rxbounce only when
mapping disks of VMs with Windows OS type, but this is not straightforward to
implement, as pve-storage currently does not have knowledge about the "owner"
of the volumes it manages. Fabian and Thomas suggested an approach to pass
well-defined "hints" from e.g. qemu-server to pve-storage [0], one of which
could be the guest's OS type, and based on which the RBD plugin could decide
whether to pass rxbounce or not. We'd need to take care to avoid too much
coupling between qemu-server and pve-storage, though.

Here I took a first attempt at implementing such "hints" after an off-list
discussion with Thomas. This RFC is intended as a base for further discussion
whether we want hints in general, and if yes, whether we want them in this
form. It should definitely not be merged like this and I tested it only
superficially.

For now, PVE::Storage::activate_volumes and PVE::Storage::map_volumes accept
"hints" an optional hash reference with a JSON schema (which they validate).
These hints are then passed to Plugin::{activate,map}_volume. PVE::Storage also
provides a subroutine to check whether the storage stack supports a particular
hint, callers (such as qemu-server) can use that to determine whether they can
pass a certain hint. Thomas suggested (something like) that so we don't
necessarily always have to bump qemu-server when we introduce a new hint in
pve-storage.

Obstacles I faced so far:

- The biggest obstacle is that we need to update all callers of
  activate_volumes to pass guest hints where possible. This means that optimally
  all callers should be able to generate the guest hints. Right now, there is
  only the 'guest-ostype' hint which is taken from the VM config. Currently, this
  is not always available to the caller of activate_volumes, sometimes
  some extra work/refactoring would be needed to get it (e.g. see
  PVE::QemuServer::QemuImage::convert or PVE::QemuServer::clone_disk),
  so this needs quite some code changes, which I have not done in all cases in
  this RFC.

- There are also some indirect callers of activate_volumes, e.g. via
  PVE::Storage::abs_filesystem_path or PVE::Storage::storage_migrate -- these
  would also need to be extended to accept hints (not done in this RFC)

- Initially, to avoid having to modify all (direct+indirect) callers of
  activate_volumes, I thought I could pass the hints only at the few "relevant"
  call sites (i.e., when starting a VM), but then noticed that volumes may be
  activated by an action unrelated to a VM start (e.g. a clone), then stay
  active, and not be re-activated by a VM start. So if e.g. we do not pass the
  hints on clone, the KRBD volume would be mapped without rxbounce, stay active,
  and when starting the VM, a user could run into the original problem again.
  So we can't get away with only passing hints to the few relevant call sites,
  and actually need to pass them everywhere (where possible).

- But sometimes, it might be fine to not pass hints to activate_volumes because
  we immediately deactivate, e.g. in QemuServer::restore_vma_archive, but only
  because the only current hint 'guest-ostype' is not relevant in such cases -
  might not be the case for future hints?

- 'guest-ostype' might be the only hint we need for now and can be taken from
  the guest config, but can we be sure that future hints are also going to be
  derived from the guest config? Is it OK to have generate_storage_hints only
  take the guest config as well?

- minor, but still: with this approach, rxbounce would also be passed for the
  EFI disk and TPM state of Windows VMs. This is likely not needed, but probably
  OK for a first version?

- should we already pass hints when activating volumes in pve-container too,
  even though they are empty?

- is it even OK to pass the hints only to {activate,map}_volumes? What if an
  external storage plugin needs the hint also when deactivating a volume? Should
  we already add a $hints parameter to other plugin methods as well, to avoid
  having to do that (and another APIVER bump) later?

- With hints being an argument to activate_volumes which takes a list of volumes,
  the hint applies to all volumes in the list. Is this OK API-wise? For the
  guest-ostype hint, this only makes sense if all passed volumes belong to the
  same guest and not several different ones (because they may have different OS
  types), which I think is the case for our call sites, but doesn't have to be
  the case?

- bumping the APIVER and docs are of course also needed, but left out of this
  RFC for now.

All of the above can likely be sorted out, but especially modifying all
direct+indirect call sites of {activate,map}_volumes will need some work and
care to avoid subtly breaking things. Before getting into that, I wanted to ask
for opinions about the general approach -- also because on the one hand I want
to avoid "overfitting" the guest hints to the one usecase we currently have,
but also avoid overengineering it to fit hypothetical usecases we don't even
know about yet.

What do you think?

[0] https://lore.proxmox.com/pve-devel/20241025111304.99680-2-f.weber@proxmox.com/T/
[1] https://lore.proxmox.com/pve-devel/0fdd8f93-8ef6-4c83-92ab-8e86d5386343@proxmox.com/

pve-storage:

Friedrich Weber (2):
  plugin: map/activate volume: allow callers to pass hints
  plugin: rbd: pass rxbounce when mapping Windows VM disks

 src/PVE/Storage.pm           | 34 ++++++++++++++++++++++++++++++----
 src/PVE/Storage/Plugin.pm    | 17 +++++++++++++++--
 src/PVE/Storage/RBDPlugin.pm | 13 +++++++++----
 3 files changed, 54 insertions(+), 10 deletions(-)


qemu-server:

Friedrich Weber (1):
  fix #5997: qemu: pass guest-ostype hint when activating volumes

 src/PVE/API2/Qemu.pm             | 18 ++++++++++---
 src/PVE/QemuConfig.pm            |  1 +
 src/PVE/QemuServer.pm            | 44 ++++++++++++++++++++++++--------
 src/PVE/QemuServer/Cloudinit.pm  |  3 ++-
 src/PVE/QemuServer/ImportDisk.pm |  4 ++-
 src/PVE/QemuServer/OVMF.pm       |  4 ++-
 src/PVE/QemuServer/QemuImage.pm  |  5 ++--
 src/PVE/QemuServer/RunState.pm   |  3 ++-
 src/PVE/VZDump/QemuServer.pm     |  6 +++--
 9 files changed, 66 insertions(+), 22 deletions(-)


Summary over all repositories:
  12 files changed, 120 insertions(+), 32 deletions(-)

-- 
Generated by git-murpp 0.8.1


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

[pve-devel] [PATCH pve-storage 1/2] plugin: map/activate volume: allow callers to pass hints

[Friedrich Weber]

Right now, the only supported hint is guest-ostype, via which
qemu-server provides the guest's OS type from the VM configuration.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 src/PVE/Storage.pm        | 34 ++++++++++++++++++++++++++++++----
 src/PVE/Storage/Plugin.pm | 17 +++++++++++++++--
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 1dde2b7..709d9ca 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -771,6 +771,7 @@ sub abs_filesystem_path {
 
     my $path;
     if (parse_volume_id($volid, 1)) {
+        # TODO: in order to pass $hints, abs_filesystem_path needs to accept it too
         activate_volumes($cfg, [$volid]);
         $path = PVE::Storage::path($cfg, $volid);
     } else {
@@ -905,6 +906,7 @@ my $volume_export_prepare = sub {
 
     volume_snapshot($cfg, $volid, $snapshot) if $migration_snapshot;
 
+    # TODO: in order to pass $hints, $volume_export_prepare would need to accept it too
     if (defined($snapshot)) {
         activate_volumes($cfg, [$volid], $snapshot);
     } else {
@@ -1122,7 +1124,9 @@ sub vdisk_create_base {
 }
 
 sub map_volume {
-    my ($cfg, $volid, $snapname) = @_;
+    my ($cfg, $volid, $snapname, $hints) = @_;
+
+    verify_hints($hints);
 
     my ($storeid, $volname) = parse_volume_id($volid);
 
@@ -1130,7 +1134,7 @@ sub map_volume {
 
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    return $plugin->map_volume($storeid, $scfg, $volname, $snapname);
+    return $plugin->map_volume($storeid, $scfg, $volname, $snapname, $hints);
 }
 
 sub unmap_volume {
@@ -1385,11 +1389,33 @@ sub deactivate_storage {
     $plugin->deactivate_storage($storeid, $scfg, $cache);
 }
 
+sub is_hint_supported {
+    my ($hint) = @_;
+
+    return defined($PVE::Storage::Plugin::hints_properties->{$hint});
+}
+
+sub verify_hints {
+    my ($hints, $noerr) = @_;
+
+    return if !defined($hints);
+
+    eval { PVE::JSONSchema::validate($hints, $PVE::Storage::Plugin::hints_format); };
+    my $err = $@;
+
+    return $hints if !$err;
+    return if $noerr;
+
+    die "hints are not valid: $@";
+}
+
 sub activate_volumes {
-    my ($cfg, $vollist, $snapname) = @_;
+    my ($cfg, $vollist, $snapname, $hints) = @_;
 
     return if !($vollist && scalar(@$vollist));
 
+    verify_hints($hints);
+
     my $storagehash = {};
     foreach my $volid (@$vollist) {
         my ($storeid, undef) = parse_volume_id($volid);
@@ -1404,7 +1430,7 @@ sub activate_volumes {
         my ($storeid, $volname) = parse_volume_id($volid);
         my $scfg = storage_config($cfg, $storeid);
         my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-        $plugin->activate_volume($storeid, $scfg, $volname, $snapname, $cache);
+        $plugin->activate_volume($storeid, $scfg, $volname, $snapname, $cache, $hints);
     }
 }
 
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8acd214..2c72179 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -59,6 +59,19 @@ cfs_register_file(
     sub { __PACKAGE__->write_config(@_); },
 );
 
+our $hints_properties = {
+    'guest-ostype' => {
+        type => 'string',
+        optional => 1,
+    },
+};
+
+our $hints_format = {
+    type => 'object',
+    additionalProperties => 0,
+    properties => $hints_properties,
+};
+
 my %prune_option = (
     optional => 1,
     type => 'integer',
@@ -1871,7 +1884,7 @@ sub deactivate_storage {
 }
 
 sub map_volume {
-    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
+    my ($class, $storeid, $scfg, $volname, $snapname, $hints) = @_;
 
     my ($path) = $class->path($scfg, $volname, $storeid, $snapname);
     return $path;
@@ -1884,7 +1897,7 @@ sub unmap_volume {
 }
 
 sub activate_volume {
-    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
+    my ($class, $storeid, $scfg, $volname, $snapname, $cache, $hints) = @_;
 
     my $path = $class->filesystem_path($scfg, $volname, $snapname);
 
-- 
2.47.3



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

[pve-devel] [PATCH pve-storage 2/2] plugin: rbd: pass rxbounce when mapping Windows VM disks

[Friedrich Weber]

rxbounce can be needed to avoid "bad crc/signature" errors when
running Windows VM disks on KRBD. When mapping a volume, check
the guest-ostype hint, and if it denotes the volume is mapped
as part of a Windows VM, pass rxbounce.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index cf371c7..54c5a57 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -793,7 +793,7 @@ sub deactivate_storage {
 }
 
 sub map_volume {
-    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
+    my ($class, $storeid, $scfg, $volname, $snapname, $hints) = @_;
 
     my ($vtype, $img_name, $vmid) = $class->parse_volname($volname);
 
@@ -807,7 +807,12 @@ sub map_volume {
     # features can only be enabled/disabled for image, not for snapshot!
     $krbd_feature_update->($scfg, $storeid, $img_name);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name);
+    my @options = ();
+    if (defined($hints->{'guest-ostype'}) && $hints->{'guest-ostype'} =~ /^w/) {
+        @options = ('--options', 'rxbounce');
+    }
+
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name, @options);
     run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
 
     return $kerneldev;
@@ -830,9 +835,9 @@ sub unmap_volume {
 }
 
 sub activate_volume {
-    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
+    my ($class, $storeid, $scfg, $volname, $snapname, $cache, $hints) = @_;
 
-    $class->map_volume($storeid, $scfg, $volname, $snapname) if $scfg->{krbd};
+    $class->map_volume($storeid, $scfg, $volname, $snapname, $hints) if $scfg->{krbd};
 
     return 1;
 }
-- 
2.47.3



_______________________________________________
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/1] fix #5997: qemu: pass guest-ostype hint when activating volumes

[Friedrich Weber]

KRBD can use this hint to pass rxbounce only when mapping disks of
Windows VMs, which should avoid "bad crc/signature" errors in the
host journal, retransmits as well as degraded performance.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 src/PVE/API2/Qemu.pm             | 18 ++++++++++---
 src/PVE/QemuConfig.pm            |  1 +
 src/PVE/QemuServer.pm            | 44 ++++++++++++++++++++++++--------
 src/PVE/QemuServer/Cloudinit.pm  |  3 ++-
 src/PVE/QemuServer/ImportDisk.pm |  4 ++-
 src/PVE/QemuServer/OVMF.pm       |  4 ++-
 src/PVE/QemuServer/QemuImage.pm  |  5 ++--
 src/PVE/QemuServer/RunState.pm   |  3 ++-
 src/PVE/VZDump/QemuServer.pm     |  6 +++--
 9 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 71bedc1e..296d73a3 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -446,6 +446,8 @@ my sub create_disks : prototype($$$$$$$$$$$) {
         my $volid = $disk->{file};
         my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
+        my $storage_hints = PVE::QemuServer::generate_storage_hints($conf);
+
         if (!$volid || $volid eq 'none' || $volid eq 'cdrom') {
             delete $disk->{size};
             $res->{$ds} = PVE::QemuServer::print_drive($disk);
@@ -548,6 +550,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
                             drivename => $ds,
                             storage => $storeid,
                             format => $disk->{format},
+                            storage_hints => $storage_hints,
                         };
 
                         $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
@@ -681,7 +684,8 @@ my sub create_disks : prototype($$$$$$$$$$$) {
                 }
             }
 
-            PVE::Storage::activate_volumes($storecfg, [$volid]) if $storeid;
+            PVE::Storage::activate_volumes($storecfg, [$volid], undef, $storage_hints)
+                if $storeid;
 
             my $size = PVE::Storage::volume_size_info($storecfg, $volid);
             die "volume $volid does not exist\n" if !$size;
@@ -4487,11 +4491,13 @@ __PACKAGE__->register_method({
             my $newvollist = [];
             my $jobs = {};
 
+            my $storage_hints = PVE::QemuServer::generate_storage_hints($newconf);
+
             eval {
                 local $SIG{INT} = local $SIG{TERM} = local $SIG{QUIT} = local $SIG{HUP} =
                     sub { die "interrupted by signal\n"; };
 
-                PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
+                PVE::Storage::activate_volumes($storecfg, $vollist, $snapname, $storage_hints);
 
                 my $bwlimit = extract_param($param, 'bwlimit');
 
@@ -4522,6 +4528,7 @@ __PACKAGE__->register_method({
                         drivename => $opt,
                         storage => $storage,
                         format => $format,
+                        storage_hints => $storage_hints,
                     };
 
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
@@ -4768,7 +4775,8 @@ __PACKAGE__->register_method({
 
             my $running = PVE::QemuServer::check_running($vmid);
 
-            PVE::Storage::activate_volumes($storecfg, [$drive->{file}]);
+            my $storage_hints = PVE::QemuServer::generate_storage_hints($conf);
+            PVE::Storage::activate_volumes($storecfg, [$drive->{file}], undef, $storage_hints);
 
             my $newvollist = [];
 
@@ -4797,6 +4805,7 @@ __PACKAGE__->register_method({
                     drivename => $disk,
                     storage => $storeid,
                     format => $format,
+                    storage_hints => $storage_hints,
                 };
 
                 $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf)
@@ -5847,7 +5856,8 @@ __PACKAGE__->register_method({
 
             $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
 
-            PVE::Storage::activate_volumes($storecfg, [$volid]);
+            my $storage_hints = PVE::QemuServer::generate_storage_hints($conf);
+            PVE::Storage::activate_volumes($storecfg, [$volid], undef, $storage_hints);
             my $size = PVE::Storage::volume_size_info($storecfg, $volid, 5);
 
             die "Could not determine current size of volume '$volid'\n" if !defined($size);
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index ad6ce6b0..ef6c3edd 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -326,6 +326,7 @@ sub __snapshot_create_vol_snapshots_hook {
         if ($hook eq "before") {
             if ($snap->{vmstate}) {
                 my $path = PVE::Storage::path($storecfg, $snap->{vmstate});
+                # TODO: no guest hints needed for VM state?
                 PVE::Storage::activate_volumes($storecfg, [$snap->{vmstate}]);
                 my $state_storage_id = PVE::Storage::parse_volume_id($snap->{vmstate});
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cf195ccc..c7b6dc63 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5248,7 +5248,9 @@ sub vmconfig_update_disk {
 
     die "skip\n" if !$hotplug || $opt =~ m/(ide|sata)(\d+)/;
     # hotplug new disks
-    PVE::Storage::activate_volumes($storecfg, [$drive->{file}]) if $drive->{file} !~ m|^/dev/.+|;
+    my $storage_hints = generate_storage_hints($conf);
+    PVE::Storage::activate_volumes($storecfg, [$drive->{file}], undef, $storage_hints)
+        if $drive->{file} !~ m|^/dev/.+|;
     vm_deviceplug($storecfg, $conf, $vmid, $opt, $drive, $arch, $machine_type);
 }
 
@@ -5363,6 +5365,15 @@ my sub remove_left_over_vmstate_opts {
     PVE::QemuConfig->write_config($vmid, $conf) if $found;
 }
 
+sub generate_storage_hints {
+    my ($conf) = @_;
+
+    my $hints = {};
+    $hints->{'guest-ostype'} = $conf->{ostype} if PVE::Storage::is_hint_supported('guest-ostype');
+
+    return $hints;
+}
+
 # see vm_start_nolock for parameters, additionally:
 # migrate_opts:
 #   storagemap = parsed storage map for allocating NBD disks
@@ -5526,11 +5537,13 @@ sub vm_start_nolock {
     my $vollist = get_current_vm_volumes($storecfg, $conf);
     push $vollist->@*, $statefile if $statefile_is_a_volume;
 
+    my $storage_hints = generate_storage_hints($conf);
+
     my ($cmd, $spice_port, $start_timeout);
     my $pci_reserve_list = [];
     eval {
         # With -blockdev, it is necessary to activate the volumes before generating the command line
-        PVE::Storage::activate_volumes($storecfg, $vollist);
+        PVE::Storage::activate_volumes($storecfg, $vollist, undef, $storage_hints);
 
         # Note that for certain cases like templates, the configuration is minimized, so need to ensure
         # the rest of the function here uses the same configuration that was used to build the command
@@ -5901,7 +5914,8 @@ sub vm_commandline {
     # With -blockdev, it is necessary to activate the volumes before generating the command line
     if (!$running) {
         $volumes = get_current_vm_volumes($storecfg, $conf);
-        PVE::Storage::activate_volumes($storecfg, $volumes);
+        my $storage_hints = generate_storage_hints($conf);
+        PVE::Storage::activate_volumes($storecfg, $volumes, undef, $storage_hints);
     }
 
     # There might be concurrent operations on the volumes, so do not deactivate.
@@ -6474,7 +6488,7 @@ my $parse_backup_hints = sub {
 #
 # Returns: { $virtdev => $volid }
 my $restore_allocate_devices = sub {
-    my ($storecfg, $virtdev_hash, $vmid) = @_;
+    my ($storecfg, $virtdev_hash, $vmid, $storage_hints) = @_;
 
     my $map = {};
     foreach my $virtdev (sort keys %$virtdev_hash) {
@@ -6502,7 +6516,7 @@ my $restore_allocate_devices = sub {
         print STDERR "new volume ID is '$volid'\n";
         $d->{volid} = $volid;
 
-        PVE::Storage::activate_volumes($storecfg, [$volid]);
+        PVE::Storage::activate_volumes($storecfg, [$volid], undef, $storage_hints);
 
         $map->{$virtdev} = $volid;
     }
@@ -6867,7 +6881,10 @@ sub restore_proxmox_backup_archive {
         $restore_cleanup_oldconf->($storecfg, $vmid, $oldconf, $virtdev_hash) if $oldconf;
 
         # allocate volumes
-        my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid);
+        my $storage_hints = {};
+        # TODO: we should also generate storage hints here for live-restore, but we do not have
+        # the config yet -- build the config here already?
+        my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid, $storage_hints);
 
         foreach my $virtdev (sort keys %$virtdev_hash) {
             my $d = $virtdev_hash->{$virtdev};
@@ -7037,7 +7054,9 @@ sub restore_external_archive {
         $restore_cleanup_oldconf->($storecfg, $vmid, $oldconf, $virtdev_hash) if $oldconf;
 
         # allocate volumes
-        my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid);
+        my $storage_hints = {};
+        # TODO: in order to generate storage hints, we'd need to build the config already here
+        my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid, $storage_hints);
 
         for my $virtdev (sort keys $virtdev_hash->%*) {
             my $d = $virtdev_hash->{$virtdev};
@@ -7062,6 +7081,7 @@ sub restore_external_archive {
                     'is-zero-initialized' => $sparseinit,
                     'source-path-format' => $source_format,
                 };
+                # no need to pass source-storage-hints for the source
                 PVE::QemuServer::QemuImage::convert(
                     $source_path,
                     $d->{volid},
@@ -7456,7 +7476,9 @@ sub restore_vma_archive {
         }
 
         # allocate volumes
-        my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid);
+        my $storage_hints = {};
+        # TODO: here we do not necessarily have to pass storage hints, as we deactivate when we're done?
+        my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid, $storage_hints);
 
         # print restore information to $fifofh
         foreach my $virtdev (sort keys %$virtdev_hash) {
@@ -7754,7 +7776,7 @@ sub clone_disk {
     my ($src_drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
 
     my ($newvmid, $dst_drivename, $efisize) = $dest->@{qw(vmid drivename efisize)};
-    my ($storage, $format) = $dest->@{qw(storage format)};
+    my ($storage, $format, $storage_hints) = $dest->@{qw(storage format storage_hints)};
 
     my $unused = defined($src_drivename) && $src_drivename =~ /^unused/;
     my $use_drive_mirror = $full && $running && $src_drivename && !$snapname && !$unused;
@@ -7820,7 +7842,7 @@ sub clone_disk {
         );
         push @$newvollist, $newvolid;
 
-        PVE::Storage::activate_volumes($storecfg, [$newvolid]);
+        PVE::Storage::activate_volumes($storecfg, [$newvolid], undef, $storage_hints);
 
         if (drive_is_cloudinit($drive)) {
             # when cloning multiple disks (e.g. during clone_vm) it might be the last disk
@@ -7877,6 +7899,8 @@ sub clone_disk {
                     bwlimit => $bwlimit,
                     'is-zero-initialized' => $sparseinit,
                     snapname => $snapname,
+                    # TODO: double-check it's OK to pass the dest's storage hints here
+                    'source-storage-hints' => $storage_hints,
                 };
                 PVE::QemuServer::QemuImage::convert($drive->{file}, $newvolid, $size, $opts);
             }
diff --git a/src/PVE/QemuServer/Cloudinit.pm b/src/PVE/QemuServer/Cloudinit.pm
index 349cf90b..f42047e0 100644
--- a/src/PVE/QemuServer/Cloudinit.pm
+++ b/src/PVE/QemuServer/Cloudinit.pm
@@ -48,7 +48,8 @@ sub commit_cloudinit_disk {
         $size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
     }
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-    $plugin->activate_volume($storeid, $scfg, $volname);
+    my $storage_hints = PVE::QemuServer::generate_storage_hints($conf);
+    $plugin->activate_volume($storeid, $scfg, $volname, $storage_hints);
 
     print "generating cloud-init ISO\n";
     eval {
diff --git a/src/PVE/QemuServer/ImportDisk.pm b/src/PVE/QemuServer/ImportDisk.pm
index 75a54aa2..030cc4a1 100755
--- a/src/PVE/QemuServer/ImportDisk.pm
+++ b/src/PVE/QemuServer/ImportDisk.pm
@@ -84,7 +84,9 @@ sub do_import {
         local $SIG{INT} = local $SIG{TERM} = local $SIG{QUIT} = local $SIG{HUP} =
             local $SIG{PIPE} = sub { die "interrupted by signal $!\n"; };
 
-        PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
+        # TODO: do we need storage hints here if we deactivate the volumes immediately?
+        PVE::Storage::activate_volumes($storecfg, [$dst_volid], undef, undef);
+        # TODO: do we need source-storage-hints here?
         PVE::QemuServer::QemuImage::convert(
             $src_path,
             $dst_volid,
diff --git a/src/PVE/QemuServer/OVMF.pm b/src/PVE/QemuServer/OVMF.pm
index 08134e30..717253b2 100644
--- a/src/PVE/QemuServer/OVMF.pm
+++ b/src/PVE/QemuServer/OVMF.pm
@@ -136,8 +136,10 @@ sub create_efidisk($$$$$$$$) {
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
     my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
-    PVE::Storage::activate_volumes($storecfg, [$volid]);
+    # TODO: storage hints needed for EFI disks?
+    PVE::Storage::activate_volumes($storecfg, [$volid], undef, undef);
 
+    # no source-storage-hints passed here, because the source is not a volume
     PVE::QemuServer::QemuImage::convert($ovmf_vars, $volid, $vars_size_b);
     my $size = PVE::Storage::volume_size_info($storecfg, $volid, 3);
 
diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index 71be3abb..0adfbee1 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -97,10 +97,11 @@ my sub qcow2_target_image_opts {
 # snapname - Use this snapshot of the source image.
 # source-path-format - Indicate the format of the source when the source is a path. For PVE-managed
 # volumes, the format from the storage layer is always used.
+# source-storage-hints - storage hints to pass when activating the source volume
 sub convert {
     my ($src_volid, $dst_volid, $size, $opts) = @_;
 
-    my ($bwlimit, $snapname) = $opts->@{qw(bwlimit snapname)};
+    my ($bwlimit, $snapname, $storage_hints) = $opts->@{qw(bwlimit snapname source-storage-hints)};
 
     my $storecfg = PVE::Storage::config();
     my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid, 1);
@@ -114,7 +115,7 @@ sub convert {
     my $src_format;
 
     if ($src_storeid) {
-        PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname);
+        PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname, $storage_hints);
         my $src_scfg = PVE::Storage::storage_config($storecfg, $src_storeid);
         $src_format = checked_volume_format($storecfg, $src_volid);
         $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
diff --git a/src/PVE/QemuServer/RunState.pm b/src/PVE/QemuServer/RunState.pm
index 6a5fdbd7..fd2771c1 100644
--- a/src/PVE/QemuServer/RunState.pm
+++ b/src/PVE/QemuServer/RunState.pm
@@ -69,7 +69,8 @@ sub vm_suspend {
 
     if ($includestate) {
         # save vm state
-        PVE::Storage::activate_volumes($storecfg, [$vmstate]);
+        # TODO: need hints for VM state?
+        PVE::Storage::activate_volumes($storecfg, [$vmstate], undef, undef);
 
         eval {
             PVE::QemuMigrate::Helpers::set_migration_caps($vmid, 1);
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index dd789652..4f387731 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -118,7 +118,8 @@ sub prepare {
         $drivehash->{$name} = $volume->{volume_config};
     }
 
-    PVE::Storage::activate_volumes($self->{storecfg}, $vollist);
+    my $storage_hints = PVE::QemuServer::generate_storage_hints($conf);
+    PVE::Storage::activate_volumes($self->{storecfg}, $vollist, undef, $storage_hints);
 
     foreach my $ds (sort keys %$drivehash) {
         my $drive = $drivehash->{$ds};
@@ -643,7 +644,8 @@ my sub attach_fleecing_images {
     detach_fleecing_images($disks, $vmid);
 
     my $vollist = [map { $_->{'fleece-volid'} } grep { $_->{'fleece-volid'} } $disks->@*];
-    PVE::Storage::activate_volumes($self->{storecfg}, $vollist);
+    # TODO: storage hints needed for fleecing images?
+    PVE::Storage::activate_volumes($self->{storecfg}, $vollist, undef, undef);
 
     for my $di ($disks->@*) {
         if (my $volid = $di->{'fleece-volid'}) {
-- 
2.47.3



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