[libvirt] [PATCH 3/3] qemuMigrationPrecreateDisk: Preserve sparse files

[John Ferlan]

On 04/02/2015 12:48 PM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=817700
> 
> When pre-creating a disk on the destination, a volume XML is
> constructed. The XML is then passed to virStorageVolCreateXML() which
> does the work. But, since there's no <allocation/> in the XML, the
> disk are fully allocated. This possibly breaks sparse allocation user
> has on the migration source.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_migration.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3adb949..a2f68ed 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -144,6 +144,7 @@ typedef qemuMigrationCookieNBDDisk *qemuMigrationCookieNBDDiskPtr;
>  struct _qemuMigrationCookieNBDDisk {
>      char *target;                   /* Disk target */
>      unsigned long long capacity;    /* And its capacity */
> +    unsigned long long allocation;  /* And its allocation */
>  };
>  
>  typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD;
> @@ -591,6 +592,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
>                         disk->dst) < 0)
>              goto cleanup;
>          mig->nbd->disks[mig->nbd->ndisks].capacity = entry->capacity;
> +        mig->nbd->disks[mig->nbd->ndisks].allocation = entry->wr_highest_offset;
>          mig->nbd->ndisks++;
>      }
>  
> @@ -831,8 +833,9 @@ qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver,
>              for (i = 0; i < mig->nbd->ndisks; i++) {
>                  virBufferEscapeString(buf, "<disk target='%s'",
>                                        mig->nbd->disks[i].target);
> -                virBufferAsprintf(buf, " capacity='%llu'/>\n",
> -                                  mig->nbd->disks[i].capacity);
> +                virBufferAsprintf(buf, " capacity='%llu' allocation='%llu'/>\n",
> +                                  mig->nbd->disks[i].capacity,
> +                                  mig->nbd->disks[i].allocation);

Here you're printing it here regardless of value (-1, 0?)...

>              }
>              virBufferAdjustIndent(buf, -2);
>              virBufferAddLit(buf, "</nbd>\n");
> @@ -970,7 +973,7 @@ static qemuMigrationCookieNBDPtr
>  qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt)
>  {
>      qemuMigrationCookieNBDPtr ret = NULL;
> -    char *port = NULL, *capacity = NULL;
> +    char *port = NULL, *capacity = NULL, *allocation = NULL;
>      size_t i;
>      int n;
>      xmlNodePtr *disks = NULL;
> @@ -998,6 +1001,7 @@ qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt)
>          for (i = 0; i < n; i++) {
>              ctxt->node = disks[i];
>              VIR_FREE(capacity);
> +            VIR_FREE(allocation);
>  
>              if (!(ret->disks[i].target = virXPathString("string(./@target)", ctxt))) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -1014,12 +1018,26 @@ qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt)
>                                 NULLSTR(capacity));
>                  goto error;
>              }
> +
> +            allocation = virXPathString("string(./@allocation)", ctxt);
> +            if (allocation) {
> +                if (virStrToLong_ull(allocation, NULL, 10,
> +                                     &ret->disks[i].allocation) < 0) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("Malformed disk allocation: '%s'"),
> +                                   allocation);
> +                    goto error;
> +                }
> +            } else {
> +                ret->disks[i].allocation = -1;

This would seem to be dangerous - while we don't have a disk that takes
up that much space, why not just leave it at zero for a "marker" of
sorts? Or set it to capacity like virStorageVolDefParseXML would if
allocation wasn't in the XML


> +            }
>          }
>      }
>  
>   cleanup:
>      VIR_FREE(port);
>      VIR_FREE(capacity);
> +    VIR_FREE(allocation);
>      VIR_FREE(disks);
>      ctxt->node = save_ctxt;
>      return ret;
> @@ -1539,6 +1557,8 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
>      virBufferAdjustIndent(&buf, 2);
>      virBufferEscapeString(&buf, "<name>%s</name>\n", volName);
>      virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", nbd->capacity);
> +    if (nbd->allocation != -1)
> +        virBufferAsprintf(&buf, "<allocation>%llu</allocation>\n", nbd->allocation);

Here of course you compare to -1 and don't print...

While it's perhaps not good to assume allocation == capacity, it's
perhaps no worse than today which would essentially allocate up to
capacity, right? Mixing ULL and -1 just seems dangerous

John

>      virBufferAddLit(&buf, "<target>\n");
>      virBufferAdjustIndent(&buf, 2);
>      virBufferAsprintf(&buf, "<format type='%s'/>\n", format);
> @@ -1583,8 +1603,9 @@ qemuMigrationPrecreateStorage(virConnectPtr conn,
>          int indx;
>          const char *diskSrcPath;
>  
> -        VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)",
> -                  nbd->disks[i].target, nbd->disks[i].capacity);
> +        VIR_DEBUG("Looking up disk target '%s' (capacity=%llu allocation=%llu)",
> +                  nbd->disks[i].target, nbd->disks[i].capacity,
> +                  nbd->disks[i].allocation);
>  
>          if ((indx = virDomainDiskIndexByName(vm->def,
>                                               nbd->disks[i].target, false)) < 0) {
>