[libvirt] [PATCH v7] qemu: Introduce state_lock_timeout to qemu.conf

[Michal Privoznik]

On 09/13/2018 01:19 PM, Peter Krempa wrote:
> On Thu, Sep 13, 2018 at 18:47:55 +0800, Yi Wang wrote:
>> When doing some job holding state lock for a long time,
>> we may come across error:
>>
>> "Timed out during operation: cannot acquire state change lock"
>>
>> Well, sometimes it's not a problem and users want to continue
>> to wait, and this patch allow users decide how long time they
>> can wait the state lock.
> 
> [1]
> 
>>
>> Signed-off-by: Yi Wang <wang.yi59 at zte.com.cn>
>> Reviewed-by: Xi Xu <xu.xi8 at zte.com.cn>
>> ---
>> changes in v7:
>> - fix multiplication issue in BeginJobInternal
>>
>> changes in v6:
>> - modify the description in qemu.conf
>> - move the multiplication to BeginJobInternal
>>
>> changes in v5:
>> - adjust position of state lock in aug file
>> - fix state lock time got from conf file from milliseconds to
>>   seconds
>>
>> changes in v4:
>> - fix syntax-check error
>>
>> changes in v3:
>> - add user-friendly description and nb of state lock
>> - check validity of stateLockTimeout
>>
>> changes in v2:
>> - change default value to 30 in qemu.conf
>> - set the default value in virQEMUDriverConfigNew()
>> ---
>>  src/qemu/libvirtd_qemu.aug         |  1 +
>>  src/qemu/qemu.conf                 |  7 +++++++
>>  src/qemu/qemu_conf.c               | 14 ++++++++++++++
>>  src/qemu/qemu_conf.h               |  2 ++
>>  src/qemu/qemu_domain.c             |  5 +----
>>  src/qemu/test_libvirtd_qemu.aug.in |  1 +
>>  6 files changed, 26 insertions(+), 4 deletions(-)
> 
> [...]
> 
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index cd57b3c..f5e34f1 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -667,6 +667,13 @@
>>  #
>>  #max_queued = 0
>>  
>> +
>> +# It is strongly recommended to not touch this setting
> 
> A statement like this seems like a strong indication that such a setting
> should not even exist in a user-configurable way. Also the [1] commit
> message justification does not inspire much confidence that it's needed.

I beg to differ. We already have a knob that allows to limit number of
threads waiting on a domain job (max_queued).

Sure, having the timeout in a config file that is read only at daemon
startup is not ideal, but what other solution do we have? The problem is
we have timeout in the first place. Making it configurable does not make
things any worse in my book. And also 30 seconds look very arbitrary to
me. IIRC it was chosen because at the point of introduction it might
have been sweet spot, a balance between erroring out too early and
leaving API wait for too long. Well, times change.

We can also look at this feature as a way to make our APIs more
responsive. 30 second timeout might be too long for some GUI based apps.
They might want to have the timeout really short (say less than 10 seconds).

Michal