[PATCH v2 0/7] qemu: implementation of transient disk option

This patchset tries to implement transient option for qcow2 and raw format disk. This uses the snapshot cleanup codes: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html It gets user available to set <transient/> to the domain xml file like as: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk> Any changes which the Guest does to the disk is dropped when the Guest is shutdowned. There are some limitations for transient disk option so far: - Supported disk format is qcow2 and raw - blockdev capability is required for qemu - Following features are blocked with transient disk option - blockjobs - Migration - Disk hotplug Masayoshi Mizuma (7): qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL qemu: Introduce functions to handle transient disk qemu: Add transient disk handler to start and stop the guest qemu: Transient option gets avaiable for qcow2 and raw format disk qemu: Block blockjobs when transient disk option is enabled qemu: Block migration when transient disk option is enabled qemu: Block disk hotplug when transient disk option is enabled src/qemu/qemu_domain.c | 7 ++ src/qemu/qemu_hotplug.c | 6 ++ src/qemu/qemu_migration.c | 22 ++++++ src/qemu/qemu_process.c | 8 +++ src/qemu/qemu_snapshot.c | 139 +++++++++++++++++++++++++++++++++++++- src/qemu/qemu_snapshot.h | 8 +++ src/qemu/qemu_validate.c | 9 ++- src/util/virstoragefile.h | 2 + 8 files changed, 196 insertions(+), 5 deletions(-) -- 2.27.0

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Get available even if snapdisk argument is NULL at qemuSnapshotDiskPrepareOne() so that the caller can setup dd->src. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_snapshot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1e8ea80b22..d310e6ff02 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -953,8 +953,9 @@ qemuSnapshotDiskPrepareOne(virQEMUDriverPtr driver, if (qemuDomainStorageSourceValidateDepth(disk->src, 1, disk->dst) < 0) return -1; - if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) - return -1; + if (snapdisk) + if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) + return -1; if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) return -1; -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:30 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Get available even if snapdisk argument is NULL at qemuSnapshotDiskPrepareOne() so that the caller can setup dd->src.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_snapshot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1e8ea80b22..d310e6ff02 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -953,8 +953,9 @@ qemuSnapshotDiskPrepareOne(virQEMUDriverPtr driver, if (qemuDomainStorageSourceValidateDepth(disk->src, 1, disk->dst) < 0) return -1;
- if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) - return -1; + if (snapdisk) + if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) + return -1;
NACK, you can pass in a 'snapdisk' with the correct data to create the overlay which will be a cleaner solution.

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Here is the implementation of transient option for qcow2 and raw format disk. This gets available <transient/> directive in domain xml file like as: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk> When the qemu command line options are built, a new qcow2 image is created with backing qcow2 by using blockdev-snapshot command. The backing image is the qcow2 file which is set as <source>. The filename of the new qcow2 image is original-source-file.TRANSIENT. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_snapshot.c | 134 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_snapshot.h | 8 +++ src/util/virstoragefile.h | 2 + 3 files changed, 144 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d310e6ff02..5c61d19f26 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2265,3 +2265,137 @@ qemuSnapshotDelete(virDomainObjPtr vm, cleanup: return ret; } + +static int +qemuTransientDiskPrepareOne(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuSnapshotDiskDataPtr data, + virHashTablePtr blockNamedNodeData, + int asyncJob, + virJSONValuePtr actions) +{ + int rc = -1; + virStorageSourcePtr dest; + virStorageSourcePtr src = data->disk->src; + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + + if (!strlen(src->path)) + return rc; + + if (!(dest = virStorageSourceNew())) + return rc; + + dest->path = g_strdup_printf("%s.TRANSIENT", src->path); + + if (virFileExists(dest->path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("Transient disk '%s' for '%s' exists"), + dest->path, src->path); + goto cleanup; + } + + dest->type = VIR_STORAGE_TYPE_FILE; + dest->format = VIR_STORAGE_FILE_QCOW2; + data->src = dest; + + if (qemuSnapshotDiskPrepareOne(driver, vm, cfg, data->disk, + NULL, data, blockNamedNodeData, + false, true, asyncJob, actions) < 0) + goto cleanup; + + rc = 0; + cleanup: + if (rc < 0) + g_free(dest->path); + + return rc; +} + +static int +qemuWaitTransaction(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob, + virJSONValuePtr *actions) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + if (qemuMonitorTransaction(priv->mon, actions) < 0) + return -1; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + return 0; +} + +int +qemuTransientCreatetDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + size_t i; + int rc = -1; + size_t ndata = 0; + qemuSnapshotDiskDataPtr data = NULL; + g_autoptr(virJSONValue) actions = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virHashTable) blockNamedNodeData = NULL; + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); + + if (!blockdev) + return rc; + + if (VIR_ALLOC_N(data, vm->def->ndisks) < 0) + return rc; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + goto cleanup; + + if (!(actions = virJSONValueNewArray())) + goto cleanup; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->src->readonly) + continue; + + if (disk->transient) { + data[ndata].disk = disk; + if (qemuTransientDiskPrepareOne(driver, vm, &data[ndata], blockNamedNodeData, + asyncJob, actions) < 0) + goto cleanup; + ndata++; + } + } + + if (qemuWaitTransaction(driver, vm, asyncJob, &actions) < 0) + goto cleanup; + + for (i = 0; i < ndata; i++) { + qemuSnapshotDiskDataPtr dd = &data[i]; + + qemuSnapshotDiskUpdateSource(driver, vm, dd, blockdev); + dd->disk->src->transientEstablished = true; + } + + VIR_FREE(data); + rc = 0; + cleanup: + qemuSnapshotDiskCleanup(data, vm->def->ndisks, driver, vm, asyncJob); + + return rc; +} + +void +qemuTransientRemoveDisk(virDomainDiskDefPtr disk) +{ + if (disk->src->transientEstablished) { + VIR_DEBUG("unlink transient disk: %s", disk->src->path); + unlink(disk->src->path); + } +} diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index 8b3ebe87b1..aecb1762d2 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -53,3 +53,11 @@ int qemuSnapshotDelete(virDomainObjPtr vm, virDomainSnapshotPtr snapshot, unsigned int flags); + +int +qemuTransientCreatetDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob); + +void +qemuTransientRemoveDisk(virDomainDiskDefPtr disk); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f73b3ee005..fd42d017f3 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -383,6 +383,8 @@ struct _virStorageSource { /* these must not be used apart from formatting the output JSON in the qemu driver */ char *ssh_user; bool ssh_host_key_check_disabled; + + bool transientEstablished; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:31 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Here is the implementation of transient option for qcow2 and raw format disk. This gets available <transient/> directive in domain xml file like as:
<disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk>
When the qemu command line options are built, a new qcow2 image is created with backing qcow2 by using blockdev-snapshot command. The backing image is the qcow2 file which is set as <source>. The filename of the new qcow2 image is original-source-file.TRANSIENT.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_snapshot.c | 134 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_snapshot.h | 8 +++ src/util/virstoragefile.h | 2 + 3 files changed, 144 insertions(+)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d310e6ff02..5c61d19f26 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2265,3 +2265,137 @@ qemuSnapshotDelete(virDomainObjPtr vm, cleanup: return ret; } + +static int +qemuTransientDiskPrepareOne(virQEMUDriverPtr driver,
This should be named with a qemuSnapshot... prefix: qemuSnapshotDiskTransientPrepareOne for example.
+ virDomainObjPtr vm, + qemuSnapshotDiskDataPtr data, + virHashTablePtr blockNamedNodeData, + int asyncJob, + virJSONValuePtr actions) +{ + int rc = -1; + virStorageSourcePtr dest;
You can use g_autoptr here.
+ virStorageSourcePtr src = data->disk->src; + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + + if (!strlen(src->path))
This will not work as expected. You must make sure that the source disk is VIR_STORAGE_TYPE_FILE. presence of src->path does not guarantee it. Also you can use virStorageSourceIsEmpty() here if you want to skip empty cdroms, but transient cdrom/readonly disks should be rejected in the config validator beforehand.
+ return rc;
Please always return constants right away. (rc is a constant at this point).
+ + if (!(dest = virStorageSourceNew())) + return rc; + + dest->path = g_strdup_printf("%s.TRANSIENT", src->path); + + if (virFileExists(dest->path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("Transient disk '%s' for '%s' exists"), + dest->path, src->path); + goto cleanup; + } + + dest->type = VIR_STORAGE_TYPE_FILE; + dest->format = VIR_STORAGE_FILE_QCOW2; + data->src = dest; + + if (qemuSnapshotDiskPrepareOne(driver, vm, cfg, data->disk, + NULL, data, blockNamedNodeData,
Create a virDomainSnapshotDiskDefPtr with the correct data from above and pass it in here, rather than working around the addition to the data structure.
+ false, true, asyncJob, actions) < 0) + goto cleanup; + + rc = 0; + cleanup: + if (rc < 0) + g_free(dest->path); + + return rc; +} + +static int +qemuWaitTransaction(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob, + virJSONValuePtr *actions)
This function isn't IMO necessary.
+{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + if (qemuMonitorTransaction(priv->mon, actions) < 0) + return -1; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + rturn 0; +} + +int +qemuTransientCreatetDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + size_t i; + int rc = -1; + size_t ndata = 0; + qemuSnapshotDiskDataPtr data = NULL; + g_autoptr(virJSONValue) actions = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virHashTable) blockNamedNodeData = NULL; + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
Imo it will be better to factor out the common parts from this and qemuSnapshotCreateDiskActive and then just use the disjunct logic in this function.
+ + if (!blockdev) + return rc;
This breaks startup of VMs with pre-blockdev qemu as it returns -1 here directly when blockdev is not supported.
+ if (VIR_ALLOC_N(data, vm->def->ndisks) < 0) + return rc; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + goto cleanup; + + if (!(actions = virJSONValueNewArray())) + goto cleanup; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->src->readonly) + continue;
This transient+readonly should be a hard error and validated in the config validator rather than here.
+ + if (disk->transient) { + data[ndata].disk = disk; + if (qemuTransientDiskPrepareOne(driver, vm, &data[ndata], blockNamedNodeData, + asyncJob, actions) < 0)
misaligned code.
+ goto cleanup; + ndata++; + } + } + + if (qemuWaitTransaction(driver, vm, asyncJob, &actions) < 0) + goto cleanup; + + for (i = 0; i < ndata; i++) { + qemuSnapshotDiskDataPtr dd = &data[i]; + + qemuSnapshotDiskUpdateSource(driver, vm, dd, blockdev); + dd->disk->src->transientEstablished = true; + }
These both can be factored out and shared with the snapshot code.
+ + VIR_FREE(data); + rc = 0; + cleanup: + qemuSnapshotDiskCleanup(data, vm->def->ndisks, driver, vm, asyncJob); + + return rc; +} + +void +qemuTransientRemoveDisk(virDomainDiskDefPtr disk)
This doesn't belong into this helper file. Removal of the snapshot images is not related to snapshots.
+{ + if (disk->src->transientEstablished) { + VIR_DEBUG("unlink transient disk: %s", disk->src->path); + unlink(disk->src->path); + } +} diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index 8b3ebe87b1..aecb1762d2 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -53,3 +53,11 @@ int qemuSnapshotDelete(virDomainObjPtr vm, virDomainSnapshotPtr snapshot, unsigned int flags); + +int +qemuTransientCreatetDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob); + +void +qemuTransientRemoveDisk(virDomainDiskDefPtr disk); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f73b3ee005..fd42d017f3 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -383,6 +383,8 @@ struct _virStorageSource { /* these must not be used apart from formatting the output JSON in the qemu driver */ char *ssh_user; bool ssh_host_key_check_disabled; + + bool transientEstablished;
The deep copy function is not updated with this new member. Also you should document this variable.
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); -- 2.27.0

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> The transient disk is attached before the guest starts. Remove the transient disk when the guest does shutdown. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_process.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 126fabf5ef..5753258135 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -60,6 +60,7 @@ #include "qemu_firmware.h" #include "qemu_backup.h" #include "qemu_dbus.h" +#include "qemu_snapshot.h" #include "cpu/cpu.h" #include "cpu/cpu_x86.h" @@ -7000,6 +7001,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) goto cleanup; + VIR_DEBUG("Setting up transient disk"); + if (qemuTransientCreatetDisk(driver, vm, asyncJob) < 0) + goto cleanup; + ret = 0; cleanup: @@ -7636,6 +7641,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, } qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); + + if (disk->transient) + qemuTransientRemoveDisk(disk); } } -- 2.27.0

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_validate.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..82818a4fdc 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2166,9 +2166,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } if (disk->transient) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("transient disks not supported yet")); - return -1; + if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) && + (disk->src->format != VIR_STORAGE_FILE_RAW)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("transient disks not supported yet")); + return -1; + } } if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE && -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:33 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_validate.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..82818a4fdc 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2166,9 +2166,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, }
if (disk->transient) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("transient disks not supported yet")); - return -1; + if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) && + (disk->src->format != VIR_STORAGE_FILE_RAW)) {
This needs a check that QEMU_CAPS_BLOCKDEV is available as it won't really work properly in pre-blockdev config. Also here you should reject any other unsupported configuration rather than in the code.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("transient disks not supported yet")); + return -1; + } }
if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE && -- 2.27.0

On Tue, Sep 08, 2020 at 03:14:42PM +0200, Peter Krempa wrote:
On Fri, Aug 28, 2020 at 10:08:33 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_validate.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..82818a4fdc 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2166,9 +2166,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, }
if (disk->transient) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("transient disks not supported yet")); - return -1; + if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) && + (disk->src->format != VIR_STORAGE_FILE_RAW)) {
This needs a check that QEMU_CAPS_BLOCKDEV is available as it won't really work properly in pre-blockdev config. Also here you should reject any other unsupported configuration rather than in the code.
Thanks. I'll add the following validation. diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 070f1c962b..9cf78ca0c9 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2002,6 +2002,28 @@ qemuValidateDomainDeviceDefDiskSerial(const char *value) } +static int +qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, + virQEMUCapsPtr qemuCaps) +{ + if ((!qemuCaps) || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) + return false; + + if (disk->src->readonly) + return false; + + if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) && + (disk->src->format != VIR_STORAGE_FILE_RAW) && + (disk->src->type != VIR_STORAGE_TYPE_FILE)) { + return false; + } + + if (virStorageSourceIsEmpty(disk->src)) + return false; + + return true; +} + static int qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, virQEMUCapsPtr qemuCaps) @@ -2186,7 +2208,8 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } } - if (disk->transient) { + if ((disk->transient) && + !qemuValidateDomainDeviceDefDiskTransient(disk, qemuCaps)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("transient disks not supported yet")); return -1;

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Block blockjobs when transient disk option is enabled so far. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e28f704dba..98a52e5476 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10678,6 +10678,13 @@ qemuDomainDiskBlockJobIsSupported(virDomainObjPtr vm, return false; } + if (disk->transient) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("block jobs are not supported on transient disk '%s'"), + disk->dst); + return false; + } + return true; } -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:34 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Block blockjobs when transient disk option is enabled so far.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e28f704dba..98a52e5476 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10678,6 +10678,13 @@ qemuDomainDiskBlockJobIsSupported(virDomainObjPtr vm, return false; }
+ if (disk->transient) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("block jobs are not supported on transient disk '%s'"), + disk->dst); + return false; + }
Please move this patch a bit sooner in the series so that all checks are in place before enabling the feature.

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Block migration when transient disk option is enabled because migration requires some blockjobs. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_migration.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0f2f92b211..6fcf5a3a07 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2949,6 +2949,22 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, } +static bool +qemuMigrationTransientDiskExists(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->transient) + return true; + } + + return false; +} + + virDomainDefPtr qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, @@ -2971,6 +2987,12 @@ qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup; + /* + * transient disk option is a blocker for migration + */ + if (qemuMigrationTransientDiskExists(def)) + goto cleanup; + if (dname) { name = def->name; def->name = g_strdup(dname); -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:35 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Block migration when transient disk option is enabled because migration requires some blockjobs.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_migration.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0f2f92b211..6fcf5a3a07 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2949,6 +2949,22 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, }
+static bool +qemuMigrationTransientDiskExists(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->transient) + return true; + } + + return false; +} + + virDomainDefPtr qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, @@ -2971,6 +2987,12 @@ qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup;
+ /* + * transient disk option is a blocker for migration + */ + if (qemuMigrationTransientDiskExists(def)) + goto cleanup;
This should really be placed into qemuMigrationSrcIsAllowed() rather than open-coded. Migration is used in other places too and doesn't use this API to trigger it.

On Tue, Sep 08, 2020 at 03:17:47PM +0200, Peter Krempa wrote:
On Fri, Aug 28, 2020 at 10:08:35 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Block migration when transient disk option is enabled because migration requires some blockjobs.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_migration.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0f2f92b211..6fcf5a3a07 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2949,6 +2949,22 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, }
+static bool +qemuMigrationTransientDiskExists(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->transient) + return true; + } + + return false; +} + + virDomainDefPtr qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, @@ -2971,6 +2987,12 @@ qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup;
+ /* + * transient disk option is a blocker for migration + */ + if (qemuMigrationTransientDiskExists(def)) + goto cleanup;
This should really be placed into qemuMigrationSrcIsAllowed() rather than open-coded. Migration is used in other places too and doesn't use this API to trigger it.
OK, I'll add the following to block the migration. diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a530c17582..7316d74677 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1397,6 +1397,16 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, _("cannot migrate this domain without dbus-vmstate support")); return false; } + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->transient) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with transient disk is not supported")); + return false; + } + } } return true;

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Block disk hotplug when transient disk option is enabled so far. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2c6c30ce03..1c1b6c3acf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1031,6 +1031,12 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, return -1; } + if (disk->transient) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("transient disk hotplug isn't supported")); + return -1; + } + if (virDomainDiskTranslateSourcePool(disk) < 0) goto cleanup; -- 2.27.0

On Fri, Aug 28, 2020 at 10:08:36 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Block disk hotplug when transient disk option is enabled so far.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2c6c30ce03..1c1b6c3acf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1031,6 +1031,12 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, return -1; }
+ if (disk->transient) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("transient disk hotplug isn't supported")); + return -1; + }
Please move this up in the series so that it's in place before enabling the feature.

[FYI pkrempa is on PTO this week so he won't review this until the next one] On a Friday in 2020, Masayoshi Mizuma wrote:
This patchset tries to implement transient option for qcow2 and raw format disk. This uses the snapshot cleanup codes: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html
It gets user available to set <transient/> to the domain xml file like as:
<disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk>
Any changes which the Guest does to the disk is dropped when the Guest is shutdowned.
There are some limitations for transient disk option so far:
- Supported disk format is qcow2 and raw - blockdev capability is required for qemu - Following features are blocked with transient disk option - blockjobs - Migration - Disk hotplug
Masayoshi Mizuma (7): qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL qemu: Introduce functions to handle transient disk qemu: Add transient disk handler to start and stop the guest qemu: Transient option gets avaiable for qcow2 and raw format disk qemu: Block blockjobs when transient disk option is enabled qemu: Block migration when transient disk option is enabled qemu: Block disk hotplug when transient disk option is enabled
src/qemu/qemu_domain.c | 7 ++ src/qemu/qemu_hotplug.c | 6 ++ src/qemu/qemu_migration.c | 22 ++++++ src/qemu/qemu_process.c | 8 +++ src/qemu/qemu_snapshot.c | 139 +++++++++++++++++++++++++++++++++++++- src/qemu/qemu_snapshot.h | 8 +++ src/qemu/qemu_validate.c | 9 ++- src/util/virstoragefile.h | 2 + 8 files changed, 196 insertions(+), 5 deletions(-)
-- 2.27.0
participants (3)
-
Ján Tomko
-
Masayoshi Mizuma
-
Peter Krempa