Re: [Qemu-devel] [PATCH 2/4] multifd: Drop x-multifd-page-count parameter

[Daniel P . Berrangé]

On Wed, Feb 06, 2019 at 02:23:29PM +0100, Juan Quintela wrote:
> Libvirt don't want to expose (and explain it).  And testing looks like
> 128 is good for all use cases, so just drop it.

One significant concern inline...

> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  hmp.c                 |  7 -------
>  migration/migration.c | 30 ------------------------------
>  migration/migration.h |  1 -
>  migration/ram.c       | 13 ++++++++-----
>  qapi/migration.json   | 13 +------------
>  5 files changed, 9 insertions(+), 55 deletions(-)


> @@ -718,7 +721,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>      packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>      packet->version = cpu_to_be32(MULTIFD_VERSION);
>      packet->flags = cpu_to_be32(p->flags);
> -    packet->size = cpu_to_be32(migrate_multifd_page_count());
> +    packet->size = cpu_to_be32(MULTIFD_PAGE_COUNT);
>      packet->used = cpu_to_be32(p->pages->used);
>      packet->packet_num = cpu_to_be64(p->packet_num);
>

Here the source QEMU sends the page size - which is now
a hardcoded constant - to the target QEMU.

> @@ -756,10 +759,10 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams 
> *p, Error **errp)
>      p->flags = be32_to_cpu(packet->flags);
>  
>      packet->size = be32_to_cpu(packet->size);
> -    if (packet->size > migrate_multifd_page_count()) {
> +    if (packet->size > MULTIFD_PAGE_COUNT) {
>          error_setg(errp, "multifd: received packet "
>                     "with size %d and expected maximum size %d",
> -                   packet->size, migrate_multifd_page_count()) ;
> +                   packet->size, MULTIFD_PAGE_COUNT) ;
>          return -1;
>      }
>

Here the dest QEMU receives the page size that the source QEMU used, and
checks that it is not larger than its constant.

IIUC, the implication here is that if we ever increase the size of this
constant in future QEMU, we will break live migration from new to old
QEMU due to this check.  In fact your previous patch in this series has
done exactly that, so this appears to mean QEMU 4.0 -> QEMU 3.2
multifd migration is broken now.

Alternatively if we decrease the size of the constant in future
QEMU, we will break live migration from old QEMU to new QEMU which
is even worse.

This problem existed before this patch, if the management app was
not explicitly using migrate-set-parameters to set the page count
on both sides of QEMU. So we're already broken, but at least the
feature was marked experimental.

What is the purpose of this packet size check ?  Is it something
we can safely remove, so that we can increase or decrease the
size at will without breaking migration compat.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|