Support streaming to an intermediate layer

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

This can have fatal consequences, like in this example:

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

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

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

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

block.c +1 -0

index bb1f1ec..c80b528 100644
@@ -2087,9 +2087,19 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
     Error *local_err = NULL;
+    BlockJob *job = NULL;
 
     assert(bs_queue != NULL);
 

block.c +3 -0

     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {

@@ -2120,6 +2130,17 @@ cleanup:
         g_free(bs_entry);
     }
     g_free(bs_queue);
+
+    /* Resume all block jobs */
+    job = NULL;

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 <[email protected]>

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 <[email protected]>

Quoted 6 lines

block.c +6 -0

+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);

block.c int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) · line 2103

Quoted 3 lines

block.c +2 -0

+    }
+
     bdrv_drain_all();

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

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

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 <[email protected]>

---
 blockdev.c           | 11 +++++++++--
 qapi/block-core.json | 10 +++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

+++ b/qapi/block-core.json

qapi/block-core.json +4 -0

@@ -1469,6 +1469,10 @@
 # with query-block-jobs.  The operation can be stopped before it has completed
 # using the block-job-cancel command.
 #
+# The node that receives the data is called the top image, can be located in
+# any part of the chain (but always above the base image; see below) and can be
+# specified using its device or node name.
+#
 # If a base file is specified then sectors are not copied from that base file and
 # its backing chain.  When streaming completes the image file will have the base
 # file as its backing file.  This can be used to stream a subset of the backing

@@ -1480,12 +1484,12 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)

Quoted 4 lines

Quoted 4 lines

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 <[email protected]>

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

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

Quoted 6 lines

Quoted 6 lines

block.c +6 -0

+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);

Quoted 8 lines

block.c int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) · line 2103

Quoted 3 lines

block.c +2 -0

+    }
+
     bdrv_drain_all();

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

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

Quoted 2 lines

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

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

Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben:

Quoted 19 lines · Kevin Wolf

Quoted 6 lines

Quoted 6 lines

block.c +6 -0

+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);

Quoted 8 lines

block.c int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) · line 2103

Quoted 3 lines

block.c +2 -0

+    }
+
     bdrv_drain_all();

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

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

Quoted 4 lines

Quoted 2 lines

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

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

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

Quoted 23 lines

Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben:

Quoted 22 lines · Kevin Wolf

Quoted 6 lines

Quoted 6 lines

block.c +6 -0

+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);

Quoted 8 lines

block.c int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) · line 2103

Quoted 3 lines

block.c +2 -0

+    }
+
     bdrv_drain_all();

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

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

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

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

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

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

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

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

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

    bdrv_drained_all_start();
    bdrv_drained_all_end();

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

Quoted 4 lines

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 <[email protected]>

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

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

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 1 line

Signed-off-by Alberto Garcia <[email protected]>

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

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

Thanks, I'll fix it.

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

Quoted 11 lines

# @job-id: #optional identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#

qapi/block-core.json +2 -3

-# @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

Quoted 1 line

+# image. This filename is not validated.

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

Quoted 11 lines

Quoted 11 lines

# @job-id: #optional identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#

qapi/block-core.json +2 -3

-# @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

Quoted 8 lines

Quoted 1 line

+# image. This filename is not validated.

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

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

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

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

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

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

Am 11.10.2016 um 16:30 hat Alberto Garcia geschrieben:

Quoted 3 lines

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

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

Quoted 4 lines · Kevin Wolf

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

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

Quoted 10 lines · Eric Blake

Quoted 4 lines · Kevin Wolf

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

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

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

A new command is trivial to detect in introspection.

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

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

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

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

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

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

Quoted 6 lines

Quoted 3 lines

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

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

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

Quoted 7 lines · Markus Armbruster

Quoted 6 lines

Quoted 3 lines

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

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

Quoted 3 lines

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

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

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

Signed-off-by Alberto Garcia <[email protected]>

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 <[email protected]>

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 <[email protected]>

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:

Quoted 4 lines

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

Signed-off-by Alberto Garcia <[email protected]>

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

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 <[email protected]>

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 <[email protected]>

---
 block.c        |  4 +++-
 block/stream.c | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

block.c +3 -1

index c80b528..8255d75 100644
@@ -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

block/stream.c +12 -0

index 3187481..b8ab89a 100644
@@ -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) {

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 <[email protected]>

---
 blockdev.c           | 11 +++++++++--
 qapi/block-core.json | 10 +++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

blockdev.c +2 -2

index f13fc69..57c8329 100644
@@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs, *iter;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;

@@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    bs = qmp_get_root_bs(device, errp);
+    bs = bdrv_lookup_bs(device, device, errp);
     if (!bs) {
         return;
     }

     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;
     }

     if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
             error_setg(errp, QERR_BASE_NOT_FOUND, base);
             goto out;
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
         base_name = base;
     }

+

     /* 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) {

block/stream.c void stream_start(const char *job_id, BlockDriverState *bs, · line 251

Quoted 5 lines

block/stream.c +5 -0

+    /* Block all intermediate nodes between bs and base, because they
+     * will disappear from the chain after this operation */
+    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
+        block_job_add_bdrv(&s->common, iter);
+    }

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

Signed-off-by Alberto Garcia <[email protected]>

---
 docs/live-block-ops.txt | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

docs/live-block-ops.txt +3 -4

index a257087..014c8c9 100644
@@ -10,9 +10,9 @@ Snapshot live merge
 Given a snapshot chain, described in this document in the following
 format:
 
-[A] -> [B] -> [C] -> [D]
+[A] <- [B] <- [C] <- [D] <- [E]
 
-Where the rightmost object ([D] in the example) described is the current
+Where the rightmost object ([E] in the example) described is the current
 image which the guest OS has write access to. To the left of it is its base
 image, and so on accordingly until the leftmost image, which has no
 base.

@@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain into a
 smaller one with fewer elements, such as this transformation relative
 to the first example:
 
-[A] -> [D]
+[A] <- [E]
 
-Currently only forward merge with target being the active image is

Quoted 9 lines

block/stream.c void stream_start(const char *job_id, BlockDriverState *bs, · line 251

Quoted 5 lines

block/stream.c +5 -0

+    /* Block all intermediate nodes between bs and base, because they
+     * will disappear from the chain after this operation */
+    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
+        block_job_add_bdrv(&s->common, iter);
+    }

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

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

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

Quoted 3 lines

     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;
     }

Added a bit more context.

This check is redundant now...

Quoted 10 lines

     if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
             error_setg(errp, QERR_BASE_NOT_FOUND, base);
             goto out;
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
         base_name = base;
     }

blockdev.c void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, · line 2977

Quoted 6 lines

blockdev.c +6 -0

+    /* Check for op blockers in the whole chain between bs and base */
+    for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
+        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
+            goto out;
+        }
+    }

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

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