Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Tue, Mar 24, 2015 at 06:48:37PM +0100, Alberto Garcia wrote:
> On Tue, Mar 24, 2015 at 04:31:45PM +0000, Stefan Hajnoczi wrote:
> 
> > > +    /* get next bs round in round robin style */
> > > +    token = throttle_group_next_bs(token);
> > > +    while (token != start  &&
> > > +           qemu_co_queue_empty(&token->throttled_reqs[is_write])) {
> > 
> > It's not safe to access bs->throttled_reqs[].  There are many of
> > other places that access bs->throttled_reqs[] without holding
> > tg->lock (e.g. block.c).
> >
> > throttled_reqs needs to be protected by tg->lock in order for this
> > to work.
> 
> Good catch, but I don't think that's the solution. throttled_reqs
> cannot be protected by that lock because it must release it before
> calling qemu_co_queue_wait(&bs->throttled_reqs[is_write]). Otherwise
> it will cause a deadlock.
> 
> My assumption was that throttled_reqs would be protected by the BDS's
> AioContext lock, but I overlooked the fact that this part of the
> algorithm needs to access other BDSs' queues so we indeed have a
> problem.
> 
> I will give it a thought to see what I can come up with. Since we only
> need to check whether other BDSs have pending requests maybe I can
> implement this with a flag.

A flag is a good solution since it is not possible to acquire other BDS'
AioContext lock from arbitrary block layer code (it's only allowed when
the QEMU global mutex is held).

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]