Re: [libvirt PATCH v5 21/32] qemu: use nbdkit to serve network disks if available

On Tue, Feb 14, 2023 at 11:08:08 -0600, Jonathon Jongsma wrote: > For virStorageSource objects that contain an nbdkitProcess, start that > nbdkit process to serve that network drive and then pass the nbdkit > socket to qemu rather than sending the network url to qemu directly. > > Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; > --- [...] > @@ -308,10 +327,36 @@ qemuExtDevicesHasDevice(virDomainDef *def) > return true; > } > > + for (i = 0; i < def->ndisks; i++) { > + qemuDomainStorageSourcePrivate *priv = > + QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(def->disks[i]->src); > + if (priv->nbdkitProcess) > + return true; This is insufficient. Also the backing images of 'src' can have 'nbdkit' > + } > + > + > return false; > } > > > +/* recursively setup nbdkit cgroups for backing chain of src */ > +static int qemuExtDevicesSetupCgroupNbdkit(virStorageSource *src, > + virCgroup *cgroup) Header style doesn't conform to our coding style and the rest of the file. > +{ > + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); > + > + if (src->backingStore) > + if (qemuExtDevicesSetupCgroupNbdkit(src->backingStore, cgroup) < 0) > + return -1; > + > + if (priv && priv->nbdkitProcess && > + qemuNbdkitProcessSetupCgroup(priv->nbdkitProcess, cgroup) < 0) > + return -1; Any particular reason why you need to setup the cgroups starting from the bottom-most image? Specifically for nbdkit it should not matter too much as we need to start them before starting qemu anyways. In QEMU we indeed must construct the backing chain from the bottom image first.

> + > + return 0; > +} > + > + > int > qemuExtDevicesSetupCgroup(virQEMUDriver *driver, > virDomainObj *vm, [...] > +/* recursively start nbdkit for backing chain of src */ > +int > +qemuNbdkitStartStorageSource(virQEMUDriver *driver, > + virDomainObj *vm, > + virStorageSource *src) > +{ > + virStorageSource *backing; > + > + for (backing = src->backingStore; backing != NULL; backing = backing->backingStore) > + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) > + return -1; > + > + return qemuNbdkitStartStorageSourceOne(driver, vm, src); > +} This is again the weird iteration mechanism where backing images are iterated from top to bottom and then the topmost image is iterated. Make it into a linear one please. Either from the end or from the start. > +/* recursively stop nbdkit processes for backing chain of src */ > +void > +qemuNbdkitStopStorageSource(virStorageSource *src) > +{ > + virStorageSource *backing; > + > + qemuNbdkitStopStorageSourceOne(src); > + > + for (backing = src->backingStore; backing != NULL; backing = backing->backingStore) > + qemuNbdkitStopStorageSourceOne(backing); > +} Same here. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;