block_job_cancel_async() did two things that were still block job
specific:

* Setting job->force. This field makes sense on the Job level, so we can
  just move it. While at it, rename it to job->force_cancel to make its
  purpose more obvious.

* Resetting the I/O status. This can't be moved because generic Jobs
  don't have an I/O status. What the function really implements is a
  user resume, except without entering the coroutine. Consequently, it
  makes sense to call the .user_resume driver callback here which
  already resets the I/O status.

  The old block_job_cancel_async() has two separate if statements that
  check job->iostatus != BLOCK_DEVICE_IO_STATUS_OK and job->user_paused.
  However, the former condition always implies the latter (as is
  asserted in block_job_iostatus_reset()), so changing the explicit call
  of block_job_iostatus_reset() on the former condition with the
  .user_resume callback on the latter condition is equivalent and
  doesn't need to access any BlockJob specific state.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/block/blockjob.h |  6 ------
 include/qemu/job.h       |  6 ++++++
 block/mirror.c           |  4 ++--
 blockjob.c               | 25 +++++++++++++------------
 4 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 3f405d1fa7..d975efea20 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -51,12 +51,6 @@ typedef struct BlockJob {
     BlockBackend *blk;
 
     /**
-     * Set to true if the job should abort immediately without waiting
-     * for data to be in sync.
-     */
-    bool force;
-
-    /**
      * Set to true when the job is ready to be completed.
      */
     bool ready;
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 3e817beee9..2648c74281 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -97,6 +97,12 @@ typedef struct Job {
      */
     bool cancelled;
 
+    /**
+     * Set to true if the job should abort immediately without waiting
+     * for data to be in sync.
+     */
+    bool force_cancel;
+
     /** Set to true when the job has deferred work to the main loop. */
     bool deferred_to_main_loop;
 
diff --git a/block/mirror.c b/block/mirror.c
index e9a90ea730..c3951d1ca2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -871,7 +871,7 @@ static void coroutine_fn mirror_run(void *opaque)
         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
         job_sleep_ns(&s->common.job, delay_ns);
         if (job_is_cancelled(&s->common.job) &&
-            (!s->synced || s->common.force))
+            (!s->synced || s->common.job.force_cancel))
         {
             break;
         }
@@ -884,7 +884,7 @@ immediate_exit:
          * or it was cancelled prematurely so that we do not guarantee that
          * the target is a copy of the source.
          */
-        assert(ret < 0 || ((s->common.force || !s->synced) &&
+        assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
                job_is_cancelled(&s->common.job)));
         assert(need_drain);
         mirror_wait_for_all_io(s);
diff --git a/blockjob.c b/blockjob.c
index 34c57da304..4cac3670ad 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -270,19 +270,20 @@ static int block_job_prepare(BlockJob *job)
     return job->job.ret;
 }
 
-static void block_job_cancel_async(BlockJob *job, bool force)
+static void job_cancel_async(Job *job, bool force)
 {
-    if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
-        block_job_iostatus_reset(job);
-    }
-    if (job->job.user_paused) {
-        /* Do not call block_job_enter here, the caller will handle it.  */
-        job->job.user_paused = false;
-        job->job.pause_count--;
+    if (job->user_paused) {
+        /* Do not call job_enter here, the caller will handle it.  */
+        job->user_paused = false;
+        if (job->driver->user_resume) {
+            job->driver->user_resume(job);
+        }
+        assert(job->pause_count > 0);
+        job->pause_count--;
     }
-    job->job.cancelled = true;
+    job->cancelled = true;
     /* To prevent 'force == false' overriding a previous 'force == true' */
-    job->force |= force;
+    job->force_cancel |= force;
 }
 
 static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *), bool lock)
@@ -367,7 +368,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
      * on the caller, so leave it. */
     QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
         if (other_job != job) {
-            block_job_cancel_async(other_job, false);
+            job_cancel_async(&other_job->job, false);
         }
     }
     while (!QLIST_EMPTY(&txn->jobs)) {
@@ -527,7 +528,7 @@ void block_job_cancel(BlockJob *job, bool force)
         job_do_dismiss(&job->job);
         return;
     }
-    block_job_cancel_async(job, force);
+    job_cancel_async(&job->job, force);
     if (!job_started(&job->job)) {
         block_job_completed(job, -ECANCELED);
     } else if (job->job.deferred_to_main_loop) {
-- 
2.13.6

On 05/18/2018 09:20 AM, Kevin Wolf wrote:
> block_job_cancel_async() did two things that were still block job
> specific:
> 
> * Setting job->force. This field makes sense on the Job level, so we can
>   just move it. While at it, rename it to job->force_cancel to make its
>   purpose more obvious.
> 
> * Resetting the I/O status. This can't be moved because generic Jobs
>   don't have an I/O status. What the function really implements is a
>   user resume, except without entering the coroutine. Consequently, it

You know, I had noticed this before but because this is called from
contexts which are NOT user-initiated, I didn't want to share the callback.

... I guess it's fine because the job is about to not exist anymore, so
it probably won't spook anything.

>   makes sense to call the .user_resume driver callback here which
>   already resets the I/O status.
> 
>   The old block_job_cancel_async() has two separate if statements that
>   check job->iostatus != BLOCK_DEVICE_IO_STATUS_OK and job->user_paused.
>   However, the former condition always implies the latter (as is
>   asserted in block_job_iostatus_reset()), so changing the explicit call
>   of block_job_iostatus_reset() on the former condition with the
>   .user_resume callback on the latter condition is equivalent and
>   doesn't need to access any BlockJob specific state.

Oh, cool! This is nice.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/blockjob.h |  6 ------
>  include/qemu/job.h       |  6 ++++++
>  block/mirror.c           |  4 ++--
>  blockjob.c               | 25 +++++++++++++------------
>  4 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 3f405d1fa7..d975efea20 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -51,12 +51,6 @@ typedef struct BlockJob {
>      BlockBackend *blk;
>  
>      /**
> -     * Set to true if the job should abort immediately without waiting
> -     * for data to be in sync.
> -     */
> -    bool force;
> -
> -    /**
>       * Set to true when the job is ready to be completed.
>       */
>      bool ready;
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 3e817beee9..2648c74281 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -97,6 +97,12 @@ typedef struct Job {
>       */
>      bool cancelled;
>  
> +    /**
> +     * Set to true if the job should abort immediately without waiting
> +     * for data to be in sync.
> +     */
> +    bool force_cancel;
> +

Does this comment need an update now, though?

Actually, in terms of "new jobs" API, it'd be really nice if cancel
*always meant cancel*.

I think "cancel" should never be used to mean "successful completion,
but different from the one we'd get if we used job_complete."

i.e., either we need a job setting:

job-set completion-mode=[pivot|no-pivot]

or optional parameters to pass to job-complete:

job-complete mode=[understood-by-job-type]

or some other mechanism that accomplishes the same type of behavior. It
would be nice if it did not have to be determined at job creation time
but instead could be determined later.


>      /** Set to true when the job has deferred work to the main loop. */
>      bool deferred_to_main_loop;
>  
> diff --git a/block/mirror.c b/block/mirror.c
> index e9a90ea730..c3951d1ca2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -871,7 +871,7 @@ static void coroutine_fn mirror_run(void *opaque)
>          trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>          job_sleep_ns(&s->common.job, delay_ns);
>          if (job_is_cancelled(&s->common.job) &&
> -            (!s->synced || s->common.force))
> +            (!s->synced || s->common.job.force_cancel))
>          {
>              break;
>          }
> @@ -884,7 +884,7 @@ immediate_exit:
>           * or it was cancelled prematurely so that we do not guarantee that
>           * the target is a copy of the source.
>           */
> -        assert(ret < 0 || ((s->common.force || !s->synced) &&
> +        assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
>                 job_is_cancelled(&s->common.job)));
>          assert(need_drain);
>          mirror_wait_for_all_io(s);
> diff --git a/blockjob.c b/blockjob.c
> index 34c57da304..4cac3670ad 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -270,19 +270,20 @@ static int block_job_prepare(BlockJob *job)
>      return job->job.ret;
>  }
>  
> -static void block_job_cancel_async(BlockJob *job, bool force)
> +static void job_cancel_async(Job *job, bool force)
>  {
> -    if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
> -        block_job_iostatus_reset(job);
> -    }
> -    if (job->job.user_paused) {
> -        /* Do not call block_job_enter here, the caller will handle it.  */
> -        job->job.user_paused = false;
> -        job->job.pause_count--;
> +    if (job->user_paused) {
> +        /* Do not call job_enter here, the caller will handle it.  */
> +        job->user_paused = false;
> +        if (job->driver->user_resume) {
> +            job->driver->user_resume(job);
> +        }
> +        assert(job->pause_count > 0);
> +        job->pause_count--;
>      }
> -    job->job.cancelled = true;
> +    job->cancelled = true;
>      /* To prevent 'force == false' overriding a previous 'force == true' */
> -    job->force |= force;
> +    job->force_cancel |= force;
>  }
>  
>  static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *), bool lock)
> @@ -367,7 +368,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
>       * on the caller, so leave it. */
>      QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
>          if (other_job != job) {
> -            block_job_cancel_async(other_job, false);
> +            job_cancel_async(&other_job->job, false);
>          }
>      }
>      while (!QLIST_EMPTY(&txn->jobs)) {
> @@ -527,7 +528,7 @@ void block_job_cancel(BlockJob *job, bool force)
>          job_do_dismiss(&job->job);
>          return;
>      }
> -    block_job_cancel_async(job, force);
> +    job_cancel_async(&job->job, force);
>      if (!job_started(&job->job)) {
>          block_job_completed(job, -ECANCELED);
>      } else if (job->job.deferred_to_main_loop) {
> 

Am 24.05.2018 um 01:18 hat John Snow geschrieben:
> > diff --git a/include/qemu/job.h b/include/qemu/job.h
> > index 3e817beee9..2648c74281 100644
> > --- a/include/qemu/job.h
> > +++ b/include/qemu/job.h
> > @@ -97,6 +97,12 @@ typedef struct Job {
> >       */
> >      bool cancelled;
> >  
> > +    /**
> > +     * Set to true if the job should abort immediately without waiting
> > +     * for data to be in sync.
> > +     */
> > +    bool force_cancel;
> > +
> 
> Does this comment need an update now, though?
> 
> Actually, in terms of "new jobs" API, it'd be really nice if cancel
> *always meant cancel*.
> 
> I think "cancel" should never be used to mean "successful completion,
> but different from the one we'd get if we used job_complete."
> 
> i.e., either we need a job setting:
> 
> job-set completion-mode=[pivot|no-pivot]
> 
> or optional parameters to pass to job-complete:
> 
> job-complete mode=[understood-by-job-type]
> 
> or some other mechanism that accomplishes the same type of behavior. It
> would be nice if it did not have to be determined at job creation time
> but instead could be determined later.

I agree. We already made sure that job-cancel really means cancel on the
QAPI level, so we're free to do that. We just need to keep supporting
block-job-cancel with the old semantics, so what I have is the easy
conversion. We can change the internal implementation when we actually
implement the selection of a completion mode.

Kevin

On 05/24/2018 04:24 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 01:18 hat John Snow geschrieben:
>>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>>> index 3e817beee9..2648c74281 100644
>>> --- a/include/qemu/job.h
>>> +++ b/include/qemu/job.h
>>> @@ -97,6 +97,12 @@ typedef struct Job {
>>>       */
>>>      bool cancelled;
>>>  
>>> +    /**
>>> +     * Set to true if the job should abort immediately without waiting
>>> +     * for data to be in sync.
>>> +     */
>>> +    bool force_cancel;
>>> +
>>
>> Does this comment need an update now, though?
>>
>> Actually, in terms of "new jobs" API, it'd be really nice if cancel
>> *always meant cancel*.
>>
>> I think "cancel" should never be used to mean "successful completion,
>> but different from the one we'd get if we used job_complete."
>>
>> i.e., either we need a job setting:
>>
>> job-set completion-mode=[pivot|no-pivot]
>>
>> or optional parameters to pass to job-complete:
>>
>> job-complete mode=[understood-by-job-type]
>>
>> or some other mechanism that accomplishes the same type of behavior. It
>> would be nice if it did not have to be determined at job creation time
>> but instead could be determined later.
> 
> I agree. We already made sure that job-cancel really means cancel on the
> QAPI level, so we're free to do that. We just need to keep supporting
> block-job-cancel with the old semantics, so what I have is the easy
> conversion. We can change the internal implementation when we actually
> implement the selection of a completion mode.
> 
> Kevin
> 

We need this before 3.0 though, yeah? unless we make job-cancel
x-job-cancel or some other warning that the way it works might change, yeah?

Or do I misunderstand our leeway to change this at a later point in time?

(can job-cancel apply to block jobs created with the legacy
infrastructure? My reading was "yes.")

--js

Am 24.05.2018 um 19:42 hat John Snow geschrieben:
> 
> 
> On 05/24/2018 04:24 AM, Kevin Wolf wrote:
> > Am 24.05.2018 um 01:18 hat John Snow geschrieben:
> >>> diff --git a/include/qemu/job.h b/include/qemu/job.h
> >>> index 3e817beee9..2648c74281 100644
> >>> --- a/include/qemu/job.h
> >>> +++ b/include/qemu/job.h
> >>> @@ -97,6 +97,12 @@ typedef struct Job {
> >>>       */
> >>>      bool cancelled;
> >>>  
> >>> +    /**
> >>> +     * Set to true if the job should abort immediately without waiting
> >>> +     * for data to be in sync.
> >>> +     */
> >>> +    bool force_cancel;
> >>> +
> >>
> >> Does this comment need an update now, though?
> >>
> >> Actually, in terms of "new jobs" API, it'd be really nice if cancel
> >> *always meant cancel*.
> >>
> >> I think "cancel" should never be used to mean "successful completion,
> >> but different from the one we'd get if we used job_complete."
> >>
> >> i.e., either we need a job setting:
> >>
> >> job-set completion-mode=[pivot|no-pivot]
> >>
> >> or optional parameters to pass to job-complete:
> >>
> >> job-complete mode=[understood-by-job-type]
> >>
> >> or some other mechanism that accomplishes the same type of behavior. It
> >> would be nice if it did not have to be determined at job creation time
> >> but instead could be determined later.
> > 
> > I agree. We already made sure that job-cancel really means cancel on the
> > QAPI level, so we're free to do that. We just need to keep supporting
> > block-job-cancel with the old semantics, so what I have is the easy
> > conversion. We can change the internal implementation when we actually
> > implement the selection of a completion mode.
> > 
> > Kevin
> > 
> 
> We need this before 3.0 though, yeah? unless we make job-cancel
> x-job-cancel or some other warning that the way it works might change, yeah?
> 
> Or do I misunderstand our leeway to change this at a later point in time?
> 
> (can job-cancel apply to block jobs created with the legacy
> infrastructure? My reading was "yes.")

It can, and it already has its final semantics, so nothing has to change
before 3.0. job-cancel is equivalent to block-job-cancel with fixed
force=true. If you want the complete-by-cancel behaviour of mirror, you
have to use block-job-cancel for now, because job-cancel doesn't provide
that functionality.

So what we can change later is adding a way to initiate this secondary
completion mode with a job-* command (probably with a new option for
job-complete). But we wouldn't change the semantics of exisiting
commands.

Kevin

On 05/25/2018 04:00 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 19:42 hat John Snow geschrieben:
>>
>>
>> On 05/24/2018 04:24 AM, Kevin Wolf wrote:
>>> Am 24.05.2018 um 01:18 hat John Snow geschrieben:
>>>>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>>>>> index 3e817beee9..2648c74281 100644
>>>>> --- a/include/qemu/job.h
>>>>> +++ b/include/qemu/job.h
>>>>> @@ -97,6 +97,12 @@ typedef struct Job {
>>>>>       */
>>>>>      bool cancelled;
>>>>>  
>>>>> +    /**
>>>>> +     * Set to true if the job should abort immediately without waiting
>>>>> +     * for data to be in sync.
>>>>> +     */
>>>>> +    bool force_cancel;
>>>>> +
>>>>
>>>> Does this comment need an update now, though?
>>>>
>>>> Actually, in terms of "new jobs" API, it'd be really nice if cancel
>>>> *always meant cancel*.
>>>>
>>>> I think "cancel" should never be used to mean "successful completion,
>>>> but different from the one we'd get if we used job_complete."
>>>>
>>>> i.e., either we need a job setting:
>>>>
>>>> job-set completion-mode=[pivot|no-pivot]
>>>>
>>>> or optional parameters to pass to job-complete:
>>>>
>>>> job-complete mode=[understood-by-job-type]
>>>>
>>>> or some other mechanism that accomplishes the same type of behavior. It
>>>> would be nice if it did not have to be determined at job creation time
>>>> but instead could be determined later.
>>>
>>> I agree. We already made sure that job-cancel really means cancel on the
>>> QAPI level, so we're free to do that. We just need to keep supporting
>>> block-job-cancel with the old semantics, so what I have is the easy
>>> conversion. We can change the internal implementation when we actually
>>> implement the selection of a completion mode.
>>>
>>> Kevin
>>>
>>
>> We need this before 3.0 though, yeah? unless we make job-cancel
>> x-job-cancel or some other warning that the way it works might change, yeah?
>>
>> Or do I misunderstand our leeway to change this at a later point in time?
>>
>> (can job-cancel apply to block jobs created with the legacy
>> infrastructure? My reading was "yes.")
> 
> It can, and it already has its final semantics, so nothing has to change
> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
> force=true. If you want the complete-by-cancel behaviour of mirror, you
> have to use block-job-cancel for now, because job-cancel doesn't provide
> that functionality.
> 
> So what we can change later is adding a way to initiate this secondary
> completion mode with a job-* command (probably with a new option for
> job-complete). But we wouldn't change the semantics of exisiting
> commands.
> 
> Kevin
> 

Oho, I _was_ missing something. I hadn't realized cancel takes on the
newer semantics. Thanks.

On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
> Am 24.05.2018 um 19:42 hat John Snow geschrieben:

[...]

(Randomly chiming in for a small clarification.)

> > >> or some other mechanism that accomplishes the same type of behavior. It
> > >> would be nice if it did not have to be determined at job creation time
> > >> but instead could be determined later.
> > > 
> > > I agree. We already made sure that job-cancel really means cancel on the
> > > QAPI level, so we're free to do that. We just need to keep supporting
> > > block-job-cancel with the old semantics, so what I have is the easy
> > > conversion. We can change the internal implementation when we actually
> > > implement the selection of a completion mode.
> > > 
> > > Kevin
> > > 
> > 
> > We need this before 3.0 though, yeah? unless we make job-cancel
> > x-job-cancel or some other warning that the way it works might change, yeah?
> > 
> > Or do I misunderstand our leeway to change this at a later point in time?
> > 
> > (can job-cancel apply to block jobs created with the legacy
> > infrastructure? My reading was "yes.")
> 
> It can, and it already has its final semantics, so nothing has to change
> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
> force=true. If you want the complete-by-cancel behaviour of mirror, you
> have to use block-job-cancel for now, because job-cancel doesn't provide
> that functionality.

I think by "complete-by-cancel" you are referring to this[*] magic
behaviour, mentioned in the last sentence here:

    When you cancel an in-progress ‘mirror’ job before the source and
    target are synchronized, block-job-cancel will emit the event
    BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
    after it has indicated (via the event BLOCK_JOB_READY) that the
    source and target have reached synchronization, then the event
    emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.


[*] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515

> So what we can change later is adding a way to initiate this secondary
> completion mode with a job-* command (probably with a new option for
> job-complete). But we wouldn't change the semantics of exisiting
> commands.

Ah, so if I'm understanding it correctly, the existing subtle magic of
"complete-by-cancel" will be rectified by separting the two distinct
operations: 'job-cancel' and 'job-complete'.


-- 
/kashyap

On 2018-05-29 13:59, Kashyap Chamarthy wrote:
> On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
>> Am 24.05.2018 um 19:42 hat John Snow geschrieben:
> 
> [...]
> 
> (Randomly chiming in for a small clarification.)
> 
>>>>> or some other mechanism that accomplishes the same type of behavior. It
>>>>> would be nice if it did not have to be determined at job creation time
>>>>> but instead could be determined later.
>>>>
>>>> I agree. We already made sure that job-cancel really means cancel on the
>>>> QAPI level, so we're free to do that. We just need to keep supporting
>>>> block-job-cancel with the old semantics, so what I have is the easy
>>>> conversion. We can change the internal implementation when we actually
>>>> implement the selection of a completion mode.
>>>>
>>>> Kevin
>>>>
>>>
>>> We need this before 3.0 though, yeah? unless we make job-cancel
>>> x-job-cancel or some other warning that the way it works might change, yeah?
>>>
>>> Or do I misunderstand our leeway to change this at a later point in time?
>>>
>>> (can job-cancel apply to block jobs created with the legacy
>>> infrastructure? My reading was "yes.")
>>
>> It can, and it already has its final semantics, so nothing has to change
>> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
>> force=true. If you want the complete-by-cancel behaviour of mirror, you
>> have to use block-job-cancel for now, because job-cancel doesn't provide
>> that functionality.
> 
> I think by "complete-by-cancel" you are referring to this[*] magic
> behaviour, mentioned in the last sentence here:
> 
>     When you cancel an in-progress ‘mirror’ job before the source and
>     target are synchronized, block-job-cancel will emit the event
>     BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
>     after it has indicated (via the event BLOCK_JOB_READY) that the
>     source and target have reached synchronization, then the event
>     emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
> 
> 
> [*] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
> 
>> So what we can change later is adding a way to initiate this secondary
>> completion mode with a job-* command (probably with a new option for
>> job-complete). But we wouldn't change the semantics of exisiting
>> commands.
> 
> Ah, so if I'm understanding it correctly, the existing subtle magic of
> "complete-by-cancel" will be rectified by separting the two distinct
> operations: 'job-cancel' and 'job-complete'.

Not really, because we already had those two operations for block jobs.
The thing is that block-job-complete (and thus job-complete) will pivot
to the target disk, but block-job-cancel doesn't.

The special behavior is that you can use block-job-cancel after
BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
we have a real plan on how to represent that with the generic job
commands, we just know that we don't want to use job-cancel.

(Maybe we can add a flag to job-complete (which to me does not sound
like a good idea), or you could set flags on jobs while they are
running, so you can set a do-not-pivot flag on the mirror job before you
complete it.)

Max

On Tue, May 29, 2018 at 02:30:47PM +0200, Max Reitz wrote:
> On 2018-05-29 13:59, Kashyap Chamarthy wrote:
> > On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:

[...]

> >> It can, and it already has its final semantics, so nothing has to change
> >> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
> >> force=true. If you want the complete-by-cancel behaviour of mirror, you
> >> have to use block-job-cancel for now, because job-cancel doesn't provide
> >> that functionality.
> > 
> > I think by "complete-by-cancel" you are referring to this[*] magic
> > behaviour, mentioned in the last sentence here:
> > 
> >     When you cancel an in-progress ‘mirror’ job before the source and
> >     target are synchronized, block-job-cancel will emit the event
> >     BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
> >     after it has indicated (via the event BLOCK_JOB_READY) that the
> >     source and target have reached synchronization, then the event
> >     emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
> > 
> > 
> > [*] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
> > 
> >> So what we can change later is adding a way to initiate this secondary
> >> completion mode with a job-* command (probably with a new option for
> >> job-complete). But we wouldn't change the semantics of exisiting
> >> commands.
> > 
> > Ah, so if I'm understanding it correctly, the existing subtle magic of
> > "complete-by-cancel" will be rectified by separting the two distinct
> > operations: 'job-cancel' and 'job-complete'.
> 
> Not really, because we already had those two operations for block jobs.
> The thing is that block-job-complete (and thus job-complete) will pivot
> to the target disk, but block-job-cancel doesn't.

Indeed; from the doc linked earlier: "Issuing the command
``block-job-complete`` (after it emits the event
``BLOCK_JOB_COMPLETED``) will adjust the guest device (i.e. live QEMU)
to point to the target image, [E], causing all the new writes from this
point on to happen there."

> The special behavior is that you can use block-job-cancel after
> BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> we have a real plan on how to represent that with the generic job
> commands, we just know that we don't want to use job-cancel.

Ah, thanks for clarifying.  Yes, what you say makes sense —  not using
'job-cancel' to represent completion.

> (Maybe we can add a flag to job-complete (which to me does not sound
> like a good idea), or you could set flags on jobs while they are
> running, so you can set a do-not-pivot flag on the mirror job before you
> complete it.)

Yeah, spelling that out, 'do-not-pivot' or something along those lines,
as a flag makes it clearer.  "Implicit is better than explicit".

-- 
/kashyap

On Tue, May 29, 2018 at 03:10:58PM +0200, Kashyap Chamarthy wrote:
> On Tue, May 29, 2018 at 02:30:47PM +0200, Max Reitz wrote:

[...]

> > The special behavior is that you can use block-job-cancel after
> > BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> > we have a real plan on how to represent that with the generic job
> > commands, we just know that we don't want to use job-cancel.
> 
> Ah, thanks for clarifying.  Yes, what you say makes sense —  not using
> 'job-cancel' to represent completion.
> 
> > (Maybe we can add a flag to job-complete (which to me does not sound
> > like a good idea), or you could set flags on jobs while they are
> > running, so you can set a do-not-pivot flag on the mirror job before you
> > complete it.)
> 
> Yeah, spelling that out, 'do-not-pivot' or something along those lines,
> as a flag makes it clearer.  "Implicit is better than explicit".

Oops, I got the quote reverse :P.  Correct: "Explicit is better than
implicit".  But you've auto-corrected in your head already...

-- 
/kashyap

On 05/29/2018 08:30 AM, Max Reitz wrote:
> On 2018-05-29 13:59, Kashyap Chamarthy wrote:
>> On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
>>> Am 24.05.2018 um 19:42 hat John Snow geschrieben:
>>
>> [...]
>>
>> (Randomly chiming in for a small clarification.)
>>
>>>>>> or some other mechanism that accomplishes the same type of behavior. It
>>>>>> would be nice if it did not have to be determined at job creation time
>>>>>> but instead could be determined later.
>>>>>
>>>>> I agree. We already made sure that job-cancel really means cancel on the
>>>>> QAPI level, so we're free to do that. We just need to keep supporting
>>>>> block-job-cancel with the old semantics, so what I have is the easy
>>>>> conversion. We can change the internal implementation when we actually
>>>>> implement the selection of a completion mode.
>>>>>
>>>>> Kevin
>>>>>
>>>>
>>>> We need this before 3.0 though, yeah? unless we make job-cancel
>>>> x-job-cancel or some other warning that the way it works might change, yeah?
>>>>
>>>> Or do I misunderstand our leeway to change this at a later point in time?
>>>>
>>>> (can job-cancel apply to block jobs created with the legacy
>>>> infrastructure? My reading was "yes.")
>>>
>>> It can, and it already has its final semantics, so nothing has to change
>>> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
>>> force=true. If you want the complete-by-cancel behaviour of mirror, you
>>> have to use block-job-cancel for now, because job-cancel doesn't provide
>>> that functionality.
>>
>> I think by "complete-by-cancel" you are referring to this[*] magic
>> behaviour, mentioned in the last sentence here:
>>
>>     When you cancel an in-progress ‘mirror’ job before the source and
>>     target are synchronized, block-job-cancel will emit the event
>>     BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
>>     after it has indicated (via the event BLOCK_JOB_READY) that the
>>     source and target have reached synchronization, then the event
>>     emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
>>
>>
>> [*] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
>>
>>> So what we can change later is adding a way to initiate this secondary
>>> completion mode with a job-* command (probably with a new option for
>>> job-complete). But we wouldn't change the semantics of exisiting
>>> commands.
>>
>> Ah, so if I'm understanding it correctly, the existing subtle magic of
>> "complete-by-cancel" will be rectified by separting the two distinct
>> operations: 'job-cancel' and 'job-complete'.
> 
> Not really, because we already had those two operations for block jobs.
> The thing is that block-job-complete (and thus job-complete) will pivot
> to the target disk, but block-job-cancel doesn't.
> 
> The special behavior is that you can use block-job-cancel after
> BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> we have a real plan on how to represent that with the generic job
> commands, we just know that we don't want to use job-cancel.
> 
> (Maybe we can add a flag to job-complete (which to me does not sound
> like a good idea), or you could set flags on jobs while they are
> running, so you can set a do-not-pivot flag on the mirror job before you
> complete it.)
> 
> Max
> 

Adding the completion-mode as a "setting" to the job has always been my
favorite way. Whether you set this setting at creation time or later
during runtime is not important.

When we issue job-complete, the job knows based on its own settings the
way it should do so, if there are multiple possibilities.

I only mentioned it as a flag to complete ... err, just in case it was
easier that way somehow. *shrug*.