[libvirt] [PATCH v3] Fixes virsh save-restore/migration when memory detach not in LIFO

[John Ferlan]

On 10/04/2016 09:06 AM, Nitesh Konkar wrote:
> Currently the migration stream references the memory
> blocks by name (which is supplied by libvirt) rather
> than by there order. With the current code that is
> assigning aliases for memory backend objects this
> won't happen and since qemu is treating the memory
> object links differently migration does not work in
> such case.
> 
> This patch ensures slot number alocation for the memory

allocation

> modules beforehand and assign alias accordingly. This
> keeps slot numbers consistent with the aliases always.
> 
> Signed-off-by: Nitesh Konkar <nitkon12 at linux.vnet.ibm.com>
> ---
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_alias.c   | 36 +++++++++++++++++++++++++-----------
>  src/qemu/qemu_alias.h   |  6 ++++--
>  src/qemu/qemu_domain.c  |  3 +++
>  src/qemu/qemu_hotplug.c |  5 ++++-
>  5 files changed, 37 insertions(+), 14 deletions(-)
> 

I believe Peter's review of your v2 mentioned a few other things... In
particular test cases... but also a subtle point about partial manual
assignment of slots by a user which I'm not sure you covered in the new
AssignSlot method below.

I'll make note of a few things below...

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index fd3ae8e..22b5fe1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2145,6 +2145,7 @@ struct _virDomainDef {
>  
>      virDomainBlkiotune blkio;
>      virDomainMemtune mem;
> +    virBitmapPtr memslotsptr;

This wouldn't belong here - it would belong in _qemuDomainObjPrivate.
You're not saving this data the user visible XML and this is not
something generic, rather it's qemu specific.

I think it would need to be saved in the "running" XML so that future
restart/reloads of running domains would have it.

You'll see that the structure already has a couple of virBitmapPtr
fields which you could use as a "model" to find all the places you'd
need to touch (and the history of how they got into private).

>  
>      virDomainVcpuDefPtr *vcpus;
>      size_t maxvcpus;
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index cc83fec..8deb054 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -332,21 +332,34 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
>  }
>  
>  
> -int
> -qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
> -                            virDomainMemoryDefPtr mem)
> +void
> +qemuAssignDeviceMemorySlot(virDomainDefPtr def,
> +                           virDomainMemoryDefPtr mem)

This should not go in qemu_alias.c - it seems to belong in qemu_domain
and thus would be appropriately named such as:

qemuDomainDeviceAssignMemorySlot (or something that starts with
qemuDomain - I'm not the "best" at creating names).


>  {
>      size_t i;
> -    int maxidx = 0;
> -    int idx;
> +    int minidx = 0;
>  
> -    for (i = 0; i < def->nmems; i++) {
> -        if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx)
> -            maxidx = idx + 1;
> +    if (mem->info.addr.dimm.base) {
> +        minidx = mem->info.addr.dimm.slot;
> +    } else {
> +        for (i = 0; i < def->mem.memory_slots; i++) {
> +            if (!virBitmapIsBitSet(def->memslotsptr, i)) {
> +                minidx = i;
> +                break;
> +            }
> +        }

Are you looking for something like virBitmapNextClearBit?

>      }
>  
> -    if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0)
> -        return -1;
> +    ignore_value(virBitmapSetBit(def->memslotsptr, minidx));
> +    mem->info.addr.dimm.slot = minidx;
> +}
> +
> +
> +int
> +qemuAssignDeviceMemoryAlias(virDomainMemoryDefPtr mem)
> +{

Right here you'll want to call your method to assign the slot...  Since
the two places where slot assignment is done (qemuAssignDeviceAliases
and qemuDomainAttachMemory) have that call first...

> +    if (virAsprintf(&mem->info.alias, "dimm%d", mem->info.addr.dimm.slot) < 0)
> +         return -1;
>  
>      return 0;

FWIW:
If this only had the virAsprintf, it could have just simply been:

    return virAsprintf(...);

>  }
> @@ -475,7 +488,8 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>              return -1;
>      }
>      for (i = 0; i < def->nmems; i++) {
> -        if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0)
> +        qemuAssignDeviceMemorySlot(def, def->mems[i]);
> +        if (virAsprintf(&def->mems[i]->info.alias, "dimm%d", def->mems[i]->info.addr.dimm.slot) < 0)

Similar to [1] below, when these two things are "paired up" - that says
to me the qemuAssignDeviceAliases should be handling that memory slot

Not doing the virAsprintf here allows one common place to generate the
alias...

>              return -1;
>      }
>  
> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 11d9fde..c6cb568 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -57,8 +57,10 @@ int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
>  int qemuAssignDeviceRNGAlias(virDomainDefPtr def,
>                               virDomainRNGDefPtr rng);
>  
> -int qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
> -                                virDomainMemoryDefPtr mems);
> +void qemuAssignDeviceMemorySlot(virDomainDefPtr def,
> +                                virDomainMemoryDefPtr);
> +
> +int qemuAssignDeviceMemoryAlias(virDomainMemoryDefPtr mems);
>  
>  int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>                                 virDomainShmemDefPtr shmem,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9b1a32e..263e78f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2386,6 +2386,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>      if (qemuDomainDefVcpusPostParse(def) < 0)
>          goto cleanup;
>  
> +    if (def->mem.memory_slots)
> +        def->memslotsptr = virBitmapNew(def->mem.memory_slots);
> +

When is this virBitmapFree()'d?  If it moves to the qemu domain private,
then check out qemuDomainObjPrivateFree.

>      ret = 0;
>   cleanup:
>      virObjectUnref(qemuCaps);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 72dd93b..5a3af10 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1898,7 +1898,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0)
>          goto cleanup;
>  
> -    if (qemuAssignDeviceMemoryAlias(vm->def, mem) < 0)
> +    qemuAssignDeviceMemorySlot(vm->def, mem);
> +
> +    if (qemuAssignDeviceMemoryAlias(mem) < 0)

[1]
Things paired like this are what cause me to believe the Alias function
can do the setting...

Secondarily, if we go to cleanup due to failure after this point, then
your bitmap slot is not cleared. One way we've been handling this type
of situation is by adding a boolean to indicate that we have to undo
something we did "on failure". In this particular case, you may need to
alter subsequent goto cleanup's to be a different label to clear the bit
similar to how there's a removedef label...  The new label would be
below removedef and above the goto audit to allow the removedef label
code to just fall into the new label (hope that makes sense!).

>          goto cleanup;
>  
>      if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0)
> @@ -4427,6 +4429,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
>      }
>  
>      mem = vm->def->mems[idx];
> +    ignore_value(virBitmapClearBit(vm->def->memslotsptr, memdef->info.addr.dimm.slot));

BTW: Since the alias and the slot are tightly coupled, if the alias
isn't found, then do we want to be clearing this?  IOW: Should this go
after the following alias check


John
>  
>      if (!mem->info.alias) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>