[Qemu-devel] [PATCH v2 00/11] block: allow blockjobs to coexist with dataplane

[Qemu-devel] [PATCH v2 00/11] block: allow blockjobs to coexist with dataplane

[Stefan Hajnoczi]

v2:
 * Protect block_job_defer_to_main_loop_bh() against AioContext change [Max]
 * Drop unnecessary if (buf) around qemu_vfree(buf) [Max]

Almost all the infrastructure is in place to make blockjobs safe for use with
dataplane:

  * Op blockers all us to exclude commands that could conflict with a blockjob
    or dataplane.

  * AioContext acquire/release allows threads to temporarily access a
    BlockDriverState that is running in another thread.

This series introduces a few additional helpers:

  * block_job_defer_to_main_loop() which allows blockjobs to run their
    completion code in the QEMU main loop.  This is necessary because some
    operations are not safe outside the QEMU global mutex.

  * bdrv_drain() which can be used in limited cases to wait for in-flight
    requests to complete (as opposed to the global bdrv_drain_all() function).

The approach taken in this series is to convert the blockdev.c monitor command
so it acquires the BlockDriverState's AioContext.  Normally only 1 AioContext
is involved at a time but the mirror job's to_replace argument can involve a
second AioContext.

Then the block job code itself is converted to defer main loop code using
block_job_defer_to_main_loop().

Example:

  $ qemu-system-x86_64 -enable-kvm -m 1024 \
        -drive if=none,id=drive0,file=test.img \
        -object iothread,iothread0 \
        -device virtio-blk-pci,drive=drive0,iothread=iothread0
  (qemu) drive_mirror drive0 test2.img

Stefan Hajnoczi (11):
  block: acquire AioContext in generic blockjob QMP commands
  blockdev: acquire AioContext in do_qmp_query_block_jobs_one()
  blockdev: acquire AioContext in blockdev_mark_auto_del()
  blockdev: add note that block_job_cb() must be thread-safe
  blockjob: add block_job_defer_to_main_loop()
  block: add bdrv_drain()
  block: let backup blockjob run in BDS AioContext
  block: let stream blockjob run in BDS AioContext
  block: let mirror blockjob run in BDS AioContext
  block: let commit blockjob run in BDS AioContext
  block: declare blockjobs and dataplane friends!

 block.c                         |  49 +++++++++--
 block/backup.c                  |  21 ++++-
 block/commit.c                  |  70 ++++++++++------
 block/mirror.c                  |  85 +++++++++++++------
 block/stream.c                  |  50 +++++++----
 blockdev.c                      | 179 +++++++++++++++++++++++++++++-----------
 blockjob.c                      |  46 +++++++++++
 hw/block/dataplane/virtio-blk.c |   5 ++
 include/block/block.h           |   1 +
 include/block/blockjob.h        |  19 +++++
 10 files changed, 402 insertions(+), 123 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 01/11] block: acquire AioContext in generic blockjob QMP commands

[Stefan Hajnoczi]

block-job-set-speed, block-job-cancel, block-job-pause,
block-job-resume, and block-job-complete must acquire the
BlockDriverState AioContext so that it is safe to access bs.

At the moment bs->job is always NULL when dataplane is active because op
blockers prevent blockjobs from starting.  Once the rest of the blockjob
API has been made aware of AioContext we can drop the op blocker.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e595910..1c7dab4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2399,20 +2399,35 @@ void qmp_drive_mirror(const char *device, const char *target,
     }
 }
 
-static BlockJob *find_block_job(const char *device)
+/* Get the block job for a given device name and acquire its AioContext */
+static BlockJob *find_block_job(const char *device, AioContext **aio_context)
 {
     BlockDriverState *bs;
 
     bs = bdrv_find(device);
-    if (!bs || !bs->job) {
-        return NULL;
+    if (!bs) {
+        goto notfound;
+    }
+
+    *aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(*aio_context);
+
+    if (!bs->job) {
+        aio_context_release(*aio_context);
+        goto notfound;
     }
+
     return bs->job;
+
+notfound:
+    *aio_context = NULL;
+    return NULL;
 }
 
 void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
-    BlockJob *job = find_block_job(device);
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context);
 
     if (!job) {
         error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
@@ -2420,34 +2435,40 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
     }
 
     block_job_set_speed(job, speed, errp);
+    aio_context_release(aio_context);
 }
 
 void qmp_block_job_cancel(const char *device,
                           bool has_force, bool force, Error **errp)
 {
-    BlockJob *job = find_block_job(device);
-
-    if (!has_force) {
-        force = false;
-    }
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context);
 
     if (!job) {
         error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
+
+    if (!has_force) {
+        force = false;
+    }
+
     if (job->paused && !force) {
         error_setg(errp, "The block job for device '%s' is currently paused",
                    device);
-        return;
+        goto out;
     }
 
     trace_qmp_block_job_cancel(job);
     block_job_cancel(job);
+out:
+    aio_context_release(aio_context);
 }
 
 void qmp_block_job_pause(const char *device, Error **errp)
 {
-    BlockJob *job = find_block_job(device);
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context);
 
     if (!job) {
         error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
@@ -2456,11 +2477,13 @@ void qmp_block_job_pause(const char *device, Error **errp)
 
     trace_qmp_block_job_pause(job);
     block_job_pause(job);
+    aio_context_release(aio_context);
 }
 
 void qmp_block_job_resume(const char *device, Error **errp)
 {
-    BlockJob *job = find_block_job(device);
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context);
 
     if (!job) {
         error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
@@ -2469,11 +2492,13 @@ void qmp_block_job_resume(const char *device, Error **errp)
 
     trace_qmp_block_job_resume(job);
     block_job_resume(job);
+    aio_context_release(aio_context);
 }
 
 void qmp_block_job_complete(const char *device, Error **errp)
 {
-    BlockJob *job = find_block_job(device);
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context);
 
     if (!job) {
         error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
@@ -2482,6 +2507,7 @@ void qmp_block_job_complete(const char *device, Error **errp)
 
     trace_qmp_block_job_complete(job);
     block_job_complete(job, errp);
+    aio_context_release(aio_context);
 }
 
 void qmp_change_backing_file(const char *device,
-- 
1.9.3

[Qemu-devel] [PATCH v2 02/11] blockdev: acquire AioContext in do_qmp_query_block_jobs_one()

[Stefan Hajnoczi]

Make sure that query-block-jobs acquires the BlockDriverState
AioContext so that the blockjob isn't running in another thread while we
access its state.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1c7dab4..fd55904 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2649,14 +2649,19 @@ fail:
 static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
 {
     BlockJobInfoList **prev = opaque;
-    BlockJob *job = bs->job;
+    AioContext *aio_context;
+
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
 
-    if (job) {
+    if (bs->job) {
         BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
         elem->value = block_job_query(bs->job);
         (*prev)->next = elem;
         *prev = elem;
     }
+
+    aio_context_release(aio_context);
 }
 
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 02/11] blockdev: acquire AioContext in do_qmp_query_block_jobs_one()

[Max Reitz]

On 2014-10-21 at 13:03, Stefan Hajnoczi wrote:
> Make sure that query-block-jobs acquires the BlockDriverState
> AioContext so that the blockjob isn't running in another thread while we
> access its state.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>   blockdev.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 1c7dab4..fd55904 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2649,14 +2649,19 @@ fail:
>   static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
>   {
>       BlockJobInfoList **prev = opaque;
> -    BlockJob *job = bs->job;
> +    AioContext *aio_context;
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
>   
> -    if (job) {
> +    if (bs->job) {
>           BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
>           elem->value = block_job_query(bs->job);
>           (*prev)->next = elem;
>           *prev = elem;
>       }
> +
> +    aio_context_release(aio_context);
>   }
>   
>   BlockJobInfoList *qmp_query_block_jobs(Error **errp)

As already said in my review for v1, this conflicts with Markus's patch 
"block: Eliminate bdrv_iterate(), use bdrv_next()"; it's still not in 
master, but by now the pull request has been sent so I guess it's up to 
you to resolve it. ;-)

Max

Re: [Qemu-devel] [PATCH v2 02/11] blockdev: acquire AioContext in do_qmp_query_block_jobs_one()

[Stefan Hajnoczi]

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

On Wed, Oct 22, 2014 at 01:10:45PM +0200, Max Reitz wrote:
> On 2014-10-21 at 13:03, Stefan Hajnoczi wrote:
> >Make sure that query-block-jobs acquires the BlockDriverState
> >AioContext so that the blockjob isn't running in another thread while we
> >access its state.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >Reviewed-by: Max Reitz <mreitz@redhat.com>
> >---
> >  blockdev.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index 1c7dab4..fd55904 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -2649,14 +2649,19 @@ fail:
> >  static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> >  {
> >      BlockJobInfoList **prev = opaque;
> >-    BlockJob *job = bs->job;
> >+    AioContext *aio_context;
> >+
> >+    aio_context = bdrv_get_aio_context(bs);
> >+    aio_context_acquire(aio_context);
> >-    if (job) {
> >+    if (bs->job) {
> >          BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
> >          elem->value = block_job_query(bs->job);
> >          (*prev)->next = elem;
> >          *prev = elem;
> >      }
> >+
> >+    aio_context_release(aio_context);
> >  }
> >  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> 
> As already said in my review for v1, this conflicts with Markus's patch
> "block: Eliminate bdrv_iterate(), use bdrv_next()"; it's still not in
> master, but by now the pull request has been sent so I guess it's up to you
> to resolve it. ;-)

Yes.  The conflict is small, here is the resolution:

diff --git a/blockdev.c b/blockdev.c
index 501473d..40fc5d6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2628,12 +2628,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
     BlockDriverState *bs;
 
     for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(aio_context);
+
         if (bs->job) {
             BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
             elem->value = block_job_query(bs->job);
             *p_next = elem;
             p_next = &elem->next;
         }
+
+        aio_context_release(aio_context);
     }
 
     return head;

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

[Qemu-devel] [PATCH v2 03/11] blockdev: acquire AioContext in blockdev_mark_auto_del()

[Stefan Hajnoczi]

When an emulated storage controller is unrealized it will call
blockdev_mark_auto_del().  This will cancel any running block job (and
that eventually releases its reference to the BDS so it can be freed).

Since the block job may be executing in another AioContext we must
acquire/release to ensure thread safety.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index fd55904..c46876c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -113,14 +113,21 @@ void override_max_devs(BlockInterfaceType type, int max_devs)
 void blockdev_mark_auto_del(BlockDriverState *bs)
 {
     DriveInfo *dinfo = drive_get_by_blockdev(bs);
+    AioContext *aio_context;
 
     if (dinfo && !dinfo->enable_auto_del) {
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (bs->job) {
         block_job_cancel(bs->job);
     }
+
+    aio_context_release(aio_context);
+
     if (dinfo) {
         dinfo->auto_del = 1;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v2 04/11] blockdev: add note that block_job_cb() must be thread-safe

[Stefan Hajnoczi]

This function is correct but we should document the constraint that
everything must be thread-safe.

Emitting QMP events and scheduling BHs are both thread-safe so nothing
needs to be done here.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index c46876c..a891f8e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1953,6 +1953,11 @@ out:
 
 static void block_job_cb(void *opaque, int ret)
 {
+    /* Note that this function may be executed from another AioContext besides
+     * the QEMU main loop.  If you need to access anything that assumes the
+     * QEMU global mutex, use a BH or introduce a mutex.
+     */
+
     BlockDriverState *bs = opaque;
     const char *msg = NULL;
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 05/11] blockjob: add block_job_defer_to_main_loop()

[Stefan Hajnoczi]

Block jobs will run in the BlockDriverState's AioContext, which may not
always be the QEMU main loop.

There are some block layer APIs that are either not thread-safe or risk
lock ordering problems.  This includes bdrv_unref(), bdrv_close(), and
anything that calls bdrv_drain_all().

The block_job_defer_to_main_loop() API allows a block job to schedule a
function to run in the main loop with the BlockDriverState AioContext
held.

This function will be used to perform cleanup and backing chain
manipulations in block jobs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockjob.c               | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/block/blockjob.h | 19 +++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 0689fdd..b73f337 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -313,3 +313,48 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
     }
     return action;
 }
+
+typedef struct {
+    BlockJob *job;
+    QEMUBH *bh;
+    AioContext *aio_context;
+    BlockJobDeferToMainLoopFn *fn;
+    void *opaque;
+} BlockJobDeferToMainLoopData;
+
+static void block_job_defer_to_main_loop_bh(void *opaque)
+{
+    BlockJobDeferToMainLoopData *data = opaque;
+    AioContext *aio_context;
+
+    qemu_bh_delete(data->bh);
+
+    /* Prevent race with block_job_defer_to_main_loop() */
+    aio_context_acquire(data->aio_context);
+
+    /* Fetch BDS AioContext again, in case it has changed */
+    aio_context = bdrv_get_aio_context(data->job->bs);
+    aio_context_acquire(aio_context);
+
+    data->fn(data->job, data->opaque);
+
+    aio_context_release(aio_context);
+
+    aio_context_release(data->aio_context);
+
+    g_free(data);
+}
+
+void block_job_defer_to_main_loop(BlockJob *job,
+                                  BlockJobDeferToMainLoopFn *fn,
+                                  void *opaque)
+{
+    BlockJobDeferToMainLoopData *data = g_malloc(sizeof(*data));
+    data->job = job;
+    data->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, data);
+    data->aio_context = bdrv_get_aio_context(job->bs);
+    data->fn = fn;
+    data->opaque = opaque;
+
+    qemu_bh_schedule(data->bh);
+}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 60aa835..5c13124 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -295,4 +295,23 @@ void block_job_iostatus_reset(BlockJob *job);
 BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
                                         BlockdevOnError on_err,
                                         int is_read, int error);
+
+typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
+
+/**
+ * block_job_defer_to_main_loop:
+ * @job: The job
+ * @fn: The function to run in the main loop
+ * @opaque: The opaque value that is passed to @fn
+ *
+ * Execute a given function in the main loop with the BlockDriverState
+ * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
+ * anything that uses bdrv_drain_all() in the main loop.
+ *
+ * The @job AioContext is held while @fn executes.
+ */
+void block_job_defer_to_main_loop(BlockJob *job,
+                                  BlockJobDeferToMainLoopFn *fn,
+                                  void *opaque);
+
 #endif
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 05/11] blockjob: add block_job_defer_to_main_loop()

[Max Reitz]

On 2014-10-21 at 13:03, Stefan Hajnoczi wrote:
> Block jobs will run in the BlockDriverState's AioContext, which may not
> always be the QEMU main loop.
>
> There are some block layer APIs that are either not thread-safe or risk
> lock ordering problems.  This includes bdrv_unref(), bdrv_close(), and
> anything that calls bdrv_drain_all().
>
> The block_job_defer_to_main_loop() API allows a block job to schedule a
> function to run in the main loop with the BlockDriverState AioContext
> held.
>
> This function will be used to perform cleanup and backing chain
> manipulations in block jobs.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockjob.c               | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   include/block/blockjob.h | 19 +++++++++++++++++++
>   2 files changed, 64 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 06/11] block: add bdrv_drain()

[Stefan Hajnoczi]

Now that op blockers are in use, we can ensure that no other sources are
generating I/O on a BlockDriverState.  Therefore it is possible to drain
requests for a single BDS.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 36 +++++++++++++++++++++++++++++-------
 include/block/block.h |  1 +
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 27533f3..76ed5cd 100644
--- a/block.c
+++ b/block.c
@@ -1913,6 +1913,34 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
     return false;
 }
 
+static bool bdrv_drain_one(BlockDriverState *bs)
+{
+    bool bs_busy;
+
+    bdrv_flush_io_queue(bs);
+    bdrv_start_throttled_reqs(bs);
+    bs_busy = bdrv_requests_pending(bs);
+    bs_busy |= aio_poll(bdrv_get_aio_context(bs), bs_busy);
+    return bs_busy;
+}
+
+/*
+ * Wait for pending requests to complete on a single BlockDriverState subtree
+ *
+ * See the warning in bdrv_drain_all().  This function can only be called if
+ * you are sure nothing can generate I/O because you have op blockers
+ * installed.
+ *
+ * Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState
+ * AioContext.
+ */
+void bdrv_drain(BlockDriverState *bs)
+{
+    while (bdrv_drain_one(bs)) {
+        /* Keep iterating */
+    }
+}
+
 /*
  * Wait for pending requests to complete across all BlockDriverStates
  *
@@ -1936,16 +1964,10 @@ void bdrv_drain_all(void)
 
         QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
             AioContext *aio_context = bdrv_get_aio_context(bs);
-            bool bs_busy;
 
             aio_context_acquire(aio_context);
-            bdrv_flush_io_queue(bs);
-            bdrv_start_throttled_reqs(bs);
-            bs_busy = bdrv_requests_pending(bs);
-            bs_busy |= aio_poll(aio_context, bs_busy);
+            busy |= bdrv_drain_one(bs);
             aio_context_release(aio_context);
-
-            busy |= bs_busy;
         }
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 3318f0d..61df804 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -372,6 +372,7 @@ int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
 int bdrv_flush_all(void);
 void bdrv_close_all(void);
+void bdrv_drain(BlockDriverState *bs);
 void bdrv_drain_all(void);
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
-- 
1.9.3

[Qemu-devel] [PATCH v2 07/11] block: let backup blockjob run in BDS AioContext

[Stefan Hajnoczi]

The backup block job must run in the BlockDriverState AioContext so that
it works with dataplane.

The basics of acquiring the AioContext are easy in blockdev.c.

The completion code in block/backup.c must call bdrv_unref() from the
main loop.  Use block_job_defer_to_main_loop() to achieve that.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 21 +++++++++++++++++++--
 blockdev.c     | 23 ++++++++++++++++-------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d0b0225..9d015b5 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -227,9 +227,25 @@ static BlockErrorAction backup_error_action(BackupBlockJob *job,
     }
 }
 
+typedef struct {
+    int ret;
+} BackupCompleteData;
+
+static void backup_complete(BlockJob *job, void *opaque)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+    BackupCompleteData *data = opaque;
+
+    bdrv_unref(s->target);
+
+    block_job_completed(job, data->ret);
+    g_free(data);
+}
+
 static void coroutine_fn backup_run(void *opaque)
 {
     BackupBlockJob *job = opaque;
+    BackupCompleteData *data;
     BlockDriverState *bs = job->common.bs;
     BlockDriverState *target = job->target;
     BlockdevOnError on_target_error = job->on_target_error;
@@ -344,9 +360,10 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
-    bdrv_unref(target);
 
-    block_job_completed(&job->common, ret);
+    data = g_malloc(sizeof(*data));
+    data->ret = ret;
+    block_job_defer_to_main_loop(&job->common, backup_complete, data);
 }
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
diff --git a/blockdev.c b/blockdev.c
index a891f8e..f2ee6e9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2132,6 +2132,7 @@ void qmp_drive_backup(const char *device, const char *target,
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
+    AioContext *aio_context;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
     int flags;
@@ -2157,9 +2158,12 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        return;
+        goto out;
     }
 
     if (!has_format) {
@@ -2169,12 +2173,12 @@ void qmp_drive_backup(const char *device, const char *target,
         drv = bdrv_find_format(format);
         if (!drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            return;
+            goto out;
         }
     }
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-        return;
+        goto out;
     }
 
     flags = bs->open_flags | BDRV_O_RDWR;
@@ -2194,7 +2198,7 @@ void qmp_drive_backup(const char *device, const char *target,
     size = bdrv_getlength(bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "bdrv_getlength failed");
-        return;
+        goto out;
     }
 
     if (mode != NEW_IMAGE_MODE_EXISTING) {
@@ -2211,23 +2215,28 @@ void qmp_drive_backup(const char *device, const char *target,
 
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
     target_bs = NULL;
     ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
+    bdrv_set_aio_context(target_bs, aio_context);
+
     backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
-- 
1.9.3

[Qemu-devel] [PATCH v2 08/11] block: let stream blockjob run in BDS AioContext

[Stefan Hajnoczi]

The stream block job must run in the BlockDriverState AioContext so that
it works with dataplane.

The basics of acquiring the AioContext are easy in blockdev.c.

The tricky part is the completion code which drops part of the backing
file chain.  This must be done in the main loop where bdrv_unref() and
bdrv_close() are safe to call.  Use block_job_defer_to_main_loop() to
achieve that.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 50 ++++++++++++++++++++++++++++++++++++--------------
 blockdev.c     | 16 ++++++++++++----
 2 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index cdea3e8..21bce12 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -79,9 +79,39 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
     bdrv_refresh_limits(top, NULL);
 }
 
+typedef struct {
+    int ret;
+    bool reached_end;
+} StreamCompleteData;
+
+static void stream_complete(BlockJob *job, void *opaque)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common);
+    StreamCompleteData *data = opaque;
+    BlockDriverState *base = s->base;
+
+    if (!block_job_is_cancelled(&s->common) && data->reached_end &&
+        data->ret == 0) {
+        const char *base_id = NULL, *base_fmt = NULL;
+        if (base) {
+            base_id = s->backing_file_str;
+            if (base->drv) {
+                base_fmt = base->drv->format_name;
+            }
+        }
+        data->ret = bdrv_change_backing_file(job->bs, base_id, base_fmt);
+        close_unused_images(job->bs, base, base_id);
+    }
+
+    g_free(s->backing_file_str);
+    block_job_completed(&s->common, data->ret);
+    g_free(data);
+}
+
 static void coroutine_fn stream_run(void *opaque)
 {
     StreamBlockJob *s = opaque;
+    StreamCompleteData *data;
     BlockDriverState *bs = s->common.bs;
     BlockDriverState *base = s->base;
     int64_t sector_num, end;
@@ -183,21 +213,13 @@ wait:
     /* Do not remove the backing file if an error was there but ignored.  */
     ret = error;
 
-    if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
-        const char *base_id = NULL, *base_fmt = NULL;
-        if (base) {
-            base_id = s->backing_file_str;
-            if (base->drv) {
-                base_fmt = base->drv->format_name;
-            }
-        }
-        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
-        close_unused_images(bs, base, base_id);
-    }
-
     qemu_vfree(buf);
-    g_free(s->backing_file_str);
-    block_job_completed(&s->common, ret);
+
+    /* Modify backing chain and close BDSes in main loop */
+    data = g_malloc(sizeof(*data));
+    data->ret = ret;
+    data->reached_end = sector_num == end;
+    block_job_defer_to_main_loop(&s->common, stream_complete, data);
 }
 
 static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/blockdev.c b/blockdev.c
index f2ee6e9..5d905ce 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1987,6 +1987,7 @@ void qmp_block_stream(const char *device,
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
+    AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;
 
@@ -2000,16 +2001,20 @@ void qmp_block_stream(const char *device,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
-        return;
+        goto out;
     }
 
     if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
             error_set(errp, QERR_BASE_NOT_FOUND, base);
-            return;
+            goto out;
         }
+        assert(bdrv_get_aio_context(base_bs) == aio_context);
         base_name = base;
     }
 
@@ -2018,7 +2023,7 @@ void qmp_block_stream(const char *device,
     if (base_bs == NULL && has_backing_file) {
         error_setg(errp, "backing file specified, but streaming the "
                          "entire chain");
-        return;
+        goto out;
     }
 
     /* backing_file string overrides base bs filename */
@@ -2028,10 +2033,13 @@ void qmp_block_stream(const char *device,
                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
     trace_qmp_block_stream(bs, bs->job);
+
+out:
+    aio_context_release(aio_context);
 }
 
 void qmp_block_commit(const char *device,
-- 
1.9.3

[Qemu-devel] [PATCH v2 09/11] block: let mirror blockjob run in BDS AioContext

[Stefan Hajnoczi]

The mirror block job must run in the BlockDriverState AioContext so that
it works with dataplane.

Acquire the AioContext in blockdev.c so starting the block job is safe.

Note that to_replace is treated separately from other BlockDriverStates
in that it does not need to be in the same AioContext.  Explicitly
acquire/release to_replace's AioContext when accessing it.

The completion code in block/mirror.c must perform BDS graph
manipulation and bdrv_reopen() from the main loop.  Use
block_job_defer_to_main_loop() to achieve that.

The bdrv_drain_all() call is not allowed outside the main loop since it
could lead to lock ordering problems.  Use bdrv_drain(bs) instead
because we have acquired the AioContext so nothing else can sneak in
I/O.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c        | 13 +++++++--
 block/mirror.c | 85 ++++++++++++++++++++++++++++++++++++++++------------------
 blockdev.c     | 38 ++++++++++++++++++--------
 3 files changed, 97 insertions(+), 39 deletions(-)

diff --git a/block.c b/block.c
index 76ed5cd..6f47bda 100644
--- a/block.c
+++ b/block.c
@@ -5929,13 +5929,19 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
 BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
 {
     BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
+    AioContext *aio_context;
+
     if (!to_replace_bs) {
         error_setg(errp, "Node name '%s' not found", node_name);
         return NULL;
     }
 
+    aio_context = bdrv_get_aio_context(to_replace_bs);
+    aio_context_acquire(aio_context);
+
     if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
-        return NULL;
+        to_replace_bs = NULL;
+        goto out;
     }
 
     /* We don't want arbitrary node of the BDS chain to be replaced only the top
@@ -5945,9 +5951,12 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
      */
     if (!bdrv_is_first_non_filter(to_replace_bs)) {
         error_setg(errp, "Only top most non filter can be replaced");
-        return NULL;
+        to_replace_bs = NULL;
+        goto out;
     }
 
+out:
+    aio_context_release(aio_context);
     return to_replace_bs;
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 18b18e0..19b87d0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -314,9 +314,56 @@ static void mirror_drain(MirrorBlockJob *s)
     }
 }
 
+typedef struct {
+    int ret;
+} MirrorExitData;
+
+static void mirror_exit(BlockJob *job, void *opaque)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+    MirrorExitData *data = opaque;
+    AioContext *replace_aio_context = NULL;
+
+    if (s->to_replace) {
+        replace_aio_context = bdrv_get_aio_context(s->to_replace);
+        aio_context_acquire(replace_aio_context);
+    }
+
+    if (s->should_complete && data->ret == 0) {
+        BlockDriverState *to_replace = s->common.bs;
+        if (s->to_replace) {
+            to_replace = s->to_replace;
+        }
+        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
+            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
+        }
+        bdrv_swap(s->target, to_replace);
+        if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
+            /* drop the bs loop chain formed by the swap: break the loop then
+             * trigger the unref from the top one */
+            BlockDriverState *p = s->base->backing_hd;
+            bdrv_set_backing_hd(s->base, NULL);
+            bdrv_unref(p);
+        }
+    }
+    if (s->to_replace) {
+        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
+        error_free(s->replace_blocker);
+        bdrv_unref(s->to_replace);
+    }
+    if (replace_aio_context) {
+        aio_context_release(replace_aio_context);
+    }
+    g_free(s->replaces);
+    bdrv_unref(s->target);
+    block_job_completed(&s->common, data->ret);
+    g_free(data);
+}
+
 static void coroutine_fn mirror_run(void *opaque)
 {
     MirrorBlockJob *s = opaque;
+    MirrorExitData *data;
     BlockDriverState *bs = s->common.bs;
     int64_t sector_num, end, sectors_per_chunk, length;
     uint64_t last_pause_ns;
@@ -467,7 +514,7 @@ static void coroutine_fn mirror_run(void *opaque)
              * mirror_populate runs.
              */
             trace_mirror_before_drain(s, cnt);
-            bdrv_drain_all();
+            bdrv_drain(bs);
             cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
         }
 
@@ -510,31 +557,10 @@ immediate_exit:
     g_free(s->in_flight_bitmap);
     bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
     bdrv_iostatus_disable(s->target);
-    if (s->should_complete && ret == 0) {
-        BlockDriverState *to_replace = s->common.bs;
-        if (s->to_replace) {
-            to_replace = s->to_replace;
-        }
-        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
-            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
-        }
-        bdrv_swap(s->target, to_replace);
-        if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
-            /* drop the bs loop chain formed by the swap: break the loop then
-             * trigger the unref from the top one */
-            BlockDriverState *p = s->base->backing_hd;
-            bdrv_set_backing_hd(s->base, NULL);
-            bdrv_unref(p);
-        }
-    }
-    if (s->to_replace) {
-        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
-        error_free(s->replace_blocker);
-        bdrv_unref(s->to_replace);
-    }
-    g_free(s->replaces);
-    bdrv_unref(s->target);
-    block_job_completed(&s->common, ret);
+
+    data = g_malloc(sizeof(*data));
+    data->ret = ret;
+    block_job_defer_to_main_loop(&s->common, mirror_exit, data);
 }
 
 static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -573,16 +599,23 @@ static void mirror_complete(BlockJob *job, Error **errp)
 
     /* check the target bs is not blocked and block all operations on it */
     if (s->replaces) {
+        AioContext *replace_aio_context;
+
         s->to_replace = check_to_replace_node(s->replaces, &local_err);
         if (!s->to_replace) {
             error_propagate(errp, local_err);
             return;
         }
 
+        replace_aio_context = bdrv_get_aio_context(s->to_replace);
+        aio_context_acquire(replace_aio_context);
+
         error_setg(&s->replace_blocker,
                    "block device is in use by block-job-complete");
         bdrv_op_block_all(s->to_replace, s->replace_blocker);
         bdrv_ref(s->to_replace);
+
+        aio_context_release(replace_aio_context);
     }
 
     s->should_complete = true;
diff --git a/blockdev.c b/blockdev.c
index 5d905ce..2bd6c58 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2269,6 +2269,7 @@ void qmp_drive_mirror(const char *device, const char *target,
 {
     BlockDriverState *bs;
     BlockDriverState *source, *target_bs;
+    AioContext *aio_context;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
     QDict *options = NULL;
@@ -2311,9 +2312,12 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        return;
+        goto out;
     }
 
     if (!has_format) {
@@ -2323,12 +2327,12 @@ void qmp_drive_mirror(const char *device, const char *target,
         drv = bdrv_find_format(format);
         if (!drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            return;
+            goto out;
         }
     }
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
-        return;
+        goto out;
     }
 
     flags = bs->open_flags | BDRV_O_RDWR;
@@ -2343,29 +2347,36 @@ void qmp_drive_mirror(const char *device, const char *target,
     size = bdrv_getlength(bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "bdrv_getlength failed");
-        return;
+        goto out;
     }
 
     if (has_replaces) {
         BlockDriverState *to_replace_bs;
+        AioContext *replace_aio_context;
+        int64_t replace_size;
 
         if (!has_node_name) {
             error_setg(errp, "a node-name must be provided when replacing a"
                              " named node of the graph");
-            return;
+            goto out;
         }
 
         to_replace_bs = check_to_replace_node(replaces, &local_err);
 
         if (!to_replace_bs) {
             error_propagate(errp, local_err);
-            return;
+            goto out;
         }
 
-        if (size != bdrv_getlength(to_replace_bs)) {
+        replace_aio_context = bdrv_get_aio_context(to_replace_bs);
+        aio_context_acquire(replace_aio_context);
+        replace_size = bdrv_getlength(to_replace_bs);
+        aio_context_release(replace_aio_context);
+
+        if (size != replace_size) {
             error_setg(errp, "cannot replace image with a mirror image of "
                              "different size");
-            return;
+            goto out;
         }
     }
 
@@ -2394,7 +2405,7 @@ void qmp_drive_mirror(const char *device, const char *target,
 
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
     if (has_node_name) {
@@ -2410,9 +2421,11 @@ void qmp_drive_mirror(const char *device, const char *target,
                     flags | BDRV_O_NO_BACKING, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
+    bdrv_set_aio_context(target_bs, aio_context);
+
     /* pass the node name to replace to mirror start since it's loose coupling
      * and will allow to check whether the node still exist at mirror completion
      */
@@ -2424,8 +2437,11 @@ void qmp_drive_mirror(const char *device, const char *target,
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 /* Get the block job for a given device name and acquire its AioContext */
-- 
1.9.3

[Qemu-devel] [PATCH v2 10/11] block: let commit blockjob run in BDS AioContext

[Stefan Hajnoczi]

The commit block job must run in the BlockDriverState AioContext so that
it works with dataplane.

Acquire the AioContext in blockdev.c so starting the block job is safe.
One detail here is that the bdrv_drain_all() must be moved inside the
aio_context_acquire() region so requests cannot sneak in between the
drain and acquire.

The completion code in block/commit.c must perform backing chain
manipulation and bdrv_reopen() from the main loop.  Use
block_job_defer_to_main_loop() to achieve that.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/commit.c | 70 ++++++++++++++++++++++++++++++++++++----------------------
 blockdev.c     | 29 ++++++++++++++++--------
 2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 91517d3..6f42105 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -60,17 +60,50 @@ static int coroutine_fn commit_populate(BlockDriverState *bs,
     return 0;
 }
 
-static void coroutine_fn commit_run(void *opaque)
+typedef struct {
+    int ret;
+} CommitCompleteData;
+
+static void commit_complete(BlockJob *job, void *opaque)
 {
-    CommitBlockJob *s = opaque;
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
+    CommitCompleteData *data = opaque;
     BlockDriverState *active = s->active;
     BlockDriverState *top = s->top;
     BlockDriverState *base = s->base;
     BlockDriverState *overlay_bs;
+    int ret = data->ret;
+
+    if (!block_job_is_cancelled(&s->common) && ret == 0) {
+        /* success */
+        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
+    }
+
+    /* restore base open flags here if appropriate (e.g., change the base back
+     * to r/o). These reopens do not need to be atomic, since we won't abort
+     * even on failure here */
+    if (s->base_flags != bdrv_get_flags(base)) {
+        bdrv_reopen(base, s->base_flags, NULL);
+    }
+    overlay_bs = bdrv_find_overlay(active, top);
+    if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
+        bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
+    }
+    g_free(s->backing_file_str);
+    block_job_completed(&s->common, ret);
+    g_free(data);
+}
+
+static void coroutine_fn commit_run(void *opaque)
+{
+    CommitBlockJob *s = opaque;
+    CommitCompleteData *data;
+    BlockDriverState *top = s->top;
+    BlockDriverState *base = s->base;
     int64_t sector_num, end;
     int ret = 0;
     int n = 0;
-    void *buf;
+    void *buf = NULL;
     int bytes_written = 0;
     int64_t base_len;
 
@@ -78,18 +111,18 @@ static void coroutine_fn commit_run(void *opaque)
 
 
     if (s->common.len < 0) {
-        goto exit_restore_reopen;
+        goto out;
     }
 
     ret = base_len = bdrv_getlength(base);
     if (base_len < 0) {
-        goto exit_restore_reopen;
+        goto out;
     }
 
     if (base_len < s->common.len) {
         ret = bdrv_truncate(base, s->common.len);
         if (ret) {
-            goto exit_restore_reopen;
+            goto out;
         }
     }
 
@@ -128,7 +161,7 @@ wait:
             if (s->on_error == BLOCKDEV_ON_ERROR_STOP ||
                 s->on_error == BLOCKDEV_ON_ERROR_REPORT||
                 (s->on_error == BLOCKDEV_ON_ERROR_ENOSPC && ret == -ENOSPC)) {
-                goto exit_free_buf;
+                goto out;
             } else {
                 n = 0;
                 continue;
@@ -140,27 +173,12 @@ wait:
 
     ret = 0;
 
-    if (!block_job_is_cancelled(&s->common) && sector_num == end) {
-        /* success */
-        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
-    }
-
-exit_free_buf:
+out:
     qemu_vfree(buf);
 
-exit_restore_reopen:
-    /* restore base open flags here if appropriate (e.g., change the base back
-     * to r/o). These reopens do not need to be atomic, since we won't abort
-     * even on failure here */
-    if (s->base_flags != bdrv_get_flags(base)) {
-        bdrv_reopen(base, s->base_flags, NULL);
-    }
-    overlay_bs = bdrv_find_overlay(active, top);
-    if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
-        bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
-    }
-    g_free(s->backing_file_str);
-    block_job_completed(&s->common, ret);
+    data = g_malloc(sizeof(*data));
+    data->ret = ret;
+    block_job_defer_to_main_loop(&s->common, commit_complete, data);
 }
 
 static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/blockdev.c b/blockdev.c
index 2bd6c58..09b2567 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2051,6 +2051,7 @@ void qmp_block_commit(const char *device,
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
+    AioContext *aio_context;
     Error *local_err = NULL;
     /* This will be part of the QMP command, if/when the
      * BlockdevOnError change for blkmirror makes it in
@@ -2061,9 +2062,6 @@ void qmp_block_commit(const char *device,
         speed = 0;
     }
 
-    /* drain all i/o before commits */
-    bdrv_drain_all();
-
     /* Important Note:
      *  libvirt relies on the DeviceNotFound error class in order to probe for
      *  live commit feature versions; for this to work, we must make sure to
@@ -2075,8 +2073,14 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
+    /* drain all i/o before commits */
+    bdrv_drain_all();
+
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
-        return;
+        goto out;
     }
 
     /* default top_bs is the active layer */
@@ -2090,9 +2094,11 @@ void qmp_block_commit(const char *device,
 
     if (top_bs == NULL) {
         error_setg(errp, "Top image file %s not found", top ? top : "NULL");
-        return;
+        goto out;
     }
 
+    assert(bdrv_get_aio_context(top_bs) == aio_context);
+
     if (has_base && base) {
         base_bs = bdrv_find_backing_image(top_bs, base);
     } else {
@@ -2101,20 +2107,22 @@ void qmp_block_commit(const char *device,
 
     if (base_bs == NULL) {
         error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-        return;
+        goto out;
     }
 
+    assert(bdrv_get_aio_context(base_bs) == aio_context);
+
     /* Do not allow attempts to commit an image into itself */
     if (top_bs == base_bs) {
         error_setg(errp, "cannot commit an image into itself");
-        return;
+        goto out;
     }
 
     if (top_bs == bs) {
         if (has_backing_file) {
             error_setg(errp, "'backing-file' specified,"
                              " but 'top' is the active layer");
-            return;
+            goto out;
         }
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
@@ -2124,8 +2132,11 @@ void qmp_block_commit(const char *device,
     }
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 void qmp_drive_backup(const char *device, const char *target,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 10/11] block: let commit blockjob run in BDS AioContext

[Max Reitz]

On 2014-10-21 at 13:03, Stefan Hajnoczi wrote:
> The commit block job must run in the BlockDriverState AioContext so that
> it works with dataplane.
>
> Acquire the AioContext in blockdev.c so starting the block job is safe.
> One detail here is that the bdrv_drain_all() must be moved inside the
> aio_context_acquire() region so requests cannot sneak in between the
> drain and acquire.
>
> The completion code in block/commit.c must perform backing chain
> manipulation and bdrv_reopen() from the main loop.  Use
> block_job_defer_to_main_loop() to achieve that.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/commit.c | 70 ++++++++++++++++++++++++++++++++++++----------------------
>   blockdev.c     | 29 ++++++++++++++++--------
>   2 files changed, 64 insertions(+), 35 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 11/11] block: declare blockjobs and dataplane friends!

[Stefan Hajnoczi]

Now that blockjobs use AioContext they are safe for use with dataplane.
Unblock them!

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockjob.c                      | 1 +
 hw/block/dataplane/virtio-blk.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index b73f337..5f93c18 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -50,6 +50,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     error_setg(&job->blocker, "block device is in use by block job: %s",
                BlockJobType_lookup[driver->job_type]);
     bdrv_op_block_all(bs, job->blocker);
+    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
     job->driver        = driver;
     job->bs            = bs;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5458f9d..0603230 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -195,6 +195,11 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     bdrv_op_block_all(blk->conf.bs, s->blocker);
     bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_RESIZE, s->blocker);
     bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
+    bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
+    bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_COMMIT, s->blocker);
+    bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_MIRROR, s->blocker);
+    bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_STREAM, s->blocker);
+    bdrv_op_unblock(blk->conf.bs, BLOCK_OP_TYPE_REPLACE, s->blocker);
 
     *dataplane = s;
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 00/11] block: allow blockjobs to coexist with dataplane

[Stefan Hajnoczi]

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

On Tue, Oct 21, 2014 at 12:03:49PM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Protect block_job_defer_to_main_loop_bh() against AioContext change [Max]
>  * Drop unnecessary if (buf) around qemu_vfree(buf) [Max]
> 
> Almost all the infrastructure is in place to make blockjobs safe for use with
> dataplane:
> 
>   * Op blockers all us to exclude commands that could conflict with a blockjob
>     or dataplane.
> 
>   * AioContext acquire/release allows threads to temporarily access a
>     BlockDriverState that is running in another thread.
> 
> This series introduces a few additional helpers:
> 
>   * block_job_defer_to_main_loop() which allows blockjobs to run their
>     completion code in the QEMU main loop.  This is necessary because some
>     operations are not safe outside the QEMU global mutex.
> 
>   * bdrv_drain() which can be used in limited cases to wait for in-flight
>     requests to complete (as opposed to the global bdrv_drain_all() function).
> 
> The approach taken in this series is to convert the blockdev.c monitor command
> so it acquires the BlockDriverState's AioContext.  Normally only 1 AioContext
> is involved at a time but the mirror job's to_replace argument can involve a
> second AioContext.
> 
> Then the block job code itself is converted to defer main loop code using
> block_job_defer_to_main_loop().
> 
> Example:
> 
>   $ qemu-system-x86_64 -enable-kvm -m 1024 \
>         -drive if=none,id=drive0,file=test.img \
>         -object iothread,iothread0 \
>         -device virtio-blk-pci,drive=drive0,iothread=iothread0
>   (qemu) drive_mirror drive0 test2.img
> 
> Stefan Hajnoczi (11):
>   block: acquire AioContext in generic blockjob QMP commands
>   blockdev: acquire AioContext in do_qmp_query_block_jobs_one()
>   blockdev: acquire AioContext in blockdev_mark_auto_del()
>   blockdev: add note that block_job_cb() must be thread-safe
>   blockjob: add block_job_defer_to_main_loop()
>   block: add bdrv_drain()
>   block: let backup blockjob run in BDS AioContext
>   block: let stream blockjob run in BDS AioContext
>   block: let mirror blockjob run in BDS AioContext
>   block: let commit blockjob run in BDS AioContext
>   block: declare blockjobs and dataplane friends!
> 
>  block.c                         |  49 +++++++++--
>  block/backup.c                  |  21 ++++-
>  block/commit.c                  |  70 ++++++++++------
>  block/mirror.c                  |  85 +++++++++++++------
>  block/stream.c                  |  50 +++++++----
>  blockdev.c                      | 179 +++++++++++++++++++++++++++++-----------
>  blockjob.c                      |  46 +++++++++++
>  hw/block/dataplane/virtio-blk.c |   5 ++
>  include/block/block.h           |   1 +
>  include/block/blockjob.h        |  19 +++++
>  10 files changed, 402 insertions(+), 123 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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