[libvirt] [PATCH 0/3] Honour sparse files on migration
When the storage is pre-created on the destination, it might have been allocated as sparse on the source. Let's keep it that way if we can. Michal Privoznik (3): qemuMigrationPrecreateStorage: Fix debug message qemuMigrationCookieNBD: Turn some items into their own struct qemuMigrationPrecreateDisk: Preserve sparse files src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 12 deletions(-) -- 2.0.5
When pre-creating storage for domains, we need to find corresponding disk in the XML on the destination (domain XML may differ there, e.g. disk is accessible under different path). For better debugging, I'm printing all info I received on a disk. But there was a typo when printing the disk capacity: "%lluu" instead of "%llu". Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8c45415..b20ede8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1579,7 +1579,7 @@ qemuMigrationPrecreateStorage(virConnectPtr conn, int indx; const char *diskSrcPath; - VIR_DEBUG("Looking up disk target '%s' (capacity=%lluu)", + VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)", nbd->disks[i].target, nbd->disks[i].capacity); if ((indx = virDomainDiskIndexByName(vm->def, -- 2.0.5
It will be more future proof if we just pass a pointer to a single NBD disk representation to the qemuMigrationPrecreateDisk() instead of copying every single item onto the stack. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b20ede8..3adb949 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -139,16 +139,20 @@ struct _qemuMigrationCookieNetwork { qemuMigrationCookieNetDataPtr net; }; +typedef struct _qemuMigrationCookieNBDDisk qemuMigrationCookieNBDDisk; +typedef qemuMigrationCookieNBDDisk *qemuMigrationCookieNBDDiskPtr; +struct _qemuMigrationCookieNBDDisk { + char *target; /* Disk target */ + unsigned long long capacity; /* And its capacity */ +}; + typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD; typedef qemuMigrationCookieNBD *qemuMigrationCookieNBDPtr; struct _qemuMigrationCookieNBD { int port; /* on which port does NBD server listen for incoming data */ size_t ndisks; /* Number of items in @disk array */ - struct { - char *target; /* Disk target */ - unsigned long long capacity; /* And its capacity */ - } *disks; + qemuMigrationCookieNBDDiskPtr disks; }; typedef struct _qemuMigrationCookie qemuMigrationCookie; @@ -1458,7 +1462,7 @@ qemuMigrationRestoreDomainState(virConnectPtr conn, virDomainObjPtr vm) static int qemuMigrationPrecreateDisk(virConnectPtr conn, virDomainDiskDefPtr disk, - unsigned long long capacity) + qemuMigrationCookieNBDDiskPtr nbd) { int ret = -1; virStoragePoolPtr pool = NULL; @@ -1534,7 +1538,7 @@ qemuMigrationPrecreateDisk(virConnectPtr conn, virBufferAddLit(&buf, "<volume>\n"); virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", volName); - virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", capacity); + virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", nbd->capacity); virBufferAddLit(&buf, "<target>\n"); virBufferAdjustIndent(&buf, 2); virBufferAsprintf(&buf, "<format type='%s'/>\n", format); @@ -1601,7 +1605,7 @@ qemuMigrationPrecreateStorage(virConnectPtr conn, VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath)); - if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0) + if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks + i) < 0) goto cleanup; } -- 2.0.5
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@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); } 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; + } } } 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); 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) { -- 2.0.5
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@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) {
participants (2)
-
John Ferlan -
Michal Privoznik