[libvirt PATCH 0/6] [RFC] qemu: implement support for pulling into non-active layers
QEMU's block-stream command supports pulling ("streaming" in QMP lingo) into arbitrary layer, however libvirt can only pull into an active layer. This series is a proof-of-concept work to extend libvirt's pull to support pulling into non-active layers as well. While this doesn't seem to be a much demanded feature by itself, it can be seen as a prerequisite to implementing full support for external snapshots in snapshot-delete(*) as that appears to be implementable in terms of a full-featured pull. As stated above, this is a proof-of-concept work so admittedly not much effort went into fully polishing the series. The expectation is that the series will have to change heavily anyway before it's good enough to be pushed. (*) https://bugzilla.redhat.com/show_bug.cgi?id=1519001 Pavel Mores (6): qemu: block: add 'top' parameter to qemuDomainBlockPullCommon() qemu: block: pull job extended with 'top' parameter qemu: block: blockpull param 'top' support in virsh proper qemu: block: blockpull param 'top' support in driver and RPC qemu: block: 'top' support in construction of QMP block-stream qemu: block: extend pull job completion with 'top' handling include/libvirt/libvirt-domain.h | 4 ++-- src/driver-hypervisor.h | 1 + src/libvirt-domain.c | 7 ++++--- src/qemu/qemu_blockjob.c | 35 ++++++++++++++++++++++++++++---- src/qemu/qemu_blockjob.h | 2 ++ src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++------- src/remote/remote_protocol.x | 1 + src/remote_protocol-structs | 1 + tools/virsh-domain.c | 14 ++++++++++--- 9 files changed, 79 insertions(+), 19 deletions(-) -- 2.24.1
qemuDomainBlockPullCommon() is one of the functions in block pull implementation that have to support 'top' to make block pull as a whole to support it. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e9a62684f0..81cca360e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17311,6 +17311,7 @@ static int qemuDomainBlockPullCommon(virDomainObjPtr vm, const char *path, const char *base, + const char *top, unsigned long bandwidth, unsigned int flags) { @@ -17322,6 +17323,9 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; g_autofree char *basePath = NULL; + virStorageSourcePtr topSource = NULL; + unsigned int topIndex = 0; + g_autofree char *topPath = NULL; g_autofree char *backingPath = NULL; unsigned long long speed = bandwidth; qemuBlockJobDataPtr job = NULL; @@ -17355,6 +17359,12 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, base, baseIndex, NULL)))) goto endjob; + if (top && + (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || + !(topSource = virStorageFileChainLookup(disk->src, disk->src, + top, topIndex, NULL)))) + goto endjob; + if (baseSource) { if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { @@ -17388,7 +17398,7 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, speed <<= 20; } - if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, flags))) + if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, /*topSource, */flags))) goto endjob; if (blockdev) { @@ -18160,7 +18170,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) - return qemuDomainBlockPullCommon(vm, path, base, bandwidth, flags); + return qemuDomainBlockPullCommon(vm, path, base, NULL, bandwidth, flags); /* If we got here, we are doing a block copy rebase. */ if (!(dest = virStorageSourceNew())) @@ -18311,7 +18321,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, } /* qemuDomainBlockPullCommon consumes the reference on @vm */ - return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); + return qemuDomainBlockPullCommon(vm, path, NULL, NULL, bandwidth, flags); } -- 2.24.1
'top' member added to qemuBlockJobPullData and the job's "constructor" function qemuBlockJobDiskNewPull() and its invocation changed to support the new member. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_blockjob.h | 2 ++ src/qemu/qemu_driver.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 71df0d1ab2..e19a2ad76b 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -253,6 +253,7 @@ qemuBlockJobDataPtr qemuBlockJobDiskNewPull(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr base, + virStorageSourcePtr top, unsigned int jobflags) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -270,6 +271,7 @@ qemuBlockJobDiskNewPull(virDomainObjPtr vm, return NULL; job->data.pull.base = base; + job->data.pull.top = top; job->jobflags = jobflags; if (qemuBlockJobRegister(job, vm, disk, true) < 0) diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 72c7fa053e..7223b4abb8 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -77,6 +77,7 @@ typedef qemuBlockJobPullData *qemuBlockJobDataPullPtr; struct _qemuBlockJobPullData { virStorageSourcePtr base; + virStorageSourcePtr top; }; @@ -177,6 +178,7 @@ qemuBlockJobDataPtr qemuBlockJobDiskNewPull(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr base, + virStorageSourcePtr top, unsigned int jobflags); qemuBlockJobDataPtr diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 81cca360e0..7970c913f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17398,7 +17398,7 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, speed <<= 20; } - if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, /*topSource, */flags))) + if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, topSource, flags))) goto endjob; if (blockdev) { -- 2.24.1
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase(). Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b440818ec2..069d7f98ec 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2560,8 +2560,8 @@ typedef enum { } virDomainBlockRebaseFlags; int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags); /** * virDomainBlockCopyFlags: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 65813b68cc..1f9d1b5b84 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file + * @top: path to top file, or device shorthand, or NULL for no top * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9d0f7d68d2..47ccdd1b94 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2436,7 +2436,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) if (bytes) flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; - if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) + if (virDomainBlockRebase(dom, path, dest, NULL, bandwidth, flags) < 0) goto cleanup; } @@ -2765,6 +2765,10 @@ static const vshCmdOptDef opts_blockpull[] = { .type = VSH_OT_STRING, .help = N_("path of backing file in chain for a partial pull") }, + {.name = "top", + .type = VSH_OT_STRING, + .help = N_("path of top backing file in chain for a partial pull") + }, {.name = "wait", .type = VSH_OT_BOOL, .help = N_("wait for job to finish") @@ -2804,6 +2808,7 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd) int timeout = 0; const char *path = NULL; const char *base = NULL; + const char *top = NULL; unsigned long bandwidth = 0; unsigned int flags = 0; virshBlockJobWaitDataPtr bjWait = NULL; @@ -2817,6 +2822,9 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) return false; + if (vshCommandOptStringReq(ctl, cmd, "top", &top) < 0) + return false; + if (vshBlockJobOptionBandwidth(ctl, cmd, bytes, &bandwidth) < 0) return false; @@ -2834,11 +2842,11 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd) verbose, timeout, async))) goto cleanup; - if (base || flags) { + if (base || top || flags) { if (bytes) flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; - if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) + if (virDomainBlockRebase(dom, path, base, top, bandwidth, flags) < 0) goto cleanup; } else { if (bytes) -- 2.24.1
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b440818ec2..069d7f98ec 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2560,8 +2560,8 @@ typedef enum { } virDomainBlockRebaseFlags;
int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags);
/** * virDomainBlockCopyFlags: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 65813b68cc..1f9d1b5b84 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file + * @top: path to top file, or device shorthand, or NULL for no top * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function. If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b440818ec2..069d7f98ec 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2560,8 +2560,8 @@ typedef enum { } virDomainBlockRebaseFlags;
int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags);
/** * virDomainBlockCopyFlags: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 65813b68cc..1f9d1b5b84 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file + * @top: path to top file, or device shorthand, or NULL for no top * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function.
If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
Yes. Seeing as the function takes just a bunch of primitive data types and a virDomainPtr, I'm afraid I don't see much room for not changing the signature, apart from arguably dubious tricks like stuffing both base and top to the existing 'base' parameter and parsing the individual values out of it in the function body. Any convention or suggestion to help pick a good name for the new function? Thanks, pvl
On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
[...]
@@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function.
If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
Yes. Seeing as the function takes just a bunch of primitive data types and a virDomainPtr, I'm afraid I don't see much room for not changing the signature, apart from arguably dubious tricks like stuffing both base and top to the existing 'base' parameter and parsing the individual values out of it in the function body.
Any convention or suggestion to help pick a good name for the new function?
If we are going to implement new APIs for blockjobs I'd prefer we take a step back and re-design the (block)-job APIs rather than add yet another oldschool one. Similarly to our criticisms of qemu's apis which didn't preserve the job state after the job finished, which is now fixed I've heard exactly the same requests from at least oVirt developers requesting we do the same thing. The new API(s) should thus return a job object (virDomainJob?) similarly to how the domain lookup APIs return them. This new object then can be used to refer to the job even after it has finished. Similarly to qemu we should also introduce provisions to auto-dismiss the job if the management APP doesn't wish to have to deal with it (and that probably should be default behaviour given how we approached it previously). This will then obviously require support APIs for looking up/listing jobs, getting the state or configuration etc. This is obviously a lot of work, thus we need to decide whether adding an old-school API is worth it in the inerim. Are there any real users who would benefit from the new pull semantics? blockpull is around for a long time already, but it seems that commit is favoured. If there is no real demand though I'd probably prefer if we don't add any more block job APIs any more.
On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
[...]
@@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function.
If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
Yes. Seeing as the function takes just a bunch of primitive data types and a virDomainPtr, I'm afraid I don't see much room for not changing the signature, apart from arguably dubious tricks like stuffing both base and top to the existing 'base' parameter and parsing the individual values out of it in the function body.
Any convention or suggestion to help pick a good name for the new function?
If we are going to implement new APIs for blockjobs I'd prefer we take a step back and re-design the (block)-job APIs rather than add yet another oldschool one.
Similarly to our criticisms of qemu's apis which didn't preserve the job state after the job finished, which is now fixed I've heard exactly the same requests from at least oVirt developers requesting we do the same thing.
The new API(s) should thus return a job object (virDomainJob?) similarly to how the domain lookup APIs return them. This new object then can be used to refer to the job even after it has finished. Similarly to qemu we should also introduce provisions to auto-dismiss the job if the management APP doesn't wish to have to deal with it (and that probably should be default behaviour given how we approached it previously).
This will then obviously require support APIs for looking up/listing jobs, getting the state or configuration etc.
This is obviously a lot of work, thus we need to decide whether adding an old-school API is worth it in the inerim. Are there any real users who would benefit from the new pull semantics? blockpull is around for a long time already, but it seems that commit is favoured.
If there is no real demand though I'd probably prefer if we don't add any more block job APIs any more.
I'm not aware of any real demand for this, however as I stated in the cover letter I believe I need full blockpull to deal with the bug I'm actually working on, which is full support for external snapshots in snapshot-delete. Considering what you said, the "dubious trick" I mentioned in the other branch of this thread might be a bearable interim solution... pvl
On Wed, Mar 04, 2020 at 15:01:07 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
[...]
This is obviously a lot of work, thus we need to decide whether adding an old-school API is worth it in the inerim. Are there any real users who would benefit from the new pull semantics? blockpull is around for a long time already, but it seems that commit is favoured.
If there is no real demand though I'd probably prefer if we don't add any more block job APIs any more.
I'm not aware of any real demand for this, however as I stated in the cover letter I believe I need full blockpull to deal with the bug I'm actually working on, which is full support for external snapshots in snapshot-delete.
Deleting/reverting external snapshots needs to be done internally under the hood of virDomainRevertToSnapshot/virDomainSnapshotDelete so if you require use of the 'block-stream' command to an intermediate layer you don't actually need to expose it via virDomainBlockPull/Rebase to take advantage of it. For reversion of external snapshots you'll probably need a new API anyways as you'll need to be able to specify a new set of disk images to hold the writes.
On Wed, Mar 04, 2020 at 03:13:41PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 15:01:07 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
[...]
This is obviously a lot of work, thus we need to decide whether adding an old-school API is worth it in the inerim. Are there any real users who would benefit from the new pull semantics? blockpull is around for a long time already, but it seems that commit is favoured.
If there is no real demand though I'd probably prefer if we don't add any more block job APIs any more.
I'm not aware of any real demand for this, however as I stated in the cover letter I believe I need full blockpull to deal with the bug I'm actually working on, which is full support for external snapshots in snapshot-delete.
Deleting/reverting external snapshots needs to be done internally under the hood of virDomainRevertToSnapshot/virDomainSnapshotDelete so if you require use of the 'block-stream' command to an intermediate layer you don't actually need to expose it via virDomainBlockPull/Rebase to take advantage of it.
That's true. I think this basically means I should drop patches 3 a 4 from this series (I'll keep them locally as they give me a reasonably easy way to test my changes), right?
For reversion of external snapshots you'll probably need a new API anyways as you'll need to be able to specify a new set of disk images to hold the writes.
Okay, I'll deal with that in due time, I guess when I'm back to working on snapshot-delete. pvl
On Wed, Mar 04, 2020 at 02:41:09PM +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b440818ec2..069d7f98ec 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2560,8 +2560,8 @@ typedef enum { } virDomainBlockRebaseFlags;
int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags);
/** * virDomainBlockCopyFlags: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 65813b68cc..1f9d1b5b84 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file + * @top: path to top file, or device shorthand, or NULL for no top * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function.
If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
Yes. Seeing as the function takes just a bunch of primitive data types and a virDomainPtr, I'm afraid I don't see much room for not changing the signature, apart from arguably dubious tricks like stuffing both base and top to the existing 'base' parameter and parsing the individual values out of it in the function body.
Actually, on second thought, it might not be as dubious as I first thought. Certainly not as clean as just adding a parameter in general but depending on what the cost of adding a new API would be, reusing the existing 'base' param might be workable. Also, how about the RPC change, is that acceptable? Thanks, pvl
On Wed, Mar 04, 2020 at 14:55:30 +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 02:41:09PM +0100, Pavel Mores wrote:
On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
A new command-line option --top was added to virsh's blockpull command. Similar to how --base is handled, in presence of --top the operation is implemented internally as a rebase. To that end, a corresponding new 'top' parameter was added to virDomainBlockRebase().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 5 +++-- tools/virsh-domain.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b440818ec2..069d7f98ec 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2560,8 +2560,8 @@ typedef enum { } virDomainBlockRebaseFlags;
int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags);
/** * virDomainBlockCopyFlags: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 65813b68cc..1f9d1b5b84 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file + * @top: path to top file, or device shorthand, or NULL for no top * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, */ int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags) + const char *base, const char *top, + unsigned long bandwidth, unsigned int flags) { virConnectPtr conn;
So this is one thing we can't do. This modifies the public API of libvirt and would cause software which is already compiled to pass wrong arguments to this function.
If the semantics can't be mapped to existing arguments of this function we'll need to add a new function in the first place.
Yes. Seeing as the function takes just a bunch of primitive data types and a virDomainPtr, I'm afraid I don't see much room for not changing the signature, apart from arguably dubious tricks like stuffing both base and top to the existing 'base' parameter and parsing the individual values out of it in the function body.
Actually, on second thought, it might not be as dubious as I first thought. Certainly not as clean as just adding a parameter in general but depending on what the cost of adding a new API would be, reusing the existing 'base' param might be workable.
That's gross. Please don't do that.
Also, how about the RPC change, is that acceptable?
No, that's on same par as API.
Thanks,
pvl
Rebase implementation in QEMU driver extended with a new 'top' parameter. Support for 'top' added to RPC's remote_domain_block_rebase_args structure. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/driver-hypervisor.h | 1 + src/libvirt-domain.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/remote/remote_protocol.x | 1 + src/remote_protocol-structs | 1 + 5 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index bce023017d..51a25c41e3 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1053,6 +1053,7 @@ typedef int (*virDrvDomainBlockRebase)(virDomainPtr dom, const char *path, const char *base, + const char *top, unsigned long bandwidth, unsigned int flags); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 1f9d1b5b84..33e307cb26 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -10287,7 +10287,7 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, if (conn->driver->domainBlockRebase) { int ret; - ret = conn->driver->domainBlockRebase(dom, disk, base, bandwidth, + ret = conn->driver->domainBlockRebase(dom, disk, base, top, bandwidth, flags); if (ret < 0) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7970c913f3..b3f97bf593 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18143,7 +18143,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, static int qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, - unsigned long bandwidth, unsigned int flags) + const char *top, unsigned long bandwidth, + unsigned int flags) { virDomainObjPtr vm; int ret = -1; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d4393680e9..5f5a921057 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1414,6 +1414,7 @@ struct remote_domain_block_rebase_args { remote_nonnull_domain dom; remote_nonnull_string path; remote_string base; + remote_string top; unsigned hyper bandwidth; unsigned int flags; }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index bae0f0b545..b317ec9102 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -992,6 +992,7 @@ struct remote_domain_block_rebase_args { remote_nonnull_domain dom; remote_nonnull_string path; remote_string base; + remote_string top; uint64_t bandwidth; u_int flags; }; -- 2.24.1
This commit only handles submission of block-stream command. Since block-stream is a job, we'll need to handle its completion as well in an upcoming commit. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b3f97bf593..b1ffc68e6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17410,7 +17410,15 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, !(backingPath = qemuBlockGetBackingStoreString(baseSource))) goto endjob; } - device = disk->src->nodeformat; + if (topSource) { + device = topSource->nodeformat; + if (qemuDomainStorageSourceAccessAllow(driver, vm, topSource, false, false) < 0) { + virReportError(VIR_ERR_ACCESS_DENIED, "can't make 'top' writable"); + goto endjob; + } + } else { + device = disk->src->nodeformat; + } } else { device = job->name; } @@ -18171,7 +18179,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) - return qemuDomainBlockPullCommon(vm, path, base, NULL, bandwidth, flags); + return qemuDomainBlockPullCommon(vm, path, base, top, bandwidth, flags); /* If we got here, we are doing a block copy rebase. */ if (!(dest = virStorageSourceNew())) @@ -18302,8 +18310,8 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, static int -qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, - unsigned int flags) +qemuDomainBlockPull(virDomainPtr dom, const char *path, + unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); -- 2.24.1
'top' handling basically requires two changes: besides the obvious adjustments to the code that fixes our backing chains (for both active and inactive configuration) after a pull, we need to make the top image read-only again if it's not identical with the active layer (it had to be made writable before submitting QMP block-stream to allow QEMU to fix the backing chain in top's qcow2). Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index e19a2ad76b..2e53821d43 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -962,6 +962,7 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, virDomainDiskDefPtr cfgdisk = NULL; virStorageSourcePtr cfgbase = NULL; virStorageSourcePtr cfgbaseparent = NULL; + virStorageSourcePtr cfgtop = NULL; virStorageSourcePtr n; virStorageSourcePtr tmp; @@ -994,16 +995,40 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, } } - tmp = job->disk->src->backingStore; - job->disk->src->backingStore = job->data.pull.base; + if (job->data.pull.top) { + cfgtop = cfgdisk->src->backingStore; + for (n = job->disk->src->backingStore; n && n != job->data.pull.top; n = n->backingStore) { + if (cfgtop) + cfgtop = cfgtop->backingStore; + } + } + + if (job->data.pull.top && job->data.pull.top != job->disk->src) { + if (qemuDomainStorageSourceAccessAllow(driver, vm, job->data.pull.top, true, false) < 0) { + virReportError(VIR_ERR_ACCESS_DENIED, "can't reset 'top' to read-only"); + } + } + + if (job->data.pull.top) { + tmp = job->data.pull.top->backingStore; + job->data.pull.top->backingStore = job->data.pull.base; + } else { + tmp = job->disk->src->backingStore; + job->disk->src->backingStore = job->data.pull.base; + } if (baseparent) baseparent->backingStore = NULL; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp); virObjectUnref(tmp); if (cfgdisk) { - tmp = cfgdisk->src->backingStore; - cfgdisk->src->backingStore = cfgbase; + if (job->data.pull.top) { + tmp = cfgtop->backingStore; + cfgtop->backingStore = cfgbase; + } else { + tmp = cfgdisk->src->backingStore; + cfgdisk->src->backingStore = cfgbase; + } if (cfgbaseparent) cfgbaseparent->backingStore = NULL; virObjectUnref(tmp); -- 2.24.1
participants (2)
-
Pavel Mores -
Peter Krempa