Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration

[Max Reitz]

On 07.02.2016 13:46, Fam Zheng wrote:
> On Sat, 02/06 14:24, Max Reitz wrote:
>> On 05.02.2016 03:00, Fam Zheng wrote:
>>> The "pnum < nb_sectors" condition in deciding whether to actually copy
>>> data is unnecessarily strict, and the qiov initialization is
>>> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
>>>
>>> Rewrite mirror_iteration to fix both flaws.
>>>
>>> The output of iotests 109 is updated because we now report the offset
>>> and len slightly differently in mirroring progress.
>>>
>>> Signed-off-by: Fam Zheng <address@hidden>
>>> ---
>>>  block/mirror.c             | 335 
>>> +++++++++++++++++++++++++++------------------
>>>  tests/qemu-iotests/109.out |  80 +++++------
>>>  trace-events               |   1 -
>>>  3 files changed, 243 insertions(+), 173 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 2c0edfa..48cd0b3 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>
>> [...]
>>
>>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
>>>       */
>>>      bdrv_get_backing_filename(s->target, backing_filename,
>>>                                sizeof(backing_filename));
>>> -    if (backing_filename[0] && !s->target->backing) {
>>> -        ret = bdrv_get_info(s->target, &bdi);
>>> -        if (ret < 0) {
>>> -            goto immediate_exit;
>>> -        }
>>> -        if (s->granularity < bdi.cluster_size) {
>>> -            s->buf_size = MAX(s->buf_size, bdi.cluster_size);
>>> -            s->cow_bitmap = bitmap_new(length);
>>> -        }
>>> +    if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
>>
>> This should be bdi.has_cluster_size...
> 
> has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is derived
> from (bdi.cluster_size != 0).

You're right, my bad.

>>> +        target_cluster_size = bdi.cluster_size;
>>
>> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here; but I
>> guess this is already assumed all over the block layer, so it may be
>> fine without.
> 
> Okay, it doesn't hurt to add an assert here.

I'd be happy to take the patch without, too (although I wouldn't decline
a follow-up adding an assertion).

Reviewed-by: Max Reitz <address@hidden>

>>>      }
>>> +    if (backing_filename[0] && !s->target->backing
>>> +        && s->granularity < target_cluster_size) {
>>> +        s->buf_size = MAX(s->buf_size, target_cluster_size);
>>> +        s->cow_bitmap = bitmap_new(length);
>>> +    }
>>> +    s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
>>
>> (this shouldn't be 0, so target_cluster_size needs to be at least
>> BDRV_SECTOR_SIZE)
>>
>> Max
>>
> 
>

signature.asc
Description: OpenPGP digital signature