[libvirt] [PATCH 0/7] qemu: Various blockdev-related fixes (blockdev-add saga)
Peter Krempa (7): qemu: command: Fix function name in comment qemu: Add possibility to prepare top image only for attachment via blockdev qemu: block: Add helper for generating snapshot transaction for -blockdev qemu: Use virStorageSourceIsEmpty in qemuDomainBlockCommit qemu: block: Use simple backing stores string format if possible qemu: snapshot: Initialize data for inactive config of snapshot earlier qemu: blockjob: Reset 'synchronous' block job handling flag prior to flushing events src/qemu/qemu_block.c | 108 ++++++++++++++---- src/qemu/qemu_block.h | 9 ++ src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_command.c | 51 +++++++-- src/qemu/qemu_command.h | 3 + src/qemu/qemu_driver.c | 34 +++--- .../imagecreate/qcow2-backing-raw-nbd.json | 2 +- 7 files changed, 158 insertions(+), 51 deletions(-) -- 2.21.0
In commit 042c95bd194 qemuBuildStorageSourceChainAttachPrepareBlockdev was added but the comment for the function mentions qemuBuildStorageSourceChainAttachPrepareDrive. Fix the mistake. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1cf165079f..eefec98022 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11160,7 +11160,7 @@ qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, /** - * qemuBuildStorageSourceChainAttachPrepareDrive: + * qemuBuildStorageSourceChainAttachPrepareBlockdev: * @top: storage source chain * @qemuCaps: qemu capabilities object * -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:45PM +0200, Peter Krempa wrote:
In commit 042c95bd194 qemuBuildStorageSourceChainAttachPrepareBlockdev was added but the comment for the function mentions qemuBuildStorageSourceChainAttachPrepareDrive. Fix the mistake.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuBuildStorageSourceChainAttachPrepareBlockdev prepares the full backing chain for attachment via blockdev. For snapshots we'll need to prepare one image only as it needs to be plugged on top of the existing chain. This patch introduces qemuBuildStorageSourceChainAttachPrepareBlockdevTop which prepares only @top similarly to the original function by splitting out the functionality into an internal function so that the API does not change. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 51 +++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 +++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eefec98022..476e710257 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11159,16 +11159,10 @@ qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, } -/** - * qemuBuildStorageSourceChainAttachPrepareBlockdev: - * @top: storage source chain - * @qemuCaps: qemu capabilities object - * - * Prepares qemuBlockStorageSourceChainDataPtr for attaching @top via -blockdev. - */ -qemuBlockStorageSourceChainDataPtr -qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSourcePtr top, - virQEMUCapsPtr qemuCaps) +static qemuBlockStorageSourceChainDataPtr +qemuBuildStorageSourceChainAttachPrepareBlockdevInternal(virStorageSourcePtr top, + virQEMUCapsPtr qemuCaps, + bool onlyTop) { VIR_AUTOPTR(qemuBlockStorageSourceAttachData) elem = NULL; VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL; @@ -11186,7 +11180,44 @@ qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSourcePtr top, if (VIR_APPEND_ELEMENT(data->srcdata, data->nsrcdata, elem) < 0) return NULL; + + if (onlyTop) + break; } VIR_RETURN_PTR(data); } + + +/** + * qemuBuildStorageSourceChainAttachPrepareBlockdev: + * @top: storage source chain + * @qemuCaps: qemu capabilities object + * + * Prepares qemuBlockStorageSourceChainDataPtr for attaching the chain of images + * starting at @top via -blockdev. + */ +qemuBlockStorageSourceChainDataPtr +qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSourcePtr top, + virQEMUCapsPtr qemuCaps) +{ + return qemuBuildStorageSourceChainAttachPrepareBlockdevInternal(top, qemuCaps, + false); +} + + +/** + * qemuBuildStorageSourceChainAttachPrepareBlockdevTop: + * @top: storage source chain + * @qemuCaps: qemu capabilities object + * + * Prepares qemuBlockStorageSourceChainDataPtr for attaching of @top image only + * via -blockdev. + */ +qemuBlockStorageSourceChainDataPtr +qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSourcePtr top, + virQEMUCapsPtr qemuCaps) +{ + return qemuBuildStorageSourceChainAttachPrepareBlockdevInternal(top, qemuCaps, + true); +} diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 8695832c16..7e2dc5a60a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -117,6 +117,9 @@ qemuBlockStorageSourceChainDataPtr qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSourcePtr top, virQEMUCapsPtr qemuCaps); +qemuBlockStorageSourceChainDataPtr +qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSourcePtr top, + virQEMUCapsPtr qemuCaps); char *qemuBuildDiskDeviceStr(const virDomainDef *def, -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:46PM +0200, Peter Krempa wrote:
qemuBuildStorageSourceChainAttachPrepareBlockdev prepares the full backing chain for attachment via blockdev. For snapshots we'll need to prepare one image only as it needs to be plugged on top of the existing chain.
This patch introduces qemuBuildStorageSourceChainAttachPrepareBlockdevTop which prepares only @top similarly to the original function by splitting out the functionality into an internal function so that the API does not change.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 51 +++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 +++ 2 files changed, 44 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
For the modern use cases we are going to use 'blockdev-snapshot' instead of 'blockdev-snapshot-sync'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 15 +++++++++++++++ src/qemu/qemu_block.h | 5 +++++ 2 files changed, 20 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 7145a2a99d..e47bc2d8f0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1886,6 +1886,21 @@ qemuBlockSnapshotAddLegacy(virJSONValuePtr actions, } +int +qemuBlockSnapshotAddBlockdev(virJSONValuePtr actions, + virDomainDiskDefPtr disk, + virStorageSourcePtr newsrc) +{ + if (qemuMonitorJSONTransactionAdd(actions, "blockdev-snapshot", + "s:node", disk->src->nodeformat, + "s:overlay", newsrc->nodeformat, + NULL) < 0) + return -1; + + return 0; +} + + /** * qemuBlockStorageGetCopyOnReadProps: * @disk: disk with copy-on-read enabled diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index a5f6a3c75b..5fe5319ab9 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -163,6 +163,11 @@ qemuBlockSnapshotAddLegacy(virJSONValuePtr actions, virStorageSourcePtr newsrc, bool reuse); +int +qemuBlockSnapshotAddBlockdev(virJSONValuePtr actions, + virDomainDiskDefPtr disk, + virStorageSourcePtr newsrc); + int qemuBlockStorageSourceCreateGetFormatProps(virStorageSourcePtr src, virStorageSourcePtr backing, -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:47PM +0200, Peter Krempa wrote:
For the modern use cases we are going to use 'blockdev-snapshot' instead of 'blockdev-snapshot-sync'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 15 +++++++++++++++ src/qemu/qemu_block.h | 5 +++++ 2 files changed, 20 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The block commit API checked 'disk->src->path' to see whether there is a reasonable disk source to be committed. As the top image can be e.g. backed by NBD the check is not good enough. Replace it by virStorageSourceIsEmpty. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 482f915b67..042efae49f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17949,7 +17949,7 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!(device = qemuAliasDiskDriveFromDisk(disk))) goto endjob; - if (!disk->src->path) { + if (virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk %s has no source file to be committed"), disk->dst); -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:48PM +0200, Peter Krempa wrote:
The block commit API checked 'disk->src->path' to see whether there is a reasonable disk source to be committed. As the top image can be e.g. backed by NBD the check is not good enough. Replace it by virStorageSourceIsEmpty.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In case when the backing store can be represented with something simpler such as an URI we can use it rather than falling back to the json: pseudo-protocol. In cases when it's not worth it (e.g. with the old ugly NBD or RBD strings let's switch to json). The function is exported as we'll need it when overwriting the ugly strings qemu would come up with during blockjobs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 93 ++++++++++++++----- src/qemu/qemu_block.h | 4 + .../imagecreate/qcow2-backing-raw-nbd.json | 2 +- 3 files changed, 76 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index e47bc2d8f0..47661fb8bd 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1923,15 +1923,80 @@ qemuBlockStorageGetCopyOnReadProps(virDomainDiskDefPtr disk) } +/** + * qemuBlockGetBackingStoreString: + * @src: storage source to get the string for + * + * Formats a string used in the backing store field of a disk image which + * supports backing store. Non-local storage may result in use of the json: + * pseudo protocol for any complex configuration. + */ +char * +qemuBlockGetBackingStoreString(virStorageSourcePtr src) +{ + int actualType = virStorageSourceGetActualType(src); + VIR_AUTOPTR(virJSONValue) backingProps = NULL; + VIR_AUTOPTR(virURI) uri = NULL; + VIR_AUTOFREE(char *) backingJSON = NULL; + char *ret = NULL; + + if (virStorageSourceIsLocalStorage(src)) { + ignore_value(VIR_STRDUP(ret, src->path)); + return ret; + } + + /* generate simplified URIs for the easy cases */ + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->nhosts == 1 && + src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_NBD: + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + if (!(uri = qemuBlockStorageSourceGetURI(src))) + return NULL; + + if (!(ret = virURIFormat(uri))) + return NULL; + + return ret; + + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_LAST: + case VIR_STORAGE_NET_PROTOCOL_NONE: + break; + } + } + + /* use json: pseudo protocol otherwise */ + if (!(backingProps = qemuBlockStorageSourceGetBackendProps(src, false, true, false))) + return NULL; + + if (!(backingJSON = virJSONValueToString(backingProps, false))) + return NULL; + + if (virAsprintf(&ret, "json:%s", backingJSON) < 0) + return NULL; + + return ret; +} + + static int qemuBlockStorageSourceCreateAddBacking(virStorageSourcePtr backing, virJSONValuePtr props, bool format) { - VIR_AUTOPTR(virJSONValue) backingProps = NULL; - VIR_AUTOFREE(char *) backingJSON = NULL; - VIR_AUTOFREE(char *) backingPseudoprotocol = NULL; - const char *backingFileStr = NULL; + VIR_AUTOFREE(char *) backingFileStr = NULL; const char *backingFormatStr = NULL; if (!virStorageSourceIsBacking(backing)) @@ -1945,24 +2010,8 @@ qemuBlockStorageSourceCreateAddBacking(virStorageSourcePtr backing, backingFormatStr = virStorageFileFormatTypeToString(backing->format); } - if (virStorageSourceIsLocalStorage(backing)) { - backingFileStr = backing->path; - } else { - if (!(backingProps = qemuBlockStorageSourceGetBackendProps(backing, false, - true, false))) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("failed to generate backing file JSON properties")); - return -1; - } - - if (!(backingJSON = virJSONValueToString(backingProps, false))) - return -1; - - if (virAsprintf(&backingPseudoprotocol, "json:%s", backingJSON) < 0) - return -1; - - backingFileStr = backingPseudoprotocol; - } + if (!(backingFileStr = qemuBlockGetBackingStoreString(backing))) + return -1; if (virJSONValueObjectAdd(props, "S:backing-file", backingFileStr, diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 5fe5319ab9..ab6b9dc791 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -168,6 +168,10 @@ qemuBlockSnapshotAddBlockdev(virJSONValuePtr actions, virDomainDiskDefPtr disk, virStorageSourcePtr newsrc); +char * +qemuBlockGetBackingStoreString(virStorageSourcePtr src) + ATTRIBUTE_NONNULL(1); + int qemuBlockStorageSourceCreateGetFormatProps(virStorageSourcePtr src, virStorageSourcePtr backing, diff --git a/tests/qemublocktestdata/imagecreate/qcow2-backing-raw-nbd.json b/tests/qemublocktestdata/imagecreate/qcow2-backing-raw-nbd.json index 34ce74a548..b9d1d97302 100644 --- a/tests/qemublocktestdata/imagecreate/qcow2-backing-raw-nbd.json +++ b/tests/qemublocktestdata/imagecreate/qcow2-backing-raw-nbd.json @@ -10,6 +10,6 @@ format: "driver": "qcow2", "file": "0123456789ABCDEF0123456789ABCDE", "size": 1337, - "backing-file": "json:{\"driver\":\"nbd\",\"server\":{\"type\":\"inet\",\"host\":\"example.com\",\"port\":\"1234\"}}", + "backing-file": "nbd://example.com:1234", "backing-fmt": "raw" } -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:49PM +0200, Peter Krempa wrote:
In case when the backing store can be represented with something simpler such as an URI we can use it rather than falling back to the
a URI
json: pseudo-protocol.
In cases when it's not worth it (e.g. with the old ugly NBD or RBD strings let's switch to json).
the parenthesis should end earlier
The function is exported as we'll need it when overwriting the ugly strings qemu would come up with during blockjobs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 93 ++++++++++++++----- src/qemu/qemu_block.h | 4 + .../imagecreate/qcow2-backing-raw-nbd.json | 2 +- 3 files changed, 76 insertions(+), 23 deletions(-)
This would have been easier to read if you had split the functional change from code movement. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuDomainSnapshotDiskDataCollect copies the source of the disk from the live config into the inactive config. Move this operation earlier so that if we initialize it for use for the particular instance the run-time-only data is not copied. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 042efae49f..69327c148a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15114,6 +15114,22 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) goto cleanup; + /* Note that it's unsafe to assume that the disks in the persistent + * definition match up with the disks in the live definition just by + * checking that the target name is the same. We've done that + * historically this way though. */ + if (vm->newDef && + (dd->persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, + false))) { + + if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) + goto cleanup; + + if (virStorageSourceInitChainElement(dd->persistsrc, + dd->persistdisk->src, false) < 0) + goto cleanup; + } + if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) goto cleanup; @@ -15131,22 +15147,6 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, VIR_FREE(backingStoreStr); } } - - /* Note that it's unsafe to assume that the disks in the persistent - * definition match up with the disks in the live definition just by - * checking that the target name is the same. We've done that - * historically this way though. */ - if (vm->newDef && - (dd->persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, - false))) { - - if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(dd->persistsrc, - dd->persistdisk->src, false) < 0) - goto cleanup; - } } VIR_STEAL_PTR(*rdata, data); -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:50PM +0200, Peter Krempa wrote:
qemuDomainSnapshotDiskDataCollect copies the source of the disk from the live config into the inactive config. Move this operation earlier so that if we initialize it for use for the particular instance the run-time-only data is not copied.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When returning to asynchronous block job handling the flag which determines the handling method should be reset prior to flushing outstanding events. If there's an event to process the handler may invoke the monitor and another event may be received. We'd not process that one. Reset the flag earlier. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 292610d089..dfa55a0e7a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -808,8 +808,8 @@ qemuBlockJobSyncEnd(virDomainObjPtr vm, diskdst = job->disk->dst; VIR_DEBUG("disk=%s", NULLSTR(diskdst)); - qemuBlockJobUpdate(vm, job, asyncJob); job->synchronous = false; + qemuBlockJobUpdate(vm, job, asyncJob); } -- 2.21.0
On Tue, Jul 23, 2019 at 02:08:51PM +0200, Peter Krempa wrote:
When returning to asynchronous block job handling the flag which determines the handling method should be reset prior to flushing outstanding events. If there's an event to process the handler may invoke the monitor and another event may be received. We'd not process that one. Reset the flag earlier.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa