Re: [PATCH v2 4/6] block/block-copy: move task size initial calculation to _task_create

[Max Reitz]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> Comment "Called only on full-dirty region" without corresponding
> assertion is a very unsafe thing.

Not sure whether it’s that unsafe for a static function with a single
caller, but, well.

> Adding assertion means call
> bdrv_dirty_bitmap_next_zero twice. Instead, let's move
> bdrv_dirty_bitmap_next_zero call to block_copy_task_create. It also
> allows to drop cur_bytes variable which partly duplicate task->bytes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/block-copy.c | 47 ++++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 63d8468b27..dd406eb4bb 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -106,12 +106,23 @@ static bool coroutine_fn 
> block_copy_wait_one(BlockCopyState *s, int64_t offset,
>      return true;
>  }
>  
> -/* Called only on full-dirty region */
>  static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>                                               int64_t offset, int64_t bytes)

A bit of documentation on the new interface might be nice.  For one
thing, that @offset must be dirty, although there is an assertion that,
well, checks it.  (An assertion doesn’t really check anything, it rather
verifies a contract.  And violation is fatal.)

For another, what the range [offset, offset + bytes) is; namely
basically the whole range of data that we might potentially copy, only
that the head must be dirty, but the tail may be clean.

Which makes me think that the interface is maybe less than intuitive.
It would make more sense if we could just call this function on the
whole region and it would figure out whether @offset is dirty by itself
(and return NULL if it isn’t).

OTOH I suppose the interface how it is here is more useful for
task-ification.  But maybe that should be documented.

>  {
> +    int64_t next_zero;
>      BlockCopyTask *task = g_new(BlockCopyTask, 1);
>  
> +    assert(bdrv_dirty_bitmap_get(s->copy_bitmap, offset));
> +
> +    bytes = MIN(bytes, s->copy_size);
> +    next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset, bytes);
> +    if (next_zero >= 0) {
> +        assert(next_zero > offset); /* offset is dirty */
> +        assert(next_zero < offset + bytes); /* no need to do MIN() */
> +        bytes = next_zero - offset;
> +    }
> +
> +    /* region is dirty, so no existent tasks possible in it */

s/existent/existing/?

(The code movement and how you replaced cur_bytes by task->bytes looks
good.)

Max

>      assert(!find_conflicting_task(s, offset, bytes));
>  
>      bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);

signature.asc
Description: OpenPGP digital signature