[libvirt] [PATCH v2] qemu: Introduce state_lock_timeout to qemu.conf
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 wanner continue to wait, and this patch allow users decide how long time they can wait the state lock. Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> Reviewed-by: Xi Xu <xu.xi8@zte.com.cn> --- 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 | 6 ++++++ src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 5 +---- src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ddc4bbf..f7287ae 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -93,6 +93,7 @@ module Libvirtd_qemu = | limits_entry "max_core" | bool_entry "dump_guest_core" | str_entry "stdio_handler" + | int_entry "state_lock_timeout" let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index cd57b3c..4284786 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -813,3 +813,9 @@ # #swtpm_user = "tss" #swtpm_group = "tss" + +# The timeout (in seconds) waiting for acquiring state lock. +# +# Default is 30 +# +#state_lock_timeout = 60 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a4f545e..31e0013 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) #endif +/* Give up waiting for mutex after 30 seconds */ +#define QEMU_JOB_WAIT_TIME (1000ull * 30) + virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { virQEMUDriverConfigPtr cfg; @@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->glusterDebugLevel = 4; cfg->stdioLogD = true; + cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME; + if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error; @@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) goto cleanup; + if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0) + goto cleanup; + if (virConfGetValueInt(conf, "seccomp_sandbox", &cfg->seccompSandbox) < 0) goto cleanup; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a8d84ef..97cf2e1 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -190,6 +190,8 @@ struct _virQEMUDriverConfig { int keepAliveInterval; unsigned int keepAliveCount; + int stateLockTimeout; + int seccompSandbox; char *migrateHost; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 886e3fb..5a2ca52 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6652,9 +6652,6 @@ qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv, priv->job.agentActive == QEMU_AGENT_JOB_NONE)); } -/* Give up waiting for mutex after 30 seconds */ -#define QEMU_JOB_WAIT_TIME (1000ull * 30) - /** * qemuDomainObjBeginJobInternal: * @driver: qemu driver @@ -6714,7 +6711,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, } priv->jobs_queued++; - then = now + QEMU_JOB_WAIT_TIME; + then = now + cfg->stateLockTimeout; retry: if ((!async && job != QEMU_JOB_DESTROY) && diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index f1e8806..dc5de96 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -105,3 +105,4 @@ module Test_libvirtd_qemu = { "pr_helper" = "/usr/bin/qemu-pr-helper" } { "swtpm_user" = "tss" } { "swtpm_group" = "tss" } +{ "state_lock_timeout" = "60" } -- 1.8.3.1
On 08/27/2018 12:57 PM, 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 wanner continue to wait, and this patch allow users decide how long time they can wait the state lock.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> Reviewed-by: Xi Xu <xu.xi8@zte.com.cn> --- 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 | 6 ++++++ src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 5 +---- src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ddc4bbf..f7287ae 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -93,6 +93,7 @@ module Libvirtd_qemu = | limits_entry "max_core" | bool_entry "dump_guest_core" | str_entry "stdio_handler" + | int_entry "state_lock_timeout"
let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index cd57b3c..4284786 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -813,3 +813,9 @@ # #swtpm_user = "tss" #swtpm_group = "tss" + +# The timeout (in seconds) waiting for acquiring state lock.
This is rather sparse description. I know that state change lock is, because I'm a libvirt devel. However, I don't expect our users to know that. How about adding the following description: When two or more threads want to work with the same domain they use a job lock to mutually exclude each other. However, waiting for the lock is limited up to state_lock_timeout seconds. Also, could you move this close to max_queued variable since they both refer to the same area?
+# +# Default is 30 +# +#state_lock_timeout = 60 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a4f545e..31e0013 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) #endif
+/* Give up waiting for mutex after 30 seconds */ +#define QEMU_JOB_WAIT_TIME (1000ull * 30) + virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { virQEMUDriverConfigPtr cfg; @@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->glusterDebugLevel = 4; cfg->stdioLogD = true;
+ cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME; + if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error;
@@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) goto cleanup;
+ if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0) + goto cleanup; +
Almost, you need to check if cfg->stateLockTimeout is not zero. Such code could go into virQEMUDriverConfigValidate(). Otherwise looking good. Michal
On 08/27/2018 09:29 AM, Michal Prívozník wrote:
On 08/27/2018 12:57 PM, 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 wanner continue
s/wanner/want to/ or "prefer to"... Perhaps users is too vague - this essentially sets the "default timeout policy" for everyone. Setting it too short could be dangerous considering it's impact. If "a user" wanted or preferred to wait longer than the default job timeout for a specific command, then we'd need to add some option for various commands/API's that allowed setting a unique timeout value for a specific job. Another thought would be to have an API that allowed changing the timeout programmatically. Of course there's the admin interface that is another area to consider altering since you're adjusting a configuration value like this. John
to wait, and this patch allow users decide how long time they can wait the state lock.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> Reviewed-by: Xi Xu <xu.xi8@zte.com.cn> --- 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 | 6 ++++++ src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 5 +---- src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ddc4bbf..f7287ae 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -93,6 +93,7 @@ module Libvirtd_qemu = | limits_entry "max_core" | bool_entry "dump_guest_core" | str_entry "stdio_handler" + | int_entry "state_lock_timeout"
let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index cd57b3c..4284786 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -813,3 +813,9 @@ # #swtpm_user = "tss" #swtpm_group = "tss" + +# The timeout (in seconds) waiting for acquiring state lock.
This is rather sparse description. I know that state change lock is, because I'm a libvirt devel. However, I don't expect our users to know that. How about adding the following description:
When two or more threads want to work with the same domain they use a job lock to mutually exclude each other. However, waiting for the lock is limited up to state_lock_timeout seconds.
Also, could you move this close to max_queued variable since they both refer to the same area?
+# +# Default is 30 +# +#state_lock_timeout = 60 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a4f545e..31e0013 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) #endif
+/* Give up waiting for mutex after 30 seconds */ +#define QEMU_JOB_WAIT_TIME (1000ull * 30) + virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { virQEMUDriverConfigPtr cfg; @@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->glusterDebugLevel = 4; cfg->stdioLogD = true;
+ cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME; + if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error;
@@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) goto cleanup;
+ if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0) + goto cleanup; +
Almost, you need to check if cfg->stateLockTimeout is not zero. Such code could go into virQEMUDriverConfigValidate().
Otherwise looking good.
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 04:29 PM, John Ferlan wrote:
On 08/27/2018 09:29 AM, Michal Prívozník wrote:
On 08/27/2018 12:57 PM, 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 wanner continue
s/wanner/want to/
or "prefer to"... Perhaps users is too vague - this essentially sets the "default timeout policy" for everyone. Setting it too short could be dangerous considering it's impact.
If "a user" wanted or preferred to wait longer than the default job timeout for a specific command, then we'd need to add some option for various commands/API's that allowed setting a unique timeout value for a specific job.
Another thought would be to have an API that allowed changing the timeout programmatically. Of course there's the admin interface that is another area to consider altering since you're adjusting a configuration value like this.
If anything, this is a job for virt-admin. I don't think we need an API that would live next to virDomainDeviceUpdate() for instance. We don't have one for max_jobs, nor for lock_manager, ... In fact, I don't think we have any API to change anything from qemu.conf. Michal
On 08/27/2018 09:29 AM, Michal Prívozník wrote:
On 08/27/2018 12:57 PM, 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 wanner continue
s/wanner/want to/
or "prefer to"... Perhaps users is too vague - this essentially sets the "default timeout policy" for everyone. Setting it too short could be dangerous considering it's impact. That's true, maybe I need add a NB in qemu.conf?
--- Best wishes Yi Wang
On 08/27/2018 12:57 PM, Yi Wang wrote:
When doing some job holding state lock for a long time, we may come across error: #swtpm_user = "tss" #swtpm_group = "tss" + +# The timeout (in seconds) waiting for acquiring state lock.
This is rather sparse description. I know that state change lock is, because I'm a libvirt devel. However, I don't expect our users to know that. How about adding the following description:
When two or more threads want to work with the same domain they use a job lock to mutually exclude each other. However, waiting for the lock is limited up to state_lock_timeout seconds. That's much better, thanks.
Also, could you move this close to max_queued variable since they both refer to the same area?
Of course, I will adjust it.
+ if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0) + goto cleanup; +
Almost, you need to check if cfg->stateLockTimeout is not zero. Such code could go into virQEMUDriverConfigValidate().
Thanks again for your time and patience, Michal. --- Best wishes Yi Wang
participants (5)
-
John Ferlan -
Michal Privoznik -
Michal Prívozník
-
wang.yi59＠zte.com.cn -
Yi Wang