The aim of this API is to allow caller do best effort. Some
functions of ours can work even when acquiring job fails (e.g.
qemuConnectGetAllDomainStats()). But what they can't bear is
delay if they have to wait up to 30 seconds for each domain.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++-----
src/qemu/qemu_domain.h | 4 ++++
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5273ab56ac..9657573342 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job)
* @obj: domain object
* @job: qemuDomainJob to start
* @asyncJob: qemuDomainAsyncJob to start
+ * @instant: don't wait trying to acquire @job
*
* Acquires job over domain object which must be locked before
* calling. If there's already a job running waits up to
* QEMU_JOB_WAIT_TIME after which the functions fails reporting
- * an error.
+ * an error unless @instant is set.
+ * If @instant is true this function tries to acquire job and if
+ * it fails returns immediately without waiting. No error is
+ * reported in this case.
*
* Returns: 0 on success,
* -2 if unable to start job because of timeout or
@@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1)
qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job,
- qemuDomainAsyncJob asyncJob)
+ qemuDomainAsyncJob asyncJob,
+ bool instant)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
unsigned long long now;
@@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
}
while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
+ if (instant)
+ goto cleanup;
+
VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name);
if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0)
goto error;
}
while (priv->job.active) {
+ if (instant)
+ goto cleanup;
+
VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name);
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
goto error;
@@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
qemuDomainJob job)
{
if (qemuDomainObjBeginJobInternal(driver, obj, job,
- QEMU_ASYNC_JOB_NONE) < 0)
+ QEMU_ASYNC_JOB_NONE, false) < 0)
return -1;
else
return 0;
@@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv;
if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
- asyncJob) < 0)
+ asyncJob, false) < 0)
return -1;
priv = obj->privateData;
@@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
return qemuDomainObjBeginJobInternal(driver, obj,
QEMU_JOB_ASYNC_NESTED,
- QEMU_ASYNC_JOB_NONE);
+ QEMU_ASYNC_JOB_NONE,
+ false);
}
+int
+qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
+ virDomainObjPtr obj,
+ qemuDomainJob job)
+{
+ return qemuDomainObjBeginJobInternal(driver, obj, job,
+ QEMU_ASYNC_JOB_NONE, true);
+}
/*
* obj must be locked and have a reference before calling
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f17157b951..4b63c00dff 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainAsyncJob asyncJob)
ATTRIBUTE_RETURN_CHECK;
+int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
+ virDomainObjPtr obj,
+ qemuDomainJob job)
+ ATTRIBUTE_RETURN_CHECK;
void qemuDomainObjEndJob(virQEMUDriverPtr driver,
virDomainObjPtr obj);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
FWIW: While I'm not the best at naming things, it would seem to me that
this should be named "Nowait" rather than "Instant".
On 06/07/2018 07:59 AM, Michal Privoznik wrote:
> The aim of this API is to allow caller do best effort. Some
s/allow caller/allow the caller/
s/do best effort/to avoid waiting for a domain job if the domain is
running something else that could take a long time./
> functions of ours can work even when acquiring job fails (e.g.
s/of ours//
s/job/the job/
> qemuConnectGetAllDomainStats()). But what they can't bear is
> delay if they have to wait up to 30 seconds for each domain.
s/./that is processing some other job./
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++-----
> src/qemu/qemu_domain.h | 4 ++++
> 2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5273ab56ac..9657573342 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job)
> * @obj: domain object
> * @job: qemuDomainJob to start
> * @asyncJob: qemuDomainAsyncJob to start
> + * @instant: don't wait trying to acquire @job
Prefer "nowait" (or some camelCase version of that)
> *
> * Acquires job over domain object which must be locked before
> * calling. If there's already a job running waits up to
> * QEMU_JOB_WAIT_TIME after which the functions fails reporting
> - * an error.
> + * an error unless @instant is set.
> + * If @instant is true this function tries to acquire job and if
> + * it fails returns immediately without waiting. No error is
s/it fails returns/it fails, then it returns/
> + * reported in this case.
Hmm... no error reported - meaning if ret = -1 and use this flag, it's
up to the caller to generate the error... For this series, perhaps
fine, but someone else using this, perhaps not.
> *
> * Returns: 0 on success,
> * -2 if unable to start job because of timeout or
> @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1)
> qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> virDomainObjPtr obj,
> qemuDomainJob job,
> - qemuDomainAsyncJob asyncJob)
> + qemuDomainAsyncJob asyncJob,
> + bool instant)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
> unsigned long long now;
> @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> }
>
> while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
> + if (instant)
> + goto cleanup;
> +
If async is not supported for this (as I believe seen in the new API),
then is this path possible?
> VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name);
> if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0)
> goto error;
> }
>
> while (priv->job.active) {
> + if (instant)
> + goto cleanup;
> +
> VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name);
> if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
> goto error;
> @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
> qemuDomainJob job)
> {
> if (qemuDomainObjBeginJobInternal(driver, obj, job,
> - QEMU_ASYNC_JOB_NONE) < 0)
> + QEMU_ASYNC_JOB_NONE, false) < 0)
> return -1;
> else
> return 0;
> @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
> qemuDomainObjPrivatePtr priv;
>
> if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
> - asyncJob) < 0)
> + asyncJob, false) < 0)
> return -1;
>
> priv = obj->privateData;
> @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
>
> return qemuDomainObjBeginJobInternal(driver, obj,
> QEMU_JOB_ASYNC_NESTED,
> - QEMU_ASYNC_JOB_NONE);
> + QEMU_ASYNC_JOB_NONE,
> + false);
> }
>
> +int
> +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
> + virDomainObjPtr obj,
> + qemuDomainJob job)
> +{
> + return qemuDomainObjBeginJobInternal(driver, obj, job,
> + QEMU_ASYNC_JOB_NONE, true);
^^^^^^^^^^^^^^^^^^^^
Doesn't this mean async jobs are not supported.
John
> +}
>
> /*
> * obj must be locked and have a reference before calling
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index f17157b951..4b63c00dff 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
> virDomainObjPtr obj,
> qemuDomainAsyncJob asyncJob)
> ATTRIBUTE_RETURN_CHECK;
> +int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
> + virDomainObjPtr obj,
> + qemuDomainJob job)
> + ATTRIBUTE_RETURN_CHECK;
>
> void qemuDomainObjEndJob(virQEMUDriverPtr driver,
> virDomainObjPtr obj);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/13/2018 05:28 PM, John Ferlan wrote:
>
> FWIW: While I'm not the best at naming things, it would seem to me that
> this should be named "Nowait" rather than "Instant".
>
> On 06/07/2018 07:59 AM, Michal Privoznik wrote:
>> The aim of this API is to allow caller do best effort. Some
>
> s/allow caller/allow the caller/
>
> s/do best effort/to avoid waiting for a domain job if the domain is
> running something else that could take a long time./
>
>> functions of ours can work even when acquiring job fails (e.g.
>
> s/of ours//
>
> s/job/the job/
>
>> qemuConnectGetAllDomainStats()). But what they can't bear is
>> delay if they have to wait up to 30 seconds for each domain.
>
> s/./that is processing some other job./
>
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++-----
>> src/qemu/qemu_domain.h | 4 ++++
>> 2 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 5273ab56ac..9657573342 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job)
>> * @obj: domain object
>> * @job: qemuDomainJob to start
>> * @asyncJob: qemuDomainAsyncJob to start
>> + * @instant: don't wait trying to acquire @job
>
> Prefer "nowait" (or some camelCase version of that)
Okay. Naming is hard :-P
>
>> *
>> * Acquires job over domain object which must be locked before
>> * calling. If there's already a job running waits up to
>> * QEMU_JOB_WAIT_TIME after which the functions fails reporting
>> - * an error.
>> + * an error unless @instant is set.
>> + * If @instant is true this function tries to acquire job and if
>> + * it fails returns immediately without waiting. No error is
>
> s/it fails returns/it fails, then it returns/
>
>> + * reported in this case.
>
> Hmm... no error reported - meaning if ret = -1 and use this flag, it's
> up to the caller to generate the error... For this series, perhaps
> fine, but someone else using this, perhaps not.
>
>> *
>> * Returns: 0 on success,
>> * -2 if unable to start job because of timeout or
>> @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1)
>> qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>> virDomainObjPtr obj,
>> qemuDomainJob job,
>> - qemuDomainAsyncJob asyncJob)
>> + qemuDomainAsyncJob asyncJob,
>> + bool instant)
>> {
>> qemuDomainObjPrivatePtr priv = obj->privateData;
>> unsigned long long now;
>> @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>> }
>>
>> while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
>> + if (instant)
>> + goto cleanup;
>> +
>
> If async is not supported for this (as I believe seen in the new API),
> then is this path possible?
Of course. For instance, if there's an async job running and another
thread is trying to start a sync, non-nested job that is not allowed for
the async job. Say, thread A is doing a snapshot which sets job mask
(=allowed sync jobs) to: query, destroy, abort, suspend, migration-op.
Then there's a thread B which is executing qemuDomainSetUserPassword().
This threads tries to grab modify job. However, since grabbing modify is
not allowed (due to job mask) thread B sits and waits until thread A
releases the async job. At that point, thread B can safely grab modify
job because there's no other (nor async) job running.
Long story short, not only synchronous jobs serialize, also async (and
any combination of those two) do.
>
>> VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name);
>> if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0)
>> goto error;
>> }
>>
>> while (priv->job.active) {
>> + if (instant)
>> + goto cleanup;
>> +
>> VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name);
>> if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
>> goto error;
>> @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
>> qemuDomainJob job)
>> {
>> if (qemuDomainObjBeginJobInternal(driver, obj, job,
>> - QEMU_ASYNC_JOB_NONE) < 0)
>> + QEMU_ASYNC_JOB_NONE, false) < 0)
>> return -1;
>> else
>> return 0;
>> @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
>> qemuDomainObjPrivatePtr priv;
>>
>> if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
>> - asyncJob) < 0)
>> + asyncJob, false) < 0)
>> return -1;
>>
>> priv = obj->privateData;
>> @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
>>
>> return qemuDomainObjBeginJobInternal(driver, obj,
>> QEMU_JOB_ASYNC_NESTED,
>> - QEMU_ASYNC_JOB_NONE);
>> + QEMU_ASYNC_JOB_NONE,
>> + false);
>> }
>>
>> +int
>> +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
>> + virDomainObjPtr obj,
>> + qemuDomainJob job)
>> +{
>> + return qemuDomainObjBeginJobInternal(driver, obj, job,
>> + QEMU_ASYNC_JOB_NONE, true);
> ^^^^^^^^^^^^^^^^^^^^
> Doesn't this mean async jobs are not supported.
I'm not quite sure what do you mean. You mean whether grabbing a sync
job while there's async job already running? This is supported. The fact
that asyncJob is QEMU_ASYNC_JOB_NONE does not mean "set sync job to @job
and async job to QEMU_ASYNC_JOB_NONE". In fact,
qemuDomainObjBeginJobInternal() is capable of grabbing either sync or
async job but not both at the same time. When grabbing a sync job,
@asynJob == QEMU_ASYNC_JOB_NONE and when grabbing an async job, @job ==
QEMU_JOB_ASYNC.
Anyway, the point of BeginJobInstant() is to be drop-in replacement for
plain BeginJob(), so whatever the latter passes to BeginJobInternal()
the former should mimic it. But then again, I'm not quite sure I
understand what you mean.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
>>> qemuDomainObjPrivatePtr priv = obj->privateData;
>>> unsigned long long now;
>>> @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>>> }
>>>
>>> while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
>>> + if (instant)
>>> + goto cleanup;
>>> +
>>
>> If async is not supported for this (as I believe seen in the new API),
>> then is this path possible?
>
> Of course. For instance, if there's an async job running and another
> thread is trying to start a sync, non-nested job that is not allowed for
> the async job. Say, thread A is doing a snapshot which sets job mask
> (=allowed sync jobs) to: query, destroy, abort, suspend, migration-op.
> Then there's a thread B which is executing qemuDomainSetUserPassword().
> This threads tries to grab modify job. However, since grabbing modify is
> not allowed (due to job mask) thread B sits and waits until thread A
> releases the async job. At that point, thread B can safely grab modify
> job because there's no other (nor async) job running.
>
> Long story short, not only synchronous jobs serialize, also async (and
> any combination of those two) do.
>
Ah... OK.
[...]
>>> +int
>>> +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
>>> + virDomainObjPtr obj,
>>> + qemuDomainJob job)
>>> +{
>>> + return qemuDomainObjBeginJobInternal(driver, obj, job,
>>> + QEMU_ASYNC_JOB_NONE, true);
>> ^^^^^^^^^^^^^^^^^^^^
>> Doesn't this mean async jobs are not supported.
>
> I'm not quite sure what do you mean. You mean whether grabbing a sync
> job while there's async job already running? This is supported. The fact
> that asyncJob is QEMU_ASYNC_JOB_NONE does not mean "set sync job to @job
> and async job to QEMU_ASYNC_JOB_NONE". In fact,
> qemuDomainObjBeginJobInternal() is capable of grabbing either sync or
> async job but not both at the same time. When grabbing a sync job,
> @asynJob == QEMU_ASYNC_JOB_NONE and when grabbing an async job, @job ==
> QEMU_JOB_ASYNC.
>
> Anyway, the point of BeginJobInstant() is to be drop-in replacement for
> plain BeginJob(), so whatever the latter passes to BeginJobInternal()
> the former should mimic it. But then again, I'm not quite sure I
> understand what you mean.
>
I should have used the [1] or something to signify that this particular
comment was related to the one above where I was asking about the need
for the bool/instant check for async jobs, but I think you figured that
out even though you left a small window of self doubt ;-)
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.