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

On Tue, Aug 20, 2024 at 22:48:53 +0800, Chun Feng Wu wrote: > On 2024/8/19 18:59, Peter Krempa wrote: >> On Mon, Aug 19, 2024 at 18:27:46 +0800, Chun Feng Wu wrote: >>> 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(a)linux.ibm.com wrote: >>>>>>> From: Chun Feng Wu&lt;wucf(a)linux.ibm.com&gt; >> [...] >> >>>>> 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> >> I suggest you don't add the privateData sub-element to the >> 'throttlefilters' as it will be very hard to plumb that in. You'd >> require private data callbacks which the XML parser (generic) calls from >> the qemu driver (specific) to parse this. >> >> I'd rather suggest you use the disk private data section and format them >> there. Use the group name as a key to find them as that is (or at least >> should be) unique. >> > if so, it could be the following xml: > > <disk> >   <privateData> >        <nodenames> >            <nodename type='throttle-filter' > name='libvirt-nodenameindex-filter' key='group_name'/> >        </nodenames> >   </privateData> > </disk> Yes. Although I'd name the 'key' field 'group' instead. > and > > struct _qemuDomainDiskPrivate { > ... > GHashTable *throttleFilters; > ... > } This shouldn't be needed. You can store the nodenames in the same data structure you do now. The formatter will format it from the data structure and parser will fill it into the appropriate fields based on the group name. Yes it's O(N2)-ish complexity but N is small, so having a hash table doesn't make sense. In fact you don't want a hash table because you need the filters ordered in the order defined in the XML and a hash table would re-order them randomly every time.