Re: [PATCH v2 05/20] block/block-copy: implement block_copy_async

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> We'll need async block-copy invocation to use in backup directly.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-copy.h | 13 +++++++++++++
>  block/block-copy.c         | 40 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index 6397505f30..ada0d99566 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -19,7 +19,10 @@
>  #include "qemu/co-shared-resource.h"
>  
>  typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
> +typedef void (*BlockCopyAsyncCallbackFunc)(int ret, bool error_is_read,
> +                                           void *opaque);
>  typedef struct BlockCopyState BlockCopyState;
> +typedef struct BlockCopyCallState BlockCopyCallState;
>  
>  BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>                                       int64_t cluster_size, bool 
> use_copy_range,
> @@ -41,6 +44,16 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
>  int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
>                              bool *error_is_read);
>  
> +/*
> + * Run block-copy in a coroutine, return state pointer. If finished early
> + * returns NULL (@cb is called anyway).

Any special reason for doing so?  Seems like the code would be a tad
simpler if we just returned it either way.  (And off the top of my head
I’d guess it’d be easier for the caller if the returned value was always
non-NULL, too.)

> + */
> +BlockCopyCallState *block_copy_async(BlockCopyState *s,
> +                                     int64_t offset, int64_t bytes,
> +                                     bool ratelimit, int max_workers,
> +                                     int64_t max_chunk,
> +                                     BlockCopyAsyncCallbackFunc cb);
> +
>  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 75882a094c..a0477d90f3 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -34,9 +34,11 @@ typedef struct BlockCopyCallState {
>      BlockCopyState *s;
>      int64_t offset;
>      int64_t bytes;
> +    BlockCopyAsyncCallbackFunc cb;
>  
>      /* State */
>      bool failed;
> +    bool finished;
>  
>      /* OUT parameters */
>      bool error_is_read;
> @@ -676,6 +678,13 @@ static int coroutine_fn 
> block_copy_common(BlockCopyCallState *call_state)
>           */
>      } while (ret > 0);
>  
> +    if (call_state->cb) {
> +        call_state->cb(ret, call_state->error_is_read,
> +                       call_state->s->progress_opaque);

I find it weird to pass progress_opaque here.  Shouldn’t we just have a
dedicated opaque object for this CB?

> +    }
> +
> +    call_state->finished = true;
> +
>      return ret;
>  }
>  
> @@ -697,6 +706,37 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t 
> start, int64_t bytes,
>      return ret;
>  }
>  
> +static void coroutine_fn block_copy_async_co_entry(void *opaque)
> +{
> +    block_copy_common(opaque);
> +}
> +
> +BlockCopyCallState *block_copy_async(BlockCopyState *s,
> +                                     int64_t offset, int64_t bytes,
> +                                     bool ratelimit, int max_workers,
> +                                     int64_t max_chunk,
> +                                     BlockCopyAsyncCallbackFunc cb)
> +{
> +    BlockCopyCallState *call_state = g_new(BlockCopyCallState, 1);
> +    Coroutine *co = qemu_coroutine_create(block_copy_async_co_entry,
> +                                          call_state);
> +
> +    *call_state = (BlockCopyCallState) {
> +        .s = s,
> +        .offset = offset,
> +        .bytes = bytes,
> +        .cb = cb,
> +    };
> +
> +    qemu_coroutine_enter(co);

Do we need/want any already-in-coroutine shenanigans here?

Max

> +
> +    if (call_state->finished) {
> +        g_free(call_state);
> +        return NULL;
> +    }
> +
> +    return call_state;
> +}
>  BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s)
>  {
>      return s->copy_bitmap;
>

signature.asc
Description: OpenPGP digital signature