[Qemu-devel] [PATCH v10 00/16] Support streaming to an intermediate layer

[Qemu-devel] [PATCH v10 00/16] Support streaming to an intermediate layer

[Alberto Garcia]

Hi all,

6 months after v9, and more than a year after v8, here's the new
version of the intermediate block streaming series.

There have been so many changes in QEMU since then that all patches in
this series are either new or have changed. Still, some of the most
important changes have been merged in separate series during all these
months (job IDs, support for "read-only" in the options QDict, etc.).

What's left can be roughly divided into:

- Patch 1: pause all block jobs during bdrv_reopen(). See the patch
  description for details.
- Patches 2-7: All jobs now block all BDSs that are involved in them,
  and not simply the root node.
- Patches 8-9: Support streaming to an intermediate layer. This
  is a rather simple change now and is similar to patches 2-7.
- Patch 10: Documentation (this one almost didn't change).
- Patches 11-16: Tests.

As you will see, the actual functional changes are rather simple. What
is there is many op blockers and many tests. I tried to write tests
for all scenarios that I could think of and I actually found a few
problems on the way, but I believe the code is stable now.

As always, comments and questions will be appreciated.

Berto

P.S. I don't include the backport-diff output because most patches are
new.

v10:
- Rebase the code.
- Pause block jobs during bdrv_reopen().
- Allow jobs in parallel again, and modify jobs to block all involved
  nodes, not just the root one.
- Add more tests.

v9: https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00411.html
- Rebase the code
- Block the active layer in order to forbid other block jobs in the
  same chain.
- Split the patch that adds block_job_next() into 4 (new patches 1-4).
- Replace the test that performs several block-stream operations in
  parallel with one that check that they're forbidden.
- Remove patches that have already been merged.

v8: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05754.html
- Rebased on top of Stefan's block branch (0a35bce416)
- The loop that pauses the block jobs in bdrv_drain_all() is now split
  in two: one that iterates the list of block jobs to stop them, and
  one that iterates the root bds in order to get the aio contexts.

v7: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html
- Rebased against the current master
- Updated bdrv_drain_all() to use the new block_job_next() API.

v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html
- fix the no-op test following Max's suggestions

v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html
- Fix a few typos
- Minor documentation updates
- Update test_stream_partial() to test no-ops
- New test case: test_stream_parallel()
- New test case: test_stream_overlapping()

v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html
- Refactor find_block_job to use the error from bdrv_lookup_bs()
- Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be
  dealing with nodes now.
- Fix @device comment in the BlockJobInfo documentation
- stream_start(): simplify the bdrv_reopen() call and use
  bdrv_get_device_or_node_name() for error messages.
- Use a different variable name for BlockDriverState *i
- Documentation fixes in docs/live-block-ops.txt
- Update iotest 30 since now test_device_not_found() returns
  GenericError
- Fix test case test_stream_partial()
- Add new test case test_stream_intermediate()
- Fix typos

v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html
- Keep a list of block jobs and make qmp_query_block_jobs() iterate
  over it.

v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html
- The 'block-stream' command does not have a 'node-name' parameter
  anymore and reuses 'device' for that purpose.
- Block jobs can now be owned by any intermediate node, and not just
  by the ones at the root. query-block-jobs is updated to reflect that
  change.
- The 'device' parameter of all 'block-job-*' commands can now take a
  node name.
- The BlockJobInfo type and all BLOCK_JOB_* events report the node
  name in the 'device' field if the node does not have a device name.
- All intermediate nodes are blocked (and checked for blockers) during
  the streaming operation.

v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html

Alberto Garcia (16):
  block: Pause all jobs during bdrv_reopen_multiple()
  block: Add block_job_add_bdrv()
  block: Use block_job_add_bdrv() in mirror_start_job()
  block: Use block_job_add_bdrv() in backup_start()
  block: Check blockers in all nodes involved in a block-commit job
  block: Block all nodes involved in the block-commit operation
  block: Block all intermediate nodes in commit_active_start()
  block: Support streaming to an intermediate layer
  block: Add QMP support for streaming to an intermediate layer
  docs: Document how to stream to an intermediate layer
  qemu-iotests: Test streaming to an intermediate layer
  qemu-iotests: Test block-stream operations in parallel
  qemu-iotests: Test overlapping stream and commit operations
  qemu-iotests: Test block-stream and block-commit in parallel
  qemu-iotests: Add iotests.supports_quorum()
  qemu-iotests: Test streaming to a Quorum child

 block.c                       |  25 +++-
 block/backup.c                |   5 +-
 block/commit.c                |  10 ++
 block/mirror.c                |  11 +-
 block/stream.c                |  24 ++++
 blockdev.c                    |  22 +++-
 blockjob.c                    |  19 ++-
 docs/live-block-ops.txt       |  31 +++--
 include/block/blockjob.h      |  14 +++
 qapi/block-core.json          |  10 +-
 tests/qemu-iotests/030        | 276 +++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out    |   4 +-
 tests/qemu-iotests/041        |  27 ++---
 tests/qemu-iotests/139        |   3 +-
 tests/qemu-iotests/iotests.py |   5 +-
 15 files changed, 438 insertions(+), 48 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Alberto Garcia]

When a BlockDriverState is about to be reopened it can trigger certain
operations that need to write to disk. During this process a different
block job can be woken up. If that block job completes and also needs
to call bdrv_reopen() it can happen that it needs to do it on the same
BlockDriverState that is still in the process of being reopened.

This can have fatal consequences, like in this example:

  1) Block job A starts and sleeps after a while.
  2) Block job B starts and tries to reopen node1 (a qcow2 file).
  3) Reopening node1 means flushing and replacing its qcow2 cache.
  4) While the qcow2 cache is being flushed, job A wakes up.
  5) Job A completes and reopens node1, replacing its cache.
  6) Job B resumes, but the cache that was being flushed no longer
     exists.

This patch pauses all block jobs during bdrv_reopen_multiple(), so
that step 4 can never happen and the operation is safe.

Note that this scenario can only happen if both bdrv_reopen() calls
are made by block jobs on the same backing chain. Otherwise there's no
chance that the same BlockDriverState appears in both reopen queues.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block.c b/block.c
index bb1f1ec..c80b528 100644
--- a/block.c
+++ b/block.c
@@ -2087,9 +2087,19 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
     Error *local_err = NULL;
+    BlockJob *job = NULL;
 
     assert(bs_queue != NULL);
 
+    /* Pause all block jobs */
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);
+    }
+
     bdrv_drain_all();
 
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
@@ -2120,6 +2130,17 @@ cleanup:
         g_free(bs_entry);
     }
     g_free(bs_queue);
+
+    /* Resume all block jobs */
+    job = NULL;
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_resume(job);
+        aio_context_release(aio_context);
+    }
+
     return ret;
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v10 02/16] block: Add block_job_add_bdrv()

[Alberto Garcia]

When a block job is created on a certain BlockDriverState, operations
are blocked there while the job exists. However, some block jobs may
involve additional BDSs, which must be blocked separately when the job
is created and unblocked manually afterwards.

This patch adds block_job_add_bdrv(), that simplifies this process by
keeping a list of BDSs that are involved in the specified block job.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockjob.c               | 19 ++++++++++++++++---
 include/block/blockjob.h | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index a167f96..b2486a7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -117,6 +117,14 @@ static void block_job_detach_aio_context(void *opaque)
     block_job_unref(job);
 }
 
+void block_job_add_bdrv(BlockJob *job, BlockDriverState *bs)
+{
+    job->nodes = g_slist_prepend(job->nodes, bs);
+    bdrv_ref(bs);
+    bdrv_op_block_all(bs, job->blocker);
+    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
+}
+
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
                        BlockDriverState *bs, int64_t speed,
                        BlockCompletionFunc *cb, void *opaque, Error **errp)
@@ -154,8 +162,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job = g_malloc0(driver->instance_size);
     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);
+    block_job_add_bdrv(job, bs);
 
     job->driver        = driver;
     job->id            = g_strdup(job_id);
@@ -193,9 +200,15 @@ void block_job_ref(BlockJob *job)
 void block_job_unref(BlockJob *job)
 {
     if (--job->refcnt == 0) {
+        GSList *l;
         BlockDriverState *bs = blk_bs(job->blk);
         bs->job = NULL;
-        bdrv_op_unblock_all(bs, job->blocker);
+        for (l = job->nodes; l; l = l->next) {
+            bs = l->data;
+            bdrv_op_unblock_all(bs, job->blocker);
+            bdrv_unref(bs);
+        }
+        g_slist_free(job->nodes);
         blk_remove_aio_context_notifier(job->blk,
                                         block_job_attached_aio_context,
                                         block_job_detach_aio_context, job);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ddb4ae..c7335f3 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -181,6 +181,9 @@ struct BlockJob {
     /** Block other operations when block job is running */
     Error *blocker;
 
+    /** BlockDriverStates that are involved in this block job */
+    GSList *nodes;
+
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
 
@@ -246,6 +249,17 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
                        BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
+ * block_job_add_bdrv:
+ * @job: A block job
+ * @bs: A BlockDriverState that is involved in @job
+ *
+ * Add @bs to the list of BlockDriverState that are involved in
+ * @job. This means that all operations (except dataplane) will be
+ * blocked on @bs while @job exists.
+ */
+void block_job_add_bdrv(BlockJob *job, BlockDriverState *bs);
+
+/**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @clock: The clock to sleep on.
-- 
2.9.3

[Qemu-devel] [PATCH v10 03/16] block: Use block_job_add_bdrv() in mirror_start_job()

[Alberto Garcia]

Use block_job_add_bdrv() instead of blocking all operations in
mirror_start_job() and unblocking them in mirror_exit().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..9b5159f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -526,7 +526,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
-    bdrv_op_unblock_all(target_bs, s->common.blocker);
     blk_unref(s->target);
     block_job_completed(&s->common, data->ret);
     g_free(data);
@@ -965,7 +964,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         return;
     }
 
-    bdrv_op_block_all(target, s->common.blocker);
+    block_job_add_bdrv(&s->common, target);
 
     s->common.co = qemu_coroutine_create(mirror_run, s);
     trace_mirror_start(bs, s, s->common.co, opaque);
-- 
2.9.3

[Qemu-devel] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Alberto Garcia]

Use block_job_add_bdrv() instead of blocking all operations in
backup_start() and unblocking them in backup_run().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/backup.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 582bd0f..3a9cb7f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -427,7 +427,6 @@ static void coroutine_fn backup_run(void *opaque)
     BackupBlockJob *job = opaque;
     BackupCompleteData *data;
     BlockDriverState *bs = blk_bs(job->common.blk);
-    BlockBackend *target = job->target;
     int64_t start, end;
     int64_t sectors_per_cluster = cluster_size_sectors(job);
     int ret = 0;
@@ -514,8 +513,6 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_unlock(&job->flush_rwlock);
     g_free(job->done_bitmap);
 
-    bdrv_op_unblock_all(blk_bs(target), job->common.blocker);
-
     data = g_malloc(sizeof(*data));
     data->ret = ret;
     block_job_defer_to_main_loop(&job->common, backup_complete, data);
@@ -629,7 +626,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
 
-    bdrv_op_block_all(target, job->common.blocker);
+    block_job_add_bdrv(&job->common, target);
     job->common.len = len;
     job->common.co = qemu_coroutine_create(backup_run, job);
     block_job_txn_add_job(txn, &job->common);
-- 
2.9.3

[Qemu-devel] [PATCH v10 05/16] block: Check blockers in all nodes involved in a block-commit job

[Alberto Garcia]

qmp_block_commit() checks for op blockers in the active and
destination (base) images. However all nodes between top_bs and base
are also involved, and they are removed from the chain afterwards.

In addition to that, if top_bs is not the active layer then top_bs's
overlay also needs to be checked because it's involved in the job (its
backing image string needs to be updated to point to 'base').

This patch checks that none of those nodes are blocked.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 07ec733..f13fc69 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3001,6 +3001,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       Error **errp)
 {
     BlockDriverState *bs;
+    BlockDriverState *iter;
     BlockDriverState *base_bs, *top_bs;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -3067,8 +3068,10 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
 
     assert(bdrv_get_aio_context(base_bs) == aio_context);
 
-    if (bdrv_op_is_blocked(base_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
-        goto out;
+    for (iter = top_bs; iter != backing_bs(base_bs); iter = backing_bs(iter)) {
+        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
+            goto out;
+        }
     }
 
     /* Do not allow attempts to commit an image into itself */
@@ -3086,6 +3089,10 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
                             on_error, block_job_cb, bs, &local_err, false);
     } else {
+        BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
+        if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
+            goto out;
+        }
         commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
                      on_error, block_job_cb, bs,
                      has_backing_file ? backing_file : NULL, &local_err);
-- 
2.9.3

[Qemu-devel] [PATCH v10 06/16] block: Block all nodes involved in the block-commit operation

[Alberto Garcia]

After a successful block-commit operation all nodes between top and
base are removed from the backing chain, and top's overlay needs to
be updated to point to base. Because of that we should prevent other
block jobs from messing with them.

This patch blocks all operations in these nodes in commit_start().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 9f67a8b..bab625f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -216,6 +216,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     BlockReopenQueue *reopen_queue = NULL;
     int orig_overlay_flags;
     int orig_base_flags;
+    BlockDriverState *iter;
     BlockDriverState *overlay_bs;
     Error *local_err = NULL;
 
@@ -260,6 +261,15 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
 
+    /* Block all nodes between top and base, because they will
+     * disappear from the chain after this operation. overlay_bs is
+     * also blocked because it needs to be modified to update the
+     * backing image string. */
+    assert(bdrv_chain_contains(overlay_bs, base));
+    for (iter = overlay_bs; iter != backing_bs(base); iter = backing_bs(iter)) {
+        block_job_add_bdrv(&s->common, iter);
+    }
+
     s->base = blk_new();
     blk_insert_bs(s->base, base);
 
-- 
2.9.3

[Qemu-devel] [PATCH v10 07/16] block: Block all intermediate nodes in commit_active_start()

[Alberto Garcia]

When block-commit is launched without the top parameter, it uses
internally a mirror block job. In that case all intermediate nodes
between the active and base nodes must be blocked as well.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 9b5159f..a5b71b7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -965,6 +965,14 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     }
 
     block_job_add_bdrv(&s->common, target);
+    /* In commit_active_start() all intermediate nodes disappear, so
+     * any jobs in them must be blocked */
+    if (bdrv_chain_contains(bs, target)) {
+        BlockDriverState *iter;
+        for (iter = backing_bs(bs); iter != target; iter = backing_bs(iter)) {
+            block_job_add_bdrv(&s->common, iter);
+        }
+    }
 
     s->common.co = qemu_coroutine_create(mirror_run, s);
     trace_mirror_start(bs, s, s->common.co, opaque);
-- 
2.9.3

[Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Alberto Garcia]

This makes sure that the image we are streaming into is open in
read-write mode during the operation.

Operation blockers are also set in all intermediate nodes, since they
will be removed from the chain afterwards.

Finally, this also unblocks the stream operation in backing files.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c        |  4 +++-
 block/stream.c | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index c80b528..8255d75 100644
--- a/block.c
+++ b/block.c
@@ -1428,9 +1428,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
             backing_hd->drv ? backing_hd->drv->format_name : "");
 
     bdrv_op_block_all(backing_hd, bs->backing_blocker);
-    /* Otherwise we won't be able to commit due to check in bdrv_commit */
+    /* Otherwise we won't be able to commit or stream */
     bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
                     bs->backing_blocker);
+    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
+                    bs->backing_blocker);
     /*
      * We do backup in 3 ways:
      * 1. drive backup
diff --git a/block/stream.c b/block/stream.c
index 3187481..b8ab89a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -37,6 +37,7 @@ typedef struct StreamBlockJob {
     BlockDriverState *base;
     BlockdevOnError on_error;
     char *backing_file_str;
+    int bs_flags;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque)
         bdrv_set_backing_hd(bs, base);
     }
 
+    /* Reopen the image back in read-only mode if necessary */
+    if (s->bs_flags != bdrv_get_flags(bs)) {
+        bdrv_reopen(bs, s->bs_flags, NULL);
+    }
+
     g_free(s->backing_file_str);
     block_job_completed(&s->common, data->ret);
     g_free(data);
@@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
     StreamBlockJob *s;
+    BlockDriverState *iter;
+    int orig_bs_flags;
 
     s = block_job_create(job_id, &stream_job_driver, bs, speed,
                          cb, opaque, errp);
@@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState *bs,
         return;
     }
 
+    /* Make sure that the image is opened in read-write mode */
+    orig_bs_flags = bdrv_get_flags(bs);
+    if (!(orig_bs_flags & BDRV_O_RDWR)) {
+        if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+            block_job_unref(&s->common);
+            return;
+        }
+    }
+
+    /* Block all intermediate nodes between bs and base, because they
+     * will disappear from the chain after this operation */
+    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
+        block_job_add_bdrv(&s->common, iter);
+    }
+
     s->base = base;
     s->backing_file_str = g_strdup(backing_file_str);
+    s->bs_flags = orig_bs_flags;
 
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(stream_run, s);
-- 
2.9.3

[Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

This patch makes the 'device' parameter of the 'block-stream' command
accept a node name that is not a root node.

In addition to that, operation blockers will be checked in all
intermediate nodes between the top and the base node.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 11 +++++++++--
 qapi/block-core.json | 10 +++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f13fc69..57c8329 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs, *iter;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    bs = qmp_get_root_bs(device, errp);
+    bs = bdrv_lookup_bs(device, device, errp);
     if (!bs) {
         return;
     }
@@ -2969,6 +2969,13 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         base_name = base;
     }
 
+    /* Check for op blockers in the whole chain between bs and base */
+    for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
+        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
+            goto out;
+        }
+    }
+
     /* if we are streaming the entire chain, the result will have no backing
      * file, and specifying one is therefore an error */
     if (base_bs == NULL && has_backing_file) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..88f4c55 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1469,6 +1469,10 @@
 # with query-block-jobs.  The operation can be stopped before it has completed
 # using the block-job-cancel command.
 #
+# The node that receives the data is called the top image, can be located in
+# any part of the chain (but always above the base image; see below) and can be
+# specified using its device or node name.
+#
 # If a base file is specified then sectors are not copied from that base file and
 # its backing chain.  When streaming completes the image file will have the base
 # file as its backing file.  This can be used to stream a subset of the backing
@@ -1480,12 +1484,12 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device: the device name or node-name of a root node
+# @device: the device or node name of the top image
 #
 # @base:   #optional the common backing file name
 #
-# @backing-file: #optional The backing file string to write into the active
-#                          layer. This filename is not validated.
+# @backing-file: #optional The backing file string to write into the top
+#                          image. This filename is not validated.
 #
 #                          If a pathname string is such that it cannot be
 #                          resolved by QEMU, that means that subsequent QMP or
-- 
2.9.3

[Qemu-devel] [PATCH v10 10/16] docs: Document how to stream to an intermediate layer

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 docs/live-block-ops.txt | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
index a257087..014c8c9 100644
--- a/docs/live-block-ops.txt
+++ b/docs/live-block-ops.txt
@@ -10,9 +10,9 @@ Snapshot live merge
 Given a snapshot chain, described in this document in the following
 format:
 
-[A] -> [B] -> [C] -> [D]
+[A] <- [B] <- [C] <- [D] <- [E]
 
-Where the rightmost object ([D] in the example) described is the current
+Where the rightmost object ([E] in the example) described is the current
 image which the guest OS has write access to. To the left of it is its base
 image, and so on accordingly until the leftmost image, which has no
 base.
@@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain into a
 smaller one with fewer elements, such as this transformation relative
 to the first example:
 
-[A] -> [D]
+[A] <- [E]
 
-Currently only forward merge with target being the active image is
-supported, that is, data copy is performed in the right direction with
-destination being the rightmost image.
+Data is copied in the right direction with destination being the
+rightmost image, but any other intermediate image can be specified
+instead. In this example data is copied from [C] into [D], so [D] can
+be backed by [B]:
+
+[A] <- [B] <- [D] <- [E]
 
 The operation is implemented in QEMU through image streaming facilities.
 
@@ -35,14 +38,20 @@ streaming operation completes it raises a QMP event. 'block_stream'
 copies data from the backing file(s) into the active image. When finished,
 it adjusts the backing file pointer.
 
-The 'base' parameter specifies an image which data need not be streamed from.
-This image will be used as the backing file for the active image when the
-operation is finished.
+The 'base' parameter specifies an image which data need not be
+streamed from. This image will be used as the backing file for the
+destination image when the operation is finished.
 
-In the example above, the command would be:
+In the first example above, the command would be:
 
-(qemu) block_stream virtio0 A
+(qemu) block_stream virtio0 file-A.img
 
+In order to specify a destination image different from the active
+(rightmost) one we can use its node name instead.
+
+In the second example above, the command would be:
+
+(qemu) block_stream node-D file-B.img
 
 Live block copy
 ===============
-- 
2.9.3

[Qemu-devel] [PATCH v10 11/16] qemu-iotests: Test streaming to an intermediate layer

[Alberto Garcia]

This adds test_stream_intermediate(), similar to test_stream() but
streams to the intermediate image instead.

It also removes the usage of blkdebug, which is unnecessary for this
test.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 21 ++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 107049b..367ecf8 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -36,7 +36,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
-        self.vm = iotests.VM().add_drive("blkdebug::" + test_img)
+        self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid")
         self.vm.launch()
 
     def tearDown(self):
@@ -60,6 +60,25 @@ class TestSingleDrive(iotests.QMPTestCase):
                          qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
                          'image file map does not match backing file after streaming')
 
+    def test_stream_intermediate(self):
+        self.assert_no_active_block_jobs()
+
+        self.assertNotEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
+                            qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
+                            'image file map matches backing file before streaming')
+
+        result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='stream-mid')
+
+        self.assert_no_active_block_jobs()
+        self.vm.shutdown()
+
+        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
+                         'image file map does not match backing file after streaming')
+
     def test_stream_pause(self):
         self.assert_no_active_block_jobs()
 
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 6323079..96961ed 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-..............
+...............
 ----------------------------------------------------------------------
-Ran 14 tests
+Ran 15 tests
 
 OK
-- 
2.9.3

[Qemu-devel] [PATCH v10 12/16] qemu-iotests: Test block-stream operations in parallel

[Alberto Garcia]

This test case checks that it's possible to launch several stream
operations in parallel in the same snapshot chain, each one involving
a different set of nodes.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 80 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 367ecf8..e487473 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -148,6 +148,86 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
 
+class TestParallelOps(iotests.QMPTestCase):
+    num_ops = 4 # Number of parallel block-stream operations
+    num_imgs = num_ops * 2 + 1
+    image_len = num_ops * 1024 * 1024
+    imgs = []
+
+    def setUp(self):
+        opts = []
+        self.imgs = []
+
+        # Initialize file names and command-line options
+        for i in range(self.num_imgs):
+            img_depth = self.num_imgs - i - 1
+            opts.append("backing." * img_depth + "node-name=node%d" % i)
+            self.imgs.append(os.path.join(iotests.test_dir, 'img-%d.img' % i))
+
+        # Create all images
+        iotests.create_image(self.imgs[0], self.image_len)
+        for i in range(1, self.num_imgs):
+            qemu_img('create', '-f', iotests.imgfmt,
+                     '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i])
+
+        # Put data into the images we are copying data from
+        for i in range(self.num_imgs / 2):
+            img_index = i * 2 + 1
+            # Alternate between 512k and 1M.
+            # This way jobs will not finish in the same order they were created
+            num_kb = 512 + 512 * (i % 2)
+            qemu_io('-f', iotests.imgfmt,
+                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
+                    self.imgs[img_index])
+
+        # Attach the drive to the VM
+        self.vm = iotests.VM()
+        self.vm.add_drive(self.imgs[-1], ','.join(opts))
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for img in self.imgs:
+            os.remove(img)
+
+    # Test that it's possible to run several block-stream operations
+    # in parallel in the same snapshot chain
+    def test_stream_parallel(self):
+        self.assert_no_active_block_jobs()
+
+        # Check that the maps don't match before the streaming operations
+        for i in range(2, self.num_imgs, 2):
+            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
+                                qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
+                                'image file map matches backing file before streaming')
+
+        # Create all streaming jobs
+        pending_jobs = []
+        for i in range(2, self.num_imgs, 2):
+            node_name = 'node%d' % i
+            job_id = 'stream-%s' % node_name
+            pending_jobs.append(job_id)
+            result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=512*1024)
+            self.assert_qmp(result, 'return', {})
+
+        # Wait for all jobs to be finished.
+        while len(pending_jobs) > 0:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    job_id = self.dictpath(event, 'data/device')
+                    self.assertTrue(job_id in pending_jobs)
+                    self.assert_qmp_absent(event, 'data/error')
+                    pending_jobs.remove(job_id)
+
+        self.assert_no_active_block_jobs()
+        self.vm.shutdown()
+
+        # Check that all maps match now
+        for i in range(2, self.num_imgs, 2):
+            self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
+                             qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
+                             'image file map does not match backing file after streaming')
+
 class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * backing_len
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 96961ed..b6f2576 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...............
+................
 ----------------------------------------------------------------------
-Ran 15 tests
+Ran 16 tests
 
 OK
-- 
2.9.3

[Qemu-devel] [PATCH v10 13/16] qemu-iotests: Test overlapping stream and commit operations

[Alberto Garcia]

These test cases check that it's not possible to perform two
block-stream or block-commit operations if there are nodes involved in
both.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index e487473..e3bded8 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -228,6 +228,95 @@ class TestParallelOps(iotests.QMPTestCase):
                              qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
                              'image file map does not match backing file after streaming')
 
+    # Test that it's not possible to perform two block-stream
+    # operations if there are nodes involved in both.
+    def test_overlapping_1(self):
+        self.assert_no_active_block_jobs()
+
+        # Set a speed limit to make sure that this job blocks the rest
+        result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4-v2')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # block-commit should also fail if it touches nodes used by the stream job
+        result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # This fails because it needs to modify the backing string in node2, which is blocked
+        result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[0], top=self.imgs[1], job_id='commit-node0')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.wait_until_completed(drive='stream-node4')
+        self.assert_no_active_block_jobs()
+
+    # Similar to test_overlapping_1, but with block-commit
+    # blocking the other jobs
+    def test_overlapping_2(self):
+        self.assertLessEqual(9, self.num_imgs)
+        self.assert_no_active_block_jobs()
+
+        # Set a speed limit to make sure that this job blocks the rest
+        result = self.vm.qmp('block-commit', device='drive0', top=self.imgs[5], base=self.imgs[3], job_id='commit-node3', speed=1024*1024)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node6', base=self.imgs[2], job_id='stream-node6')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node4', base=self.imgs[2], job_id='stream-node4')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node6', base=self.imgs[4], job_id='stream-node6-v2')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # This fails because block-commit needs to block node6, the overlay of the 'top' image
+        result = self.vm.qmp('block-stream', device='node7', base=self.imgs[5], job_id='stream-node6-v3')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # This fails because block-commit currently blocks the active layer even if it's not used
+        result = self.vm.qmp('block-stream', device='drive0', base=self.imgs[5], job_id='stream-drive0')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.wait_until_completed(drive='commit-node3')
+
+    # Similar to test_overlapping_2, but here block-commit doesn't use the 'top' parameter.
+    # Internally this uses a mirror block job, hence the separate test case.
+    def test_overlapping_3(self):
+        self.assertLessEqual(8, self.num_imgs)
+        self.assert_no_active_block_jobs()
+
+        # Set a speed limit to make sure that this job blocks the rest
+        result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[3], job_id='commit-drive0', speed=1024*1024)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-stream', device='node5', base=self.imgs[3], job_id='stream-node6')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        event = self.vm.get_qmp_event(wait=True)
+        self.assertEqual(event['event'], 'BLOCK_JOB_READY')
+        self.assert_qmp(event, 'data/device', 'commit-drive0')
+        self.assert_qmp(event, 'data/type', 'commit')
+        self.assert_qmp_absent(event, 'data/error')
+
+        result = self.vm.qmp('block-job-complete', device='commit-drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='commit-drive0')
+        self.assert_no_active_block_jobs()
+
 class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * backing_len
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index b6f2576..4176bb9 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-................
+...................
 ----------------------------------------------------------------------
-Ran 16 tests
+Ran 19 tests
 
 OK
-- 
2.9.3

[Qemu-devel] [PATCH v10 14/16] qemu-iotests: Test block-stream and block-commit in parallel

[Alberto Garcia]

As with test_stream_parallel(), we allow mixing block-stream and
block-commit operations in the same backing chain as long as there's
no overlap among the involved nodes.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index e3bded8..d88823a 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -315,6 +315,36 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.wait_until_completed(drive='commit-drive0')
+
+    # Test a block-stream and a block-commit job in parallel
+    def test_stream_commit(self):
+        self.assertLessEqual(8, self.num_imgs)
+        self.assert_no_active_block_jobs()
+
+        # Stream from node0 into node2
+        result = self.vm.qmp('block-stream', device='node2', job_id='node2')
+        self.assert_qmp(result, 'return', {})
+
+        # Commit from the active layer into node3
+        result = self.vm.qmp('block-commit', device='drive0', top=self.imgs[5], base=self.imgs[3])
+        self.assert_qmp(result, 'return', {})
+
+        # Wait for all jobs to be finished.
+        pending_jobs = ['node2', 'drive0']
+        while len(pending_jobs) > 0:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    node_name = self.dictpath(event, 'data/device')
+                    self.assertTrue(node_name in pending_jobs)
+                    self.assert_qmp_absent(event, 'data/error')
+                    pending_jobs.remove(node_name)
+                if event['event'] == 'BLOCK_JOB_READY':
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/type', 'commit')
+                    self.assert_qmp_absent(event, 'data/error')
+                    self.assertTrue('drive0' in pending_jobs)
+                    self.vm.qmp('block-job-complete', device='drive0')
+
         self.assert_no_active_block_jobs()
 
 class TestSmallerBackingFile(iotests.QMPTestCase):
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 4176bb9..3a89159 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...................
+....................
 ----------------------------------------------------------------------
-Ran 19 tests
+Ran 20 tests
 
 OK
-- 
2.9.3

[Qemu-devel] [PATCH v10 15/16] qemu-iotests: Add iotests.supports_quorum()

[Alberto Garcia]

There's many tests that need Quorum support in order to run. At the
moment each test implements its own check to see if Quorum is
enabled. This patch centralizes all those checks in a new function
called iotests.supports_quorum().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/041        | 27 ++++++++++++---------------
 tests/qemu-iotests/139        |  3 ++-
 tests/qemu-iotests/iotests.py |  5 ++++-
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index d1e1ad8..b4f3748 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -761,9 +761,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
     image_len = 1 * 1024 * 1024 # MB
     IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
 
-    def has_quorum(self):
-        return 'quorum' in iotests.qemu_img_pipe('--help')
-
     def setUp(self):
         self.vm = iotests.VM()
 
@@ -784,7 +781,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         #assemble the quorum block device from the individual files
         args = { "options" : { "driver": "quorum", "node-name": "quorum0",
                  "vote-threshold": 2, "children": [ "img0", "img1", "img2" ] } }
-        if self.has_quorum():
+        if iotests.supports_quorum():
             result = self.vm.qmp("blockdev-add", **args)
             self.assert_qmp(result, 'return', {})
 
@@ -799,7 +796,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
                 pass
 
     def test_complete(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         self.assert_no_active_block_jobs()
@@ -818,7 +815,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
                         'target image does not match source after mirroring')
 
     def test_cancel(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         self.assert_no_active_block_jobs()
@@ -835,7 +832,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.vm.shutdown()
 
     def test_cancel_after_ready(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         self.assert_no_active_block_jobs()
@@ -854,7 +851,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
                         'target image does not match source after mirroring')
 
     def test_pause(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         self.assert_no_active_block_jobs()
@@ -884,7 +881,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
                         'target image does not match source after mirroring')
 
     def test_medium_not_found(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         if iotests.qemu_default_machine != 'pc':
@@ -898,7 +895,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -908,7 +905,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('drive-mirror', job_id='job0',
@@ -919,7 +916,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_wrong_sync_mode(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('drive-mirror', device='quorum0', job_id='job0',
@@ -929,7 +926,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_no_node_name(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -938,7 +935,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_nonexistent_replaces(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -947,7 +944,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_after_a_quorum_snapshot(self):
-        if not self.has_quorum():
+        if not iotests.supports_quorum():
             return
 
         result = self.vm.qmp('blockdev-snapshot-sync', node_name='img1',
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index 47a4c26..376b16d 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -336,8 +336,9 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.checkBlockDriverState('node1', False)
 
     def testQuorum(self):
-        if not 'quorum' in iotests.qemu_img_pipe('--help'):
+        if not iotests.supports_quorum():
             return
+
         self.addQuorum('quorum0', 'node0', 'node1')
         # We cannot remove the children of a Quorum device
         self.delBlockDriverState('node0', expect_error = True)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3329bc1..ef63ee4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -318,9 +318,12 @@ def verify_platform(supported_oses=['linux']):
     if True not in [sys.platform.startswith(x) for x in supported_oses]:
         notrun('not suitable for this OS: %s' % sys.platform)
 
+def supports_quorum():
+    return 'quorum' in qemu_img_pipe('--help')
+
 def verify_quorum():
     '''Skip test suite if quorum support is not available'''
-    if 'quorum' not in qemu_img_pipe('--help'):
+    if not supports_quorum():
         notrun('quorum support missing')
 
 def main(supported_fmts=[], supported_oses=['linux']):
-- 
2.9.3

[Qemu-devel] [PATCH v10 16/16] qemu-iotests: Test streaming to a Quorum child

[Alberto Garcia]

Quorum children are special in the sense that they're not directly
attached to a block backend but they're not used as backing images
either. However the intermediate block streaming code supports
streaming to them. This is a test case for that scenario.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index d88823a..6736004 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -347,6 +347,62 @@ class TestParallelOps(iotests.QMPTestCase):
 
         self.assert_no_active_block_jobs()
 
+class TestQuorum(iotests.QMPTestCase):
+    num_children = 3
+    children = []
+    backing = []
+
+    def setUp(self):
+        opts = ['driver=quorum', 'vote-threshold=2']
+
+        # Initialize file names and command-line options
+        for i in range(self.num_children):
+            child_img = os.path.join(iotests.test_dir, 'img-%d.img' % i)
+            backing_img = os.path.join(iotests.test_dir, 'backing-%d.img' % i)
+            self.children.append(child_img)
+            self.backing.append(backing_img)
+            qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
+            qemu_io('-f', iotests.imgfmt,
+                    '-c', 'write -P 0x55 0 1024', backing_img)
+            qemu_img('create', '-f', iotests.imgfmt,
+                     '-o', 'backing_file=%s' % backing_img, child_img)
+            opts.append("children.%d.file.filename=%s" % (i, child_img))
+            opts.append("children.%d.node-name=node%d" % (i, i))
+
+        # Attach the drive to the VM
+        self.vm = iotests.VM()
+        self.vm.add_drive(path = None, opts = ','.join(opts))
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for img in self.children:
+            os.remove(img)
+        for img in self.backing:
+            os.remove(img)
+
+    def test_stream_quorum(self):
+        if not iotests.supports_quorum():
+            return
+
+        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.children[0]),
+                            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.backing[0]),
+                            'image file map matches backing file before streaming')
+
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('block-stream', device='node0', job_id='stream-node0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='stream-node0')
+
+        self.assert_no_active_block_jobs()
+        self.vm.shutdown()
+
+        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.children[0]),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', self.backing[0]),
+                         'image file map does not match backing file after streaming')
+
 class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * backing_len
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 3a89159..c6a10f8 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-....................
+.....................
 ----------------------------------------------------------------------
-Ran 20 tests
+Ran 21 tests
 
 OK
-- 
2.9.3

Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> When a BlockDriverState is about to be reopened it can trigger certain
> operations that need to write to disk. During this process a different
> block job can be woken up. If that block job completes and also needs
> to call bdrv_reopen() it can happen that it needs to do it on the same
> BlockDriverState that is still in the process of being reopened.
> 
> This can have fatal consequences, like in this example:
> 
>   1) Block job A starts and sleeps after a while.
>   2) Block job B starts and tries to reopen node1 (a qcow2 file).
>   3) Reopening node1 means flushing and replacing its qcow2 cache.
>   4) While the qcow2 cache is being flushed, job A wakes up.
>   5) Job A completes and reopens node1, replacing its cache.
>   6) Job B resumes, but the cache that was being flushed no longer
>      exists.
> 
> This patch pauses all block jobs during bdrv_reopen_multiple(), so
> that step 4 can never happen and the operation is safe.
> 
> Note that this scenario can only happen if both bdrv_reopen() calls
> are made by block jobs on the same backing chain. Otherwise there's no
> chance that the same BlockDriverState appears in both reopen queues.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/block.c b/block.c
> index bb1f1ec..c80b528 100644
> --- a/block.c
> +++ b/block.c
> @@ -2087,9 +2087,19 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>      int ret = -1;
>      BlockReopenQueueEntry *bs_entry, *next;
>      Error *local_err = NULL;
> +    BlockJob *job = NULL;
>  
>      assert(bs_queue != NULL);
>  
> +    /* Pause all block jobs */
> +    while ((job = block_job_next(job))) {
> +        AioContext *aio_context = blk_get_aio_context(job->blk);
> +
> +        aio_context_acquire(aio_context);
> +        block_job_pause(job);
> +        aio_context_release(aio_context);
> +    }
> +
>      bdrv_drain_all();

We already have a bdrv_drain_all() here, which does the same thing (and
more) internally, except that it resumes all jobs before it returns.
Maybe what we should do is split bdrv_drain_all() in a begin/end pair,
too.

If we don't split it, we'd have to do the "and more" part here as well,
disabling all other potential users of the BDSes. This would involve at
least calling bdrv_parent_drained_begin/end().

The other point I'm wondering now is whether bdrv_drain_all() should
have the aio_disable/enable_external() pair that bdrv_drain() has.

Paolo, any opinion?

>      QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
> @@ -2120,6 +2130,17 @@ cleanup:
>          g_free(bs_entry);
>      }
>      g_free(bs_queue);
> +
> +    /* Resume all block jobs */
> +    job = NULL;
> +    while ((job = block_job_next(job))) {
> +        AioContext *aio_context = blk_get_aio_context(job->blk);
> +
> +        aio_context_acquire(aio_context);
> +        block_job_resume(job);
> +        aio_context_release(aio_context);
> +    }
> +
>      return ret;
>  }

Kevin

Re: [Qemu-devel] [PATCH v10 02/16] block: Add block_job_add_bdrv()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> When a block job is created on a certain BlockDriverState, operations
> are blocked there while the job exists. However, some block jobs may
> involve additional BDSs, which must be blocked separately when the job
> is created and unblocked manually afterwards.
> 
> This patch adds block_job_add_bdrv(), that simplifies this process by
> keeping a list of BDSs that are involved in the specified block job.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v10 03/16] block: Use block_job_add_bdrv() in mirror_start_job()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Use block_job_add_bdrv() instead of blocking all operations in
> mirror_start_job() and unblocking them in mirror_exit().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Compared to the old code, this unblocks BLOCK_OP_TYPE_DATAPLANE, i.e.
you can now run a dataplane device on a BDS used as the mirror target.

This means that the target could require a different AioContext than the
source, which we can't support. So it seems unlikely to me that we can
lift this restriction.

Kevin

Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Paolo Bonzini]

On 10/10/2016 17:37, Kevin Wolf wrote:
> > +    while ((job = block_job_next(job))) {
> > +        AioContext *aio_context = blk_get_aio_context(job->blk);
> > +
> > +        aio_context_acquire(aio_context);
> > +        block_job_pause(job);
> > +        aio_context_release(aio_context);
> > +    }
> > +
> >      bdrv_drain_all();
>
> We already have a bdrv_drain_all() here, which does the same thing (and
> more) internally, except that it resumes all jobs before it returns.
> Maybe what we should do is split bdrv_drain_all() in a begin/end pair,
> too.

Hey, haven't I just suggested the same? :)  I swear I hadn't read this
before.

> The other point I'm wondering now is whether bdrv_drain_all() should
> have the aio_disable/enable_external() pair that bdrv_drain() has.

bdrv_drain_all need not have it, but its start/end replacement should.

Paolo

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Eric Blake]

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

On 10/06/2016 08:02 AM, Alberto Garcia wrote:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 11 +++++++++--
>  qapi/block-core.json | 10 +++++++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1469,6 +1469,10 @@
>  # with query-block-jobs.  The operation can be stopped before it has completed
>  # using the block-job-cancel command.
>  #
> +# The node that receives the data is called the top image, can be located in
> +# any part of the chain (but always above the base image; see below) and can be
> +# specified using its device or node name.
> +#
>  # If a base file is specified then sectors are not copied from that base file and
>  # its backing chain.  When streaming completes the image file will have the base
>  # file as its backing file.  This can be used to stream a subset of the backing
> @@ -1480,12 +1484,12 @@
>  # @job-id: #optional identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
> -# @device: the device name or node-name of a root node
> +# @device: the device or node name of the top image
>  #
>  # @base:   #optional the common backing file name
>  #
> -# @backing-file: #optional The backing file string to write into the active
> -#                          layer. This filename is not validated.
> +# @backing-file: #optional The backing file string to write into the top
> +#                          image. This filename is not validated.

No change to the actual introspection. What's the easiest way for
libvirt to learn if the new style command will work, short of actually
trying it to see if it succeeds or fails?  (That may be sufficient, but
the error message quality can often be improved in libvirt if it is
known up front if qemu is new enough rather than taking the 'try and
see' approach and getting stuck with qemu's error message)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v10 03/16] block: Use block_job_add_bdrv() in mirror_start_job()

[Paolo Bonzini]

On 10/10/2016 18:03, Kevin Wolf wrote:
>> > Use block_job_add_bdrv() instead of blocking all operations in
>> > mirror_start_job() and unblocking them in mirror_exit().
>> > 
>> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> Compared to the old code, this unblocks BLOCK_OP_TYPE_DATAPLANE, i.e.
> you can now run a dataplane device on a BDS used as the mirror target.
> 
> This means that the target could require a different AioContext than the
> source, which we can't support. So it seems unlikely to me that we can
> lift this restriction.

Indeed, this is in the pipeline, but still quite far away!

Paolo

Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Kevin Wolf]

Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben:
> On 10/10/2016 17:37, Kevin Wolf wrote:
> > > +    while ((job = block_job_next(job))) {
> > > +        AioContext *aio_context = blk_get_aio_context(job->blk);
> > > +
> > > +        aio_context_acquire(aio_context);
> > > +        block_job_pause(job);
> > > +        aio_context_release(aio_context);
> > > +    }
> > > +
> > >      bdrv_drain_all();
> >
> > We already have a bdrv_drain_all() here, which does the same thing (and
> > more) internally, except that it resumes all jobs before it returns.
> > Maybe what we should do is split bdrv_drain_all() in a begin/end pair,
> > too.
> 
> Hey, haven't I just suggested the same? :)  I swear I hadn't read this
> before.
> 
> > The other point I'm wondering now is whether bdrv_drain_all() should
> > have the aio_disable/enable_external() pair that bdrv_drain() has.
> 
> bdrv_drain_all need not have it, but its start/end replacement should.

Doesn't need it because it holds the AioContext lock?

Kevin

Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Paolo Bonzini]

On 11/10/2016 11:39, Kevin Wolf wrote:
> Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben:
>> On 10/10/2016 17:37, Kevin Wolf wrote:
>>>> +    while ((job = block_job_next(job))) {
>>>> +        AioContext *aio_context = blk_get_aio_context(job->blk);
>>>> +
>>>> +        aio_context_acquire(aio_context);
>>>> +        block_job_pause(job);
>>>> +        aio_context_release(aio_context);
>>>> +    }
>>>> +
>>>>      bdrv_drain_all();
>>>
>>> We already have a bdrv_drain_all() here, which does the same thing (and
>>> more) internally, except that it resumes all jobs before it returns.
>>> Maybe what we should do is split bdrv_drain_all() in a begin/end pair,
>>> too.
>>
>> Hey, haven't I just suggested the same? :)  I swear I hadn't read this
>> before.
>>
>>> The other point I'm wondering now is whether bdrv_drain_all() should
>>> have the aio_disable/enable_external() pair that bdrv_drain() has.
>>
>> bdrv_drain_all need not have it, but its start/end replacement should.
> 
> Doesn't need it because it holds the AioContext lock?

No, because as soon as bdrv_drain_all exits, external file descriptors
can be triggered again so I don't think the aio_disable/enable_external
actually provides any protection.

bdrv_drain_all should really only be used in cases where you already
have some kind of "hold" on external file descriptors, like bdrv_close
uses bdrv_drain():

    bdrv_drained_begin(bs); /* complete I/O */
    bdrv_flush(bs);
    bdrv_drain(bs); /* in case flush left pending I/O */

That said, because the simplest implementation of bdrv_drain_all() does

    bdrv_drained_all_start();
    bdrv_drained_all_end();

just like bdrv_drain() does it, it's not a very interesting point.
bdrv_drain_all will probably disable/enable external file descriptors,
it just doesn't need that.

Paolo

Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple()

[Kevin Wolf]

Am 11.10.2016 um 11:54 hat Paolo Bonzini geschrieben:
> On 11/10/2016 11:39, Kevin Wolf wrote:
> > Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben:
> >> On 10/10/2016 17:37, Kevin Wolf wrote:
> >>>> +    while ((job = block_job_next(job))) {
> >>>> +        AioContext *aio_context = blk_get_aio_context(job->blk);
> >>>> +
> >>>> +        aio_context_acquire(aio_context);
> >>>> +        block_job_pause(job);
> >>>> +        aio_context_release(aio_context);
> >>>> +    }
> >>>> +
> >>>>      bdrv_drain_all();
> >>>
> >>> We already have a bdrv_drain_all() here, which does the same thing (and
> >>> more) internally, except that it resumes all jobs before it returns.
> >>> Maybe what we should do is split bdrv_drain_all() in a begin/end pair,
> >>> too.
> >>
> >> Hey, haven't I just suggested the same? :)  I swear I hadn't read this
> >> before.
> >>
> >>> The other point I'm wondering now is whether bdrv_drain_all() should
> >>> have the aio_disable/enable_external() pair that bdrv_drain() has.
> >>
> >> bdrv_drain_all need not have it, but its start/end replacement should.
> > 
> > Doesn't need it because it holds the AioContext lock?
> 
> No, because as soon as bdrv_drain_all exits, external file descriptors
> can be triggered again so I don't think the aio_disable/enable_external
> actually provides any protection.

Right, what I was thinking of was more like new requests coming in
while bdrv_drain_all() is still running. If that happened, the result
after bdrv_drain_all() wouldn't be different, but if external file
descriptors are still active and the guest keeps sending requests, it
could take forever until it returns.

But that part is addressed by the AioContext lock, I think.

> bdrv_drain_all should really only be used in cases where you already
> have some kind of "hold" on external file descriptors, like bdrv_close
> uses bdrv_drain():
> 
>     bdrv_drained_begin(bs); /* complete I/O */
>     bdrv_flush(bs);
>     bdrv_drain(bs); /* in case flush left pending I/O */
> 
> That said, because the simplest implementation of bdrv_drain_all() does
> 
>     bdrv_drained_all_start();
>     bdrv_drained_all_end();
> 
> just like bdrv_drain() does it, it's not a very interesting point.
> bdrv_drain_all will probably disable/enable external file descriptors,
> it just doesn't need that.

Yes, of course.

Kevin

Re: [Qemu-devel] [PATCH v10 03/16] block: Use block_job_add_bdrv() in mirror_start_job()

[Alberto Garcia]

On Mon 10 Oct 2016 06:03:41 PM CEST, Kevin Wolf wrote:

>> Use block_job_add_bdrv() instead of blocking all operations in
>> mirror_start_job() and unblocking them in mirror_exit().
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> Compared to the old code, this unblocks BLOCK_OP_TYPE_DATAPLANE, i.e.
> you can now run a dataplane device on a BDS used as the mirror target.
>
> This means that the target could require a different AioContext than
> the source, which we can't support. So it seems unlikely to me that we
> can lift this restriction.

Thanks, I'll fix it.

What happens if you run a dataplane on the source, though? That's
currently allowed as far as I'm aware. Wouldn't that have a similar
effect?

Berto

Re: [Qemu-devel] [PATCH v10 03/16] block: Use block_job_add_bdrv() in mirror_start_job()

[Kevin Wolf]

Am 11.10.2016 um 15:46 hat Alberto Garcia geschrieben:
> On Mon 10 Oct 2016 06:03:41 PM CEST, Kevin Wolf wrote:
> 
> >> Use block_job_add_bdrv() instead of blocking all operations in
> >> mirror_start_job() and unblocking them in mirror_exit().
> >> 
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >
> > Compared to the old code, this unblocks BLOCK_OP_TYPE_DATAPLANE, i.e.
> > you can now run a dataplane device on a BDS used as the mirror target.
> >
> > This means that the target could require a different AioContext than
> > the source, which we can't support. So it seems unlikely to me that we
> > can lift this restriction.
> 
> Thanks, I'll fix it.
> 
> What happens if you run a dataplane on the source, though? That's
> currently allowed as far as I'm aware. Wouldn't that have a similar
> effect?

The block job takes care to put the target into the same dataplane
AioContext then. The job doesn't really care whether it works in the
main thread or a separate I/O thread, it just requires that it's a
single context, which is currently defined by the source.

Kevin

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Mon 10 Oct 2016 09:09:25 PM CEST, Eric Blake wrote:
>>  # @job-id: #optional identifier for the newly-created block job. If
>>  #          omitted, the device name will be used. (Since 2.7)
>>  #
>> -# @device: the device name or node-name of a root node
>> +# @device: the device or node name of the top image
>>  #
>>  # @base:   #optional the common backing file name
>>  #
>> -# @backing-file: #optional The backing file string to write into the active
>> -#                          layer. This filename is not validated.
>> +# @backing-file: #optional The backing file string to write into the top
>> +#                          image. This filename is not validated.
>
> No change to the actual introspection. What's the easiest way for
> libvirt to learn if the new style command will work, short of actually
> trying it to see if it succeeds or fails?  (That may be sufficient,
> but the error message quality can often be improved in libvirt if it
> is known up front if qemu is new enough rather than taking the 'try
> and see' approach and getting stuck with qemu's error message)

I don't think there's any straightforward way. Some ideas:

1) Try to start a block-stream job that does nothing. When base ==
backing_bs(device) that's a no-op, but it fails if device is not the
root node and intermediate block streaming is not supported.

2) We could add an extra parameter. For example 'base-node', which would
be the same as 'base' but would take a node name instead of a file name.

3) QEMU could advertise that feature to the client. This is probably
simpler than trying to figure it out from the API. I guess that's the
idea of 'qmp_capabilities'?

Berto

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 11.10.2016 um 16:30 hat Alberto Garcia geschrieben:
> On Mon 10 Oct 2016 09:09:25 PM CEST, Eric Blake wrote:
> >>  # @job-id: #optional identifier for the newly-created block job. If
> >>  #          omitted, the device name will be used. (Since 2.7)
> >>  #
> >> -# @device: the device name or node-name of a root node
> >> +# @device: the device or node name of the top image
> >>  #
> >>  # @base:   #optional the common backing file name
> >>  #
> >> -# @backing-file: #optional The backing file string to write into the active
> >> -#                          layer. This filename is not validated.
> >> +# @backing-file: #optional The backing file string to write into the top
> >> +#                          image. This filename is not validated.
> >
> > No change to the actual introspection. What's the easiest way for
> > libvirt to learn if the new style command will work, short of actually
> > trying it to see if it succeeds or fails?  (That may be sufficient,
> > but the error message quality can often be improved in libvirt if it
> > is known up front if qemu is new enough rather than taking the 'try
> > and see' approach and getting stuck with qemu's error message)
> 
> I don't think there's any straightforward way. Some ideas:
> 
> 1) Try to start a block-stream job that does nothing. When base ==
> backing_bs(device) that's a no-op, but it fails if device is not the
> root node and intermediate block streaming is not supported.
> 
> 2) We could add an extra parameter. For example 'base-node', which would
> be the same as 'base' but would take a node name instead of a file name.

Oh, we still have those filename-based parameters? :-/

Should we introduce a new, clean blockdev-stream command that fixes this
and matches the common name pattern? Of course, block-stream vs.
blockdev-stream could be a bit confusing, too...

> 3) QEMU could advertise that feature to the client. This is probably
> simpler than trying to figure it out from the API. I guess that's the
> idea of 'qmp_capabilities'?

I think that was the idea, though it was never used. If we had used it,
I'm not sure how long the capabilities list would be today. :-)

Kevin

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Eric Blake]

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

On 10/11/2016 09:57 AM, Kevin Wolf wrote:
> Should we introduce a new, clean blockdev-stream command that fixes this
> and matches the common name pattern? Of course, block-stream vs.
> blockdev-stream could be a bit confusing, too...
> 

A new command is easy to introspect (query-commands), lets us get rid of
cruft, and makes it obvious that we support everything from the get-go.
I'm favoring that option, even if it leads to slightly confusing names
of the deprecated vs. the new command.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.10.2016 um 16:30 hat Alberto Garcia geschrieben:
[...]
>> 3) QEMU could advertise that feature to the client. This is probably
>> simpler than trying to figure it out from the API. I guess that's the
>> idea of 'qmp_capabilities'?
>
> I think that was the idea, though it was never used. If we had used it,
> I'm not sure how long the capabilities list would be today. :-)

QMP capabilities are for changes in the QMP protocol, not for changes in
commands, events and types.  The protocol has been good enough so far,
thus no capabilities.

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/11/2016 09:57 AM, Kevin Wolf wrote:
>> Should we introduce a new, clean blockdev-stream command that fixes this
>> and matches the common name pattern? Of course, block-stream vs.
>> blockdev-stream could be a bit confusing, too...
>> 
>
> A new command is easy to introspect (query-commands), lets us get rid of
> cruft, and makes it obvious that we support everything from the get-go.
> I'm favoring that option, even if it leads to slightly confusing names
> of the deprecated vs. the new command.

Let's take a step back and consider extending old commands vs. adding
new commands.

A new command is trivial to detect in introspection.

Command extensions are not as trivial to detect in introspection.  Many
extensions are exposed in query-qmp-schema, but not all.

Back when QMP was young, Anthony argued for always adding new commands,
never extend existing ones.  I opposed it, because it would lead to a
confusing mess of related commands, unreadable or incomplete
documentation, and abysmal test coverage.

However, the other extreme is also unwise: we shouldn't shoehorn new
functionality into existing commands just because we can.  We should ask
ourselves questions like these:

* Is the extended command still a sane interface?  If writing clear
  documentation for it is hard, it perhaps isn't.  Pay special attention
  to failure modes.  Overloaded arguments are prone to confusing errors.

* How will the command's users use the extension?  If it requires new
  code paths, a new command may be more convenient for them.

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 11.10.2016 um 18:50 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 10/11/2016 09:57 AM, Kevin Wolf wrote:
> >> Should we introduce a new, clean blockdev-stream command that fixes this
> >> and matches the common name pattern? Of course, block-stream vs.
> >> blockdev-stream could be a bit confusing, too...
> >> 
> >
> > A new command is easy to introspect (query-commands), lets us get rid of
> > cruft, and makes it obvious that we support everything from the get-go.
> > I'm favoring that option, even if it leads to slightly confusing names
> > of the deprecated vs. the new command.
> 
> Let's take a step back and consider extending old commands vs. adding
> new commands.
> 
> A new command is trivial to detect in introspection.
> 
> Command extensions are not as trivial to detect in introspection.  Many
> extensions are exposed in query-qmp-schema, but not all.
> 
> Back when QMP was young, Anthony argued for always adding new commands,
> never extend existing ones.  I opposed it, because it would lead to a
> confusing mess of related commands, unreadable or incomplete
> documentation, and abysmal test coverage.
> 
> However, the other extreme is also unwise: we shouldn't shoehorn new
> functionality into existing commands just because we can.  We should ask
> ourselves questions like these:
> 
> * Is the extended command still a sane interface?  If writing clear
>   documentation for it is hard, it perhaps isn't.  Pay special attention
>   to failure modes.  Overloaded arguments are prone to confusing errors.

It has never been a sane interface in the first place (identifying a
backing file node by its filename).

We ended up having two versions of all block job commands anyway (one
that creates an image file, and later one that just takes a node-name of
an existing node), except for image streaming so far. So it would be
consistent (and enable consistent naming for the preferred commands) to
have it here, too.

> * How will the command's users use the extension?  If it requires new
>   code paths, a new command may be more convenient for them.

Kevin

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Tue 11 Oct 2016 06:50:27 PM CEST, Markus Armbruster wrote:

> * Is the extended command still a sane interface?  If writing clear
>   documentation for it is hard, it perhaps isn't.  Pay special
>   attention to failure modes.  Overloaded arguments are prone to
>   confusing errors.

This is what the current command looks like:

{ 'command': 'block-stream',
  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
            '*backing-file': 'str', '*speed': 'int',
            '*on-error': 'BlockdevOnError' } }

If we decide to add a new command, this is what it could look like:

{ 'command': 'blockdev-stream',
  'data': { '*job-id': 'str', 'top': 'str', '*base': 'str',
            '*backing-file': 'str', '*speed': 'int',
            '*on-error': 'BlockdevOnError' } }

If we decide to extend the existing command, there's essentially two
changes that we have to do:

1) 'device' refers to a device name, it should refer to (or allow) a
   node name. This is trivial to do, the only problem is that the name
   of the parameter is not the best.

2) 'base' takes a file name, but we should have a way to pass a node
    name instead. Overloading here is not an option, we need a new
    parameter ('base-node' or something like that). 'base' and
    'base-node' would be optional but mutually exclusive.

{ 'command': 'block-stream',
  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
            '*base-node': 'str', '*backing-file': 'str',
            '*speed': 'int', '*on-error': 'BlockdevOnError' } }

Considering that a new command would be very similar to the original one
(the only problems being an ill-named parameter and an obsolete one), I
actually don't think that extending the current command is such a bad
idea. But I don't have a strong opinion.

Berto

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote:

>>> 3) QEMU could advertise that feature to the client. This is probably
>>> simpler than trying to figure it out from the API. I guess that's
>>> the idea of 'qmp_capabilities'?
>>
>> I think that was the idea, though it was never used. If we had used
>> it, I'm not sure how long the capabilities list would be today. :-)
>
> QMP capabilities are for changes in the QMP protocol, not for changes
> in commands, events and types.  The protocol has been good enough so
> far, thus no capabilities.

I see. Wouldn't it make sense in general to have a way to ask QEMU
whether some certain feature is supported?

Berto

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote:
>
>>>> 3) QEMU could advertise that feature to the client. This is probably
>>>> simpler than trying to figure it out from the API. I guess that's
>>>> the idea of 'qmp_capabilities'?
>>>
>>> I think that was the idea, though it was never used. If we had used
>>> it, I'm not sure how long the capabilities list would be today. :-)
>>
>> QMP capabilities are for changes in the QMP protocol, not for changes
>> in commands, events and types.  The protocol has been good enough so
>> far, thus no capabilities.
>
> I see. Wouldn't it make sense in general to have a way to ask QEMU
> whether some certain feature is supported?

Feature bits work fine for interfaces that see relatively little change.
Some QEMU interfaces are like that, for instance the QMP protocol
itself.  Other interfaces, however, change at a rapid pace.  If we tried
to provide a feature bit for every change there, we'd end up with a huge
number of them, almost certainly underdocumented, and most of them not
used by any client.  We'd probably fail to provide feature bits for some
of the interface changes, and have "feature X" bits followed some time
later by "feature X, except now it actually works" bits.

Instead, we opted for making external interfaces introspectable, so that
clients can detect stuff whether we thought of the need to detect it or
not.

The first interface with decent introspection is QMP: there's
query-qmp-schema.  Changes are easy to detect there as long as they're
*syntactic*: new commands or events, type extensions and so forth.  QMP
introspection is blind to purely *semantic* changes, say a string
argument that can now denote a node-name in addition to a filename.
Such changes should be avoided.

Re: [Qemu-devel] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Use block_job_add_bdrv() instead of blocking all operations in
> backup_start() and unblocking them in backup_run().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

This has the same problem as mirror (dataplane must be blocked).

Kevin

Re: [Qemu-devel] [PATCH v10 05/16] block: Check blockers in all nodes involved in a block-commit job

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> qmp_block_commit() checks for op blockers in the active and
> destination (base) images. However all nodes between top_bs and base
> are also involved, and they are removed from the chain afterwards.
> 
> In addition to that, if top_bs is not the active layer then top_bs's
> overlay also needs to be checked because it's involved in the job (its
> backing image string needs to be updated to point to 'base').
> 
> This patch checks that none of those nodes are blocked.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v10 06/16] block: Block all nodes involved in the block-commit operation

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> After a successful block-commit operation all nodes between top and
> base are removed from the backing chain, and top's overlay needs to
> be updated to point to base. Because of that we should prevent other
> block jobs from messing with them.
> 
> This patch blocks all operations in these nodes in commit_start().

Again, except for dataplane. But this time, I think it's actually okay
because backing files of the source automatically stay in the same
AioContext as the source. Just mention it in the commit message.

> Signed-off-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Alberto Garcia]

On Wed 12 Oct 2016 03:47:34 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
>> Use block_job_add_bdrv() instead of blocking all operations in
>> backup_start() and unblocking them in backup_run().
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> This has the same problem as mirror (dataplane must be blocked).

Yeah, I think I'll keep dataplane blocked in all cases except in
block_job_create(). BLOCK_OP_TYPE_DATAPLANE is only checked in root
nodes anyway.

Berto

Re: [Qemu-devel] [PATCH v10 07/16] block: Block all intermediate nodes in commit_active_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> When block-commit is launched without the top parameter, it uses
> internally a mirror block job. In that case all intermediate nodes
> between the active and base nodes must be blocked as well.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Same as the patch before, dataplane is okay because AioContexts are
switched together with the source image.

On second thought, however, maybe both places should set a blocker that
prevents attaching intermediate nodes to a new block device because you
can't read consistent data there. That's a preexisting problem, though,
and needs its own patch.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This makes sure that the image we are streaming into is open in
> read-write mode during the operation.
> 
> Operation blockers are also set in all intermediate nodes, since they
> will be removed from the chain afterwards.
> 
> Finally, this also unblocks the stream operation in backing files.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c        |  4 +++-
>  block/stream.c | 24 ++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c80b528..8255d75 100644
> --- a/block.c
> +++ b/block.c
> @@ -1428,9 +1428,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>              backing_hd->drv ? backing_hd->drv->format_name : "");
>  
>      bdrv_op_block_all(backing_hd, bs->backing_blocker);
> -    /* Otherwise we won't be able to commit due to check in bdrv_commit */
> +    /* Otherwise we won't be able to commit or stream */
>      bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>                      bs->backing_blocker);
> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
> +                    bs->backing_blocker);
>      /*
>       * We do backup in 3 ways:
>       * 1. drive backup
> diff --git a/block/stream.c b/block/stream.c
> index 3187481..b8ab89a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -37,6 +37,7 @@ typedef struct StreamBlockJob {
>      BlockDriverState *base;
>      BlockdevOnError on_error;
>      char *backing_file_str;
> +    int bs_flags;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>          bdrv_set_backing_hd(bs, base);
>      }
>  
> +    /* Reopen the image back in read-only mode if necessary */
> +    if (s->bs_flags != bdrv_get_flags(bs)) {
> +        bdrv_reopen(bs, s->bs_flags, NULL);
> +    }
> +
>      g_free(s->backing_file_str);
>      block_job_completed(&s->common, data->ret);
>      g_free(data);
> @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>                    BlockCompletionFunc *cb, void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
> +    BlockDriverState *iter;
> +    int orig_bs_flags;
>  
>      s = block_job_create(job_id, &stream_job_driver, bs, speed,
>                           cb, opaque, errp);
> @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>          return;
>      }
>  
> +    /* Make sure that the image is opened in read-write mode */
> +    orig_bs_flags = bdrv_get_flags(bs);
> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
> +        if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
> +            block_job_unref(&s->common);
> +            return;
> +        }
> +    }
> +
> +    /* Block all intermediate nodes between bs and base, because they
> +     * will disappear from the chain after this operation */
> +    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
> +        block_job_add_bdrv(&s->common, iter);
> +    }

In patch 6, you had a comment that the top node must be blocked as well
because its backing file string will be updated. Isn't it the same for
streaming?

>      s->base = base;
>      s->backing_file_str = g_strdup(backing_file_str);
> +    s->bs_flags = orig_bs_flags;
>  
>      s->on_error = on_error;
>      s->common.co = qemu_coroutine_create(stream_run, s);

Kevin

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 11 +++++++++--
>  qapi/block-core.json | 10 +++++++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f13fc69..57c8329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                        bool has_on_error, BlockdevOnError on_error,
>                        Error **errp)
>  {
> -    BlockDriverState *bs;
> +    BlockDriverState *bs, *iter;
>      BlockDriverState *base_bs = NULL;
>      AioContext *aio_context;
>      Error *local_err = NULL;
> @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>          on_error = BLOCKDEV_ON_ERROR_REPORT;
>      }
>  
> -    bs = qmp_get_root_bs(device, errp);
> +    bs = bdrv_lookup_bs(device, device, errp);
>      if (!bs) {
>          return;
>      }
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>          goto out;
>      }

Added a bit more context.

This check is redundant now...

>      if (has_base) {
>          base_bs = bdrv_find_backing_image(bs, base);
>          if (base_bs == NULL) {
>              error_setg(errp, QERR_BASE_NOT_FOUND, base);
>              goto out;
>          }
>          assert(bdrv_get_aio_context(base_bs) == aio_context);
>          base_name = base;
>      }
>  
> +    /* Check for op blockers in the whole chain between bs and base */
> +    for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
> +        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
> +            goto out;
> +        }
> +    }

...because you start with iter = bs here.

Another thought I had while looking at the previous few patches is
whether all the op blocker checks wouldn't be better moved to the actual
block job code (i.e. stream_start in this case). In this case it doesn't
matter much because this is the only caller of stream_start(), but for
other jobs the situation is more complicated and it would be easy for a
caller to forget the checks.

Probably also matter for another series, but I wanted to mention it.

> +
>      /* if we are streaming the entire chain, the result will have no backing
>       * file, and specifying one is therefore an error */
>      if (base_bs == NULL && has_backing_file) {

Kevin

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
>> +    /* Block all intermediate nodes between bs and base, because they
>> +     * will disappear from the chain after this operation */
>> +    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
>> +        block_job_add_bdrv(&s->common, iter);
>> +    }
>
> In patch 6, you had a comment that the top node must be blocked as
> well because its backing file string will be updated. Isn't it the
> same for streaming?

In the block-stream case, 'device' is always the top node, and creating
the block job there already blocks all operations in it.

In the block-commit case, 'device' and 'top' are different parameters,
that's why 'top' must be explicitly blocked. I actually don't think that
we need to block 'device' at all, and we could even make the parameter
optional. That would be for a future patch, though. Also, the backing
file string is not updated in 'top', but in its overlay (unlike in
block-stream, 'top' disappears after a block-commit job).

Berto

Re: [Qemu-devel] [PATCH v10 10/16] docs: Document how to stream to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  docs/live-block-ops.txt | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
> index a257087..014c8c9 100644
> --- a/docs/live-block-ops.txt
> +++ b/docs/live-block-ops.txt
> @@ -10,9 +10,9 @@ Snapshot live merge
>  Given a snapshot chain, described in this document in the following
>  format:
>  
> -[A] -> [B] -> [C] -> [D]
> +[A] <- [B] <- [C] <- [D] <- [E]
>  
> -Where the rightmost object ([D] in the example) described is the current
> +Where the rightmost object ([E] in the example) described is the current
>  image which the guest OS has write access to. To the left of it is its base
>  image, and so on accordingly until the leftmost image, which has no
>  base.
> @@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain into a
>  smaller one with fewer elements, such as this transformation relative
>  to the first example:
>  
> -[A] -> [D]
> +[A] <- [E]
>  
> -Currently only forward merge with target being the active image is
> -supported, that is, data copy is performed in the right direction with
> -destination being the rightmost image.
> +Data is copied in the right direction with destination being the
> +rightmost image, but any other intermediate image can be specified
> +instead. In this example data is copied from [C] into [D], so [D] can
> +be backed by [B]:
> +
> +[A] <- [B] <- [D] <- [E]
>  
>  The operation is implemented in QEMU through image streaming facilities.

This whole document is hopelessly outdated. At least, we need to clarify
here that streaming isn't the only operation that exists and that the
explanation refers to streaming only.

Ideally we would also add a section on commit. And likeweise, the next
section about "live block copy" should be extended to cover mirror and
backup.

Kevin

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 12.10.2016 um 16:33 hat Alberto Garcia geschrieben:
> On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> >> +    /* Block all intermediate nodes between bs and base, because they
> >> +     * will disappear from the chain after this operation */
> >> +    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
> >> +        block_job_add_bdrv(&s->common, iter);
> >> +    }
> >
> > In patch 6, you had a comment that the top node must be blocked as
> > well because its backing file string will be updated. Isn't it the
> > same for streaming?
> 
> In the block-stream case, 'device' is always the top node, and creating
> the block job there already blocks all operations in it.
> 
> In the block-commit case, 'device' and 'top' are different parameters,
> that's why 'top' must be explicitly blocked. I actually don't think that
> we need to block 'device' at all, and we could even make the parameter
> optional. That would be for a future patch, though. Also, the backing
> file string is not updated in 'top', but in its overlay (unlike in
> block-stream, 'top' disappears after a block-commit job).

I see. So the block job for commit is owned by a node that is
potentially completely unrelated to the operation except that it holds
op blockers and because we use it to finde the overlay to change the
backing file pointers. This is _so_ broken. :-)

With your series (for the op blockers) and BdrvChild (for the operations
on the overlay) we should actually be much closer to finally remove
this. Good news.

Kevin

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
>>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>>          goto out;
>>      }
>
> Added a bit more context.
>
> This check is redundant now...
>
>>      if (has_base) {
>>          base_bs = bdrv_find_backing_image(bs, base);
>>          if (base_bs == NULL) {
>>              error_setg(errp, QERR_BASE_NOT_FOUND, base);
>>              goto out;
>>          }
>>          assert(bdrv_get_aio_context(base_bs) == aio_context);
>>          base_name = base;
>>      }
>>  
>> +    /* Check for op blockers in the whole chain between bs and base */
>> +    for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
>> +        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>> +            goto out;
>> +        }
>> +    }
>
> ...because you start with iter = bs here.

You're right, I'll fix it.

> Another thought I had while looking at the previous few patches is
> whether all the op blocker checks wouldn't be better moved to the
> actual block job code (i.e. stream_start in this case).

I thought about that too. In some cases I don't know if it's a good idea
because the qmp_foo_bar() functions do a bit more than simply checking
blockers (e.g. blockdev-mirror creates the target image), so you would
want to have the checks before that.

However doing it in the actual block job code could allow us to do other
things. For example: at the moment when we call block-stream we check
whether a number of BDSs are blocked (in qmp_block_stream()), and if
they're not we proceed to block them (in stream_start()). We could make
block_job_add_bdrv() do both things. On the other hand, since the plan
is to move to a new block job API maybe it's better not to overdo things
now.

It's worth considering for the future anyway.

Berto