[pve-devel] [RFC qemu-server] API/create: move locking inside worker

[Thomas Lamprecht]

Am 04/06/2018 um 12:28 PM schrieb Fabian Grünbichler:
> On Fri, Apr 06, 2018 at 11:54:03AM +0200, Thomas Lamprecht wrote:
>> Move the locking inside worker, so that the process doing the actual
>> work (create or restore) holds the lock, and can call functions which
>> do locking without deadlocking.
>>
>> This mirrors the behaviour we use for containers, and allows to add
>> an 'autostart' parameter which starts the VM after successful
>> creation. vm_start needs the lock and as not the worker but it's
>> parents held it, it couldn't know that it was actually save to
>> continue...
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>
>> I discussed this with Fabian a few months ago and have something in
>> mind that this shouldn't be that easy, but I cannot remember what
>> exactly that reason was, so RFC. :)
> 
> there is one issue - if somebody holds the flock and you only realize it
> after you have forked, you did a fork for nothing (and instead of a
> rather fast "timeout" error message, you have to check the task log.
> this is not nice from a usability perspective, although it should not
> cause problems from a technical/lockdep one ;)
> 

Ah, yeah, I could add a simple check if the VM is already locked before
forking the worker, I mean that's obviously racy but should catch most
cases, and we do that elsewhere too, AFAIK.

I'll try to add it and just send it out, if it's accetable to you then
we could apply it as a temporary improvement :)

> the clean solution is
> 
> flock {
>    set_config_lock
> }
> 
> fork {
>    do stuff
>    flock {
>      (re)read config
>      check_config_lock
>      modify/write config
>    }
>    do some more stuff
>    flock {
>      (re)read config
>      check_config_lock
>      remove_config_lock
>      final modify/write config
>    }
> }
> 
> of course, this is much more involved and harder to get all corner cases
> right ;) I would like to move all the create/restore/clone API paths to
> this flow in the long run, but I am not opposed to switching the
> fork/lock order in places where we need the flock sooner/now.
> 

Ah yes I remember now, thanks!
I'll try to give that a look in one and a half week, after my vacation.

cheers,
Thomas