Re: [Qemu-block] [PATCH v2 6/7] mirror: efficiently zero out target

[Eric Blake]

On 07/07/2016 03:35 AM, Denis V. Lunev wrote:
> With a bdrv_co_write_zeroes method on a target BDS zeroes will not be placed

The commit message doesn't match the code. A target BDS may have
bdrv_co_write_zeroes but still send zeroes across the wire (example: NBD
client that has my patch for using NBD_CMD_WRITE_ZEROES but talking to a
server that did not implement it).  The code is better off for having
checked the per-BDS bdrv_can_write_zeroes_with_unmap(), but maybe you
want to update the commit message to reflect that.

s/BDS zeroes/BDS, zeroes/

> into the wire. Thus the target could be very efficiently zeroed out. This
> is should be done with the largest chunk possible.

s/is should/should/

> 
> Signed-off-by: Denis V. Lunev <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy<address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Fam Zheng <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> CC: Jeff Cody <address@hidden>
> CC: Eric Blake <address@hidden>
> ---
>  block/mirror.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 7208023..4ecfbf1 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -526,8 +526,37 @@ static int mirror_dirty_init(MirrorBlockJob *s)
>      last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>  
>      if (base == NULL && !bdrv_has_zero_init(target_bs)) {
> -        bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
> -        return 0;
> +        if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> +            bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
> +            return 0;
> +        }
> +
> +        for (sector_num = 0; sector_num < end; ) {
> +            int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
> +            int nb_sectors = QEMU_ALIGN_DOWN(INT_MAX, s->granularity);
> +            nb_sectors = MIN(nb_sectors >> BDRV_SECTOR_BITS, end - 
> sector_num);

Why are we scaling sectors down?  Oh, because you did a weird
calculation where you stored bytes in nb_sectors in the line before.
That's confusing.  Can you just write:

int nb_sectors = MIN(end - sector_num,
    QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS);

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature