Re: [PATCH RFC v3 09/16] qemu: hotplug: Support hot attach and detach block disk along with throttle filters

On 2024/8/9 22:04, Peter Krempa wrote: > On Wed, Jun 12, 2024 at 03:02:17 -0700,wucf(a)linux.ibm.com wrote: > > From: Chun Feng Wu<wucf(a)linux.ibm.com>

> > + * 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.

> > @@ -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>