11:19 a.m.
On 09/10/2018 10:22 PM, John Ferlan wrote:
On 09/05/2018 11:09 PM, Yi Wang wrote:
> When doing some job holding state lock for a long time,
> we may come across error:
blank line
> "Timed out during operation: cannot acquire state change lock"
blank line
> 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.
As I noted in a previous review... With this patch it's not the
individual user or command that can decide how long to wait, rather it's
set by qemu.conf policy. The duration of the wait just becomes
customize-able. Not that it's any different the current implementation,
since the value is set in qemuDomainObjBeginJobInternal that means
there's no distinguishing between job, asyncjob, and agentjob. There's
also no attempt to allow changing the value via virt-admin.
I'm not sure how we would implement per API timeout. We could perhaps do
per connection but that's also not desired.
And regarding "how does one know how long to wait" - well, they don't.
That's the thing with timeouts. It's not an argument against them
though. Even primitive functions (e.g. those from pthread) have
XXX_timed() variants.
A "strong recommendation" of longer than 30 seconds doesn't provide much
guidance. The setting of value generally depends on the configuration
and what causes an API to hold the lock for longer than 30 seconds.
That's probably limited to primarily virDomainListGetStats and/or
virConnectGetAllDomainStats. There's other factors in play including CPU
speed, CPU quota allowed, network throughput/latency, disk speed, etc.
Tough to create a value the works for every command/purpose.
Agreed, "strong recommendation" is use case dependant (although it's
perhaps what initiated this patch). If anything, we should discourage
people from touching this knob.
Ironically (perhaps) allowing a value of 0 would essentially make it so
no one waited - such as how qemuDomainObjBeginJobNowait operates
essentially. OTOH, in order to have everything wait "as long as
possible" passing -1 would be a lot easier than providing 2147483647 (or
if properly typed to uint, then 4294967295U).
Passing 0 is explicitly forbidden (although in a bit clumsy way - we
need to check the value right after it's parsed because multiplying it
by 1000 can lead to overflow and a negative value becomes positive or
vice versa).
In the long run, regardless of what value is chosen it may never be
"long enough". Similarly how does one know what is causing the
"wait". I
have doubts that "typically impatient" users would wait more than few
seconds before hitting ^c. Since there's no indication that something is
waiting or hung, how is one to truly understand the difference between
the two.
Still, I have a feeling something similar could be done for those
commands that are primarily QUERY jobs for which the consumer desires to
override the default of 30 seconds via a new option controlled by a
flag. Commands could add the --nowait flag just as easily as a --wait
NN. To me that would be far better improvement than a one time config
adjustment for everyone.
Ughr. This is not the way to go IMO. Can you imagine how many APIs we
have and thus how many flags we would have to introduce?
If anything we can have async versions of *some* APIs. But that is long
way to go too.
Not that the MODIFY jobs are any less important vis-a-vis waiting, but
those I would think would want a shorter wait time. How comfortable do
you feel when you go to modify something simple only to get no response.
Leaves one wondering, did my change take affect or did my change cause
some sort of problem?
Anyway, while not a full on objection, I figured I'd voice my preference
for a different solution and see where it takes things.
I think having this in qemu.conf makes sense. After all, we already have
max_queued, keepalive_* (which is also a timeout, sort of) and probably
some others.
John
>
> Signed-off-by: Yi Wang <wang.yi59(a)zte.com.cn>
> Reviewed-by: Xi Xu <xu.xi8(a)zte.com.cn>
> ---
> 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 | 10 ++++++++++
> src/qemu/qemu_conf.c | 15 +++++++++++++++
> src/qemu/qemu_conf.h | 2 ++
> src/qemu/qemu_domain.c | 5 +----
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 6 files changed, 30 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..8920a1a 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -667,6 +667,16 @@
> #
> #max_queued = 0
>
> +
> +# 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.
> +# NB, strong recommendation to set the timeout longer than 30 seconds.
s/.*/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..012f4d1 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,10 @@ 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;
> + cfg->stateLockTimeout *= 1000;
> +
> if (virConfGetValueInt(conf, "seccomp_sandbox",
&cfg->seccompSandbox) < 0)
> goto cleanup;
>
> @@ -1055,6 +1064,12 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg)
> return -1;
> }
>
> + if (cfg->stateLockTimeout <= 0) {
If this were an unsigned int, then the < 0 check wouldn't necessary.
So then the question becomes, should 0 (zero) be special cased as wait
forever.
This check needs to be done before multiplication otherwise an overflow
might occur. Also, the multiplication can be done in BeginJobInternal so
that cfg->stateLockTimeout holds value entered by user.
> + virReportError(VIR_ERR_CONF_SYNTAX, "%s",
> + _("state_lock_timeout should larger than zero"));
If 0 were left as invalid, then s/should/must be/
> + return -1;
> + }
> +
> return 0;
> }
>
Michal