Re: [Qemu-devel] [PATCH v3 07/20] mirror: Switch MirrorBlockJob to byte-based

[Kevin Wolf <kwolf@redhat.com>]

Am 27.06.2017 um 21:24 hat Eric Blake geschrieben:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Continue by converting an
> internal structure (no semantic change), and all references to the
> buffer size.
> 
> [checkpatch has a false positive on use of MIN() in this patch]
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>

I wouldn't mind an assertion that granularity is a multiple of
BDRV_SECTOR_SIZE, along with a comment that explains that this is
required so that we avoid rounding problems when dealing with the bitmap
functions.

blockdev_mirror_common() does already check this, but it feels like it's
a bit far away from where the actual problem would happen in the mirror
job code.

> @@ -768,17 +765,17 @@ static void coroutine_fn mirror_run(void *opaque)
>       * the destination do COW.  Instead, we copy sectors around the
>       * dirty data if needed.  We need a bitmap to do that.
>       */
> +    s->target_cluster_size = BDRV_SECTOR_SIZE;
>      bdrv_get_backing_filename(target_bs, backing_filename,
>                                sizeof(backing_filename));
>      if (!bdrv_get_info(target_bs, &bdi) && bdi.cluster_size) {
> -        target_cluster_size = bdi.cluster_size;
> +        s->target_cluster_size = bdi.cluster_size;
>      }

Why have the unrelated bdrv_get_backing_filename() between the two
assignments of s->target_cluster_size? Or actually, wouldn't it be
even easier to read with an else branch?

    if (!bdrv_get_info(target_bs, &bdi) && bdi.cluster_size) {
        s->target_cluster_size = bdi.cluster_size;
    } else {
        s->target_cluster_size = BDRV_SECTOR_SIZE;
    }

None of these comments are critical, so anyway:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>