On Wed, Mar 04, 2015 at 02:53:42PM +0100, Alberto Garcia wrote:
> On Tue, Mar 03, 2015 at 03:00:06PM -0600, Stefan Hajnoczi wrote:
>
> > > + throttle_group_lock(bs->throttle_state);
> > > + bdrv_throttle_group_remove(bs);
> > > + throttle_group_unlock(bs->throttle_state);
> > > +
> > > + throttle_group_unref(bs->throttle_state);
> > > + bs->throttle_state = NULL;
> > > +
> > > throttle_timers_destroy(&bs->throttle_timers);
> > > }
> > >
> > > static void bdrv_throttle_read_timer_cb(void *opaque)
> > > {
> > > BlockDriverState *bs = opaque;
> > > - throttle_timer_fired(&bs->throttle_state, false);
> > > +
> > > + throttle_group_lock(bs->throttle_state);
> > > + throttle_timer_fired(bs->throttle_state, false);
> > > + throttle_group_unlock(bs->throttle_state);
> >
> > This pattern suggests throttle_timer_fired() should acquire the lock
> > internally instead.
>
> The idea is that the ThrottleState code itself doesn't know anything
> about locks or groups. As I understood it Benoît designed the
> ThrottleState code to be independent from the block layer and reusable
> for other things (that's why it's in util/).Then ThrottleGroup could offer an API throttle_group_timer_fired() that does the locking. The advantage of encapsulating locking in ThrottleGroup is that callers don't have to remember the take the lock. (But they must still be careful about sequences of calls which will not be atomic.) Stefan
pgppH5CdMhMzg.pgp
Description: PGP signature
