[Qemu-devel] [PATCH] block: fix streaming/closing race

* [Qemu-devel] [PATCH] block: fix streaming/closing race
@ 2012-03-29 16:08 Paolo Bonzini
  2012-03-30  7:19 ` Stefan Hajnoczi
  0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2012-03-29 16:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, stefanha

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, so add a flag
saying whether streaming has in-flight I/O.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c        |   14 ++++++++++++++
 block/stream.c |    4 +++-
 block_int.h    |    2 ++
 3 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index b88ee90..1aab4bf 100644
--- a/block.c
+++ b/block.c
@@ -813,6 +813,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;
         }
@@ -4069,3 +4072,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 d1b3986..b8a1628 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -175,7 +175,7 @@ retry:
             break;
         }
 
-
+        s->common.busy = true;
         if (base) {
             ret = is_allocated_base(bs, base, sector_num,
                                     STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
@@ -189,6 +189,7 @@ retry:
             if (s->common.speed) {
                 uint64_t delay_ns = ratelimit_calculate_delay(&s->limit, n);
                 if (delay_ns > 0) {
+                    s->common.busy = false;
                     co_sleep_ns(rt_clock, delay_ns);
 
                     /* Recheck cancellation and that sectors are unallocated */
@@ -208,6 +209,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_ns(rt_clock, 0);
     }
 
diff --git a/block_int.h b/block_int.h
index b460c36..f5c3dff 100644
--- a/block_int.h
+++ b/block_int.h
@@ -89,6 +89,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;
@@ -329,6 +330,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.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread