The Monday 17 Mar 2014 à 11:12:31 (+0800), Fam Zheng wrote : > On Fri, 03/14 16:57, Benoît Canet wrote: > > > > Hello list, > > > > I plan to convert throttling to a block filter and write n way throttling > > support. > > > > I discussed a bit with Stefan on the list and we came to the conclusion > > that the > > block filter API need group support. > > > > filter group: > > ------------- > > > > My current plan to implement this is to add the following fields to the > > BlockDriver > > structure. > > > > int bdrv_add_filter_group(const char *name, QDict options); > > int bdrv_reconfigure_filter_group(const char *name, QDict options); > > int bdrv_destroy_filter_group(const char *name); > > > > These three extra method would allow to create, reconfigure or destroy a > > block > > filter group. A block filter group contain the shared or non shared state > > of the > > blockfilter. For throttling it would contains the ThrottleState structure. > > > > Each block filter driver would contains a linked list of linked list where > > the > > BDS are registered grouped by filter groups state. > > Sorry I don't fully understand this. Does a filter group contain multiple > block > filters, and every block filter has effect on multiple BDSes? Could you give > an > example?
The driver module says block/throttle.c would contains a linked list of ThrottleGroup in a static variable. Each ThrottleGroup would have a name, a ThrottleState (as in utils/throttle.c) and a linked list of members BDS. The s variable of the bdrv_* method of throttle.c would have a pointer to the ThrottleGroup this BDS is a member of hence the driver can access the ThrottleState So each BDS filter would have only one BDS parent and one BDS ->file child but would also be member of a ThrottleGroup. This would allow to do throttling on a group of devices. Throttling only one device would create a ThrottleGroup of name bds->device_name containing only the throttled BDS. > > > > > The regular bdrv_open callback would be used to instantiate a block filter > > and > > add it to a filter group. This method would also take a new-node-name for > > the new > > filter. This node-name would become the name of the new filter. > > bdrv_close would cleanup and deregister from a filter group. > > > > An extra filter-group field in the option dict would allow the bdrv_open > > method > > to register the newly opened block filter in it's filter group. > > The BDS structure would have a direct pointer to it's filter group state. > > > > Utility methods to do the bdrv_add_filter_group bdrv_open then bdrv_swap to > > install a new filter can be provided by block.c. The same can be done for > > filter > > close and desinstallation. > > So you are defining block filter as a new kind of block driver. Is a filter > always on top above everything else by definition? Yes, A filter is instanciated on top of another BDS. Filters can be stacked on top of another to form a chain. > But I am afraid BlockDriverState is already taking too many responsibilities > here (backend, protocol driver, format driver, filter...). I was wondering if > it is clearer to rather introduce bs->filter_list to point to a list of > BlockFilter (a new sturcture, tailored for a block filter), and don't bother > with bdrv_swap. Hmm blkverify and quorum are already block filters reusing the BlockDriver interface. The good this with this is that it would allow to move some code from block.c to block/filter_name*.c: block throttling for example. Crypto would be another use case. Best regards Benoît > > Thanks, > Fam > > > > > Legacy throttling QMP API > > ------------------------- > > > > The legacy throttling API would create throttling filters groups containing > > only > > one BDS. > > > > By default for every 1 way block filter block.c would create a filter group > > using the BDS id or node-name as group name. This allow for easy filer > > removal > > with the bds reference. > > > > Group throttling API > > -------------------- > > > > Commands would be added to create throttling filter groups reconfigure and > > remove > > them. > > > > Two additional commands would be added to create and insert a block filter > > in a > > given group or close and remove it. > > > > Before I start implementing something what are your thougths on this ? > > > > Best regards > > > > Benoît > > > > > > >