On 2024/8/16 23:14, Peter Krempa wrote:
On Thu, Aug 15, 2024 at 13:04:47 +0800, Chun Feng Wu wrote:
(I'd suggest you trim irrelevant stuff when responding. It's hard to
find what you've responded to in this massive message .
On 2024/8/9 22:04, Peter Krempa wrote:
On Wed, Jun 12, 2024 at 03:02:17 -0700,wucf@xxxxxxxxxxxxx wrote:
From: Chun Feng Wu<wucf@xxxxxxxxxxxxx>
[...]
+ * qemuBuildThrottleFiltersDetachPrepareBlockdev:
+ * @disk: domain disk
+ *
+ * Build filters data for later "blockdev-del"
+ */
+qemuBlockThrottleFiltersData *
+qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk)
+{
+ g_autoptr(qemuBlockThrottleFiltersData) data = g_new0(qemuBlockThrottleFiltersData, 1);
+ size_t i;
+
+ /* build filterdata, which contains filters info and sequence info */
+ for (i = 0; i < disk->nthrottlefilters; i++) {
+ g_autoptr(qemuBlockThrottleFilterAttachData) elem = g_new0(qemuBlockThrottleFilterAttachData, 1);
+ /* ignore other fields since the following info are enough for "blockdev-del" */
+ elem->filterNodeName = qemuBlockThrottleFilterGetNodename(disk->throttlefilters[i]);
So I didn't yet see any code that serializes any of this in the status
XML, thus it seems that this will not work after you restart
libvirtd/virtqemud if a VM with filters is running, and then try to
detach disks. You'll need to add that, or base the filter nodename on
something else.
Note that presence of the node name is authoritative and we must not try
to regenerate it. That would hinder changing the node names in the
future.
See how the copyOnRead layer node name is stored.
Filter node name is generated by rule "libvirt-nodenameindex-filter" in
method
"qemuDomainPrepareThrottleFilterBlockdev", which is called by
"qemuDomainPrepareDiskSourceBlockdev".
it follows the same way like "libvirt-nodenameindex-format" node.
And I tested restarting libvirtd, vm with throttle works well in this case.
The problem is that the actual error shows only in logs as the detaching
of the backend-nodes is on a code path that can't meaningfully report
errors. Did you check in the logs that:
- the removal of the throttling node was actually requested
- that it did have a proper node name
That is on hot-unplug of the disk having such config after you've
restarted libvirt.
The code for other disks always stores the nodenames as-generated in the
status XML (XML describing a running VM) which I don't see in this
series thus I infer it doesn't/can't work.
[...]
@@ -921,6 +937,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
bool releaseSeclabel = false;
int ret = -1;
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ if (disk->nthrottlefilters > 0) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cdrom device with throttle filters isn't supported"));
So and now this is why I don't like the fact that you are doing this on
a per-disk level. On a per-image level (if you'd instantiate 'throttle'
layers when adding the image) this would not be a problem.
I'd strongly prefer if you modify this such that the trottle layers will
be instantiated at per-storage-source level both in XML and in the code.
regarding per-storage-source, do you mean xml like below? if so, when
specifying "--throttle-groups" in "attach-disk"
it apply groups on top source(vm1_disk_2.qcow2) only? is there scenario to
apply throttle-groups on backing store source?
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2'/>
<source file='/virt/disks/vm1_disk_2.qcow2' index='1'>
<throttlefilters>
<throttlefilter group='limit0'/>
</throttlefilters>
</source>
<backingStore type='file' index='4'>
<format type='qcow2'/>
<source file='/virt/disks/backing.qcow2'>
<throttlefilters>
<throttlefilter group='limit1'/>
</throttlefilters>
</source>
<backingStore/>
</backingStore>
<target dev='vdc' bus='virtio'/>
<alias name='virtio-disk2'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x06'
function='0x0'/>
</disk>
Yeah, something like this. That allows adding filters on other layers
too.
As I said it depends on how you expect to use this feature, because both
make sense.
I do reckon that in most cases this is an overkill though.
If you decide that this is too complicated, you can use the approach you
did, but then need to store the nodename of the throttle layer on the
disk level in the status XML.
sure, then let's stick to current implementation about filter
chain(per-disk), about status XML, do you mean <privateData> to store
nodename? if so, does the following xml look good to you?
<throttlefilters>
<throttlefilter group='limit2'>
<privateData>
<nodename type='throttle-filter'
name='0123456789ABCDEF0123456789ABCDE'/>
</privateData>
</throttlefilter>
</throttlefilters>
--
Thanks and Regards,
Wu
