On 07/05/2017 06:42 AM, Kevin Wolf wrote: > 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.
That goes away later when series two converts the bitmap functions to be
byte-based, but you're right that the intermediate state should be
easier to follow.
>
> 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.
Indeed.
>
>> @@ -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;
> }
Yes, that looks nicer.
>
> None of these comments are critical, so anyway:
>
> Reviewed-by: Kevin Wolf <kw...@redhat.com>
I'm respinning v4 anyways, so I'll make the change (and while it is
small, it's still enough that I'll drop R-b).
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
