[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 07/20] mirror: Switch MirrorBlockJob to byte-
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/20] mirror: Switch MirrorBlockJob to byte-based |
Date: |
Wed, 5 Jul 2017 15:18:06 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
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