[libvirt] [PATCH 3/5] qemu_domain: Introduce qemuDomainObjBeginJobInstant

[Michal Privoznik]

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 at 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