Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

[Vladimir Sementsov-Ogievskiy]

22.07.2020 14:28, Max Reitz wrote:

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:

Add function to cancel running async block-copy call. It will be used
in backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
  include/block/block-copy.h |  7 +++++++
  block/block-copy.c         | 22 +++++++++++++++++++---
  2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index d40e691123..370a194d3c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
  void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
                            uint64_t speed);

+/*

+ * Cancel running block-copy call.
+ * Cancel leaves block-copy state valid: dirty bits are correct and you may use
+ * cancel + <run block_copy with same parameters> to emulate pause/resume.
+ */
+void block_copy_cancel(BlockCopyCallState *call_state);
+
  BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
  void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);

diff --git a/block/block-copy.c b/block/block-copy.c

index 851d9c8aaf..b551feb6c2 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
      bool failed;
      bool finished;
      QemuCoSleepState *sleep_state;
+    bool cancelled;
+    Coroutine *canceller;

/* OUT parameters */

      bool error_is_read;
@@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
      assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
      assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));

- while (bytes && aio_task_pool_status(aio) == 0) {

+    while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
          BlockCopyTask *task;
          int64_t status_bytes;

@@ -693,7 +695,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)

      do {
          ret = block_copy_dirty_clusters(call_state);

- if (ret == 0) {

+        if (ret == 0 && !call_state->cancelled) {
              ret = block_copy_wait_one(call_state->s, call_state->offset,
                                        call_state->bytes);
          }
@@ -707,13 +709,18 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
           * 2. We have waited for some intersecting block-copy request
           *    It may have failed and produced new dirty bits.
           */
-    } while (ret > 0);
+    } while (ret > 0 && !call_state->cancelled);

Would it be cleaner if block_copy_dirty_cluster() just returned
-ECANCELED?  Or would that pose a problem for its callers or the async
callback?

I'd prefer not to merge io ret with block-copy logic: who knows what underlying 
operations may return.. Can't it be _another_ ECANCELED?
And it would be just a sugar for block_copy_dirty_clusters() call, I'll have to 
check ->cancelled after block_copy_wait_one() anyway.
Also, for the next version I try to make it more obvious that finished 
block-copy call is in one of thee states:
 - success
 - failed
 - cancelled

Hmm. Also, cancelled should be OK for copy-on-write operations in filter, it 
just mean that we don't need to care anymore.

      if (call_state->cb) {
          call_state->cb(ret, call_state->error_is_read,
                         call_state->s->progress_opaque);
      }

+ if (call_state->canceller) {

+        aio_co_wake(call_state->canceller);
+        call_state->canceller = NULL;
+    }
+
      call_state->finished = true;

return ret;

--
Best regards,
Vladimir