Re: [Qemu-devel] n ways block filters

[Benoît Canet]

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