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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3293 bytes --]

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 <vsementsov@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?

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


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