| Message ID | 20170511144208.24075-10-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 11, 2017 at 04:41:59PM +0200, Paolo Bonzini wrote: > Another possibility is to use tg->lock, which we're holding anyway in > both schedule_next_request and throttle_group_co_io_limits_intercept. > This would require open-coding the CoQueue however, so I've chosen this > alternative. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > v1->v2: rename CoMutex [Fam] > > block/block-backend.c | 1 + > block/throttle-groups.c | 12 ++++++++++-- > include/sysemu/block-backend.h | 7 ++----- > 3 files changed, 13 insertions(+), 7 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Thu 11 May 2017 04:41:59 PM CEST, Paolo Bonzini wrote: > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -72,11 +72,8 @@ typedef struct BlockDevOps { > * fields that must be public. This is in particular for QLIST_ENTRY() and > * friends so that BlockBackends can be kept in lists outside block-backend.c */ > typedef struct BlockBackendPublic { > - /* I/O throttling has its own locking, but also some fields are > - * protected by the AioContext lock. > - */ > - > - /* Protected by AioContext lock. */ > + /* throttled_reqs_lock protects the CoQueues for throttled requests. */ > + CoMutex throttled_reqs_lock; > CoQueue throttled_reqs[2]; > > /* Nonzero if the I/O limits are currently being ignored; generally > * it is zero. */ > unsigned int io_limits_disabled; Does io_limits_disabled need locking too? Berto
On 18/05/2017 14:06, Alberto Garcia wrote: > On Thu 11 May 2017 04:41:59 PM CEST, Paolo Bonzini wrote: > >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -72,11 +72,8 @@ typedef struct BlockDevOps { >> * fields that must be public. This is in particular for QLIST_ENTRY() and >> * friends so that BlockBackends can be kept in lists outside block-backend.c */ >> typedef struct BlockBackendPublic { >> - /* I/O throttling has its own locking, but also some fields are >> - * protected by the AioContext lock. >> - */ >> - >> - /* Protected by AioContext lock. */ >> + /* throttled_reqs_lock protects the CoQueues for throttled requests. */ >> + CoMutex throttled_reqs_lock; >> CoQueue throttled_reqs[2]; >> >> /* Nonzero if the I/O limits are currently being ignored; generally >> * it is zero. */ >> unsigned int io_limits_disabled; > > Does io_limits_disabled need locking too? It uses atomic access since patch 3. Paolo
On Thu 18 May 2017 02:08:32 PM CEST, Paolo Bonzini wrote: >> Does io_limits_disabled need locking too? > > It uses atomic access since patch 3. Ah, I overlooked that one, sorry! Berto
diff --git a/block/block-backend.c b/block/block-backend.c index 915ccc5e56..17b0ef2dc3 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -163,6 +163,7 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm) blk->shared_perm = shared_perm; blk_set_enable_write_cache(blk, true); + qemu_co_mutex_init(&blk->public.throttled_reqs_lock); qemu_co_queue_init(&blk->public.throttled_reqs[0]); qemu_co_queue_init(&blk->public.throttled_reqs[1]); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 8bf1031efa..a181cb1dee 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -270,8 +270,13 @@ static bool coroutine_fn throttle_group_co_restart_queue(BlockBackend *blk, bool is_write) { BlockBackendPublic *blkp = blk_get_public(blk); + bool ret; - return qemu_co_queue_next(&blkp->throttled_reqs[is_write]); + qemu_co_mutex_lock(&blkp->throttled_reqs_lock); + ret = qemu_co_queue_next(&blkp->throttled_reqs[is_write]); + qemu_co_mutex_unlock(&blkp->throttled_reqs_lock); + + return ret; } /* Look for the next pending I/O request and schedule it. @@ -340,7 +345,10 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk, if (must_wait || blkp->pending_reqs[is_write]) { blkp->pending_reqs[is_write]++; qemu_mutex_unlock(&tg->lock); - qemu_co_queue_wait(&blkp->throttled_reqs[is_write], NULL); + qemu_co_mutex_lock(&blkp->throttled_reqs_lock); + qemu_co_queue_wait(&blkp->throttled_reqs[is_write], + &blkp->throttled_reqs_lock); + qemu_co_mutex_unlock(&blkp->throttled_reqs_lock); qemu_mutex_lock(&tg->lock); blkp->pending_reqs[is_write]--; } diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 87a43b07ab..f8b7eaa4f2 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -72,11 +72,8 @@ typedef struct BlockDevOps { * fields that must be public. This is in particular for QLIST_ENTRY() and * friends so that BlockBackends can be kept in lists outside block-backend.c */ typedef struct BlockBackendPublic { - /* I/O throttling has its own locking, but also some fields are - * protected by the AioContext lock. - */ - - /* Protected by AioContext lock. */ + /* throttled_reqs_lock protects the CoQueues for throttled requests. */ + CoMutex throttled_reqs_lock; CoQueue throttled_reqs[2]; /* Nonzero if the I/O limits are currently being ignored; generally
Another possibility is to use tg->lock, which we're holding anyway in both schedule_next_request and throttle_group_co_io_limits_intercept. This would require open-coding the CoQueue however, so I've chosen this alternative. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- v1->v2: rename CoMutex [Fam] block/block-backend.c | 1 + block/throttle-groups.c | 12 ++++++++++-- include/sysemu/block-backend.h | 7 ++----- 3 files changed, 13 insertions(+), 7 deletions(-)