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

Yi Wang posted 1 patch 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1536835675-39311-1-git-send-email-wang.yi59@zte.com.cn
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(-)
[libvirt] [PATCH v7] qemu: Introduce state_lock_timeout to qemu.conf
Posted by Yi Wang 5 years, 6 months ago
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
Re: [libvirt] [PATCH v7] qemu: Introduce state_lock_timeout to qemu.conf
Posted by Peter Krempa 5 years, 6 months ago
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
Re: [libvirt] [PATCH v7] qemu: Introduce state_lock_timeout to qemu.conf
Posted by Michal Privoznik 5 years, 6 months ago
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
Re: [libvirt][PATCH v7] qemu: Introduce state_lock_timeout toqemu.conf
Posted by wang.yi59@zte.com.cn 5 years, 6 months ago
> 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