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

[Fiona Ebner]

Am 13.06.25 um 12:16 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -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).

Will drop the deactivation in v2.

>> +	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?

I went with the latter approach now, as it's cleaner to not reserve at all.