[Qemu-devel] [PATCH v11 00/19] Support streaming to an intermediate layer

[Qemu-devel] [PATCH v11 00/19] Support streaming to an intermediate layer

[Alberto Garcia]

Hi all,

here's version 11 of the series.

See below for the list of changes. The most important one is, I think,
the addition of the 'base-node' parameter to 'block-stream'. I decided
to go for this one rather than writing a new 'blockdev-stream' command
because (a) it's not obvious to me that introducing a new command for
such a small API change is justified in this case, and (b) it didn't
seem to me that there was a very clear agreement on the mailing list,
so I decided to go for the easier alternative.

Anyway I don't have a problem with adding a new command if that's what
most people prefer. If everything else in the series is fine I can
change that in v12.

Berto

Changes:

v11:
- Patches 1-2: Add bdrv_drain_all_{begin,end}() and use these
  functions in bdrv_reopen_multiple() instead of pausing all block
  jobs.
- Patch 3: Don't unblock BLOCK_OP_TYPE_DATAPLANE in
  block_job_add_bdrv()
- Patch 7: In commit_start(), don't call block_job_add_bdrv() twice on
  the active node.
- Patch 10: In qmp_block_stream(), don't check twice for op blockers
  in 'bs'.
- Patch 11: Clarify that live-block-ops.txt is incomplete and it only
  covers the block-stream operation.
- Patches 18-19: Add a 'base-node' parameter to 'block-stream', and
  iotests for it.

v10: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg01047.html
- 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

backport-diff against v10:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/19:[down] 'block: Add bdrv_drain_all_{begin,end}()'
002/19:[0022] [FC] 'block: Pause all jobs during bdrv_reopen_multiple()'
003/19:[0006] [FC] 'block: Add block_job_add_bdrv()'
004/19:[----] [--] 'block: Use block_job_add_bdrv() in mirror_start_job()'
005/19:[----] [--] 'block: Use block_job_add_bdrv() in backup_start()'
006/19:[----] [--] 'block: Check blockers in all nodes involved in a block-commit job'
007/19:[0014] [FC] 'block: Block all nodes involved in the block-commit operation'
008/19:[----] [--] 'block: Block all intermediate nodes in commit_active_start()'
009/19:[----] [--] 'block: Support streaming to an intermediate layer'
010/19:[0004] [FC] 'block: Add QMP support for streaming to an intermediate layer'
011/19:[0005] [FC] 'docs: Document how to stream to an intermediate layer'
012/19:[----] [--] 'qemu-iotests: Test streaming to an intermediate layer'
013/19:[----] [--] 'qemu-iotests: Test block-stream operations in parallel'
014/19:[----] [--] 'qemu-iotests: Test overlapping stream and commit operations'
015/19:[----] [--] 'qemu-iotests: Test block-stream and block-commit in parallel'
016/19:[----] [--] 'qemu-iotests: Add iotests.supports_quorum()'
017/19:[----] [--] 'qemu-iotests: Test streaming to a Quorum child'
018/19:[down] 'block: Add 'base-node' parameter to the 'block-stream' command'
019/19:[down] 'qemu-iotests: Test the 'base-node' parameter of 'block-stream''

Alberto Garcia (19):
  block: Add bdrv_drain_all_{begin,end}()
  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: Add 'base-node' parameter to the 'block-stream' command
  qemu-iotests: Test the 'base-node' parameter of 'block-stream'

 block.c                       |   9 +-
 block/backup.c                |   5 +-
 block/commit.c                |  14 ++
 block/io.c                    |  24 +++-
 block/mirror.c                |  11 +-
 block/stream.c                |  24 ++++
 blockdev.c                    |  41 +++++-
 blockjob.c                    |  17 ++-
 docs/live-block-ops.txt       |  36 +++--
 docs/qmp-commands.txt         |   7 +-
 hmp.c                         |   2 +-
 include/block/block.h         |   2 +
 include/block/blockjob.h      |  14 ++
 qapi/block-core.json          |  18 ++-
 tests/qemu-iotests/030        | 309 +++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out    |   4 +-
 tests/qemu-iotests/041        |  27 ++--
 tests/qemu-iotests/139        |   3 +-
 tests/qemu-iotests/iotests.py |   5 +-
 19 files changed, 515 insertions(+), 57 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

bdrv_drain_all() doesn't allow the caller to do anything after all
pending requests have been completed but before block jobs are
resumed.

This patch splits bdrv_drain_all() into _begin() and _end() for that
purpose. It also adds aio_{disable,enable}_external() calls to disable
external clients in the meantime.

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

diff --git a/block/io.c b/block/io.c
index b136c89..9418f8b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -275,8 +275,11 @@ void bdrv_drain(BlockDriverState *bs)
  *
  * This function does not flush data to disk, use bdrv_flush_all() for that
  * after calling this function.
+ *
+ * This pauses all block jobs and disables external clients. It must
+ * be paired with bdrv_drain_all_end().
  */
-void bdrv_drain_all(void)
+void bdrv_drain_all_begin(void)
 {
     /* Always run first iteration so any pending completion BHs run */
     bool busy = true;
@@ -300,6 +303,7 @@ void bdrv_drain_all(void)
         bdrv_parent_drained_begin(bs);
         bdrv_io_unplugged_begin(bs);
         bdrv_drain_recurse(bs);
+        aio_disable_external(aio_context);
         aio_context_release(aio_context);
 
         if (!g_slist_find(aio_ctxs, aio_context)) {
@@ -333,17 +337,25 @@ void bdrv_drain_all(void)
         }
     }
 
+    g_slist_free(aio_ctxs);
+}
+
+void bdrv_drain_all_end(void)
+{
+    BlockDriverState *bs;
+    BdrvNextIterator it;
+    BlockJob *job = NULL;
+
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
+        aio_enable_external(aio_context);
         bdrv_io_unplugged_end(bs);
         bdrv_parent_drained_end(bs);
         aio_context_release(aio_context);
     }
-    g_slist_free(aio_ctxs);
 
-    job = NULL;
     while ((job = block_job_next(job))) {
         AioContext *aio_context = blk_get_aio_context(job->blk);
 
@@ -353,6 +365,12 @@ void bdrv_drain_all(void)
     }
 }
 
+void bdrv_drain_all(void)
+{
+    bdrv_drain_all_begin();
+    bdrv_drain_all_end();
+}
+
 /**
  * Remove an active request from the tracked requests list
  *
diff --git a/include/block/block.h b/include/block/block.h
index 107c603..301d713 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -338,6 +338,8 @@ int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
+void bdrv_drain_all_begin(void);
+void bdrv_drain_all_end(void);
 void bdrv_drain_all(void);
 
 int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int count);
-- 
2.9.3

[Qemu-devel] [PATCH v11 02/19] 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 splits the bdrv_drain_all() call to keep all block jobs
paused 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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 7f3e7bc..adbecd0 100644
--- a/block.c
+++ b/block.c
@@ -2090,7 +2090,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 
     assert(bs_queue != NULL);
 
-    bdrv_drain_all();
+    bdrv_drain_all_begin();
 
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) {
@@ -2120,6 +2120,9 @@ cleanup:
         g_free(bs_entry);
     }
     g_free(bs_queue);
+
+    bdrv_drain_all_end();
+
     return ret;
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v11 03/19] 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               | 17 +++++++++++++++--
 include/block/blockjob.h | 14 ++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 43fecbe..c67c46d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -117,6 +117,13 @@ 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);
+}
+
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
                        BlockDriverState *bs, int64_t speed,
                        BlockCompletionFunc *cb, void *opaque, Error **errp)
@@ -154,7 +161,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);
+    block_job_add_bdrv(job, bs);
     bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
     job->driver        = driver;
@@ -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..bab91b1 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 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 v11 04/19] 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 v11 05/19] 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 v11 06/19] 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.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 v11 07/19] 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 9f67a8b..1500d7c 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,19 @@ 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. */
+    assert(bdrv_chain_contains(top, base));
+    for (iter = top; iter != backing_bs(base); iter = backing_bs(iter)) {
+        block_job_add_bdrv(&s->common, iter);
+    }
+    /* overlay_bs must be blocked because it needs to be modified to
+     * update the backing image string, but if it's the root node then
+     * don't block it again */
+    if (bs != overlay_bs) {
+        block_job_add_bdrv(&s->common, overlay_bs);
+    }
+
     s->base = blk_new();
     blk_insert_bs(s->base, base);
 
-- 
2.9.3

[Qemu-devel] [PATCH v11 08/19] 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.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 v11 09/19] 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 adbecd0..1ef36c9 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 v11 10/19] 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           | 15 +++++++++------
 qapi/block-core.json | 10 +++++++---
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f13fc69..c01776a 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;
     }
@@ -2955,10 +2955,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     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;
-    }
-
     if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
@@ -2969,6 +2965,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 1b7aa1b..cbbbbb9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1464,6 +1464,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
@@ -1475,12 +1479,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 v11 11/19] docs: Document how to stream to an intermediate layer

[Alberto Garcia]

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

diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
index a257087..2211d14 100644
--- a/docs/live-block-ops.txt
+++ b/docs/live-block-ops.txt
@@ -4,15 +4,20 @@ LIVE BLOCK OPERATIONS
 High level description of live block operations. Note these are not
 supported for use with the raw format at the moment.
 
+Note also that this document is incomplete and it currently only
+covers the 'stream' operation. Other operations supported by QEMU such
+as 'commit', 'mirror' and 'backup' are not described here yet. Please
+refer to the qapi/block-core.json file for an overview of those.
+
 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 +26,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 +43,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 v11 12/19] 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 v11 13/19] 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 v11 14/19] 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 v11 15/19] 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 v11 16/19] 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 v11 17/19] 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

[Qemu-devel] [PATCH v11 18/19] block: Add 'base-node' parameter to the 'block-stream' command

[Alberto Garcia]

The way to specify the node from which to copy data in the
block-stream operation is by using the 'base' parameter. This
parameter however takes a file name, not a node name.

Since we want to be able to perform this operation using only node
names, this patch adds a new 'base-node' parameter.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c            | 21 +++++++++++++++++++++
 docs/qmp-commands.txt |  7 +++++--
 hmp.c                 |  2 +-
 qapi/block-core.json  |  8 ++++++--
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index c01776a..4ed718d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2932,6 +2932,7 @@ static void block_job_cb(void *opaque, int ret)
 
 void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_base, const char *base,
+                      bool has_base_node, const char *base_node,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
@@ -2955,6 +2956,12 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
+    if (has_base && has_base_node) {
+        error_setg(errp, "'base' and 'base-node' cannot be specified "
+                   "at the same time");
+        goto out;
+    }
+
     if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
@@ -2965,6 +2972,20 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         base_name = base;
     }
 
+    if (has_base_node) {
+        base_bs = bdrv_lookup_bs(NULL, base_node, errp);
+        if (!base_bs) {
+            goto out;
+        }
+        if (bs == base_bs || !bdrv_chain_contains(bs, base_bs)) {
+            error_setg(errp, "Node '%s' is not a backing image of '%s'",
+                       base_node, device);
+            goto out;
+        }
+        assert(bdrv_get_aio_context(base_bs) == aio_context);
+        base_name = base_bs->filename;
+    }
+
     /* 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)) {
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index 7f652e0..7409865 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -740,8 +740,11 @@ Arguments:
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
 - "device": The device name or node-name of a root node (json-string)
-- "base": The file name of the backing image above which copying starts
-          (json-string, optional)
+- "base": The file name of the backing image above which copying starts.
+          It cannot be set if 'base-node' is also set (json-string, optional)
+- "base-node": the node name of the backing image above which copying starts.
+               It cannot be set if 'base' is also set.
+               (json-string, optional) (Since 2.8)
 - "backing-file": The backing file string to write into the active layer. This
                   filename is not validated.
 
diff --git a/hmp.c b/hmp.c
index 42bef84..0731266 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1526,7 +1526,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(false, NULL, device, base != NULL, base, false, NULL,
-                     qdict_haskey(qdict, "speed"), speed,
+                     false, NULL, qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
     hmp_handle_error(mon, &error);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cbbbbb9..fa1e0ec 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1481,7 +1481,11 @@
 #
 # @device: the device or node name of the top image
 #
-# @base:   #optional the common backing file name
+# @base:   #optional the common backing file name.
+#                    It cannot be set if @base-node is also set.
+#
+# @base-node: #optional the node name of the backing file.
+#                       It cannot be set if @base is also set. (Since 2.8)
 #
 # @backing-file: #optional The backing file string to write into the top
 #                          image. This filename is not validated.
@@ -1508,7 +1512,7 @@
 ##
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
-            '*backing-file': 'str', '*speed': 'int',
+            '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
             '*on-error': 'BlockdevOnError' } }
 
 ##
-- 
2.9.3

[Qemu-devel] [PATCH v11 19/19] qemu-iotests: Test the 'base-node' parameter of 'block-stream'

[Alberto Garcia]

The block-stream command has traditionally used the 'base' parameter
to indicate the image to copy the data from. This test checks that the
'base-node' parameter can also be used for the same purpose.

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

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 6736004..44729f5 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -347,6 +347,39 @@ class TestParallelOps(iotests.QMPTestCase):
 
         self.assert_no_active_block_jobs()
 
+    # Test the base_node parameter
+    def test_stream_base_node_name(self):
+        self.assert_no_active_block_jobs()
+
+        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]),
+                            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]),
+                            'image file map matches backing file before streaming')
+
+        # Error: the base node does not exist
+        result = self.vm.qmp('block-stream', device='node4', base_node='none', job_id='stream')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # Error: the base node is not a backing file of the top node
+        result = self.vm.qmp('block-stream', device='node4', base_node='node6', job_id='stream')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # Error: the base node is the same as the top node
+        result = self.vm.qmp('block-stream', device='node4', base_node='node4', job_id='stream')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        # Success: the base node is a backing file of the top node
+        result = self.vm.qmp('block-stream', device='node4', base_node='node2', job_id='stream')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='stream')
+
+        self.assert_no_active_block_jobs()
+        self.vm.shutdown()
+
+        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]),
+                         'image file map matches backing file after streaming')
+
 class TestQuorum(iotests.QMPTestCase):
     num_children = 3
     children = []
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index c6a10f8..84bfd63 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-.....................
+......................
 ----------------------------------------------------------------------
-Ran 21 tests
+Ran 22 tests
 
 OK
-- 
2.9.3

Re: [Qemu-devel] [PATCH v11 02/19] block: Pause all jobs during bdrv_reopen_multiple()

[Paolo Bonzini]

On 14/10/2016 15:08, Alberto Garcia wrote:
> 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 splits the bdrv_drain_all() call to keep all block jobs
> paused 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 7f3e7bc..adbecd0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2090,7 +2090,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>  
>      assert(bs_queue != NULL);
>  
> -    bdrv_drain_all();
> +    bdrv_drain_all_begin();
>  
>      QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>          if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) {
> @@ -2120,6 +2120,9 @@ cleanup:
>          g_free(bs_entry);
>      }
>      g_free(bs_queue);
> +
> +    bdrv_drain_all_end();
> +
>      return ret;
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

On 14/10/2016 15:08, Alberto Garcia wrote:
> bdrv_drain_all() doesn't allow the caller to do anything after all
> pending requests have been completed but before block jobs are
> resumed.
> 
> This patch splits bdrv_drain_all() into _begin() and _end() for that
> purpose. It also adds aio_{disable,enable}_external() calls to disable
> external clients in the meantime.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/io.c            | 24 +++++++++++++++++++++---
>  include/block/block.h |  2 ++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index b136c89..9418f8b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -275,8 +275,11 @@ void bdrv_drain(BlockDriverState *bs)
>   *
>   * This function does not flush data to disk, use bdrv_flush_all() for that
>   * after calling this function.
> + *
> + * This pauses all block jobs and disables external clients. It must
> + * be paired with bdrv_drain_all_end().
>   */
> -void bdrv_drain_all(void)
> +void bdrv_drain_all_begin(void)
>  {
>      /* Always run first iteration so any pending completion BHs run */
>      bool busy = true;
> @@ -300,6 +303,7 @@ void bdrv_drain_all(void)
>          bdrv_parent_drained_begin(bs);
>          bdrv_io_unplugged_begin(bs);
>          bdrv_drain_recurse(bs);
> +        aio_disable_external(aio_context);
>          aio_context_release(aio_context);
>  
>          if (!g_slist_find(aio_ctxs, aio_context)) {
> @@ -333,17 +337,25 @@ void bdrv_drain_all(void)
>          }
>      }
>  
> +    g_slist_free(aio_ctxs);
> +}
> +
> +void bdrv_drain_all_end(void)
> +{
> +    BlockDriverState *bs;
> +    BdrvNextIterator it;
> +    BlockJob *job = NULL;
> +
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(aio_context);
> +        aio_enable_external(aio_context);
>          bdrv_io_unplugged_end(bs);
>          bdrv_parent_drained_end(bs);
>          aio_context_release(aio_context);
>      }
> -    g_slist_free(aio_ctxs);
>  
> -    job = NULL;
>      while ((job = block_job_next(job))) {
>          AioContext *aio_context = blk_get_aio_context(job->blk);
>  
> @@ -353,6 +365,12 @@ void bdrv_drain_all(void)
>      }
>  }
>  
> +void bdrv_drain_all(void)
> +{
> +    bdrv_drain_all_begin();
> +    bdrv_drain_all_end();
> +}
> +
>  /**
>   * Remove an active request from the tracked requests list
>   *
> diff --git a/include/block/block.h b/include/block/block.h
> index 107c603..301d713 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -338,6 +338,8 @@ int bdrv_flush_all(void);
>  void bdrv_close_all(void);
>  void bdrv_drain(BlockDriverState *bs);
>  void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
> +void bdrv_drain_all_begin(void);
> +void bdrv_drain_all_end(void);
>  void bdrv_drain_all(void);
>  
>  int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int count);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 14.10.2016 um 15:08 hat Alberto Garcia geschrieben:
> bdrv_drain_all() doesn't allow the caller to do anything after all
> pending requests have been completed but before block jobs are
> resumed.
> 
> This patch splits bdrv_drain_all() into _begin() and _end() for that
> purpose. It also adds aio_{disable,enable}_external() calls to disable
> external clients in the meantime.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

This looks okay as a first step, possibly enough for this series (we'll
have to review this carefully), but it leaves us with a rather limited
version of bdrv_drain_all_begin/end that excludes many useful cases. One
of them is that John wants to use it around QMP transactions.

Specifically, you can't add a new BDS or a new block job in a drain_all
section because then bdrv_drain_all_end() would try to unpause the new
thing even though it has never been paused. Depending on what else we
did with it, this will either corrupt the pause counters or just
directly fail an assertion.

My first thoughts were about how to let an unpause succeed without a
previous pause for these objects, but actually I think this isn't what
we should do. We rather want to actually do the pause instead because
even new BDSes and block jobs should probably start in a quiesced state
when created inside a drain_all section.

This is somewhat similar to attaching a BlockBackend to a drained BDS.
We already take care to immediately quiesce the BB in this case (even
though this isn't very effective because the BB doesn't propagate it
correctly to its users yet...)

Thoughts?
(Paolo, I'm looking at you.)

Kevin

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote:

>> bdrv_drain_all() doesn't allow the caller to do anything after all
>> pending requests have been completed but before block jobs are
>> resumed.
>> 
>> This patch splits bdrv_drain_all() into _begin() and _end() for that
>> purpose. It also adds aio_{disable,enable}_external() calls to
>> disable external clients in the meantime.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> This looks okay as a first step, possibly enough for this series
> (we'll have to review this carefully), but it leaves us with a rather
> limited version of bdrv_drain_all_begin/end that excludes many useful
> cases. One of them is that John wants to use it around QMP
> transactions.
>
> Specifically, you can't add a new BDS or a new block job in a
> drain_all section because then bdrv_drain_all_end() would try to
> unpause the new thing even though it has never been paused. Depending
> on what else we did with it, this will either corrupt the pause
> counters or just directly fail an assertion.

The problem is: do you want to be able to create a new block job and let
it run? Because then you can end up having the same problem that this
patch is trying to prevent if the new job attempts to reopen a
BlockDriverState.

Berto

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 20.10.2016 um 10:25 hat Alberto Garcia geschrieben:
> On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote:
> >> bdrv_drain_all() doesn't allow the caller to do anything after all
> >> pending requests have been completed but before block jobs are
> >> resumed.
> >> 
> >> This patch splits bdrv_drain_all() into _begin() and _end() for that
> >> purpose. It also adds aio_{disable,enable}_external() calls to
> >> disable external clients in the meantime.
> >> 
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >
> > This looks okay as a first step, possibly enough for this series
> > (we'll have to review this carefully), but it leaves us with a rather
> > limited version of bdrv_drain_all_begin/end that excludes many useful
> > cases. One of them is that John wants to use it around QMP
> > transactions.
> >
> > Specifically, you can't add a new BDS or a new block job in a
> > drain_all section because then bdrv_drain_all_end() would try to
> > unpause the new thing even though it has never been paused. Depending
> > on what else we did with it, this will either corrupt the pause
> > counters or just directly fail an assertion.
> 
> The problem is: do you want to be able to create a new block job and let
> it run? Because then you can end up having the same problem that this
> patch is trying to prevent if the new job attempts to reopen a
> BlockDriverState.

No, as I wrote it would have to be automatically paused on creation if
it is created in a drained_all section. It would only actually start to
run after bdrv_drain_all_end().

Kevin

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[John Snow]

On 10/20/2016 04:25 AM, Alberto Garcia wrote:
> On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote:
>
>>> bdrv_drain_all() doesn't allow the caller to do anything after all
>>> pending requests have been completed but before block jobs are
>>> resumed.
>>>
>>> This patch splits bdrv_drain_all() into _begin() and _end() for that
>>> purpose. It also adds aio_{disable,enable}_external() calls to
>>> disable external clients in the meantime.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>
>> This looks okay as a first step, possibly enough for this series
>> (we'll have to review this carefully), but it leaves us with a rather
>> limited version of bdrv_drain_all_begin/end that excludes many useful
>> cases. One of them is that John wants to use it around QMP
>> transactions.
>>
>> Specifically, you can't add a new BDS or a new block job in a
>> drain_all section because then bdrv_drain_all_end() would try to
>> unpause the new thing even though it has never been paused. Depending
>> on what else we did with it, this will either corrupt the pause
>> counters or just directly fail an assertion.
>
> The problem is: do you want to be able to create a new block job and let
> it run? Because then you can end up having the same problem that this
> patch is trying to prevent if the new job attempts to reopen a
> BlockDriverState.
>
> Berto
>

The plan was to create jobs in a pre-started mode and only to engage 
them after the drained section. Do any jobs re-open a BDS prior to the 
creation of their coroutine?

--js

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

On Fri 21 Oct 2016 08:55:51 PM CEST, John Snow <jsnow@redhat.com> wrote:

>>>> bdrv_drain_all() doesn't allow the caller to do anything after all
>>>> pending requests have been completed but before block jobs are
>>>> resumed.
>>>>
>>>> This patch splits bdrv_drain_all() into _begin() and _end() for
>>>> that purpose. It also adds aio_{disable,enable}_external() calls to
>>>> disable external clients in the meantime.
>>>>
>>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>>
>>> This looks okay as a first step, possibly enough for this series
>>> (we'll have to review this carefully), but it leaves us with a
>>> rather limited version of bdrv_drain_all_begin/end that excludes
>>> many useful cases. One of them is that John wants to use it around
>>> QMP transactions.
>>>
>>> Specifically, you can't add a new BDS or a new block job in a
>>> drain_all section because then bdrv_drain_all_end() would try to
>>> unpause the new thing even though it has never been
>>> paused. Depending on what else we did with it, this will either
>>> corrupt the pause counters or just directly fail an assertion.
>>
>> The problem is: do you want to be able to create a new block job and
>> let it run? Because then you can end up having the same problem that
>> this patch is trying to prevent if the new job attempts to reopen a
>> BlockDriverState.
>>
>
> The plan was to create jobs in a pre-started mode and only to engage
> them after the drained section. Do any jobs re-open a BDS prior to the
> creation of their coroutine?

Yeah, block-commit for example (see commit_start()), and block-stream
after this series.

Berto

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[John Snow]

On 10/21/2016 03:03 PM, Alberto Garcia wrote:
> On Fri 21 Oct 2016 08:55:51 PM CEST, John Snow <jsnow@redhat.com> wrote:
>
>>>>> bdrv_drain_all() doesn't allow the caller to do anything after all
>>>>> pending requests have been completed but before block jobs are
>>>>> resumed.
>>>>>
>>>>> This patch splits bdrv_drain_all() into _begin() and _end() for
>>>>> that purpose. It also adds aio_{disable,enable}_external() calls to
>>>>> disable external clients in the meantime.
>>>>>
>>>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>>>
>>>> This looks okay as a first step, possibly enough for this series
>>>> (we'll have to review this carefully), but it leaves us with a
>>>> rather limited version of bdrv_drain_all_begin/end that excludes
>>>> many useful cases. One of them is that John wants to use it around
>>>> QMP transactions.
>>>>
>>>> Specifically, you can't add a new BDS or a new block job in a
>>>> drain_all section because then bdrv_drain_all_end() would try to
>>>> unpause the new thing even though it has never been
>>>> paused. Depending on what else we did with it, this will either
>>>> corrupt the pause counters or just directly fail an assertion.
>>>
>>> The problem is: do you want to be able to create a new block job and
>>> let it run? Because then you can end up having the same problem that
>>> this patch is trying to prevent if the new job attempts to reopen a
>>> BlockDriverState.
>>>
>>
>> The plan was to create jobs in a pre-started mode and only to engage
>> them after the drained section. Do any jobs re-open a BDS prior to the
>> creation of their coroutine?
>
> Yeah, block-commit for example (see commit_start()), and block-stream
> after this series.
>
> Berto
>

Ah, that is a problem for that use case then, but no matter.

I think I've worked out with Kevin the other day (Kevin hit me with a 
rather large trout) that a drained_all shouldn't really be necessary for 
qmp_transactions so long as each action is diligent in using 
bdrv_drained_begin/end for any given BDS that is relevant to it.

I was worried at one point about this flow:

1) bdrv_drained_begin(0x01), do_stuff()
2) bdrv_drained_begin(0x02), do_stuff()
[...]

And thought I might need to rework it as:

bdrv_drained_all_begin()
1) do_stuff()
2) do_stuff()
bdrv_drained_all_end()

but Kevin has pointed out that even though actions and drains are 
interspersed, the point-in-time simply becomes the time at last drain 
and it still should be coherent, so I won't need the drain-all after all.

--js

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

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



On 19/10/2016 19:11, Kevin Wolf wrote:
> Am 14.10.2016 um 15:08 hat Alberto Garcia geschrieben:
>> bdrv_drain_all() doesn't allow the caller to do anything after all
>> pending requests have been completed but before block jobs are
>> resumed.
>>
>> This patch splits bdrv_drain_all() into _begin() and _end() for that
>> purpose. It also adds aio_{disable,enable}_external() calls to disable
>> external clients in the meantime.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
> 
> This looks okay as a first step, possibly enough for this series (we'll
> have to review this carefully), but it leaves us with a rather limited
> version of bdrv_drain_all_begin/end that excludes many useful cases. One
> of them is that John wants to use it around QMP transactions.
> 
> Specifically, you can't add a new BDS or a new block job in a drain_all
> section because then bdrv_drain_all_end() would try to unpause the new
> thing even though it has never been paused. Depending on what else we
> did with it, this will either corrupt the pause counters or just
> directly fail an assertion.
> 
> My first thoughts were about how to let an unpause succeed without a
> previous pause for these objects, but actually I think this isn't what
> we should do. We rather want to actually do the pause instead because
> even new BDSes and block jobs should probably start in a quiesced state
> when created inside a drain_all section.

Yes, I agree with this.  It shouldn't be too hard to implement it.  It
would require a BlockDriverState to look at the global "inside
bdrv_drain_all_begin" state, and ask its BlockBackend to disable itself
upon bdrv_replace_child.

Basically, "foo->quiesce_counter" should become "foo->quiesce_counter ||
all_quiesce_counter", I think.  It may well be done as a separate patch
if there is a TODO comment in bdrv_replace_child; as Kevin said, there
are assertions to protect against misuse.

Paolo


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

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:

>> My first thoughts were about how to let an unpause succeed without a
>> previous pause for these objects, but actually I think this isn't
>> what we should do. We rather want to actually do the pause instead
>> because even new BDSes and block jobs should probably start in a
>> quiesced state when created inside a drain_all section.
>
> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
> would require a BlockDriverState to look at the global "inside
> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
> itself upon bdrv_replace_child.

Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
if you add one with "blockdev-add" it's not going to be disabled using
this method.

In addition to that block jobs need the same, don't they? Something like
"job->pause_count = all_quiesce_counter" in the initialization.

I think we'd also need to add block_job_pause_point() at the beginning
of each one of their coroutines, in order to make sure that they really
start paused.

Berto

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

On 25/10/2016 15:39, Alberto Garcia wrote:
> On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:
> 
>>> My first thoughts were about how to let an unpause succeed without a
>>> previous pause for these objects, but actually I think this isn't
>>> what we should do. We rather want to actually do the pause instead
>>> because even new BDSes and block jobs should probably start in a
>>> quiesced state when created inside a drain_all section.
>>
>> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
>> would require a BlockDriverState to look at the global "inside
>> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
>> itself upon bdrv_replace_child.
> 
> Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
> if you add one with "blockdev-add" it's not going to be disabled using
> this method.

You only need to disable it when blk_insert_bs is called.  In fact...

> In addition to that block jobs need the same, don't they? Something like
> "job->pause_count = all_quiesce_counter" in the initialization.

... we discussed a couple weeks ago that block jobs should register
BlkDeviceOps that pause the job in the drained_begin callback.  So when
the block job calls blk_insert_bs, the drained_begin callback would
start the job as paused.

> I think we'd also need to add block_job_pause_point() at the beginning
> of each one of their coroutines, in order to make sure that they really
> start paused.

It depends on the job, for example streaming starts with
block_job_sleep_ns.  Commit also does, except for some blk_getlength and
blk_truncate calls that could be moved to the caller.

Paolo

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 25.10.2016 um 15:53 hat Paolo Bonzini geschrieben:
> 
> 
> On 25/10/2016 15:39, Alberto Garcia wrote:
> > On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:
> > 
> >>> My first thoughts were about how to let an unpause succeed without a
> >>> previous pause for these objects, but actually I think this isn't
> >>> what we should do. We rather want to actually do the pause instead
> >>> because even new BDSes and block jobs should probably start in a
> >>> quiesced state when created inside a drain_all section.
> >>
> >> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
> >> would require a BlockDriverState to look at the global "inside
> >> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
> >> itself upon bdrv_replace_child.
> > 
> > Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
> > if you add one with "blockdev-add" it's not going to be disabled using
> > this method.
> 
> You only need to disable it when blk_insert_bs is called.  In fact...

This assumes that the block driver doesn't issue internal background I/O
by itself. Probably true for everything that we have today, but it would
probably be cleaner to quiesce it directly in bdrv_open_common().

> > In addition to that block jobs need the same, don't they? Something like
> > "job->pause_count = all_quiesce_counter" in the initialization.
> 
> ... we discussed a couple weeks ago that block jobs should register
> BlkDeviceOps that pause the job in the drained_begin callback.  So when
> the block job calls blk_insert_bs, the drained_begin callback would
> start the job as paused.

Yes, should, but doing this kind of infrastructure work isn't something
for this series.

> > I think we'd also need to add block_job_pause_point() at the beginning
> > of each one of their coroutines, in order to make sure that they really
> > start paused.
> 
> It depends on the job, for example streaming starts with
> block_job_sleep_ns.  Commit also does, except for some blk_getlength and
> blk_truncate calls that could be moved to the caller.

Kevin

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

On 25/10/2016 16:38, Kevin Wolf wrote:
> Am 25.10.2016 um 15:53 hat Paolo Bonzini geschrieben:
>>
>>
>> On 25/10/2016 15:39, Alberto Garcia wrote:
>>> On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:
>>>
>>>>> My first thoughts were about how to let an unpause succeed without a
>>>>> previous pause for these objects, but actually I think this isn't
>>>>> what we should do. We rather want to actually do the pause instead
>>>>> because even new BDSes and block jobs should probably start in a
>>>>> quiesced state when created inside a drain_all section.
>>>>
>>>> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
>>>> would require a BlockDriverState to look at the global "inside
>>>> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
>>>> itself upon bdrv_replace_child.
>>>
>>> Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
>>> if you add one with "blockdev-add" it's not going to be disabled using
>>> this method.
>>
>> You only need to disable it when blk_insert_bs is called.  In fact...
> 
> This assumes that the block driver doesn't issue internal background I/O
> by itself. Probably true for everything that we have today, but it would
> probably be cleaner to quiesce it directly in bdrv_open_common().

So

	bs->quiesce_counter = all_quiesce_counter;

?  That would work.  Should bdrv_close assert bs->quiesce_counter==0
(which implies all_quiesce_counter == 0), since it usually has to do I/O?

>>> In addition to that block jobs need the same, don't they? Something like
>>> "job->pause_count = all_quiesce_counter" in the initialization.
>>
>> ... we discussed a couple weeks ago that block jobs should register
>> BlkDeviceOps that pause the job in the drained_begin callback.  So when
>> the block job calls blk_insert_bs, the drained_begin callback would
>> start the job as paused.
> 
> Yes, should, but doing this kind of infrastructure work isn't something
> for this series.

I agree.  I was just explaining why it wouldn't be necessary to
initialize job->pause_count.

Paolo

>>> I think we'd also need to add block_job_pause_point() at the beginning
>>> of each one of their coroutines, in order to make sure that they really
>>> start paused.
>>
>> It depends on the job, for example streaming starts with
>> block_job_sleep_ns.  Commit also does, except for some blk_getlength and
>> blk_truncate calls that could be moved to the caller.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

On Tue 25 Oct 2016 04:38:27 PM CEST, Kevin Wolf wrote:

>> >>> My first thoughts were about how to let an unpause succeed without a
>> >>> previous pause for these objects, but actually I think this isn't
>> >>> what we should do. We rather want to actually do the pause instead
>> >>> because even new BDSes and block jobs should probably start in a
>> >>> quiesced state when created inside a drain_all section.
>> >>
>> >> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
>> >> would require a BlockDriverState to look at the global "inside
>> >> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
>> >> itself upon bdrv_replace_child.
>> > 
>> > Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
>> > if you add one with "blockdev-add" it's not going to be disabled using
>> > this method.
>> 
>> You only need to disable it when blk_insert_bs is called.  In fact...
>
> This assumes that the block driver doesn't issue internal background I/O
> by itself. Probably true for everything that we have today, but it would
> probably be cleaner to quiesce it directly in bdrv_open_common().

And how about the rest of the things that are going on in
bdrv_drain_all_begin()?

        bdrv_parent_drained_begin(bs);
        bdrv_io_unplugged_begin(bs);
        bdrv_drain_recurse(bs);
        aio_disable_external(aio_context);

Berto

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

On 25/10/2016 16:48, Alberto Garcia wrote:
> And how about the rest of the things that are going on in
> bdrv_drain_all_begin()?
> 
>         bdrv_parent_drained_begin(bs);

No BlockBackend yet, and BlockDriverStates have been quiesced already,
so that's okay.

>         bdrv_io_unplugged_begin(bs);

No I/O yet, so that's okay.

>         bdrv_drain_recurse(bs);

Children have been created before, so they're already quiescent.

>         aio_disable_external(aio_context);

This is also a hack for what should be in BlockBackend---which means
that we're safe because there's no BlockBackend yet.

Paolo

Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 25.10.2016 um 16:41 hat Paolo Bonzini geschrieben:
> 
> 
> On 25/10/2016 16:38, Kevin Wolf wrote:
> > Am 25.10.2016 um 15:53 hat Paolo Bonzini geschrieben:
> >>
> >>
> >> On 25/10/2016 15:39, Alberto Garcia wrote:
> >>> On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:
> >>>
> >>>>> My first thoughts were about how to let an unpause succeed without a
> >>>>> previous pause for these objects, but actually I think this isn't
> >>>>> what we should do. We rather want to actually do the pause instead
> >>>>> because even new BDSes and block jobs should probably start in a
> >>>>> quiesced state when created inside a drain_all section.
> >>>>
> >>>> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
> >>>> would require a BlockDriverState to look at the global "inside
> >>>> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
> >>>> itself upon bdrv_replace_child.
> >>>
> >>> Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
> >>> if you add one with "blockdev-add" it's not going to be disabled using
> >>> this method.
> >>
> >> You only need to disable it when blk_insert_bs is called.  In fact...
> > 
> > This assumes that the block driver doesn't issue internal background I/O
> > by itself. Probably true for everything that we have today, but it would
> > probably be cleaner to quiesce it directly in bdrv_open_common().
> 
> So
> 
> 	bs->quiesce_counter = all_quiesce_counter;
> 
> ?  That would work.

Yes, that's what I had in mind.

> Should bdrv_close assert bs->quiesce_counter==0
> (which implies all_quiesce_counter == 0), since it usually has to do I/O?

Hm... Not sure about that. We're still using bdrv_drain_all_begin/end as
a function to isolate the BDSes, so some I/O from the caller of
drain_all is expected, and that could involve deleting a BDS.

But once we fully implemented what you proposed, probably yes.

Kevin