all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Fiona Ebner <f.ebner@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks
Date: Mon, 25 Sep 2023 12:30:38 +0200	[thread overview]
Message-ID: <5d552a06-26ac-f797-655b-72f88a7ab4d0@proxmox.com> (raw)
In-Reply-To: <c21a0808-16e6-4b18-aceb-94206a40f7ad@proxmox.com>

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?




  reply	other threads:[~2023-09-25 10:30 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-12  9:16 Fiona Ebner
2023-09-12  9:16 ` [pve-devel] [PATCH qemu-server 2/3] restore vma: add comment describing timeout Fiona Ebner
2023-09-12  9:16 ` [pve-devel] [PATCH qemu-server 3/3] restore vma: inline one timeout variable and move other closer to usage Fiona Ebner
2023-09-20 11:23 ` [pve-devel] [PATCH qemu-server 1/3] fix #2816: restore: remove timeout when allocating disks Dominik Csapak
2023-09-20 11:28   ` Dominik Csapak
2023-09-25  8:46   ` Fiona Ebner
2023-09-25  8:57     ` Dominik Csapak
2023-09-25 10:30       ` Fiona Ebner [this message]
2023-09-25 11:25         ` Dominik Csapak
2023-11-17  8:49 ` [pve-devel] applied-series: " Thomas Lamprecht

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5d552a06-26ac-f797-655b-72f88a7ab4d0@proxmox.com \
    --to=f.ebner@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal