Re: [PATCH v2 06/20] block/block-copy: add max_chunk and max_workers parameters

[Max Reitz <mreitz@redhat.com>]

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

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> They will be used for backup.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-copy.h |  5 +++++
>  block/block-copy.c         | 10 ++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index ada0d99566..600984c733 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -47,6 +47,11 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
>  /*
>   * Run block-copy in a coroutine, return state pointer. If finished early
>   * returns NULL (@cb is called anyway).
> + *
> + * @max_workers means maximum of parallel coroutines to execute sub-requests,
> + * must be > 0.
> + *
> + * @max_chunk means maximum length for one IO operation. Zero means unlimited.
>   */
>  BlockCopyCallState *block_copy_async(BlockCopyState *s,
>                                       int64_t offset, int64_t bytes,

I only just now notice that @max_workers and @max_chunk were already
added in the previous patch, even though they aren’t used there.  Should
we defer adding them until this patch?

> diff --git a/block/block-copy.c b/block/block-copy.c
> index a0477d90f3..4114d1fd25 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -34,6 +34,8 @@ typedef struct BlockCopyCallState {
>      BlockCopyState *s;
>      int64_t offset;
>      int64_t bytes;
> +    int max_workers;
> +    int64_t max_chunk;
>      BlockCopyAsyncCallbackFunc cb;
>  
>      /* State */
> @@ -144,10 +146,11 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>                                               int64_t offset, int64_t bytes)
>  {
>      BlockCopyTask *task;
> +    int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
>  
>      if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
>                                             offset, offset + bytes,
> -                                           s->copy_size, &offset, &bytes))
> +                                           max_chunk, &offset, &bytes))
>      {
>          return NULL;
>      }
> @@ -616,7 +619,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
>          bytes = end - offset;
>  
>          if (!aio && bytes) {
> -            aio = aio_task_pool_new(BLOCK_COPY_MAX_WORKERS);
> +            aio = aio_task_pool_new(call_state->max_workers);
>          }
>  
>          ret = block_copy_task_run(aio, task);
> @@ -695,6 +698,7 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
>          .s = s,
>          .offset = start,
>          .bytes = bytes,
> +        .max_workers = BLOCK_COPY_MAX_WORKERS,
>      };
>  
>      int ret = block_copy_common(&call_state);
> @@ -726,6 +730,8 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
>          .offset = offset,
>          .bytes = bytes,
>          .cb = cb,
> +        .max_workers = max_workers ?: BLOCK_COPY_MAX_WORKERS,

I thought this must be > 0?

> +        .max_chunk = max_chunk,
>      };
>  
>      qemu_coroutine_enter(co);
> 

And I now notice that there’s no newline after block_copy_async().
(Doesn’t concern this patch, of course, but the previous one.)

Max


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