Re: [Qemu-devel] [PATCH] mirror: add target-zeroed flag

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3935 bytes --]

On 06/03/2016 08:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add target-zeroed flag to allow user specify that target is already
> zeroed. With this flag set zeroes which was in source before mirror
> start will not be copyed.

With this flag set, any runs of zeroes in the source before the mirror
starts will not be copied.

> 
> Without this libvirt migration of empty disk takes too long time.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> I've tested it with
> time virsh migrate --live test qemu+ssh://other_node/system --copy-storage-all

Presumably with a libvirt patch to turn on the optional flag.

I'm not sure I like this patch.  Libvirt uses NBD to implement
--copy-storage-all, I think we're better off improving NBD to
automatically handle sparse writes, than we are to add a one-off hack
that requires libvirt to change.  That is, once NBD is smarter, the copy
will be faster without needing a tweak.  And we ARE working on making
NBD smarter (one of my goals for the 2.7 release is to get all the
sparse file additions to NBD implemented)

That said, I'll still review it.

> 
> Without 'target-zeroed' libvirt migration of vm with empty qcow2 disk of
> 400Mb to another node takes for me more than 5 minutes. Migration of 5Gb
> disk was not finished in 28 minutes.
> 
> With new flag on, migration of 16Tb empty disk takes about a minute.
> 
>  block/mirror.c            | 16 +++++++++++-----
>  blockdev.c                |  9 ++++++++-
>  hmp.c                     |  2 +-
>  include/block/block_int.h |  2 +-
>  qapi/block-core.json      |  5 ++++-
>  qmp-commands.hx           |  4 +++-
>  6 files changed, 28 insertions(+), 10 deletions(-)
> 

> +++ b/hmp.c
> @@ -1097,7 +1097,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>                       false, NULL, false, NULL,
>                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
>                       true, mode, false, 0, false, 0, false, 0,
> -                     false, 0, false, 0, false, true, &err);
> +                     false, 0, false, 0, false, true, false, false, &err);

I'd like for my qapi 'box' patches to land, so that this can be simpler.

https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03569.html

> +++ b/qapi/block-core.json
> @@ -1154,6 +1154,9 @@
>  #         written. Both will result in identical contents.
>  #         Default is true. (Since 2.4)
>  #
> +# @target-zeroed: #optional Whether target is already zeroed, so most of zeroes
> +#                 should not be transferred. (Since 2.7)

Grammar suggestion:

#optional True if target already reads as all zeroes, so that holes in
the source need not be transferred. (Since 2.7)

> +++ b/qmp-commands.hx
> @@ -1632,7 +1632,7 @@ EQMP
>          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>                        "node-name:s?,replaces:s?,"
>                        "on-source-error:s?,on-target-error:s?,"
> -                      "unmap:b?,"
> +                      "unmap:b?,target-zeroed:b?"
>                        "granularity:i?,buf-size:i?",

I'm trying to get rid of .args_type (or rather, Marc-Andre's qapi
patches, that are stalled on my qapi patches, do the job); but for now
this part is correct.

>          .mhandler.cmd_new = qmp_marshal_drive_mirror,
>      },
> @@ -1674,6 +1674,8 @@ Arguments:
>    (BlockdevOnError, default 'report')
>  - "unmap": whether the target sectors should be discarded where source has only
>    zeroes. (json-bool, optional, default true)
> +- "target-zeroed": whether target is already zeroed, so most of zeroes should
> +  not be transferred. (json-bool, optional, default false)

Similar wording as above.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]