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(-)
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.
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/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ddc4bbf..a5601e1 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -100,6 +100,7 @@ module Libvirtd_qemu =
| str_entry "lock_manager"
let rpc_entry = int_entry "max_queued"
+ | int_entry "state_lock_timeout"
| int_entry "keepalive_interval"
| int_entry "keepalive_count"
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
+#
+# Default is 30
+#
+#state_lock_timeout = 60
+
###################################################################
# Keepalive protocol:
# This allows qemu driver to detect broken connections to remote
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545e..5be37dc 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 (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;
@@ -1055,6 +1063,12 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg)
return -1;
}
+ if (cfg->stateLockTimeout <= 0) {
+ virReportError(VIR_ERR_CONF_SYNTAX, "%s",
+ _("state_lock_timeout must be larger than zero"));
+ return -1;
+ }
+
return 0;
}
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..4dea85f 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 * 1000;
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..8e072d0 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -82,6 +82,7 @@ module Test_libvirtd_qemu =
{ "relaxed_acs_check" = "1" }
{ "lock_manager" = "lockd" }
{ "max_queued" = "0" }
+{ "state_lock_timeout" = "60" }
{ "keepalive_interval" = "5" }
{ "keepalive_count" = "5" }
{ "seccomp_sandbox" = "1" }
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
> 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] > > > # > > #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. The statement has been discussed and changed for many times, this time is modified as this thread suggested: https://www.redhat.com/archives/libvir-list/2018-September/msg00477.html Well, it seemed that this patch does not intrigue many maintainers' interest, but in some condition it is really useful and convenient, and I suppose this is the most simplest way to achieve this at present. Of course if this is not suitable to let this patch into upstream, I will not continue to update it :) Thanks to everyone again. --- Best wishes Yi Wang-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.