[pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Fiona Ebner]

10 minutes is not long enough when disks are large and/or network
storages are used when preallocation is not disabled. The default is
metadata preallocation for qcow2, so there are still reports of the
issue [0][1]. If allocation really does not finish like the comment
describing the timeout feared, just let the user cancel it.

Also note that when restoring a PBS backup, there is no timeout for
disk allocation, and there don't seem to be any user complaints yet.

The 5 second timeout for receiving the config from vma is kept,
because certain corruptions in the VMA header can lead to the
operation hanging there.

There is no need for the $tmp variable before setting back the old
timeout, because that is at least one second, so we'll always be able
to set the $oldtimeout variable to undef in time in practice.
Currently, there shouldn't even be an outer timeout in the first
place, because the only call path leading to here is via the create
API (also used by qmrestore), both of which don't set a timeout.

[0]: https://forum.proxmox.com/threads/126825/
[1]: https://forum.proxmox.com/threads/128093/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bf1de179..05283562 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7483,14 +7483,11 @@ sub restore_vma_archive {
 		$devinfo->{$devname} = { size => $size, dev_id => $dev_id };
 	    } elsif ($line =~ m/^CTIME: /) {
 		# we correctly received the vma config, so we can disable
-		# the timeout now for disk allocation (set to 10 minutes, so
-		# that we always timeout if something goes wrong)
-		alarm(600);
+		# the timeout now for disk allocation
+		alarm($oldtimeout || 0);
+		$oldtimeout = undef;
 		&$print_devmap();
 		print $fifofh "done\n";
-		my $tmp = $oldtimeout || 0;
-		$oldtimeout = undef;
-		alarm($tmp);
 		close($fifofh);
 		$fifofh = undef;
 	    }
-- 
2.39.2

[pve-devel] [PATCH qemu-server 2/3] restore vma: add comment describing timeout

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 05283562..f7ee7686 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7367,7 +7367,7 @@ sub restore_vma_archive {
     $add_pipe->(['vma', 'extract', '-v', '-r', $mapfifo, $readfrom, $tmpdir]);
 
     my $oldtimeout;
-    my $timeout = 5;
+    my $timeout = 5; # for reading the VMA header - might hang with a corrupted one
 
     my $devinfo = {}; # info about drives included in backup
     my $virtdev_hash = {}; # info about allocated drives
-- 
2.39.2

[pve-devel] [PATCH qemu-server 3/3] restore vma: inline one timeout variable and move other closer to usage

[Fiona Ebner]

No functional change intended.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f7ee7686..b52dfbf4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7366,9 +7366,6 @@ sub restore_vma_archive {
 
     $add_pipe->(['vma', 'extract', '-v', '-r', $mapfifo, $readfrom, $tmpdir]);
 
-    my $oldtimeout;
-    my $timeout = 5; # for reading the VMA header - might hang with a corrupted one
-
     my $devinfo = {}; # info about drives included in backup
     my $virtdev_hash = {}; # info about allocated drives
 
@@ -7462,6 +7459,8 @@ sub restore_vma_archive {
 	$fh->close();
     };
 
+    my $oldtimeout;
+
     eval {
 	# enable interrupts
 	local $SIG{INT} =
@@ -7471,7 +7470,7 @@ sub restore_vma_archive {
 	    local $SIG{PIPE} = sub { die "interrupted by signal\n"; };
 	local $SIG{ALRM} = sub { die "got timeout\n"; };
 
-	$oldtimeout = alarm($timeout);
+	$oldtimeout = alarm(5); # for reading the VMA header - might hang with a corrupted one
 
 	my $parser = sub {
 	    my $line = shift;
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Dominik Csapak]

comment inline:

On 9/12/23 11:16, Fiona Ebner wrote:
> 10 minutes is not long enough when disks are large and/or network
> storages are used when preallocation is not disabled. The default is
> metadata preallocation for qcow2, so there are still reports of the
> issue [0][1]. If allocation really does not finish like the comment
> describing the timeout feared, just let the user cancel it.
> 
> Also note that when restoring a PBS backup, there is no timeout for
> disk allocation, and there don't seem to be any user complaints yet.
> 
> The 5 second timeout for receiving the config from vma is kept,
> because certain corruptions in the VMA header can lead to the
> operation hanging there.
> 
> There is no need for the $tmp variable before setting back the old
> timeout, because that is at least one second, so we'll always be able
> to set the $oldtimeout variable to undef in time in practice.
> Currently, there shouldn't even be an outer timeout in the first
> place, because the only call path leading to here is via the create
> API (also used by qmrestore), both of which don't set a timeout.
> 
> [0]: https://forum.proxmox.com/threads/126825/
> [1]: https://forum.proxmox.com/threads/128093/
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   PVE/QemuServer.pm | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index bf1de179..05283562 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -7483,14 +7483,11 @@ sub restore_vma_archive {
>   		$devinfo->{$devname} = { size => $size, dev_id => $dev_id };
>   	    } elsif ($line =~ m/^CTIME: /) {
>   		# we correctly received the vma config, so we can disable
> -		# the timeout now for disk allocation (set to 10 minutes, so
> -		# that we always timeout if something goes wrong)
> -		alarm(600);
> +		# the timeout now for disk allocation
> +		alarm($oldtimeout || 0);
> +		$oldtimeout = undef;


this part looks wrong to me, because AFAIU you want to disable the timeout
(by canceling the alarm), but what you do here is to set it to $oldtimeout
if that was set before?

i guess what we want to do here is:

----
alarm(0);
<... do stuff ...>
alarm($oldtimeout || 0);
$oldtimeout = undef;
----

?


>   		&$print_devmap();
>   		print $fifofh "done\n";
> -		my $tmp = $oldtimeout || 0;
> -		$oldtimeout = undef;
> -		alarm($tmp);
>   		close($fifofh);
>   		$fifofh = undef;
>   	    }

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Dominik Csapak]

sry just saw that this was already applied (some version of it)

and the discussion had included about oldtimeout always being 0 here, but
my point still stands for the applied code in case $oldtimeout is not 0

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Fiona Ebner]

Am 20.09.23 um 13:23 schrieb Dominik Csapak:
> comment inline:

Feel free to cut out irrelevant parts in the reply ;)

> On 9/12/23 11:16, Fiona Ebner wrote:
>> @@ -7483,14 +7483,11 @@ sub restore_vma_archive {
>>           $devinfo->{$devname} = { size => $size, dev_id => $dev_id };
>>           } elsif ($line =~ m/^CTIME: /) {
>>           # we correctly received the vma config, so we can disable
>> -        # the timeout now for disk allocation (set to 10 minutes, so
>> -        # that we always timeout if something goes wrong)
>> -        alarm(600);
>> +        # the timeout now for disk allocation

I would interpret this comment about disabling of the timeout to be
talking about the short 5 second timeout for reading the config.

>> +        alarm($oldtimeout || 0);
>> +        $oldtimeout = undef;
> 
> 
> this part looks wrong to me, because AFAIU you want to disable the timeout
> (by canceling the alarm), but what you do here is to set it to $oldtimeout
> if that was set before?
> 
> i guess what we want to do here is:
> 
> ----
> alarm(0);
> <... do stuff ...>
> alarm($oldtimeout || 0);
> $oldtimeout = undef;
> ----
> 
> ?

Hmm, I see what you mean. But I'd argue that it's unexpected to disable
the outer timeout for the full duration of the allocation from a
caller's perspective.

sub in_a_hurry {
    alarm(120); # outer/old timeout
    restore_vma_archive(...);
}

With the code before the patch, it could take up to 5 + 600 + 120
seconds to hit the outer timeout, with your suggestion up to 5 +
potentially unlimited + 120 seconds, with patched code up to 5 + 120
seconds. Since there currently are no callers setting an outer timeout,
the patch doesn't make the situation worse.

We could even make the calculation more complicated and have the timeout
always be hit within 120 seconds in the example above, but not sure if
worth it.

AFAICS, we do similar "delay" of the outer timeout in e.g.
run_with_timeout(), where it can also take up to $inner_timeout +
$outer_timeout seconds to hit the outer timeout.

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Dominik Csapak]

On 9/25/23 10:46, Fiona Ebner wrote:
> Am 20.09.23 um 13:23 schrieb Dominik Csapak:
>> comment inline:
> 
> Feel free to cut out irrelevant parts in the reply ;)
> 
>> On 9/12/23 11:16, Fiona Ebner wrote:
>>> @@ -7483,14 +7483,11 @@ sub restore_vma_archive {
>>>            $devinfo->{$devname} = { size => $size, dev_id => $dev_id };
>>>            } elsif ($line =~ m/^CTIME: /) {
>>>            # we correctly received the vma config, so we can disable
>>> -        # the timeout now for disk allocation (set to 10 minutes, so
>>> -        # that we always timeout if something goes wrong)
>>> -        alarm(600);
>>> +        # the timeout now for disk allocation
> 
> I would interpret this comment about disabling of the timeout to be
> talking about the short 5 second timeout for reading the config.

ok, i interpreted it to be disabling *any* timeout to be able
to allocate the disks properly, and since there is only one global
timeout here, selectively disabling one seems strange?

> 
>>> +        alarm($oldtimeout || 0);
>>> +        $oldtimeout = undef;
>>
>>
>> this part looks wrong to me, because AFAIU you want to disable the timeout
>> (by canceling the alarm), but what you do here is to set it to $oldtimeout
>> if that was set before?
>>
>> i guess what we want to do here is:
>>
>> ----
>> alarm(0);
>> <... do stuff ...>
>> alarm($oldtimeout || 0);
>> $oldtimeout = undef;
>> ----
>>
>> ?
> 
> Hmm, I see what you mean. But I'd argue that it's unexpected to disable
> the outer timeout for the full duration of the allocation from a
> caller's perspective.
> 
> sub in_a_hurry {
>      alarm(120); # outer/old timeout
>      restore_vma_archive(...);
> }
> 
> With the code before the patch, it could take up to 5 + 600 + 120
> seconds to hit the outer timeout, with your suggestion up to 5 +
> potentially unlimited + 120 seconds, with patched code up to 5 + 120
> seconds. Since there currently are no callers setting an outer timeout,
> the patch doesn't make the situation worse.


i get what you mean, but maybe that would warrant a comment on the function?
or maybe we should be able to clean up half allocated disks in there
in case the outer timeout triggers?

in any case, i'd find it good to improve the comment that speaks of
'disabling the timeout' that it's meant to only disable the inner 5s one.

> 
> We could even make the calculation more complicated and have the timeout
> always be hit within 120 seconds in the example above, but not sure if
> worth it.

meh. imho thats not worth it for the (up to) 5 seconds that the extraction
can take.

> 
> AFAICS, we do similar "delay" of the outer timeout in e.g.
> run_with_timeout(), where it can also take up to $inner_timeout +
> $outer_timeout seconds to hit the outer timeout.


exactly, only our "inner" timeout here is undefined/unlimited because
disk allocation can take forever?

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Fiona Ebner]

Am 25.09.23 um 10:57 schrieb Dominik Csapak:
> On 9/25/23 10:46, Fiona Ebner wrote:
>> Am 20.09.23 um 13:23 schrieb Dominik Csapak:
>>> On 9/12/23 11:16, Fiona Ebner wrote:
>>>> @@ -7483,14 +7483,11 @@ sub restore_vma_archive {
>>>>            $devinfo->{$devname} = { size => $size, dev_id => $dev_id };
>>>>            } elsif ($line =~ m/^CTIME: /) {
>>>>            # we correctly received the vma config, so we can disable
>>>> -        # the timeout now for disk allocation (set to 10 minutes, so
>>>> -        # that we always timeout if something goes wrong)
>>>> -        alarm(600);
>>>> +        # the timeout now for disk allocation
>>
>> I would interpret this comment about disabling of the timeout to be
>> talking about the short 5 second timeout for reading the config.
> 
> ok, i interpreted it to be disabling *any* timeout to be able
> to allocate the disks properly, and since there is only one global
> timeout here, selectively disabling one seems strange?

With that interpretation the code would be wrong of course.

> i get what you mean, but maybe that would warrant a comment on the
> function?
> or maybe we should be able to clean up half allocated disks in there
> in case the outer timeout triggers?

AFAICS, my patch didn't change cleanup behavior and what you suggest
already happens? The allocation is within an eval and we call
restore_destroy_volumes() if there was an error during allocation (that
also applies for a timeout error).

> 
> in any case, i'd find it good to improve the comment that speaks of
> 'disabling the timeout' that it's meant to only disable the inner 5s one.
> 

It can be seen from the code, but feel free to send a patch to improve it ;)

>> AFAICS, we do similar "delay" of the outer timeout in e.g.
>> run_with_timeout(), where it can also take up to $inner_timeout +
>> $outer_timeout seconds to hit the outer timeout.
> 
> 
> exactly, only our "inner" timeout here is undefined/unlimited because
> disk allocation can take forever?
> 

Why treat the operation as having unlimited inner timeout? What is the
benefit? I'd expect our caller to have a good reason to set a timeout if
it does, so why not try to honor it?

Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Dominik Csapak]

On 9/25/23 12:30, Fiona Ebner wrote:
> Am 25.09.23 um 10:57 schrieb Dominik Csapak:
>> On 9/25/23 10:46, Fiona Ebner wrote:
>>> Am 20.09.23 um 13:23 schrieb Dominik Csapak:
>>>> On 9/12/23 11:16, Fiona Ebner wrote:
>>>>> @@ -7483,14 +7483,11 @@ sub restore_vma_archive {
>>>>>             $devinfo->{$devname} = { size => $size, dev_id => $dev_id };
>>>>>             } elsif ($line =~ m/^CTIME: /) {
>>>>>             # we correctly received the vma config, so we can disable
>>>>> -        # the timeout now for disk allocation (set to 10 minutes, so
>>>>> -        # that we always timeout if something goes wrong)
>>>>> -        alarm(600);
>>>>> +        # the timeout now for disk allocation
>>>
>>> I would interpret this comment about disabling of the timeout to be
>>> talking about the short 5 second timeout for reading the config.
>>
>> ok, i interpreted it to be disabling *any* timeout to be able
>> to allocate the disks properly, and since there is only one global
>> timeout here, selectively disabling one seems strange?
> 
> With that interpretation the code would be wrong of course.
> 
>> i get what you mean, but maybe that would warrant a comment on the
>> function?
>> or maybe we should be able to clean up half allocated disks in there
>> in case the outer timeout triggers?
> 
> AFAICS, my patch didn't change cleanup behavior and what you suggest
> already happens? The allocation is within an eval and we call
> restore_destroy_volumes() if there was an error during allocation (that
> also applies for a timeout error).

true, did not see that

> 
>>
>> in any case, i'd find it good to improve the comment that speaks of
>> 'disabling the timeout' that it's meant to only disable the inner 5s one.
>>
> 
> It can be seen from the code, but feel free to send a patch to improve it ;)

sure, i'll see if i can come up with better wording ;)

> 
>>> AFAICS, we do similar "delay" of the outer timeout in e.g.
>>> run_with_timeout(), where it can also take up to $inner_timeout +
>>> $outer_timeout seconds to hit the outer timeout.
>>
>>
>> exactly, only our "inner" timeout here is undefined/unlimited because
>> disk allocation can take forever?
>>
> 
> Why treat the operation as having unlimited inner timeout? What is the
> benefit? I'd expect our caller to have a good reason to set a timeout if
> it does, so why not try to honor it?

no sure, having the outer timeout as upper bound is fine for me too,
i was seemingly just confused by the two timeouts and the wording of the
comment

just to note, the function can still take an infinite time if
it hangs in the cleanup for example, but that's then expected i guess.

[pve-devel] applied-series: [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks

[Thomas Lamprecht]

Am 12/09/2023 um 11:16 schrieb Fiona Ebner:
> 10 minutes is not long enough when disks are large and/or network
> storages are used when preallocation is not disabled. The default is
> metadata preallocation for qcow2, so there are still reports of the
> issue [0][1]. If allocation really does not finish like the comment
> describing the timeout feared, just let the user cancel it.
> 
> Also note that when restoring a PBS backup, there is no timeout for
> disk allocation, and there don't seem to be any user complaints yet.
> 
> The 5 second timeout for receiving the config from vma is kept,
> because certain corruptions in the VMA header can lead to the
> operation hanging there.
> 
> There is no need for the $tmp variable before setting back the old
> timeout, because that is at least one second, so we'll always be able
> to set the $oldtimeout variable to undef in time in practice.
> Currently, there shouldn't even be an outer timeout in the first
> place, because the only call path leading to here is via the create
> API (also used by qmrestore), both of which don't set a timeout.
> 
> [0]: https://forum.proxmox.com/threads/126825/
> [1]: https://forum.proxmox.com/threads/128093/
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
>

applied series a bit ago already, thanks!

It's actually a v3, as there where two older revisions sent for this
in 2020, sorry for the delay I guess.