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

[Eric Blake]

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 <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
> 
> 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 <address@hidden>

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