[Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Liang Li]

When doing drive mirror to a low speed shared storage, if there was heavy
BLK IO write workload in VM after the 'ready' event, drive mirror block job
can't be canceled immediately, it would keep running until the heavy BLK IO
workload stopped in the VM.

Because libvirt depends on block-job-cancel for block live migration, the
current block-job-cancel has the semantic to make sure data is in sync after
the 'ready' event.  This semantic can't meet some requirement, for example,
people may use drive mirror for realtime backup while need the ability of
block live migration. If drive mirror can't not be cancelled immediately,
it means block live migration need to wait, because libvirt make use drive
mirror to implement block live migration and only one drive mirror block
job is allowed at the same time for a give block dev.

We need a new interface for 'force cancel', which could quit block job
immediately if don't care about whether data is in sync or not.

'force' is not used by libvirt currently, to make things simple, change
it's semantic slightly, hope it will not break some use case which need its
original semantic.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Jeff Cody <jcody@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
Signed-off-by: Liang Li <liliangleo@didichuxing.com>
---
 block/mirror.c            |  9 +++------
 blockdev.c                |  4 ++--
 blockjob.c                | 11 ++++++-----
 hmp-commands.hx           |  3 ++-
 include/block/blockjob.h  |  9 ++++++++-
 qapi/block-core.json      |  6 ++++--
 tests/test-blockjob-txn.c |  8 ++++----
 7 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9badc1..c22dff9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
 
         ret = 0;
         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
-        if (!s->synced) {
-            block_job_sleep_ns(&s->common, delay_ns);
-            if (block_job_is_cancelled(&s->common)) {
-                break;
-            }
+        if (block_job_is_cancelled(&s->common) && s->common.force) {
+            break;
         } else if (!should_complete) {
             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
             block_job_sleep_ns(&s->common, delay_ns);
@@ -887,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->synced && block_job_is_cancelled(&s->common)));
+        assert(ret < 0 || block_job_is_cancelled(&s->common));
         assert(need_drain);
         mirror_wait_for_all_io(s);
     }
diff --git a/blockdev.c b/blockdev.c
index 8e977ee..039f156 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
         aio_context_acquire(aio_context);
 
         if (bs->job) {
-            block_job_cancel(bs->job);
+            block_job_cancel(bs->job, false);
         }
 
         aio_context_release(aio_context);
@@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
     }
 
     trace_qmp_block_job_cancel(job);
-    block_job_cancel(job);
+    block_job_cancel(job, force);
 out:
     aio_context_release(aio_context);
 }
diff --git a/blockjob.c b/blockjob.c
index f5cea84..0aacb50 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
     block_job_unref(job);
 }
 
-static void block_job_cancel_async(BlockJob *job)
+static void block_job_cancel_async(BlockJob *job, bool force)
 {
     if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
         block_job_iostatus_reset(job);
@@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
         job->pause_count--;
     }
     job->cancelled = true;
+    job->force = force;
 }
 
 static int block_job_finish_sync(BlockJob *job,
@@ -437,7 +438,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);
+            block_job_cancel_async(other_job, true);
         }
     }
     while (!QLIST_EMPTY(&txn->jobs)) {
@@ -542,10 +543,10 @@ void block_job_user_resume(BlockJob *job)
     }
 }
 
-void block_job_cancel(BlockJob *job)
+void block_job_cancel(BlockJob *job, bool force)
 {
     if (block_job_started(job)) {
-        block_job_cancel_async(job);
+        block_job_cancel_async(job, force);
         block_job_enter(job);
     } else {
         block_job_completed(job, -ECANCELED);
@@ -557,7 +558,7 @@ void block_job_cancel(BlockJob *job)
  * function pointer casts there. */
 static void block_job_cancel_err(BlockJob *job, Error **errp)
 {
-    block_job_cancel(job);
+    block_job_cancel(job, false);
 }
 
 int block_job_cancel_sync(BlockJob *job)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 15620c9..c8bb414 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -106,7 +106,8 @@ ETEXI
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
         .help       = "stop an active background block operation (use -f"
-                      "\n\t\t\t if the operation is currently paused)",
+                      "\n\t\t\t if you want to abort the operation immediately"
+                      "\n\t\t\t instead of keep running until data is in sync )",
         .cmd        = hmp_block_job_cancel,
     },
 
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 00403d9..4a96c42 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -63,6 +63,12 @@ typedef struct BlockJob {
     bool cancelled;
 
     /**
+     * Set to true if the job should be abort immediately without waiting
+     * for data is in sync.
+     */
+    bool force;
+
+    /**
      * Counter for pause request. If non-zero, the block job is either paused,
      * or if busy == true will pause itself as soon as possible.
      */
@@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
 /**
  * block_job_cancel:
  * @job: The job to be canceled.
+ * @force: Quit a job without waiting data is in sync.
  *
  * Asynchronously cancel the specified job.
  */
-void block_job_cancel(BlockJob *job);
+void block_job_cancel(BlockJob *job, bool force);
 
 /**
  * block_job_complete:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8225308..7c4dbfe 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2098,8 +2098,10 @@
 #          the name of the parameter), but since QEMU 2.7 it can have
 #          other values.
 #
-# @force: whether to allow cancellation of a paused job (default
-#         false).  Since 1.3.
+# @force: #optional whether to allow cancellation a job without waiting data is
+#         in sync, please not that since 2.12 it's semantic is not exactly the
+#         same as before, from 1.3 to 2.11 it means whether to allow cancellation
+#         of a paused job (default false).  Since 1.3.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 3591c96..53daadb 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -125,7 +125,7 @@ static void test_single_job(int expected)
     block_job_start(job);
 
     if (expected == -ECANCELED) {
-        block_job_cancel(job);
+        block_job_cancel(job, false);
     }
 
     while (result == -EINPROGRESS) {
@@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int expected2)
     block_job_txn_unref(txn);
 
     if (expected1 == -ECANCELED) {
-        block_job_cancel(job1);
+        block_job_cancel(job1, false);
     }
     if (expected2 == -ECANCELED) {
-        block_job_cancel(job2);
+        block_job_cancel(job2, false);
     }
 
     while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
@@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
     block_job_start(job1);
     block_job_start(job2);
 
-    block_job_cancel(job1);
+    block_job_cancel(job1, false);
 
     /* Now make job2 finish before the main loop kicks jobs.  This simulates
      * the race between a pending kick and another job completing.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5532 bytes --]

On 01/30/2018 02:38 AM, Liang Li wrote:
> When doing drive mirror to a low speed shared storage, if there was heavy
> BLK IO write workload in VM after the 'ready' event, drive mirror block job
> can't be canceled immediately, it would keep running until the heavy BLK IO
> workload stopped in the VM.

So far so good.   But the grammar and explanation in the rest of the
commit is a bit hard to read; let me give a shot at an alternative wording:

Libvirt depends on the current block-job-cancel semantics, which is that
when used without a flag after the 'ready' event, the command blocks
until data is in sync.  However, these semantics are awkward in other
situations, for example, people may use drive mirror for realtime
backups while still wanting to use block live migration.  Libvirt cannot
start a block live migration while another drive mirror is in progress,
but the user would rather abandon the backup attempt as broken and
proceed with the live migration than be stuck waiting for the current
drive mirror backup to finish.

The drive-mirror command already includes a 'force' flag, which libvirt
does not use, although it documented the flag as only being useful to
quit a job which is paused.  However, since quitting a paused job has
the same effect as abandoning a backup in a non-paused job (namely, the
destination file is not in sync, and the command completes immediately),
we can just improve the documentation to make the force flag obviously
useful.

> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
> ---


> +++ b/hmp-commands.hx
> @@ -106,7 +106,8 @@ ETEXI
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "stop an active background block operation (use -f"
> -                      "\n\t\t\t if the operation is currently paused)",
> +                      "\n\t\t\t if you want to abort the operation immediately"
> +                      "\n\t\t\t instead of keep running until data is in sync )",

s/sync )/sync)/

>          .cmd        = hmp_block_job_cancel,
>      },
>  
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 00403d9..4a96c42 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -63,6 +63,12 @@ typedef struct BlockJob {
>      bool cancelled;
>  
>      /**
> +     * Set to true if the job should be abort immediately without waiting

s/be //

> +     * for data is in sync.

s/is/to be/

> +     */
> +    bool force;
> +
> +    /**
>       * Counter for pause request. If non-zero, the block job is either paused,
>       * or if busy == true will pause itself as soon as possible.
>       */
> @@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
>  /**
>   * block_job_cancel:
>   * @job: The job to be canceled.
> + * @force: Quit a job without waiting data is in sync.

s/data is/for data to be/

> +++ b/qapi/block-core.json
> @@ -2098,8 +2098,10 @@
>  #          the name of the parameter), but since QEMU 2.7 it can have
>  #          other values.
>  #
> -# @force: whether to allow cancellation of a paused job (default
> -#         false).  Since 1.3.
> +# @force: #optional whether to allow cancellation a job without waiting data is

The '#optional' tag should no longer be added.

> +#         in sync, please not that since 2.12 it's semantic is not exactly the
> +#         same as before, from 1.3 to 2.11 it means whether to allow cancellation
> +#         of a paused job (default false).  Since 1.3.

Reads awkwardly.  I suggest:

@force: If true, and the job has already emitted the event
BLOCK_JOB_READY, abandon the job immediately (even if it is paused)
instead of waiting for the destination to complete its final
synchronization (since 1.3)


> +++ b/tests/test-blockjob-txn.c
> @@ -125,7 +125,7 @@ static void test_single_job(int expected)
>      block_job_start(job);
>  
>      if (expected == -ECANCELED) {
> -        block_job_cancel(job);
> +        block_job_cancel(job, false);
>      }
>  
>      while (result == -EINPROGRESS) {
> @@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int expected2)
>      block_job_txn_unref(txn);
>  
>      if (expected1 == -ECANCELED) {
> -        block_job_cancel(job1);
> +        block_job_cancel(job1, false);
>      }
>      if (expected2 == -ECANCELED) {
> -        block_job_cancel(job2);
> +        block_job_cancel(job2, false);
>      }
>  
>      while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
> @@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
>      block_job_start(job1);
>      block_job_start(job2);
>  
> -    block_job_cancel(job1);
> +    block_job_cancel(job1, false);
>  
>      /* Now make job2 finish before the main loop kicks jobs.  This simulates
>       * the race between a pending kick and another job completing.
> 

The testsuite should probably also test block_job_cancel(..., true).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[John Snow]

On 01/30/2018 03:38 AM, Liang Li wrote:
> When doing drive mirror to a low speed shared storage, if there was heavy
> BLK IO write workload in VM after the 'ready' event, drive mirror block job
> can't be canceled immediately, it would keep running until the heavy BLK IO
> workload stopped in the VM.
> 
> Because libvirt depends on block-job-cancel for block live migration, the
> current block-job-cancel has the semantic to make sure data is in sync after
> the 'ready' event.  This semantic can't meet some requirement, for example,
> people may use drive mirror for realtime backup while need the ability of
> block live migration. If drive mirror can't not be cancelled immediately,
> it means block live migration need to wait, because libvirt make use drive
> mirror to implement block live migration and only one drive mirror block
> job is allowed at the same time for a give block dev.
> 
> We need a new interface for 'force cancel', which could quit block job
> immediately if don't care about whether data is in sync or not.
> 
> 'force' is not used by libvirt currently, to make things simple, change
> it's semantic slightly, hope it will not break some use case which need its
> original semantic.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Liang Li <liliangleo@didichuxing.com>

Just a note to JTC that this will conflict with my two series trying to
refactor jobs:

(1) [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle
(2) [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management​

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[John Snow]

On 01/30/2018 03:38 AM, Liang Li wrote:
> When doing drive mirror to a low speed shared storage, if there was heavy
> BLK IO write workload in VM after the 'ready' event, drive mirror block job
> can't be canceled immediately, it would keep running until the heavy BLK IO
> workload stopped in the VM.
> 
> Because libvirt depends on block-job-cancel for block live migration, the
> current block-job-cancel has the semantic to make sure data is in sync after
> the 'ready' event.  This semantic can't meet some requirement, for example,
> people may use drive mirror for realtime backup while need the ability of
> block live migration. If drive mirror can't not be cancelled immediately,
> it means block live migration need to wait, because libvirt make use drive
> mirror to implement block live migration and only one drive mirror block
> job is allowed at the same time for a give block dev.
> 
> We need a new interface for 'force cancel', which could quit block job
> immediately if don't care about whether data is in sync or not.
> 
> 'force' is not used by libvirt currently, to make things simple, change
> it's semantic slightly, hope it will not break some use case which need its
> original semantic.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
> ---
>  block/mirror.c            |  9 +++------
>  blockdev.c                |  4 ++--
>  blockjob.c                | 11 ++++++-----
>  hmp-commands.hx           |  3 ++-
>  include/block/blockjob.h  |  9 ++++++++-
>  qapi/block-core.json      |  6 ++++--
>  tests/test-blockjob-txn.c |  8 ++++----
>  7 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index c9badc1..c22dff9 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
>  
>          ret = 0;
>          trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
> -        if (!s->synced) {
> -            block_job_sleep_ns(&s->common, delay_ns);
> -            if (block_job_is_cancelled(&s->common)) {
> -                break;
> -            }
> +        if (block_job_is_cancelled(&s->common) && s->common.force) {
> +            break;

what's the justification for removing the sleep in the case that
!s->synced && !block_job_is_cancelled(...) ?

>          } else if (!should_complete) {
>              delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
>              block_job_sleep_ns(&s->common, delay_ns);
> @@ -887,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->synced && block_job_is_cancelled(&s->common)));
> +        assert(ret < 0 || block_job_is_cancelled(&s->common));

This assertion gets weaker in the case where force isn't provided, is
that desired?

>          assert(need_drain);
>          mirror_wait_for_all_io(s);
>      }
> diff --git a/blockdev.c b/blockdev.c
> index 8e977ee..039f156 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>          aio_context_acquire(aio_context);
>  
>          if (bs->job) {
> -            block_job_cancel(bs->job);
> +            block_job_cancel(bs->job, false);
>          }
>  
>          aio_context_release(aio_context);
> @@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
>      }
>  
>      trace_qmp_block_job_cancel(job);
> -    block_job_cancel(job);
> +    block_job_cancel(job, force);
>  out:
>      aio_context_release(aio_context);
>  }
> diff --git a/blockjob.c b/blockjob.c
> index f5cea84..0aacb50 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
>      block_job_unref(job);
>  }
>  
> -static void block_job_cancel_async(BlockJob *job)
> +static void block_job_cancel_async(BlockJob *job, bool force)
>  {
>      if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
>          block_job_iostatus_reset(job);
> @@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
>          job->pause_count--;
>      }
>      job->cancelled = true;
> +    job->force = force;
>  }
>  

I suppose this is okay; we're setting a permanent property of the job as
part of a limited operation.

Granted, the job should disappear afterwards, so it should never
conflict, but it made me wonder for a moment.

What happens if we cancel with force = true and then immediately cancel
again with force = false? What happens? Can it cause issues?

>  static int block_job_finish_sync(BlockJob *job,
> @@ -437,7 +438,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);
> +            block_job_cancel_async(other_job, true);

What's the rationale for deciding that transactional cancellations are
always force=true?

Granted, this doesn't matter currently because mirror isn't a
transactional job, but that makes the decision all the more curious.

>          }
>      }
>      while (!QLIST_EMPTY(&txn->jobs)) {
> @@ -542,10 +543,10 @@ void block_job_user_resume(BlockJob *job)
>      }
>  }
>  
> -void block_job_cancel(BlockJob *job)
> +void block_job_cancel(BlockJob *job, bool force)
>  {
>      if (block_job_started(job)) {
> -        block_job_cancel_async(job);
> +        block_job_cancel_async(job, force);
>          block_job_enter(job);
>      } else {
>          block_job_completed(job, -ECANCELED);
> @@ -557,7 +558,7 @@ void block_job_cancel(BlockJob *job)
>   * function pointer casts there. */
>  static void block_job_cancel_err(BlockJob *job, Error **errp)
>  {
> -    block_job_cancel(job);
> +    block_job_cancel(job, false);
>  }
>  
>  int block_job_cancel_sync(BlockJob *job)
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 15620c9..c8bb414 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -106,7 +106,8 @@ ETEXI
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "stop an active background block operation (use -f"
> -                      "\n\t\t\t if the operation is currently paused)",
> +                      "\n\t\t\t if you want to abort the operation immediately"
> +                      "\n\t\t\t instead of keep running until data is in sync )",
>          .cmd        = hmp_block_job_cancel,
>      },
>  
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 00403d9..4a96c42 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -63,6 +63,12 @@ typedef struct BlockJob {
>      bool cancelled;
>  
>      /**
> +     * Set to true if the job should be abort immediately without waiting
> +     * for data is in sync.
> +     */
> +    bool force;
> +
> +    /**
>       * Counter for pause request. If non-zero, the block job is either paused,
>       * or if busy == true will pause itself as soon as possible.
>       */
> @@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
>  /**
>   * block_job_cancel:
>   * @job: The job to be canceled.
> + * @force: Quit a job without waiting data is in sync.
>   *
>   * Asynchronously cancel the specified job.
>   */
> -void block_job_cancel(BlockJob *job);
> +void block_job_cancel(BlockJob *job, bool force);
>  
>  /**
>   * block_job_complete:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308..7c4dbfe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2098,8 +2098,10 @@
>  #          the name of the parameter), but since QEMU 2.7 it can have
>  #          other values.
>  #
> -# @force: whether to allow cancellation of a paused job (default
> -#         false).  Since 1.3.
> +# @force: #optional whether to allow cancellation a job without waiting data is
> +#         in sync, please not that since 2.12 it's semantic is not exactly the
> +#         same as before, from 1.3 to 2.11 it means whether to allow cancellation
> +#         of a paused job (default false).  Since 1.3.
>  #
>  # Returns: Nothing on success
>  #          If no background operation is active on this device, DeviceNotActive
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index 3591c96..53daadb 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -125,7 +125,7 @@ static void test_single_job(int expected)
>      block_job_start(job);
>  
>      if (expected == -ECANCELED) {
> -        block_job_cancel(job);
> +        block_job_cancel(job, false);
>      }
>  
>      while (result == -EINPROGRESS) {
> @@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int expected2)
>      block_job_txn_unref(txn);
>  
>      if (expected1 == -ECANCELED) {
> -        block_job_cancel(job1);
> +        block_job_cancel(job1, false);
>      }
>      if (expected2 == -ECANCELED) {
> -        block_job_cancel(job2);
> +        block_job_cancel(job2, false);
>      }
>  
>      while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
> @@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
>      block_job_start(job1);
>      block_job_start(job2);
>  
> -    block_job_cancel(job1);
> +    block_job_cancel(job1, false);
>  
>      /* Now make job2 finish before the main loop kicks jobs.  This simulates
>       * the race between a pending kick and another job completing.
> 

--js

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[John Snow]

On 01/30/2018 03:18 PM, John Snow wrote:
> 
> 
> On 01/30/2018 03:38 AM, Liang Li wrote:
>> When doing drive mirror to a low speed shared storage, if there was heavy
>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
>> can't be canceled immediately, it would keep running until the heavy BLK IO
>> workload stopped in the VM.
>>
>> Because libvirt depends on block-job-cancel for block live migration, the
>> current block-job-cancel has the semantic to make sure data is in sync after
>> the 'ready' event.  This semantic can't meet some requirement, for example,
>> people may use drive mirror for realtime backup while need the ability of
>> block live migration. If drive mirror can't not be cancelled immediately,
>> it means block live migration need to wait, because libvirt make use drive
>> mirror to implement block live migration and only one drive mirror block
>> job is allowed at the same time for a give block dev.
>>
>> We need a new interface for 'force cancel', which could quit block job
>> immediately if don't care about whether data is in sync or not.
>>
>> 'force' is not used by libvirt currently, to make things simple, change
>> it's semantic slightly, hope it will not break some use case which need its
>> original semantic.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Jeff Cody <jcody@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: John Snow <jsnow@redhat.com>
>> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
>> ---
>>  block/mirror.c            |  9 +++------
>>  blockdev.c                |  4 ++--
>>  blockjob.c                | 11 ++++++-----
>>  hmp-commands.hx           |  3 ++-
>>  include/block/blockjob.h  |  9 ++++++++-
>>  qapi/block-core.json      |  6 ++++--
>>  tests/test-blockjob-txn.c |  8 ++++----
>>  7 files changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index c9badc1..c22dff9 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
>>  
>>          ret = 0;
>>          trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>> -        if (!s->synced) {
>> -            block_job_sleep_ns(&s->common, delay_ns);
>> -            if (block_job_is_cancelled(&s->common)) {
>> -                break;
>> -            }
>> +        if (block_job_is_cancelled(&s->common) && s->common.force) {
>> +            break;
> 
> what's the justification for removing the sleep in the case that
> !s->synced && !block_job_is_cancelled(...) ?
> 
>>          } else if (!should_complete) {
>>              delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
>>              block_job_sleep_ns(&s->common, delay_ns);
>> @@ -887,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->synced && block_job_is_cancelled(&s->common)));
>> +        assert(ret < 0 || block_job_is_cancelled(&s->common));
> 
> This assertion gets weaker in the case where force isn't provided, is
> that desired?
> 
>>          assert(need_drain);
>>          mirror_wait_for_all_io(s);
>>      }
>> diff --git a/blockdev.c b/blockdev.c
>> index 8e977ee..039f156 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>>          aio_context_acquire(aio_context);
>>  
>>          if (bs->job) {
>> -            block_job_cancel(bs->job);
>> +            block_job_cancel(bs->job, false);
>>          }
>>  
>>          aio_context_release(aio_context);
>> @@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
>>      }
>>  
>>      trace_qmp_block_job_cancel(job);
>> -    block_job_cancel(job);
>> +    block_job_cancel(job, force);
>>  out:
>>      aio_context_release(aio_context);
>>  }
>> diff --git a/blockjob.c b/blockjob.c
>> index f5cea84..0aacb50 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
>>      block_job_unref(job);
>>  }
>>  
>> -static void block_job_cancel_async(BlockJob *job)
>> +static void block_job_cancel_async(BlockJob *job, bool force)
>>  {
>>      if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
>>          block_job_iostatus_reset(job);
>> @@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
>>          job->pause_count--;
>>      }
>>      job->cancelled = true;
>> +    job->force = force;
>>  }
>>  
> 
> I suppose this is okay; we're setting a permanent property of the job as
> part of a limited operation.
> 
> Granted, the job should disappear afterwards, so it should never
> conflict, but it made me wonder for a moment.
> 
> What happens if we cancel with force = true and then immediately cancel
> again with force = false? What happens? Can it cause issues?
> 
>>  static int block_job_finish_sync(BlockJob *job,
>> @@ -437,7 +438,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);
>> +            block_job_cancel_async(other_job, true);
> 
> What's the rationale for deciding that transactional cancellations are
> always force=true?
> 
> Granted, this doesn't matter currently because mirror isn't a
> transactional job, but that makes the decision all the more curious.
> 
>>          }
>>      }
>>      while (!QLIST_EMPTY(&txn->jobs)) {
>> @@ -542,10 +543,10 @@ void block_job_user_resume(BlockJob *job)
>>      }
>>  }
>>  
>> -void block_job_cancel(BlockJob *job)
>> +void block_job_cancel(BlockJob *job, bool force)
>>  {
>>      if (block_job_started(job)) {
>> -        block_job_cancel_async(job);
>> +        block_job_cancel_async(job, force);
>>          block_job_enter(job);
>>      } else {
>>          block_job_completed(job, -ECANCELED);
>> @@ -557,7 +558,7 @@ void block_job_cancel(BlockJob *job)
>>   * function pointer casts there. */
>>  static void block_job_cancel_err(BlockJob *job, Error **errp)
>>  {
>> -    block_job_cancel(job);
>> +    block_job_cancel(job, false);
>>  }
>>  
>>  int block_job_cancel_sync(BlockJob *job)
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 15620c9..c8bb414 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -106,7 +106,8 @@ ETEXI
>>          .args_type  = "force:-f,device:B",
>>          .params     = "[-f] device",
>>          .help       = "stop an active background block operation (use -f"
>> -                      "\n\t\t\t if the operation is currently paused)",
>> +                      "\n\t\t\t if you want to abort the operation immediately"
>> +                      "\n\t\t\t instead of keep running until data is in sync )",
>>          .cmd        = hmp_block_job_cancel,
>>      },
>>  
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 00403d9..4a96c42 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -63,6 +63,12 @@ typedef struct BlockJob {
>>      bool cancelled;
>>  
>>      /**
>> +     * Set to true if the job should be abort immediately without waiting
>> +     * for data is in sync.
>> +     */
>> +    bool force;
>> +
>> +    /**
>>       * Counter for pause request. If non-zero, the block job is either paused,
>>       * or if busy == true will pause itself as soon as possible.
>>       */
>> @@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
>>  /**
>>   * block_job_cancel:
>>   * @job: The job to be canceled.
>> + * @force: Quit a job without waiting data is in sync.
>>   *
>>   * Asynchronously cancel the specified job.
>>   */
>> -void block_job_cancel(BlockJob *job);
>> +void block_job_cancel(BlockJob *job, bool force);
>>  
>>  /**
>>   * block_job_complete:
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 8225308..7c4dbfe 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2098,8 +2098,10 @@
>>  #          the name of the parameter), but since QEMU 2.7 it can have
>>  #          other values.
>>  #
>> -# @force: whether to allow cancellation of a paused job (default
>> -#         false).  Since 1.3.
>> +# @force: #optional whether to allow cancellation a job without waiting data is
>> +#         in sync, please not that since 2.12 it's semantic is not exactly the
>> +#         same as before, from 1.3 to 2.11 it means whether to allow cancellation
>> +#         of a paused job (default false).  Since 1.3.
>>  #
>>  # Returns: Nothing on success
>>  #          If no background operation is active on this device, DeviceNotActive
>> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
>> index 3591c96..53daadb 100644
>> --- a/tests/test-blockjob-txn.c
>> +++ b/tests/test-blockjob-txn.c
>> @@ -125,7 +125,7 @@ static void test_single_job(int expected)
>>      block_job_start(job);
>>  
>>      if (expected == -ECANCELED) {
>> -        block_job_cancel(job);
>> +        block_job_cancel(job, false);
>>      }
>>  
>>      while (result == -EINPROGRESS) {
>> @@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int expected2)
>>      block_job_txn_unref(txn);
>>  
>>      if (expected1 == -ECANCELED) {
>> -        block_job_cancel(job1);
>> +        block_job_cancel(job1, false);
>>      }
>>      if (expected2 == -ECANCELED) {
>> -        block_job_cancel(job2);
>> +        block_job_cancel(job2, false);
>>      }
>>  
>>      while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
>> @@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
>>      block_job_start(job1);
>>      block_job_start(job2);
>>  
>> -    block_job_cancel(job1);
>> +    block_job_cancel(job1, false);
>>  
>>      /* Now make job2 finish before the main loop kicks jobs.  This simulates
>>       * the race between a pending kick and another job completing.
>>
> 
> --js
> 

This patch also causes an interesting regression in iotest 185:

185 1s ... - output mismatch (see 185.out.bad)
--- /home/bos/jhuston/src/qemu/tests/qemu-iotests/185.out	2017-12-21
16:15:50.879455552 -0500
+++ /home/bos/jhuston/src/qemu/bin/git/tests/qemu-iotests/185.out.bad
2018-01-30 15:38:47.936925014 -0500
@@ -28,16 +28,18 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len":
4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304,
"offset": 4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len":
4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}

 === Start mirror job and exit qemu ===

 {"return": {}}
 Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864
cluster_size=65536 lazy_refcounts=off refcount_bits=16
 {"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304,
"offset": 4194304, "speed": 65536, "type": "mirror"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len":
4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len":
4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Liang Li]

On Mon, Feb 05, 2018 at 02:28:55PM -0500, John Snow wrote:
> 
> 
> On 01/31/2018 09:19 PM, Liang Li wrote:
> > On Tue, Jan 30, 2018 at 03:18:31PM -0500, John Snow wrote:
> >>
> >>
> >> On 01/30/2018 03:38 AM, Liang Li wrote:
> >>> When doing drive mirror to a low speed shared storage, if there was heavy
> >>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
> >>> can't be canceled immediately, it would keep running until the heavy BLK IO
> >>> workload stopped in the VM.
> >>>
> >>> Because libvirt depends on block-job-cancel for block live migration, the
> >>> current block-job-cancel has the semantic to make sure data is in sync after
> >>> the 'ready' event.  This semantic can't meet some requirement, for example,
> >>> people may use drive mirror for realtime backup while need the ability of
> >>> block live migration. If drive mirror can't not be cancelled immediately,
> >>> it means block live migration need to wait, because libvirt make use drive
> >>> mirror to implement block live migration and only one drive mirror block
> >>> job is allowed at the same time for a give block dev.
> >>>
> >>> We need a new interface for 'force cancel', which could quit block job
> >>> immediately if don't care about whether data is in sync or not.
> >>>
> >>> 'force' is not used by libvirt currently, to make things simple, change
> >>> it's semantic slightly, hope it will not break some use case which need its
> >>> original semantic.
> >>>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Jeff Cody <jcody@redhat.com>
> >>> Cc: Kevin Wolf <kwolf@redhat.com>
> >>> Cc: Max Reitz <mreitz@redhat.com>
> >>> Cc: Eric Blake <eblake@redhat.com>
> >>> Cc: John Snow <jsnow@redhat.com>
> >>> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
> >>> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
> >>> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
> >>> ---
> >>> block/mirror.c            |  9 +++------
> >>> blockdev.c                |  4 ++--
> >>> blockjob.c                | 11 ++++++-----
> >>> hmp-commands.hx           |  3 ++-
> >>> include/block/blockjob.h  |  9 ++++++++-
> >>> qapi/block-core.json      |  6 ++++--
> >>> tests/test-blockjob-txn.c |  8 ++++----
> >>> 7 files changed, 29 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/block/mirror.c b/block/mirror.c
> >>> index c9badc1..c22dff9 100644
> >>> --- a/block/mirror.c
> >>> +++ b/block/mirror.c
> >>> @@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
> >>>
> >>>         ret = 0;
> >>>         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
> >>> -        if (!s->synced) {
> >>> -            block_job_sleep_ns(&s->common, delay_ns);
> >>> -            if (block_job_is_cancelled(&s->common)) {
> >>> -                break;
> >>> -            }
> >>> +        if (block_job_is_cancelled(&s->common) && s->common.force) {
> >>> +            break;
> >>
> >> what's the justification for removing the sleep in the case that
> >> !s->synced && !block_job_is_cancelled(...) ?
> >>
> > if !block_job_is_cancelled() satisfied, the code in 'if (!should_complete) {}'
> > will execute, there is a block_job_sleep_ns there.
> > 
> > block_job_sleep_ns is for rate throttling, if there is no more data to sync, 
> > sleep is not needed, right?
> > 
> >>>         } else if (!should_complete) {
> >>>             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
> >>>             block_job_sleep_ns(&s->common, delay_ns);
> >>> @@ -887,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->synced && block_job_is_cancelled(&s->common)));
> >>> +        assert(ret < 0 || block_job_is_cancelled(&s->common));
> >>
> >> This assertion gets weaker in the case where force isn't provided, is
> >> that desired?
> >>
> > yes. if force quit is used, the following condition can be true
> > 
> > (ret >= 0) && (s->synced) && (block_job_is_cancelled(&s->common)) 
> > 
> > so the above assert should be changed, or it will be failed.
> > 
> 
> What I mean is that when we *don't* use force quit, the assertion here
> is weakened. You can work the force conditional in here:
> 
> ret < 0 || (block_job_is_cancelled(job) && (job->force || !s->synced))
> 
> which preserves the stricter assertion when force isn't provided.
> 

you are right, will change.
> >>>         assert(need_drain);
> >>>         mirror_wait_for_all_io(s);
> >>>     }
> >>> diff --git a/blockdev.c b/blockdev.c
> >>> index 8e977ee..039f156 100644
> >>> --- a/blockdev.c
> >>> +++ b/blockdev.c
> >>> @@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
> >>>         aio_context_acquire(aio_context);
> >>>
> >>>         if (bs->job) {
> >>> -            block_job_cancel(bs->job);
> >>> +            block_job_cancel(bs->job, false);
> >>>         }
> >>>
> >>>         aio_context_release(aio_context);
> >>> @@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
> >>>     }
> >>>
> >>>     trace_qmp_block_job_cancel(job);
> >>> -    block_job_cancel(job);
> >>> +    block_job_cancel(job, force);
> >>> out:
> >>>     aio_context_release(aio_context);
> >>> }
> >>> diff --git a/blockjob.c b/blockjob.c
> >>> index f5cea84..0aacb50 100644
> >>> --- a/blockjob.c
> >>> +++ b/blockjob.c
> >>> @@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
> >>>     block_job_unref(job);
> >>> }
> >>>
> >>> -static void block_job_cancel_async(BlockJob *job)
> >>> +static void block_job_cancel_async(BlockJob *job, bool force)
> >>> {
> >>>     if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
> >>>         block_job_iostatus_reset(job);
> >>> @@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
> >>>         job->pause_count--;
> >>>     }
> >>>     job->cancelled = true;
> >>> +    job->force = force;
> >>> }
> >>>
> >>
> >> I suppose this is okay; we're setting a permanent property of the job as
> >> part of a limited operation.
> >>
> >> Granted, the job should disappear afterwards, so it should never
> >> conflict, but it made me wonder for a moment.
> >>
> >> What happens if we cancel with force = true and then immediately cancel
> >> again with force = false? What happens? Can it cause issues?
> >>
> > 
> > Indeed. It can be fixed by:
> > 
> > if (!job->force)
> >    job->force = force
> > 
> > it's that ok ?
> > 
> 
> I think so, yes. or job->force |= force, or your preferred equivalent
> for only allowing false-->true here.
> 
> >>> static int block_job_finish_sync(BlockJob *job,
> >>> @@ -437,7 +438,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);
> >>> +            block_job_cancel_async(other_job, true);
> >>
> >> What's the rationale for deciding that transactional cancellations are
> >> always force=true?
> >>
> > 
> > no particular reason, just try to speed up the abort process. :)
> > 
> 
> I'd prefer we not change the semantics of how transactions fail without
> a stronger justification than wanting to make it faster!
> 

OK.  I will send the v2, thanks!


Liang
> >> Granted, this doesn't matter currently because mirror isn't a
> >> transactional job, but that makes the decision all the more curious.
> >>
> >>>         }
> >>>     }
> > 
> > Thanks for your comments.
> > 
> > Liang
> > 

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[John Snow]

On 01/31/2018 09:19 PM, Liang Li wrote:
> On Tue, Jan 30, 2018 at 03:18:31PM -0500, John Snow wrote:
>>
>>
>> On 01/30/2018 03:38 AM, Liang Li wrote:
>>> When doing drive mirror to a low speed shared storage, if there was heavy
>>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
>>> can't be canceled immediately, it would keep running until the heavy BLK IO
>>> workload stopped in the VM.
>>>
>>> Because libvirt depends on block-job-cancel for block live migration, the
>>> current block-job-cancel has the semantic to make sure data is in sync after
>>> the 'ready' event.  This semantic can't meet some requirement, for example,
>>> people may use drive mirror for realtime backup while need the ability of
>>> block live migration. If drive mirror can't not be cancelled immediately,
>>> it means block live migration need to wait, because libvirt make use drive
>>> mirror to implement block live migration and only one drive mirror block
>>> job is allowed at the same time for a give block dev.
>>>
>>> We need a new interface for 'force cancel', which could quit block job
>>> immediately if don't care about whether data is in sync or not.
>>>
>>> 'force' is not used by libvirt currently, to make things simple, change
>>> it's semantic slightly, hope it will not break some use case which need its
>>> original semantic.
>>>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Jeff Cody <jcody@redhat.com>
>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>> Cc: Max Reitz <mreitz@redhat.com>
>>> Cc: Eric Blake <eblake@redhat.com>
>>> Cc: John Snow <jsnow@redhat.com>
>>> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
>>> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
>>> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
>>> ---
>>> block/mirror.c            |  9 +++------
>>> blockdev.c                |  4 ++--
>>> blockjob.c                | 11 ++++++-----
>>> hmp-commands.hx           |  3 ++-
>>> include/block/blockjob.h  |  9 ++++++++-
>>> qapi/block-core.json      |  6 ++++--
>>> tests/test-blockjob-txn.c |  8 ++++----
>>> 7 files changed, 29 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index c9badc1..c22dff9 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
>>>
>>>         ret = 0;
>>>         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>>> -        if (!s->synced) {
>>> -            block_job_sleep_ns(&s->common, delay_ns);
>>> -            if (block_job_is_cancelled(&s->common)) {
>>> -                break;
>>> -            }
>>> +        if (block_job_is_cancelled(&s->common) && s->common.force) {
>>> +            break;
>>
>> what's the justification for removing the sleep in the case that
>> !s->synced && !block_job_is_cancelled(...) ?
>>
> if !block_job_is_cancelled() satisfied, the code in 'if (!should_complete) {}'
> will execute, there is a block_job_sleep_ns there.
> 
> block_job_sleep_ns is for rate throttling, if there is no more data to sync, 
> sleep is not needed, right?
> 
>>>         } else if (!should_complete) {
>>>             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
>>>             block_job_sleep_ns(&s->common, delay_ns);
>>> @@ -887,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->synced && block_job_is_cancelled(&s->common)));
>>> +        assert(ret < 0 || block_job_is_cancelled(&s->common));
>>
>> This assertion gets weaker in the case where force isn't provided, is
>> that desired?
>>
> yes. if force quit is used, the following condition can be true
> 
> (ret >= 0) && (s->synced) && (block_job_is_cancelled(&s->common)) 
> 
> so the above assert should be changed, or it will be failed.
> 

What I mean is that when we *don't* use force quit, the assertion here
is weakened. You can work the force conditional in here:

ret < 0 || (block_job_is_cancelled(job) && (job->force || !s->synced))

which preserves the stricter assertion when force isn't provided.

>>>         assert(need_drain);
>>>         mirror_wait_for_all_io(s);
>>>     }
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 8e977ee..039f156 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>>>         aio_context_acquire(aio_context);
>>>
>>>         if (bs->job) {
>>> -            block_job_cancel(bs->job);
>>> +            block_job_cancel(bs->job, false);
>>>         }
>>>
>>>         aio_context_release(aio_context);
>>> @@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
>>>     }
>>>
>>>     trace_qmp_block_job_cancel(job);
>>> -    block_job_cancel(job);
>>> +    block_job_cancel(job, force);
>>> out:
>>>     aio_context_release(aio_context);
>>> }
>>> diff --git a/blockjob.c b/blockjob.c
>>> index f5cea84..0aacb50 100644
>>> --- a/blockjob.c
>>> +++ b/blockjob.c
>>> @@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
>>>     block_job_unref(job);
>>> }
>>>
>>> -static void block_job_cancel_async(BlockJob *job)
>>> +static void block_job_cancel_async(BlockJob *job, bool force)
>>> {
>>>     if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
>>>         block_job_iostatus_reset(job);
>>> @@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
>>>         job->pause_count--;
>>>     }
>>>     job->cancelled = true;
>>> +    job->force = force;
>>> }
>>>
>>
>> I suppose this is okay; we're setting a permanent property of the job as
>> part of a limited operation.
>>
>> Granted, the job should disappear afterwards, so it should never
>> conflict, but it made me wonder for a moment.
>>
>> What happens if we cancel with force = true and then immediately cancel
>> again with force = false? What happens? Can it cause issues?
>>
> 
> Indeed. It can be fixed by:
> 
> if (!job->force)
>    job->force = force
> 
> it's that ok ?
> 

I think so, yes. or job->force |= force, or your preferred equivalent
for only allowing false-->true here.

>>> static int block_job_finish_sync(BlockJob *job,
>>> @@ -437,7 +438,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);
>>> +            block_job_cancel_async(other_job, true);
>>
>> What's the rationale for deciding that transactional cancellations are
>> always force=true?
>>
> 
> no particular reason, just try to speed up the abort process. :)
> 

I'd prefer we not change the semantics of how transactions fail without
a stronger justification than wanting to make it faster!

>> Granted, this doesn't matter currently because mirror isn't a
>> transactional job, but that makes the decision all the more curious.
>>
>>>         }
>>>     }
> 
> Thanks for your comments.
> 
> Liang
> 

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Liang Li]

On Tue, Jan 30, 2018 at 03:18:31PM -0500, John Snow wrote:
> 
> 
> On 01/30/2018 03:38 AM, Liang Li wrote:
>> When doing drive mirror to a low speed shared storage, if there was heavy
>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
>> can't be canceled immediately, it would keep running until the heavy BLK IO
>> workload stopped in the VM.
>> 
>> Because libvirt depends on block-job-cancel for block live migration, the
>> current block-job-cancel has the semantic to make sure data is in sync after
>> the 'ready' event.  This semantic can't meet some requirement, for example,
>> people may use drive mirror for realtime backup while need the ability of
>> block live migration. If drive mirror can't not be cancelled immediately,
>> it means block live migration need to wait, because libvirt make use drive
>> mirror to implement block live migration and only one drive mirror block
>> job is allowed at the same time for a give block dev.
>> 
>> We need a new interface for 'force cancel', which could quit block job
>> immediately if don't care about whether data is in sync or not.
>> 
>> 'force' is not used by libvirt currently, to make things simple, change
>> it's semantic slightly, hope it will not break some use case which need its
>> original semantic.
>> 
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Jeff Cody <jcody@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: John Snow <jsnow@redhat.com>
>> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
>> ---
>> block/mirror.c            |  9 +++------
>> blockdev.c                |  4 ++--
>> blockjob.c                | 11 ++++++-----
>> hmp-commands.hx           |  3 ++-
>> include/block/blockjob.h  |  9 ++++++++-
>> qapi/block-core.json      |  6 ++++--
>> tests/test-blockjob-txn.c |  8 ++++----
>> 7 files changed, 29 insertions(+), 21 deletions(-)
>> 
>> diff --git a/block/mirror.c b/block/mirror.c
>> index c9badc1..c22dff9 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -869,11 +869,8 @@ static void coroutine_fn mirror_run(void *opaque)
>> 
>>         ret = 0;
>>         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>> -        if (!s->synced) {
>> -            block_job_sleep_ns(&s->common, delay_ns);
>> -            if (block_job_is_cancelled(&s->common)) {
>> -                break;
>> -            }
>> +        if (block_job_is_cancelled(&s->common) && s->common.force) {
>> +            break;
> 
> what's the justification for removing the sleep in the case that
> !s->synced && !block_job_is_cancelled(...) ?
> 
if !block_job_is_cancelled() satisfied, the code in 'if (!should_complete) {}'
will execute, there is a block_job_sleep_ns there.

block_job_sleep_ns is for rate throttling, if there is no more data to sync, 
sleep is not needed, right?

>>         } else if (!should_complete) {
>>             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
>>             block_job_sleep_ns(&s->common, delay_ns);
>> @@ -887,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->synced && block_job_is_cancelled(&s->common)));
>> +        assert(ret < 0 || block_job_is_cancelled(&s->common));
> 
> This assertion gets weaker in the case where force isn't provided, is
> that desired?
> 
yes. if force quit is used, the following condition can be true

(ret >= 0) && (s->synced) && (block_job_is_cancelled(&s->common)) 

so the above assert should be changed, or it will be failed.

>>         assert(need_drain);
>>         mirror_wait_for_all_io(s);
>>     }
>> diff --git a/blockdev.c b/blockdev.c
>> index 8e977ee..039f156 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -145,7 +145,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>>         aio_context_acquire(aio_context);
>> 
>>         if (bs->job) {
>> -            block_job_cancel(bs->job);
>> +            block_job_cancel(bs->job, false);
>>         }
>> 
>>         aio_context_release(aio_context);
>> @@ -3802,7 +3802,7 @@ void qmp_block_job_cancel(const char *device,
>>     }
>> 
>>     trace_qmp_block_job_cancel(job);
>> -    block_job_cancel(job);
>> +    block_job_cancel(job, force);
>> out:
>>     aio_context_release(aio_context);
>> }
>> diff --git a/blockjob.c b/blockjob.c
>> index f5cea84..0aacb50 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -365,7 +365,7 @@ static void block_job_completed_single(BlockJob *job)
>>     block_job_unref(job);
>> }
>> 
>> -static void block_job_cancel_async(BlockJob *job)
>> +static void block_job_cancel_async(BlockJob *job, bool force)
>> {
>>     if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
>>         block_job_iostatus_reset(job);
>> @@ -376,6 +376,7 @@ static void block_job_cancel_async(BlockJob *job)
>>         job->pause_count--;
>>     }
>>     job->cancelled = true;
>> +    job->force = force;
>> }
>> 
> 
> I suppose this is okay; we're setting a permanent property of the job as
> part of a limited operation.
> 
> Granted, the job should disappear afterwards, so it should never
> conflict, but it made me wonder for a moment.
> 
> What happens if we cancel with force = true and then immediately cancel
> again with force = false? What happens? Can it cause issues?
> 

Indeed. It can be fixed by:

if (!job->force)
   job->force = force

it's that ok ?

>> static int block_job_finish_sync(BlockJob *job,
>> @@ -437,7 +438,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);
>> +            block_job_cancel_async(other_job, true);
> 
> What's the rationale for deciding that transactional cancellations are
> always force=true?
> 

no particular reason, just try to speed up the abort process. :)

> Granted, this doesn't matter currently because mirror isn't a
> transactional job, but that makes the decision all the more curious.
> 
>>         }
>>     }

Thanks for your comments.

Liang

Re: [Qemu-devel] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel

[Liang Li]

On Tue, Jan 30, 2018 at 08:20:03AM -0600, Eric Blake wrote:
> On 01/30/2018 02:38 AM, Liang Li wrote:
>> When doing drive mirror to a low speed shared storage, if there was heavy
>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
>> can't be canceled immediately, it would keep running until the heavy BLK IO
>> workload stopped in the VM.
> 
> So far so good.   But the grammar and explanation in the rest of the
> commit is a bit hard to read; let me give a shot at an alternative wording:
> 
> Libvirt depends on the current block-job-cancel semantics, which is that
> when used without a flag after the 'ready' event, the command blocks
> until data is in sync.  However, these semantics are awkward in other
> situations, for example, people may use drive mirror for realtime
> backups while still wanting to use block live migration.  Libvirt cannot
> start a block live migration while another drive mirror is in progress,
> but the user would rather abandon the backup attempt as broken and
> proceed with the live migration than be stuck waiting for the current
> drive mirror backup to finish.
> 
> The drive-mirror command already includes a 'force' flag, which libvirt
> does not use, although it documented the flag as only being useful to
> quit a job which is paused.  However, since quitting a paused job has
> the same effect as abandoning a backup in a non-paused job (namely, the
> destination file is not in sync, and the command completes immediately),
> we can just improve the documentation to make the force flag obviously
> useful.
> 

much better, will include in the v2. Thanks!
>> 
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Jeff Cody <jcody@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: John Snow <jsnow@redhat.com>
>> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
>> Signed-off-by: Liang Li <liliangleo@didichuxing.com>
>> ---
> 
> 
>> +++ b/hmp-commands.hx
>> @@ -106,7 +106,8 @@ ETEXI
>>         .args_type  = "force:-f,device:B",
>>         .params     = "[-f] device",
>>         .help       = "stop an active background block operation (use -f"
>> -                      "\n\t\t\t if the operation is currently paused)",
>> +                      "\n\t\t\t if you want to abort the operation immediately"
>> +                      "\n\t\t\t instead of keep running until data is in sync )",
> 
> s/sync )/sync)/
> 

done
>>         .cmd        = hmp_block_job_cancel,
>>     },
>> 
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 00403d9..4a96c42 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -63,6 +63,12 @@ typedef struct BlockJob {
>>     bool cancelled;
>> 
>>     /**
>> +     * Set to true if the job should be abort immediately without waiting
> 
> s/be //

done
> 
>> +     * for data is in sync.
> 
> s/is/to be/
> 

done
>> +     */
>> +    bool force;
>> +
>> +    /**
>>      * Counter for pause request. If non-zero, the block job is either paused,
>>      * or if busy == true will pause itself as soon as possible.
>>      */
>> @@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
>> /**
>>  * block_job_cancel:
>>  * @job: The job to be canceled.
>> + * @force: Quit a job without waiting data is in sync.
> 
> s/data is/for data to be/
> 

done
>> +++ b/qapi/block-core.json
>> @@ -2098,8 +2098,10 @@
>> #          the name of the parameter), but since QEMU 2.7 it can have
>> #          other values.
>> #
>> -# @force: whether to allow cancellation of a paused job (default
>> -#         false).  Since 1.3.
>> +# @force: #optional whether to allow cancellation a job without waiting data is
> 
> The '#optional' tag should no longer be added.
> 
>> +#         in sync, please not that since 2.12 it's semantic is not exactly the
>> +#         same as before, from 1.3 to 2.11 it means whether to allow cancellation
>> +#         of a paused job (default false).  Since 1.3.
> 
> Reads awkwardly.  I suggest:
> 
> @force: If true, and the job has already emitted the event
> BLOCK_JOB_READY, abandon the job immediately (even if it is paused)
> instead of waiting for the destination to complete its final
> synchronization (since 1.3)
> 

much more clear
> 
>> +++ b/tests/test-blockjob-txn.c
>> @@ -125,7 +125,7 @@ static void test_single_job(int expected)
>>     block_job_start(job);
>> 
>>     if (expected == -ECANCELED) {
>> -        block_job_cancel(job);
>> +        block_job_cancel(job, false);
>>     }
>> 
>>     while (result == -EINPROGRESS) {
>> @@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int expected2)
>>     block_job_txn_unref(txn);
>> 
>>     if (expected1 == -ECANCELED) {
>> -        block_job_cancel(job1);
>> +        block_job_cancel(job1, false);
>>     }
>>     if (expected2 == -ECANCELED) {
>> -        block_job_cancel(job2);
>> +        block_job_cancel(job2, false);
>>     }
>> 
>>     while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
>> @@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
>>     block_job_start(job1);
>>     block_job_start(job2);
>> 
>> -    block_job_cancel(job1);
>> +    block_job_cancel(job1, false);
>> 
>>     /* Now make job2 finish before the main loop kicks jobs.  This simulates
>>      * the race between a pending kick and another job completing.
>> 
> 
> The testsuite should probably also test block_job_cancel(..., true).
> 

will change in v2, thanks!

Liang


> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org