[Qemu-devel] [PATCH 0/4] Job API improvements and bugfixes

[Qemu-devel] [PATCH 0/4] Job API improvements and bugfixes

[Paolo Bonzini]

This patch includes a few small changes to the job API.  Do not be
fooled by the diffstat, since that is mostly due to the new documentation
in patch 4.

Patch 3 has a small change to the BlockJobType interface, because I
found hard to document the old one.

Paolo Bonzini (4):
  block: cancel jobs when a device is ready to go away
  block: fix streaming/closing race
  block: set job->speed in block_set_speed
  block: document job API

 block.c        |   24 +++++++++++-
 block/stream.c |    7 ++-
 block_int.h    |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c     |    3 +
 4 files changed, 144 insertions(+), 7 deletions(-)

-- 
1.7.9.1

[Qemu-devel] [PATCH 1/4] block: cancel jobs when a device is ready to go away

[Paolo Bonzini]

We do not want jobs to keep a device busy for a possibly very long
time, and management could become confused because they thought a
device was not even there anymore.  So, cancel long-running jobs
as soon as their device is going to disappear.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1a500b8..855a42d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -64,6 +64,9 @@ void blockdev_mark_auto_del(BlockDriverState *bs)
 {
     DriveInfo *dinfo = drive_get_by_blockdev(bs);
 
+    if (bs->job) {
+        block_job_cancel(bs->job);
+    }
     if (dinfo) {
         dinfo->auto_del = 1;
     }
-- 
1.7.9.1

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

[Paolo Bonzini]

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 <pbonzini@redhat.com>
---
 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 26dd4ce..f244f99 100644
--- a/block.c
+++ b/block.c
@@ -815,6 +815,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;
         }
@@ -968,6 +971,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);
@@ -4075,3 +4080,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..f186bfd 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);
     }
 
@@ -215,7 +217,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 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

[Qemu-devel] [PATCH 3/4] block: set job->speed in block_set_speed

[Paolo Bonzini]

There is no need to do this in every implementation of set_speed
(even though there is only one right now).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c        |    8 +++++++-
 block/stream.c |    1 -
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index f244f99..3b6065b 100644
--- a/block.c
+++ b/block.c
@@ -4065,10 +4065,16 @@ void block_job_complete(BlockJob *job, int ret)
 
 int block_job_set_speed(BlockJob *job, int64_t value)
 {
+    int rc;
+
     if (!job->job_type->set_speed) {
         return -ENOTSUP;
     }
-    return job->job_type->set_speed(job, value);
+    rc = job->job_type->set_speed(job, value);
+    if (rc == 0) {
+        job->speed = value;
+    }
+    return rc;
 }
 
 void block_job_cancel(BlockJob *job)
diff --git a/block/stream.c b/block/stream.c
index f186bfd..61ff7a2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -236,7 +236,6 @@ static int stream_set_speed(BlockJob *job, int64_t value)
     if (value < 0) {
         return -EINVAL;
     }
-    job->speed = value;
     ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
     return 0;
 }
-- 
1.7.9.1

[Qemu-devel] [PATCH 4/4] block: document job API

[Paolo Bonzini]

I am not sure that these are really proper GtkDoc, but they follow
the existing documentation in block_int.h.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block_int.h |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/block_int.h b/block_int.h
index f5c3dff..5664b98 100644
--- a/block_int.h
+++ b/block_int.h
@@ -69,8 +69,13 @@ typedef struct BlockIOBaseValue {
     uint64_t ios[2];
 } BlockIOBaseValue;
 
-typedef void BlockJobCancelFunc(void *opaque);
 typedef struct BlockJob BlockJob;
+
+/**
+ * BlockJobType:
+ *
+ * A class type for block job objects.
+ */
 typedef struct BlockJobType {
     /** Derived BlockJob struct size */
     size_t instance_size;
@@ -83,20 +88,48 @@ typedef struct BlockJobType {
 } BlockJobType;
 
 /**
- * Long-running operation on a BlockDriverState
+ * BlockJob:
+ *
+ * Long-running operation on a BlockDriverState.
  */
 struct BlockJob {
+    /** The job type, including the job vtable.  */
     const BlockJobType *job_type;
+
+    /** The block device on which the job is operating.  */
     BlockDriverState *bs;
+
+    /**
+     * Set to true if the job should cancel itself.  The flag must
+     * always be tested just before toggling the busy flag from false
+     * to true.  After a job has detected that the cancelled flag is
+     * true, it should not anymore issue any I/O operation to the
+     * block device.
+     */
     bool cancelled;
+
+    /**
+     * Set to false by the job while it is in a quiescent state, where
+     * no I/O is pending and cancellation can be processed without
+     * issuing new I/O.  The busy flag must be set to false when the
+     * job goes to sleep on any condition that is not detected by
+     * #qemu_aio_wait, such as a timer.
+     */
     bool busy;
 
-    /* These fields are published by the query-block-jobs QMP API */
+    /** Offset that is published by the query-block-jobs QMP API */
     int64_t offset;
+
+    /** Length that is published by the query-block-jobs QMP API */
     int64_t len;
+
+    /** Speed that was set with @block_job_set_speed.  */
     int64_t speed;
 
+    /** The completion function that will be called when the job completes.  */
     BlockDriverCompletionFunc *cb;
+
+    /** The opaque value that is passed to the completion function.  */
     void *opaque;
 };
 
@@ -324,14 +357,90 @@ void bdrv_set_io_limits(BlockDriverState *bs,
 int is_windows_drive(const char *filename);
 #endif
 
+/**
+ * block_job_create:
+ * @job_type: The class object for the newly-created job.
+ * @bs: The block
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ *
+ * Create a new long-running block device job and return it.  The job
+ * will call @cb asynchronously when the job completes.  Note that
+ * @bs may have been closed at the time the @cb it is called.  If
+ * this is the case, the job may be reported as either cancelled or
+ * completed.
+ *
+ * This function is not part of the public job interface; it should be
+ * called from a wrapper that is specific to the job type.
+ */
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
                        BlockDriverCompletionFunc *cb, void *opaque);
+
+/**
+ * block_job_complete:
+ * @job: The job being completed.
+ * @ret: The status code.
+ *
+ * Call the completion function that was registered at creation time, and
+ * free @job.
+ */
 void block_job_complete(BlockJob *job, int ret);
+
+/**
+ * block_job_set_speed:
+ * @job: The job to set the speed for.
+ * @speed: The new value
+ *
+ * Set a rate-limiting parameter for the job; the actual meaning may
+ * vary depending on the job type.
+ */
 int block_job_set_speed(BlockJob *job, int64_t value);
+
+/**
+ * block_job_cancel:
+ * @job: The job to be canceled.
+ *
+ * Asynchronously cancel the specified job.
+ */
 void block_job_cancel(BlockJob *job);
+
+/**
+ * block_job_is_cancelled:
+ * @job: The job being queried.
+ *
+ * Returns whether the job is scheduled for cancellation.
+ */
 bool block_job_is_cancelled(BlockJob *job);
+
+/**
+ * block_job_cancel:
+ * @job: The job to be canceled.
+ *
+ * Asynchronously cancel the job and wait for it to reach a quiescent
+ * state.  Note that the completion callback will still be called
+ * asynchronously, hence it is *not* valid to call #bdrv_delete
+ * immediately after #block_job_cancel_sync.  Users of block jobs
+ * will usually protect the BlockDriverState objects with a reference
+ * count, should this be a concern.
+ */
 void block_job_cancel_sync(BlockJob *job);
 
+/**
+ * stream_start:
+ * @bs: Block device to operate on.
+ * @base: Block device that will become the new base, or %NULL to
+ * flatten the whole backing file chain onto @bs.
+ * @base_id: The file name that will be written to @bs as the new
+ * backing file if the job completes.  Ignored if @base is %NULL.
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ *
+ * Start a streaming operation on @bs.  Clusters that are unallocated
+ * in @bs, but allocated in any image between @base and @bs (both
+ * exclusive) will be written to @bs.  At the end of a successful
+ * streaming job, the backing file of @bs will be changed to
+ * @base_id in the written image and to @base in the live BlockDriverState.
+ */
 int stream_start(BlockDriverState *bs, BlockDriverState *base,
                  const char *base_id, BlockDriverCompletionFunc *cb,
                  void *opaque);
-- 
1.7.9.1

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

[Stefan Hajnoczi]

On Fri, Mar 30, 2012 at 12:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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 <pbonzini@redhat.com>
> ---
>  block.c        |   16 ++++++++++++++++
>  block/stream.c |    6 ++++--
>  block_int.h    |    2 ++
>  3 files changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 0/4] Job API improvements and bugfixes

[Stefan Hajnoczi]

On Fri, Mar 30, 2012 at 12:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This patch includes a few small changes to the job API.  Do not be
> fooled by the diffstat, since that is mostly due to the new documentation
> in patch 4.
>
> Patch 3 has a small change to the BlockJobType interface, because I
> found hard to document the old one.
>
> Paolo Bonzini (4):
>  block: cancel jobs when a device is ready to go away
>  block: fix streaming/closing race
>  block: set job->speed in block_set_speed
>  block: document job API
>
>  block.c        |   24 +++++++++++-
>  block/stream.c |    7 ++-
>  block_int.h    |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c     |    3 +
>  4 files changed, 144 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 1/4] block: cancel jobs when a device is ready to go away

[Kevin Wolf]

Am 30.03.2012 13:17, schrieb Paolo Bonzini:
> We do not want jobs to keep a device busy for a possibly very long
> time, and management could become confused because they thought a
> device was not even there anymore.  So, cancel long-running jobs
> as soon as their device is going to disappear.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Shouldn't management tools know what jobs they have running?

We can take this for now, but once we switch to blockdev-add/del,
management tools will have to be explicit about the BlockDriverState
deletion (and I think the right semantics there is that it fails if jobs
are still running).

Kevin

Re: [Qemu-devel] [PATCH 0/4] Job API improvements and bugfixes

[Kevin Wolf]

Am 30.03.2012 13:17, schrieb Paolo Bonzini:
> This patch includes a few small changes to the job API.  Do not be
> fooled by the diffstat, since that is mostly due to the new documentation
> in patch 4.
> 
> Patch 3 has a small change to the BlockJobType interface, because I
> found hard to document the old one.
> 
> Paolo Bonzini (4):
>   block: cancel jobs when a device is ready to go away
>   block: fix streaming/closing race
>   block: set job->speed in block_set_speed
>   block: document job API
> 
>  block.c        |   24 +++++++++++-
>  block/stream.c |    7 ++-
>  block_int.h    |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c     |    3 +
>  4 files changed, 144 insertions(+), 7 deletions(-)
> 

Thanks, applied all to the block branch.

Kevin