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@zte.com.cn> Reviewed-by: Xi Xu <xu.xi8@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