From c9c16cf8e0f0ea170c9cb164ca283e686aea8e76 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: <50c33b164bbd7fa0d613bd09b072ef7deb09168c.1334231944.git.minovotn@redhat.com> References: <50c33b164bbd7fa0d613bd09b072ef7deb09168c.1334231944.git.minovotn@redhat.com> From: Paolo Bonzini Date: Fri, 30 Mar 2012 18:32:04 +0200 Subject: [PATCH 4/4] block: fix streaming/closing race RH-Author: Paolo Bonzini Message-id: <1333132324-20958-5-git-send-email-pbonzini@redhat.com> Patchwork-id: 39052 O-Subject: [RHEL 6.3 qemu-kvm PATCH v2 4/4] block: fix streaming/closing race Bugzilla: 807898 RH-Acked-by: Jeffrey Cody RH-Acked-by: Kevin Wolf RH-Acked-by: Laszlo Ersek Upstream ref: http://permalink.gmane.org/gmane.comp.emulators.qemu/144354 Streaming can issue I/O while qcow2_close is running. This causes the L2 caches to become very confused or, alternatively, could cause a segfault when the streaming coroutine is reentered after closing its block device. The fix is to cancel streaming jobs when closing their underlying device. The cancellation must be synchronous, on the other hand qemu_aio_wait will not restart a coroutine that is sleeping in co_sleep. So add a flag saying whether streaming has in-flight I/O. If the busy flag is false, the coroutine is quiescent and, when cancelled, will not issue any new I/O. This protects streaming against closing, but not against deleting. We have a reference count protecting us against concurrent deletion, but I still added an assertion to ensure nothing bad happens. Signed-off-by: Paolo Bonzini --- block.c | 16 ++++++++++++++++ block/stream.c | 6 ++++-- block_int.h | 2 ++ 3 files changed, 22 insertions(+), 2 deletions(-) Signed-off-by: Michal Novotny --- block.c | 16 ++++++++++++++++ block/stream.c | 6 ++++-- block_int.h | 2 ++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index e3c4f64..438449f 100644 --- a/block.c +++ b/block.c @@ -684,6 +684,9 @@ unlink_and_fail: void bdrv_close(BlockDriverState *bs) { if (bs->drv) { + if (bs->job) { + block_job_cancel_sync(bs->job); + } if (bs == bs_snapshots) { bs_snapshots = NULL; } @@ -808,6 +811,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) void bdrv_delete(BlockDriverState *bs) { assert(!bs->dev); + assert(!bs->job); + assert(!bs->in_use); /* remove from list, if necessary */ bdrv_make_anon(bs); @@ -3695,3 +3700,14 @@ bool block_job_is_cancelled(BlockJob *job) { return job->cancelled; } + +void block_job_cancel_sync(BlockJob *job) +{ + BlockDriverState *bs = job->bs; + + assert(bs->job == job); + block_job_cancel(job); + while (bs->job != NULL && bs->job->busy) { + qemu_aio_wait(); + } +} diff --git a/block/stream.c b/block/stream.c index 661f59f..4d28dc7 100644 --- a/block/stream.c +++ b/block/stream.c @@ -208,7 +208,7 @@ retry: break; } - + s->common.busy = true; if (base) { ret = is_allocated_base(bs, base, sector_num, STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n); @@ -222,6 +222,7 @@ retry: if (s->common.speed) { uint64_t delay_ms = ratelimit_calculate_delay(&s->limit, n); if (delay_ms > 0) { + s->common.busy = false; co_sleep(rt_clock, delay_ms); /* Recheck cancellation and that sectors are unallocated */ @@ -241,6 +242,7 @@ retry: /* Note that even when no rate limit is applied we need to yield * with no pending I/O here so that qemu_aio_flush() returns. */ + s->common.busy = false; co_sleep(rt_clock, 0); } @@ -248,7 +250,7 @@ retry: bdrv_disable_copy_on_read(bs); } - if (sector_num == end && ret == 0) { + if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) { const char *base_id = NULL; if (base) { base_id = s->backing_file_id; diff --git a/block_int.h b/block_int.h index fcfe7d0..e195ffe 100644 --- a/block_int.h +++ b/block_int.h @@ -70,6 +70,7 @@ struct BlockJob { const BlockJobType *job_type; BlockDriverState *bs; bool cancelled; + bool busy; /* These fields are published by the query-block-jobs QMP API */ int64_t offset; @@ -289,6 +290,7 @@ void block_job_complete(BlockJob *job, int ret); int block_job_set_speed(BlockJob *job, int64_t value); void block_job_cancel(BlockJob *job); bool block_job_is_cancelled(BlockJob *job); +void block_job_cancel_sync(BlockJob *job); int stream_start(BlockDriverState *bs, BlockDriverState *base, const char *base_id, BlockDriverCompletionFunc *cb, -- 1.7.7.6