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

[Jonathon Jongsma]

On 2/16/23 9:55 AM, Peter Krempa wrote:
> 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 at redhat.com>
>> ---
> 
> [...]
> 
>> @@ -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.

I just figured that logically the backing source should always be 
available to the upper layers, so I would try to be consistent and 
always set them up that way. But I agree that this is not really 
necessary here and makes the code less concise, so I'll change them.

> 
>> +
>> +        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 at redhat.com>
>