Re: [PATCH v3 4/5] block/block-copy: refactor task creation

[Max Reitz <mreitz@redhat.com>]

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

On 29.04.20 08:10, Vladimir Sementsov-Ogievskiy wrote:
> Instead of just relying on the comment "Called only on full-dirty
> region" in block_copy_task_create() let's move initial dirty area
> search directly to block_copy_task_create(). Let's also use effective
> bdrv_dirty_bitmap_next_dirty_area instead of looping through all
> non-dirty clusters.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 78 ++++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 35ff9cc3ef..5cf032c4d8 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -106,17 +111,27 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
>      return true;
>  }
>  
> -/* Called only on full-dirty region */
> +/*
> + * Search for the first dirty area in offset/bytes range and create task at
> + * the beginning of it.

Oh, that’s even better.

> + */
>  static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>                                               int64_t offset, int64_t bytes)
>  {
> -    BlockCopyTask *task = g_new(BlockCopyTask, 1);
> +    if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
> +                                           offset, offset + bytes,
> +                                           s->copy_size, &offset, &bytes))
> +    {
> +        return NULL;
> +    }
>  
> +    /* region is dirty, so no existent tasks possible in it */
>      assert(!find_conflicting_task(s, offset, bytes));
>  
>      bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
>      s->in_flight_bytes += bytes;
>  
> +    BlockCopyTask *task = g_new(BlockCopyTask, 1);

This should be declared at the top of the function.

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>      *task = (BlockCopyTask) {
>          .s = s,
>          .offset = offset,


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