2:52 p.m.
This will allow us to start or stop nbdkit for just a single disk source
or for every source in the backing chain. This will be used in following
patches.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_extdevice.c | 8 +++---
src/qemu/qemu_hotplug.c | 6 ++---
src/qemu/qemu_nbdkit.c | 51 ++++++++++++++++++++++++++++++---------
src/qemu/qemu_nbdkit.h | 6 +++--
4 files changed, 51 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 3cf3867056..ed5976d1f7 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -234,12 +234,12 @@ qemuExtDevicesStart(virQEMUDriver *driver,
for (i = 0; i < def->ndisks; i++) {
virDomainDiskDef *disk = def->disks[i];
- if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0)
+ if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
return -1;
}
if (def->os.loader && def->os.loader->nvram) {
- if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram) <
0)
+ if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram, true)
< 0)
return -1;
}
@@ -297,11 +297,11 @@ qemuExtDevicesStop(virQEMUDriver *driver,
for (i = 0; i < def->ndisks; i++) {
virDomainDiskDef *disk = def->disks[i];
- qemuNbdkitStopStorageSource(disk->src, vm);
+ qemuNbdkitStopStorageSource(disk->src, vm, true);
}
if (def->os.loader && def->os.loader->nvram)
- qemuNbdkitStopStorageSource(def->os.loader->nvram, vm);
+ qemuNbdkitStopStorageSource(def->os.loader->nvram, vm, true);
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 31b00e05ca..e67673b762 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1018,7 +1018,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0)
goto cleanup;
- if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0)
+ if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
goto cleanup;
}
@@ -1045,7 +1045,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
if (virStorageSourceChainHasManagedPR(disk->src))
ignore_value(qemuHotplugRemoveManagedPR(vm, VIR_ASYNC_JOB_NONE));
- qemuNbdkitStopStorageSource(disk->src, vm);
+ qemuNbdkitStopStorageSource(disk->src, vm, true);
}
qemuDomainSecretDiskDestroy(disk);
qemuDomainCleanupStorageSourceFD(disk->src);
@@ -4562,7 +4562,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,
qemuHotplugRemoveManagedPR(vm, VIR_ASYNC_JOB_NONE) < 0)
goto cleanup;
- qemuNbdkitStopStorageSource(disk->src, vm);
+ qemuNbdkitStopStorageSource(disk->src, vm, true);
if (disk->transient) {
VIR_DEBUG("Removing transient overlay '%s' of disk
'%s'",
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 1c72b6fe6a..39f9c58a48 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -893,18 +893,34 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps
WITHOUT_NBDKIT_UNUSED,
}
+static int
+qemuNbdkitStartStorageSourceOne(virQEMUDriver *driver,
+ virDomainObj *vm,
+ virStorageSource *src)
+{
+ qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+
+ if (priv && priv->nbdkitProcess &&
+ qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0)
+ return -1;
+
+ return 0;
+}
+
+
int
qemuNbdkitStartStorageSource(virQEMUDriver *driver,
virDomainObj *vm,
- virStorageSource *src)
+ virStorageSource *src,
+ bool chain)
{
virStorageSource *backing;
- for (backing = src; backing != NULL; backing = backing->backingStore) {
- qemuDomainStorageSourcePrivate *priv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing);
+ if (!chain)
+ return qemuNbdkitStartStorageSourceOne(driver, vm, src);
- if (priv && priv->nbdkitProcess &&
- qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0)
+ for (backing = src; backing != NULL; backing = backing->backingStore) {
+ if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0)
return -1;
}
@@ -912,18 +928,31 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver,
}
+static void
+qemuNbdkitStopStorageSourceOne(virStorageSource *src,
+ virDomainObj *vm)
+{
+ qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+
+ if (priv && priv->nbdkitProcess &&
+ qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0)
+ VIR_WARN("Unable to stop nbdkit for storage source '%s'",
+ qemuBlockStorageSourceGetStorageNodename(src));
+}
+
+
void
qemuNbdkitStopStorageSource(virStorageSource *src,
- virDomainObj *vm)
+ virDomainObj *vm,
+ bool chain)
{
virStorageSource *backing;
- for (backing = src; backing != NULL; backing = backing->backingStore) {
- qemuDomainStorageSourcePrivate *priv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing);
+ if (! chain)
+ return qemuNbdkitStopStorageSourceOne(src, vm);
- if (priv && priv->nbdkitProcess &&
- qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0)
- VIR_WARN("Unable to stop nbdkit for storage source '%s'",
qemuBlockStorageSourceGetStorageNodename(src));
+ for (backing = src; backing != NULL; backing = backing->backingStore) {
+ qemuNbdkitStopStorageSourceOne(backing, vm);
}
}
diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
index 853b2cca6f..637bf962a7 100644
--- a/src/qemu/qemu_nbdkit.h
+++ b/src/qemu/qemu_nbdkit.h
@@ -63,11 +63,13 @@ qemuNbdkitReconnectStorageSource(virStorageSource *source,
int
qemuNbdkitStartStorageSource(virQEMUDriver *driver,
virDomainObj *vm,
- virStorageSource *src);
+ virStorageSource *src,
+ bool chain);
void
qemuNbdkitStopStorageSource(virStorageSource *src,
- virDomainObj *vm);
+ virDomainObj *vm,
+ bool chain);
int
qemuNbdkitStorageSourceManageProcess(virStorageSource *src,
--
2.43.0
2:52 p.m.
New subject: [PATCH 2/3] qemu: roll back if not all nbdkit backends are successful
When starting nbdkit processes for the backing store of a disk, we were
returning an error if any backing store failed, but we were not cleaning
up processes that succeeded higher in the chain. Make sure that if we
return a failure status from qemuNbdkitStartStorageSource() that we roll
back any processes that had been started.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_nbdkit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 39f9c58a48..edbe2137d0 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver,
return qemuNbdkitStartStorageSourceOne(driver, vm, src);
for (backing = src; backing != NULL; backing = backing->backingStore) {
- if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0)
+ if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) {
+ /* roll back any previously-started sources */
+ qemuNbdkitStopStorageSource(src, vm, true);
return -1;
+ }
}
return 0;
--
2.43.0
2:30 a.m.
New subject: [PATCH 2/3] qemu: roll back if not all nbdkit backends are
successful
On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
When starting nbdkit processes for the backing store of a disk, we
were
returning an error if any backing store failed, but we were not cleaning
up processes that succeeded higher in the chain. Make sure that if we
return a failure status from qemuNbdkitStartStorageSource() that we roll
back any processes that had been started.
qemuNbdkitStartStorageSource is called from:
src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver,
src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
disk->src, true) < 0)
src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
def->os.loader->nvram, true) < 0)
The counterpart is qemuExtDevicesStop.
src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src,
true) < 0)
This has cleanup inline.
Thus I don't see a possibility where what you describe in the commit
message can happen.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_nbdkit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 39f9c58a48..edbe2137d0 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver,
return qemuNbdkitStartStorageSourceOne(driver, vm, src);
for (backing = src; backing != NULL; backing = backing->backingStore) {
- if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0)
+ if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) {
+ /* roll back any previously-started sources */
+ qemuNbdkitStopStorageSource(src, vm, true);
return -1;
+ }
}
return 0;
--
2.43.0
_______________________________________________
Devel mailing list -- devel(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
3:11 p.m.
New subject: [PATCH 2/3] qemu: roll back if not all nbdkit backends are
successful
On 1/29/24 2:30 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
> When starting nbdkit processes for the backing store of a disk, we were
> returning an error if any backing store failed, but we were not cleaning
> up processes that succeeded higher in the chain. Make sure that if we
> return a failure status from qemuNbdkitStartStorageSource() that we roll
> back any processes that had been started.
qemuNbdkitStartStorageSource is called from:
src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver,
src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
disk->src, true) < 0)
src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
def->os.loader->nvram, true) < 0)
The counterpart is qemuExtDevicesStop.
src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm,
disk->src, true) < 0)
This has cleanup inline.
Thus I don't see a possibility where what you describe in the commit
message can happen.
Oops it seems that i never actually responded to this comment. Perhaps
because I was waiting for a resolution on patch 3.
The motivation for this patch was actually in anticipation of patch 3.
In that patch, I followed the existing pattern inside of
qemuDomainStorageSourceAccessModify() where we only set a 'revoke_foo'
variable if the initial 'allow' call succeeds. In other words, if a
function like qemuDomainStorageSourceAccessModifyNVMe() fails, we assume
that it has left things in a consistent state and doesn't need to be
revoked in the error path. So I followed the same pattern for this function.
If you think it would be better, I could just squash this patch with
patch 3?
Jonathon
>
> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> ---
> src/qemu/qemu_nbdkit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 39f9c58a48..edbe2137d0 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver,
> return qemuNbdkitStartStorageSourceOne(driver, vm, src);
>
> for (backing = src; backing != NULL; backing = backing->backingStore) {
> - if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0)
> + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) {
> + /* roll back any previously-started sources */
> + qemuNbdkitStopStorageSource(src, vm, true);
> return -1;
> + }
> }
>
> return 0;
> --
> 2.43.0
> _______________________________________________
> Devel mailing list -- devel(a)lists.libvirt.org
> To unsubscribe send an email to devel-leave(a)lists.libvirt.org
6:51 a.m.
New subject: [PATCH 2/3] qemu: roll back if not all nbdkit backends are
successful
On Thu, Feb 08, 2024 at 15:11:25 -0600, Jonathon Jongsma wrote:
On 1/29/24 2:30 AM, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
> > When starting nbdkit processes for the backing store of a disk, we were
> > returning an error if any backing store failed, but we were not cleaning
> > up processes that succeeded higher in the chain. Make sure that if we
> > return a failure status from qemuNbdkitStartStorageSource() that we roll
> > back any processes that had been started.
>
> qemuNbdkitStartStorageSource is called from:
>
> src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver,
> src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
disk->src, true) < 0)
> src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm,
def->os.loader->nvram, true) < 0)
>
> The counterpart is qemuExtDevicesStop.
>
> src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver
*driver,
> src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm,
disk->src, true) < 0)
>
> This has cleanup inline.
>
> Thus I don't see a possibility where what you describe in the commit
> message can happen.
Oops it seems that i never actually responded to this comment. Perhaps
because I was waiting for a resolution on patch 3.
The motivation for this patch was actually in anticipation of patch 3. In
that patch, I followed the existing pattern inside of
qemuDomainStorageSourceAccessModify() where we only set a 'revoke_foo'
variable if the initial 'allow' call succeeds. In other words, if a function
like qemuDomainStorageSourceAccessModifyNVMe() fails, we assume that it has
left things in a consistent state and doesn't need to be revoked in the
error path. So I followed the same pattern for this function.
If you think it would be better, I could just squash this patch with patch
3?
Fair enough;
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
2:52 p.m.
New subject: [PATCH 3/3] qemu: handle adding/removing nbdkit-backed disk sources
Previously we were only starting or stopping nbdkit when the guest was
started or stopped or when hotplugging/unplugging a disk. But when doing
block operations, the disk backing store sources can also be be added or
removed independently of the disk device. When this happens the nbdkit
backend was not being handled properly. For example, when doing a
blockcopy from a nbdkit-backed disk to a new disk and pivoting to that
new location, the nbdkit process did not get cleaned up properly. Add
some functionality to qemuDomainStorageSourceAccessModify() to handle
this scenario.
Since we're now potentially starting nbdkit from another place in the
code, add a check to qemuNbdkitProcessStart() to report an error if we
are trying to start nbdkit for a disk source that already has a running
nbdkit process. This should never happen. If it does, it indicates an
error in another part of our code.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_domain.c | 10 ++++++++++
src/qemu/qemu_nbdkit.c | 7 +++++++
2 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index de36641137..973a9af0b6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8026,6 +8026,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
bool revoke_namespace = false;
bool revoke_nvme = false;
bool revoke_lockspace = false;
+ bool revoke_nbdkit = false;
VIR_DEBUG("src='%s' readonly=%d force_ro=%d force_rw=%d revoke=%d
chain=%d",
NULLSTR(src->path), src->readonly, force_ro, force_rw, revoke,
chain);
@@ -8044,6 +8045,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
revoke_namespace = true;
revoke_nvme = true;
revoke_lockspace = true;
+ revoke_nbdkit = true;
ret = 0;
goto revoke;
}
@@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
revoke_nvme = true;
+ if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0)
+ goto revoke;
+
+ revoke_nbdkit = true;
+
if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0)
goto revoke;
}
@@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
VIR_WARN("Unable to release lock on %s", srcstr);
}
+ if (revoke_nbdkit)
+ qemuNbdkitStopStorageSource(src, vm, chain);
+
cleanup:
src->readonly = was_readonly;
virErrorRestore(&orig_err);
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index edbe2137d0..73dc90af3e 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
struct nbd_handle *nbd = NULL;
#endif
+ /* don't try to start nbdkit again if we've already started it */
+ if (proc->pid > 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Attempting to start nbdkit twice"));
+ return -1;
+ }
+
if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
return -1;
--
2.43.0
2:25 a.m.
New subject: [PATCH 3/3] qemu: handle adding/removing nbdkit-backed disk
sources
On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
Previously we were only starting or stopping nbdkit when the guest
was
started or stopped or when hotplugging/unplugging a disk. But when doing
block operations, the disk backing store sources can also be be added or
removed independently of the disk device. When this happens the nbdkit
backend was not being handled properly. For example, when doing a
blockcopy from a nbdkit-backed disk to a new disk and pivoting to that
new location, the nbdkit process did not get cleaned up properly. Add
some functionality to qemuDomainStorageSourceAccessModify() to handle
this scenario.
Since we're now potentially starting nbdkit from another place in the
code, add a check to qemuNbdkitProcessStart() to report an error if we
are trying to start nbdkit for a disk source that already has a running
nbdkit process. This should never happen. If it does, it indicates an
error in another part of our code.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_domain.c | 10 ++++++++++
src/qemu/qemu_nbdkit.c | 7 +++++++
2 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index de36641137..973a9af0b6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
[...]
@@ -8059,6 +8061,11 @@
qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
revoke_nvme = true;
+ if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0)
+ goto revoke;
+
+ revoke_nbdkit = true;
+
Note that qemuDomainStorageSourceAccessModify can be called on already
existing 'virStorageSource' to e.g. change from read-only to read-write
mode or back.
[...]
if (qemuDomainNamespaceSetupDisk(vm, src,
&revoke_namespace) < 0)
goto revoke;
}
@@ -8113,6 +8120,9 @@
qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
VIR_WARN("Unable to release lock on %s", srcstr);
}
+ if (revoke_nbdkit)
+ qemuNbdkitStopStorageSource(src, vm, chain);
+
cleanup:
src->readonly = was_readonly;
virErrorRestore(&orig_err);
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index edbe2137d0..73dc90af3e 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
struct nbd_handle *nbd = NULL;
#endif
+ /* don't try to start nbdkit again if we've already started it */
+ if (proc->pid > 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Attempting to start nbdkit twice"));
+ return -1;
+ }
[...] Which means that this WILL get hit multiple times. Additionally
don't forget that qemuDomainStorageSourceAccessModify is called from.
qemuDomainStorageSourceChainAccessAllow.
Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where
we have this code:
if (!virStorageSourceIsEmpty(disk->src)) {
if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
goto cleanup;
if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0)
goto cleanup;
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
^^^^^^^^^^^^^^^^^^^
goto cleanup;
releaseSeclabel = true;
if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
goto cleanup;
if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0)
goto cleanup;
if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
^^^^^^^^^^^^^^^^^
goto cleanup;
}
+
if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
return -1;
Also conceptually the qemuDomainStorageSourceAccessModify function deals
with security and permissions of image files. I don't think that the
NBDKit helper process code belongs into it.
NACK
4:38 p.m.
New subject: [PATCH 3/3] qemu: handle adding/removing nbdkit-backed disk
sources
On 1/29/24 2:25 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
> Previously we were only starting or stopping nbdkit when the guest was
> started or stopped or when hotplugging/unplugging a disk. But when doing
> block operations, the disk backing store sources can also be be added or
> removed independently of the disk device. When this happens the nbdkit
> backend was not being handled properly. For example, when doing a
> blockcopy from a nbdkit-backed disk to a new disk and pivoting to that
> new location, the nbdkit process did not get cleaned up properly. Add
> some functionality to qemuDomainStorageSourceAccessModify() to handle
> this scenario.
>
> Since we're now potentially starting nbdkit from another place in the
> code, add a check to qemuNbdkitProcessStart() to report an error if we
> are trying to start nbdkit for a disk source that already has a running
> nbdkit process. This should never happen. If it does, it indicates an
> error in another part of our code.
>
> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 10 ++++++++++
> src/qemu/qemu_nbdkit.c | 7 +++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index de36641137..973a9af0b6 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
[...]
> @@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
>
> revoke_nvme = true;
>
> + if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0)
> + goto revoke;
> +
> + revoke_nbdkit = true;
> +
Note that qemuDomainStorageSourceAccessModify can be called on already
existing 'virStorageSource' to e.g. change from read-only to read-write
mode or back.
That's precisely why I put it inside of this block:
if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) {
And it seems that the MODIFY_ACCESS flag is always set unless we are
adding a new source since this is also the way that new nvme devices are
set up.
[...]
> if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0)
> goto revoke;
> }
> @@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
> VIR_WARN("Unable to release lock on %s", srcstr);
> }
>
> + if (revoke_nbdkit)
> + qemuNbdkitStopStorageSource(src, vm, chain);
> +
> cleanup:
> src->readonly = was_readonly;
> virErrorRestore(&orig_err);
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index edbe2137d0..73dc90af3e 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> struct nbd_handle *nbd = NULL;
> #endif
>
> + /* don't try to start nbdkit again if we've already started it */
> + if (proc->pid > 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Attempting to start nbdkit twice"));
> + return -1;
> + }
[...] Which means that this WILL get hit multiple times. Additionally
don't forget that qemuDomainStorageSourceAccessModify is called from.
qemuDomainStorageSourceChainAccessAllow.
Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where
we have this code:
if (!virStorageSourceIsEmpty(disk->src)) {
if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
goto cleanup;
if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0)
goto cleanup;
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
^^^^^^^^^^^^^^^^^^^
goto cleanup;
releaseSeclabel = true;
if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
goto cleanup;
if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0)
goto cleanup;
if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
^^^^^^^^^^^^^^^^^
goto cleanup;
}
As you point out, I didn't catch this case. It seems to me that the
right approach here is to just remove the call to
qemuNbdkitStartStorageSource() from here and instead rely on the
qemuDomainStorageSourceChainAccessAllow() call above to initialize nbdkit.
> +
> if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
> return -1;
Also conceptually the qemuDomainStorageSourceAccessModify function deals
with security and permissions of image files. I don't think that the
NBDKit helper process code belongs into it.
NACK
Fair point, but we'll still need to do something to start the nbdkit
process in the same basic code path as these calls to
AccessAllow()/AccessRevoke() if we want to support nbdkit-backed disks
in these paths. And IMO it's not all that dissimilar to the work that
we're doing here to detach the nvme devices from the host, etc. Or is
there a better spot to handle this?
Jonathon
9:42 a.m.
New subject: [PATCH 3/3] qemu: handle adding/removing nbdkit-backed disk
sources
On Mon, Jan 29, 2024 at 16:38:44 -0600, Jonathon Jongsma wrote:
On 1/29/24 2:25 AM, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
[...]
> [...]
>
> > @@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver
*driver,
> > revoke_nvme = true;
> > + if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0)
> > + goto revoke;
> > +
> > + revoke_nbdkit = true;
> > +
>
> Note that qemuDomainStorageSourceAccessModify can be called on already
> existing 'virStorageSource' to e.g. change from read-only to read-write
> mode or back.
That's precisely why I put it inside of this block:
if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) {
And it seems that the MODIFY_ACCESS flag is always set unless we are adding
a new source since this is also the way that new nvme devices are set up.
> [...]
>
> > if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) <
0)
> > goto revoke;
> > }
>
>
>
> > @@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver
*driver,
> > VIR_WARN("Unable to release lock on %s", srcstr);
> > }
> > + if (revoke_nbdkit)
> > + qemuNbdkitStopStorageSource(src, vm, chain);
> > +
> > cleanup:
> > src->readonly = was_readonly;
> > virErrorRestore(&orig_err);
> > diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> > index edbe2137d0..73dc90af3e 100644
> > --- a/src/qemu/qemu_nbdkit.c
> > +++ b/src/qemu/qemu_nbdkit.c
> > @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> > struct nbd_handle *nbd = NULL;
> > #endif
> > + /* don't try to start nbdkit again if we've already started it */
> > + if (proc->pid > 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Attempting to start nbdkit twice"));
> > + return -1;
> > + }
>
> [...] Which means that this WILL get hit multiple times. Additionally
> don't forget that qemuDomainStorageSourceAccessModify is called from.
> qemuDomainStorageSourceChainAccessAllow.
>
> Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where
> we have this code:
>
> if (!virStorageSourceIsEmpty(disk->src)) {
> if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
> goto cleanup;
>
> if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0)
> goto cleanup;
>
> if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) <
0)
>
> ^^^^^^^^^^^^^^^^^^^
>
> goto cleanup;
>
> releaseSeclabel = true;
>
> if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
> goto cleanup;
>
> if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) <
0)
> goto cleanup;
>
> if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
>
>
> ^^^^^^^^^^^^^^^^^
>
> goto cleanup;
> }
As you point out, I didn't catch this case. It seems to me that the right
approach here is to just remove the call to qemuNbdkitStartStorageSource()
from here and instead rely on the qemuDomainStorageSourceChainAccessAllow()
call above to initialize nbdkit.
> > +
> > if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
> > return -1;
>
> Also conceptually the qemuDomainStorageSourceAccessModify function deals
> with security and permissions of image files. I don't think that the
> NBDKit helper process code belongs into it.
>
> NACK
>
Fair point, but we'll still need to do something to start the nbdkit process
in the same basic code path as these calls to AccessAllow()/AccessRevoke()
if we want to support nbdkit-backed disks in these paths. And IMO it's not
all that dissimilar to the work that we're doing here to detach the nvme
devices from the host, etc. Or is there a better spot to handle this?
Hmm, good point. I didn't realize that the setup of the NVMe device is
so complex and that it has crept into this function. Based on that I
can't really argue against setting up nbdkit there too.
Once you fix the issue I've pointed out above:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
2:10 a.m.
On Thu, Jan 25, 2024 at 14:52:09 -0600, Jonathon Jongsma wrote:
This will allow us to start or stop nbdkit for just a single disk
source
or for every source in the backing chain. This will be used in following
patches.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_extdevice.c | 8 +++---
src/qemu/qemu_hotplug.c | 6 ++---
src/qemu/qemu_nbdkit.c | 51 ++++++++++++++++++++++++++++++---------
src/qemu/qemu_nbdkit.h | 6 +++--
4 files changed, 51 insertions(+), 20 deletions(-)
[...]
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 1c72b6fe6a..39f9c58a48 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -912,18 +928,31 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver,
}
+static void
+qemuNbdkitStopStorageSourceOne(virStorageSource *src,
+ virDomainObj *vm)
+{
+ qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+
+ if (priv && priv->nbdkitProcess &&
+ qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0)
+ VIR_WARN("Unable to stop nbdkit for storage source '%s'",
+ qemuBlockStorageSourceGetStorageNodename(src));
+}
+
+
void
No return value.
qemuNbdkitStopStorageSource(virStorageSource *src,
- virDomainObj *vm)
+ virDomainObj *vm,
+ bool chain)
{
virStorageSource *backing;
- for (backing = src; backing != NULL; backing = backing->backingStore) {
- qemuDomainStorageSourcePrivate *priv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing);
+ if (! chain)
s/! chain/!chain/
+ return qemuNbdkitStopStorageSourceOne(src, vm);
Return in a function with no return value. Compilers don't complain but
it may mislead readers.
- if (priv && priv->nbdkitProcess &&
- qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0)
- VIR_WARN("Unable to stop nbdkit for storage source '%s'",
qemuBlockStorageSourceGetStorageNodename(src));
+ for (backing = src; backing != NULL; backing = backing->backingStore) {
+ qemuNbdkitStopStorageSourceOne(backing, vm);
Alternatively rather than having an extra invocation before this loop
you can break out if (!chain) here for same result. Same applies also to
the 'Start' function.
}
}
With the extra space and 'return' removed:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
97
days inactive
115
days old
9 comments
2 participants
participants (2)
-
Jonathon Jongsma -
Peter Krempa