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 <ebl...@redhat.com>
> Reviewed-by: John Snow <js...@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 <kw...@redhat.com>
