Re: [PATCH qemu-server 1/1] fix #7590: qemu-server: apply timeout to QEMU start fork

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

On 7/13/26 4:37 PM, Fabian Grünbichler wrote:
> On June 25, 2026 1:45 pm, Samuel Rufinatscha wrote:
>> Apply the existing start timeout to the forked QEMU startup path and
>> clean up the VM scope on timeout. This prevents stuck qmstart tasks
>> from holding the config lock indefinitely or leaving a partially
>> started QEMU process running.
>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7590
>> ---
>>   src/PVE/QemuServer.pm | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>> index 55e9f520..df400376 100644
>> --- a/src/PVE/QemuServer.pm
>> +++ b/src/PVE/QemuServer.pm
>> @@ -5762,7 +5762,7 @@ sub vm_start_nolock {
>>       };
>>   
>>       my $run_qemu = sub {
>> -        PVE::Tools::run_fork sub {
>> +        my $run_qemu_child = sub {
>>               PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid",
>>                   %systemd_properties);
>>   
>> @@ -5791,6 +5791,32 @@ sub vm_start_nolock {
>>                   die "QEMU exited with code $exitcode\n";
>>               }
>>           };
>> +
>> +        my (undef, $timed_out) =
>> +            PVE::Tools::run_fork_with_timeout($start_timeout || undef, $run_qemu_child);
> 
> the timeout here is not the same that is used by the run_command
> invocation in the sub above.. and it would also need to account for the
> time it takes to start swtpm and all virtiofsd instances on top of that.

I see, makes sense.. we need to count for the helpers. Will fix in v2.

> 
>> +
>> +        if ($timed_out) {
>> +            eval {
>> +                run_command(
>> +                    [
>> +                        '/bin/systemctl',
>> +                        'kill',
>> +                        '--kill-whom=all',
>> +                        '--signal=KILL',
>> +                        "$vmid.scope",
>> +                    ],
>> +                    %silence_std_outs,
>> +                    noerr => 1,
>> +                    timeout => 10,
>> +                );
>> +                PVE::Systemd::wait_for_unit_removed("$vmid.scope", 20);
>> +            };
>> +            warn "failed to clean up timed-out VM start scope - $@" if $@;
>> +
>> +            $cleanup_qsd->();
> 
> isn't the order here wrong? first we should cleanup any running QSD
> instances, then kill the scope - after all the QSD instance is running
> inside the scope.. similarly, the tpmpid killing should also be done

Good point, yes. $cleanup_qsd expects QSD to be running else it is not 
doing much. tpmpid should be explicitly cleaned too like in the regular
cleanup path.

> here.. and in the regular cleanup path, we do not explicitly kill the
> scope?

Good point, one surviving helper would be enough to keep $vmid.scope 
active, so this should be fixed too.

> 
> I think we want unified error handling here, whether the start
> explicitly failed, or implicitly failed by timing out..

Makes sense, I will align the 2 paths in v2 too. Thanks @Fabian!

> 
>> +
>> +            die "QEMU start timed out after $start_timeout seconds\n";
>> +        }
>>       };
>>   
>>       if ($conf->{hugepages}) {
>> -- 
>> 2.47.3
>>
>>
>>
>>
>>
>>