Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> With '-blockdev', it is necessary to activate the volumes to generate
> the command line, because it can be necessary to check whether the
> volume is a block device or a regular file.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm                | 61 +++++++++++++++++++++++---------
>  test/run_config2command_tests.pl | 10 ++++++
>  2 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 934adf60..82304096 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3844,7 +3844,6 @@ sub config_to_command {
>  	push @$devices, '-watchdog-action', $wdopts->{action} if $wdopts->{action};
>      }
>  
> -    my $vollist = [];
>      my $scsicontroller = {};
>      my $ahcicontroller = {};
>      my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} : $defaults->{scsihw};
> @@ -3857,11 +3856,6 @@ sub config_to_command {
>      PVE::QemuConfig->foreach_volume($conf, sub {
>  	my ($ds, $drive) = @_;
>  
> -	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> -	    check_volume_storage_type($storecfg, $drive->{file});
> -	    push @$vollist, $drive->{file};
> -	}
> -
>  	# ignore efidisk here, already added in bios/fw handling code above
>  	return if $drive->{interface} eq 'efidisk';
>  	# similar for TPM
> @@ -4035,7 +4029,6 @@ sub config_to_command {
>  
>      if (my $vmstate = $conf->{vmstate}) {
>  	my $statepath = PVE::Storage::path($storecfg, $vmstate);
> -	push @$vollist, $vmstate;
>  	push @$cmd, '-loadstate', $statepath;
>  	print "activating and using '$vmstate' as vmstate\n";
>      }
> @@ -4051,7 +4044,7 @@ sub config_to_command {
>  	push @$cmd, @$aa;
>      }
>  
> -    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
> +    return wantarray ? ($cmd, $spice_port, $pci_devices, $conf) : $cmd;
>  }
>  
>  sub spice_port {
> @@ -5649,12 +5642,18 @@ sub vm_start_nolock {
>      my $memory = get_current_memory($conf->{memory});
>      my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
>  
> -    my ($cmd, $vollist, $spice_port);
> +    my $vollist = get_current_vm_volumes($storecfg, $conf);
> +    push $vollist->@*, $statefile if $statefile_is_a_volume;
> +
> +    my ($cmd, $spice_port);
>      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);
> +
>  	# 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
> -	($cmd, $vollist, $spice_port, my $pci_devices, $conf) = config_to_command(
> +	($cmd, $spice_port, my $pci_devices, $conf) = config_to_command(
>  	    $storecfg,
>  	    $vmid,
>  	    $conf,
> @@ -5665,7 +5664,6 @@ sub vm_start_nolock {
>  	);
>  
>  	push $cmd->@*, $state_cmdline->@*;
> -	push @$vollist, $statefile if $statefile_is_a_volume;
>  
>  	for my $device (values $pci_devices->%*) {
>  	    next if $device->{mdev}; # we don't reserve for mdev devices
> @@ -5707,14 +5705,13 @@ sub vm_start_nolock {
>  	push @$cmd, '-uuid', $uuid if defined($uuid);
>      };
>      if (my $err = $@) {
> +	eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
> +	warn $@ if $@;
>  	eval { cleanup_pci_devices($vmid, $conf) };
>  	warn $@ if $@;
>  	die $err;
>      }
>  
> -    PVE::Storage::activate_volumes($storecfg, $vollist);
> -
> -
>      my %silence_std_outs = (outfunc => sub {}, errfunc => sub {});
>      eval { run_command(['/bin/systemctl', 'reset-failed', "$vmid.scope"], %silence_std_outs) };
>      eval { run_command(['/bin/systemctl', 'stop', "$vmid.scope"], %silence_std_outs) };
> @@ -5982,14 +5979,25 @@ sub vm_commandline {
>  
>      my $defaults = load_defaults();
>  
> +    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
> +    my $volumes = [];
> +
> +    # 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 $cmd;
>      eval {
>  	$cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
>      };
>      my $err = $@;
>  
> -    # if the vm is not running, we need to clean up the reserved/created devices
> -    if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
> +    # if the vm is not running, need to clean up the reserved/created devices and activated volumes
> +    if (!$running) {
> +	eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); };

I don't think we are allowed to do that here - `qm showcmd` can run
concurrently with other tasks (move disk, offline migration, ..) that
might be at a point in time where they've just activated but not yet
started using the volume..

in general, volume deactivation is tricky, and should often be avoided
unless it's 100% clear cut that it's safe to do (e.g., freshly allocated
volume while holding a lock, freeing a volume, ..) or required
(migration).

> +	warn $@ if $@;
>  	eval { cleanup_pci_devices($vmid, $conf) };

and this here might be dangerous as well, given that we don't take a
lock and the running state might have changed already since we checked?

should `qm showcmd` take a lock? or should we avoid doing the actual PCI
reservation if we are in the "just show command" code path?

>  	warn $@ if $@;
>      }
> @@ -6030,6 +6038,27 @@ sub get_vm_volumes {
>      return $vollist;
>  }
>  
> +# Get volumes defined in the current VM configuration, including the VM state file.
> +sub get_current_vm_volumes {
> +    my ($storecfg, $conf) = @_;
> +
> +    my $volumes = [];
> +
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +
> +	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> +	    check_volume_storage_type($storecfg, $drive->{file});
> +	    push $volumes->@*, $drive->{file};
> +	}
> +    });
> +    if (my $vmstate = $conf->{vmstate}) {
> +	push $volumes->@*, $vmstate;
> +    }
> +
> +    return $volumes;
> +}
> +
>  sub cleanup_pci_devices {
>      my ($vmid, $conf) = @_;
>  
> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
> index 7c581ad2..5217a020 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -255,6 +255,16 @@ $qemu_server_module->mock(
>      },
>  );
>  
> +my $storage_module = Test::MockModule->new("PVE::Storage");
> +$storage_module->mock(
> +    activate_volumes => sub {
> +	return;
> +    },
> +    deactivate_volumes => sub {
> +	return;
> +    },
> +);
> +
>  my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin");
>  $zfsplugin_module->mock(
>      zfs_get_lu_name => sub {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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