Skip to contentSkip to conversation

Support streaming to an intermediate layer

PATCH Superseded Alberto Garcia 3 participants 23 messages Apr 16, 2015 – Apr 23, 2015 Block layer

v4 of 13 Superseded Latest is v13 →

lore
Share & tools
lore
#0 lore raw

v4 cover Support streaming to an intermediate layer

New series.

As I wrote in a separate e-mail, we don't need to have any additional checks on the top image before reopening it in R/W mode since bdrv_reopen_prepare() already takes care of checking the BDRV_O_ALLOW_RDWR flag, which serves our purpose.

v4:
- 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.

Regards,

Berto

Alberto Garcia (8):
block: keep a list of block jobs
block: allow block jobs in any arbitrary node
block: never cancel a streaming job without running stream_complete()
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: fix test_stream_partial()
qemu-iotests: test streaming to an intermediate layer

 block.c                    |  4 +++-
 block/mirror.c             |  5 +++--
 block/stream.c             | 44 ++++++++++++++++++++++++++++++++-----
 blockdev.c                 | 55 ++++++++++++++++++++++------------------------
 blockjob.c                 | 31 +++++++++++++++++++-------
 docs/live-block-ops.txt    | 30 +++++++++++++++----------
 docs/qmp/qmp-events.txt    |  8 +++----
 include/block/blockjob.h   | 14 ++++++++++++
 include/qapi/qmp/qerror.h  |  3 ---
 qapi/block-core.json       | 30 ++++++++++++++-----------
 tests/qemu-iotests/030     | 23 ++++++++++++++++---
 tests/qemu-iotests/030.out |  4 ++--
 12 files changed, 170 insertions(+), 81 deletions(-)
Signature · 1 line
2.1.4
#10 lore raw

PATCH v4 5/8 block: Add QMP support for streaming to an intermediate layer

This patch makes the 'device' paramater of the 'block-stream' command accept a node name as well as a device name.

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

Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and no longer returns DeviceNotFound, iotest 030 is updated to expect GenericError instead.

---
 blockdev.c             | 20 ++++++++++----------
 qapi/block-core.json   | 10 +++++++---
 tests/qemu-iotests/030 |  2 +-
 3 files changed, 18 insertions(+), 14 deletions(-)
blockdev.c +10 -10
index 677432f..d62e548 100644
@@ -2104,8 +2104,7 @@ void qmp_block_stream(const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockDriverState *bs, *iter;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -2115,20 +2114,14 @@ void qmp_block_stream(const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+    bs = bdrv_lookup_bs(device, device, errp);
+    if (!bs) {
         return;
     }
-    bs = blk_bs(blk);
 
     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) {
@@ -2139,6 +2132,13 @@ void qmp_block_stream(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 = iter->backing_hd) {
+        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) {
qapi/block-core.json +7 -3
index 5126ae0..befc5ce 100644
@@ -1014,6 +1014,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 whole chain 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
@@ -1022,12 +1026,12 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @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
tests/qemu-iotests/030 +1 -1
index 952a524..7ca9b25 100755
@@ -107,7 +107,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-stream', device='nonexistent')
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
 
 class TestSmallerBackingFile(iotests.QMPTestCase):
Signature · 1 line
2.1.4
#13 lore raw

PATCH v4 6/8 docs: Document how to stream to an intermediate layer

In reply to Alberto Garcia
---
 docs/live-block-ops.txt | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
docs/live-block-ops.txt +19 -11
index a257087..3bf86be 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,13 @@ 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, for instance from [B] into [D]:
+
+[A] -> [B] -> [D] -> [E]
 
 The operation is implemented in QEMU through image streaming facilities.
 
@@ -35,14 +37,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 first example above, the command would be:
+
+(qemu) block_stream virtio0 file-A.img
 
-In the example above, the command would be:
+In order to specify a destination image different from the active
+(rightmost) one we can use its (previously set) node name instead.
 
-(qemu) block_stream virtio0 A
+In the second example above, the command would be:
 
+(qemu) block_stream node-D file-B.img
 
 Live block copy
 ===============
Signature · 1 line
2.1.4
#20 lore raw

PATCH v4 8/8 qemu-iotests: test streaming to an intermediate layer

In reply to Alberto Garcia

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

---
 tests/qemu-iotests/030     | 18 +++++++++++++++++-
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 19 insertions(+), 3 deletions(-)
tests/qemu-iotests/030 +17 -1
index 6e6cb5a..69ada2b 100755
@@ -36,7 +36,8 @@ 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)
+        opts = "backing.node-name=mid"
+        self.vm = iotests.VM().add_drive("blkdebug::" + test_img, opts)
         self.vm.launch()
 
     def tearDown(self):
@@ -60,6 +61,21 @@ 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()
+
+        result = self.vm.qmp('block-stream', device='mid')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='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()
 
tests/qemu-iotests/030.out +2 -2
index fa16b5c..6323079 100644
@@ -1,5 +1,5 @@
-.............
+..............
 ----------------------------------------------------------------------
-Ran 13 tests
+Ran 14 tests
 
 OK
Signature · 1 line
2.1.4
#3 lore raw

PATCH v4 2/8 block: allow block jobs in any arbitrary node

In reply to Alberto Garcia

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

---
 block/mirror.c            |  5 +++--
 blockdev.c                | 16 ++++++++--------
 blockjob.c                | 18 ++++++++++--------
 docs/qmp/qmp-events.txt   |  8 ++++----
 include/qapi/qmp/qerror.h |  3 ---
 qapi/block-core.json      | 20 ++++++++++----------
 6 files changed, 35 insertions(+), 35 deletions(-)
block/mirror.c +3 -2
index 4056164..189e8f8 100644
@@ -607,8 +607,9 @@ static void mirror_complete(BlockJob *job, Error **errp)
         return;
     }
     if (!s->synced) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_READY,
-                  bdrv_get_device_name(job->bs));
+        error_setg(errp,
+                   "The active block job for device '%s' cannot be completed",
+                   bdrv_get_device_name(job->bs));
         return;
     }
 
blockdev.c +8 -8
index 567d5e3..677432f 100644
@@ -2643,32 +2643,32 @@ out:
     aio_context_release(aio_context);
 }
 
-/* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context,
+/* Get the block job for a given device or node name
+ * and acquire its AioContext */
+static BlockJob *find_block_job(const char *device_or_node,
+                                AioContext **aio_context,
                                 Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);
+    if (!bs) {
         goto notfound;
     }
-    bs = blk_bs(blk);
 
     *aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(*aio_context);
 
     if (!bs->job) {
         aio_context_release(*aio_context);
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
+              "No active block job on node '%s'", device_or_node);
         goto notfound;
     }
 
     return bs->job;
 
 notfound:
-    error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
-              "No active block job on device '%s'", device);
     *aio_context = NULL;
     return NULL;
 }
blockjob.c +10 -8
index 562362b..57cf0c3 100644
@@ -52,7 +52,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     BlockJob *job;
 
     if (bs->job) {
-        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
+        error_setg(errp, "Node '%s' is in use",
+                   bdrv_get_device_or_node_name(bs));
         return NULL;
     }
     bdrv_ref(bs);
@@ -121,8 +122,9 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 void block_job_complete(BlockJob *job, Error **errp)
 {
     if (job->paused || job->cancelled || !job->driver->complete) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_READY,
-                  bdrv_get_device_name(job->bs));
+        error_setg(errp,
+                   "The active block job for node '%s' cannot be completed",
+                   bdrv_get_device_or_node_name(job->bs));
         return;
     }
 
@@ -268,7 +270,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
 {
     BlockJobInfo *info = g_new0(BlockJobInfo, 1);
     info->type      = g_strdup(BlockJobType_lookup[job->driver->job_type]);
-    info->device    = g_strdup(bdrv_get_device_name(job->bs));
+    info->device    = g_strdup(bdrv_get_device_or_node_name(job->bs));
     info->len       = job->len;
     info->busy      = job->busy;
     info->paused    = job->paused;
@@ -290,7 +292,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
 void block_job_event_cancelled(BlockJob *job)
 {
     qapi_event_send_block_job_cancelled(job->driver->job_type,
-                                        bdrv_get_device_name(job->bs),
+                                        bdrv_get_device_or_node_name(job->bs),
                                         job->len,
                                         job->offset,
                                         job->speed,
@@ -300,7 +302,7 @@ void block_job_event_cancelled(BlockJob *job)
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
     qapi_event_send_block_job_completed(job->driver->job_type,
-                                        bdrv_get_device_name(job->bs),
+                                        bdrv_get_device_or_node_name(job->bs),
                                         job->len,
                                         job->offset,
                                         job->speed,
@@ -314,7 +316,7 @@ void block_job_event_ready(BlockJob *job)
     job->ready = true;
 
     qapi_event_send_block_job_ready(job->driver->job_type,
-                                    bdrv_get_device_name(job->bs),
+                                    bdrv_get_device_or_node_name(job->bs),
                                     job->len,
                                     job->offset,
                                     job->speed, &error_abort);
@@ -343,7 +345,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
     default:
         abort();
     }
-    qapi_event_send_block_job_error(bdrv_get_device_name(job->bs),
+    qapi_event_send_block_job_error(bdrv_get_device_or_node_name(job->bs),
                                     is_read ? IO_OPERATION_TYPE_READ :
                                     IO_OPERATION_TYPE_WRITE,
                                     action, &error_abort);
docs/qmp/qmp-events.txt +4 -4
index b19e490..db90c61 100644
@@ -89,7 +89,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Device name, or node name if not present (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
@@ -113,7 +113,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Device name, or node name if not present (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
@@ -140,7 +140,7 @@ Emitted when a block job encounters an error.
 
 Data:
 
-- "device": device name (json-string)
+- "device": device name, or node name if not present (json-string)
 - "operation": I/O operation (json-string, "read" or "write")
 - "action": action that has been taken, it's one of the following (json-string):
     "ignore": error has been ignored, the job may fail later
@@ -164,7 +164,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Device name, or node name if not present (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
include/qapi/qmp/qerror.h +0 -3
index e567339..4ba442e 100644
@@ -37,9 +37,6 @@ void qerror_report_err(Error *err);
 #define QERR_BASE_NOT_FOUND \
     ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
 
-#define QERR_BLOCK_JOB_NOT_READY \
-    ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"
-
 #define QERR_BUS_NO_HOTPLUG \
     ERROR_CLASS_GENERIC_ERROR, "Bus '%s' does not support hotplugging"
 
qapi/block-core.json +10 -10
index 21e6cb5..5126ae0 100644
@@ -540,7 +540,7 @@
 #
 # @type: the job type ('stream' for image streaming)
 #
-# @device: the block device name
+# @device: the block device name, or node name if not present
 #
 # @len: the maximum progress value
 #
@@ -1065,7 +1065,7 @@
 #
 # Throttling can be disabled by setting the speed to 0.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.
 #
 # @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
 #          Defaults to 0.
@@ -1096,7 +1096,7 @@
 # operation can be started at a later time to finish copying all data from the
 # backing file.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.
 #
 # @force: #optional whether to allow cancellation of a paused job (default
 #         false).  Since 1.3.
@@ -1122,7 +1122,7 @@
 # the operation is actually paused.  Cancelling a paused job automatically
 # resumes it.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -1142,7 +1142,7 @@
 #
 # This command also clears the error status of the job.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -1168,7 +1168,7 @@
 #
 # A cancelled or paused job cannot be completed.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -1821,7 +1821,7 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: device name, or node name if not present
 #
 # @len: maximum progress value
 #
@@ -1852,7 +1852,7 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: device name, or node name if not present
 #
 # @len: maximum progress value
 #
@@ -1875,7 +1875,7 @@
 #
 # Emitted when a block job encounters an error
 #
-# @device: device name
+# @device: device name, or node name if not present
 #
 # @operation: I/O operation
 #
@@ -1895,7 +1895,7 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: device name, or node name if not present
 #
 # @len: maximum progress value
 #
Signature · 1 line
2.1.4
#1 lore raw

PATCH v4 1/8 block: keep a list of block jobs

In reply to Alberto Garcia

The current way to obtain the list of existing block jobs is to iterate over all root nodes and check which ones own a job.

Since we want to be able to support block jobs in other nodes as well, this patch keeps a list of jobs that is updated everytime one is created or destroyed.

This also updates qmp_query_block_jobs() to use this new list.

---
 blockdev.c               | 19 ++++++++-----------
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 14 ++++++++++++++
 3 files changed, 35 insertions(+), 11 deletions(-)
blockdev.c +8 -11
index 30dc9d2..567d5e3 100644
@@ -2901,21 +2901,18 @@ fail:
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
     BlockJobInfoList *head = NULL, **p_next = &head;
-    BlockDriverState *bs;
+    BlockJob *job;
 
-    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
+        AioContext *aio_context = bdrv_get_aio_context(job->bs);
 
         aio_context_acquire(aio_context);
-
-        if (bs->job) {
-            BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
-            elem->value = block_job_query(bs->job);
-            *p_next = elem;
-            p_next = &elem->next;
-        }
-
+        elem->value = block_job_query(job);
         aio_context_release(aio_context);
+
+        *p_next = elem;
+        p_next = &elem->next;
     }
 
     return head;
blockjob.c +13 -0
index ba2255d..562362b 100644
@@ -35,6 +35,16 @@
 #include "qemu/timer.h"
 #include "qapi-event.h"
 
+static QLIST_HEAD(, BlockJob) block_jobs = QLIST_HEAD_INITIALIZER(block_jobs);
+
+BlockJob *block_job_next(BlockJob *job)
+{
+    if (!job) {
+        return QLIST_FIRST(&block_jobs);
+    }
+    return QLIST_NEXT(job, job_list);
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
@@ -73,6 +83,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
             return NULL;
         }
     }
+
+    QLIST_INSERT_HEAD(&block_jobs, job, job_list);
     return job;
 }
 
@@ -85,6 +97,7 @@ void block_job_completed(BlockJob *job, int ret)
     bs->job = NULL;
     bdrv_op_unblock_all(bs, job->blocker);
     error_free(job->blocker);
+    QLIST_REMOVE(job, job_list);
     g_free(job);
 }
 
include/block/blockjob.h +14 -0
index b6d4ebb..636204b 100644
@@ -96,6 +96,9 @@ struct BlockJob {
      */
     bool ready;
 
+    /** Element of the list of block jobs */
+    QLIST_ENTRY(BlockJob) job_list;
+
     /** Status that is published by the query-block-jobs QMP API */
     BlockDeviceIoStatus iostatus;
 
@@ -119,6 +122,17 @@ struct BlockJob {
 };
 
 /**
+ * block_job_next:
+ * @job: A block job, or %NULL.
+ *
+ * Get the next element from the list of block jobs after @job, or the
+ * first one if @job is %NULL.
+ *
+ * Returns the requested job, or %NULL if there are no more jobs left.
+ */
+BlockJob *block_job_next(BlockJob *job);
+
+/**
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
Signature · 1 line
2.1.4
#7 lore raw

PATCH v4 3/8 block: never cancel a streaming job without running stream_complete()

In reply to Alberto Garcia

We need to call stream_complete() in order to do all the necessary clean-ups, even if there's an early failure. At the moment it's only useful to make sure that s->backing_file_str is not leaked, but it will become more important as we introduce support for streaming to any intermediate node.

---
 block/stream.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
block/stream.c +6 -5
index a628901..37bfd8b 100644
@@ -114,21 +114,21 @@ static void coroutine_fn stream_run(void *opaque)
     StreamCompleteData *data;
     BlockDriverState *bs = s->common.bs;
     BlockDriverState *base = s->base;
-    int64_t sector_num, end;
+    int64_t sector_num = 0;
+    int64_t end = -1;
     int error = 0;
     int ret = 0;
     int n = 0;
     void *buf;
 
     if (!bs->backing_hd) {
-        block_job_completed(&s->common, 0);
-        return;
+        goto out;
     }
 
     s->common.len = bdrv_getlength(bs);
     if (s->common.len < 0) {
-        block_job_completed(&s->common, s->common.len);
-        return;
+        ret = s->common.len;
+        goto out;
     }
 
     end = s->common.len >> BDRV_SECTOR_BITS;
@@ -215,6 +215,7 @@ wait:
 
     qemu_vfree(buf);
 
+out:
     /* Modify backing chain and close BDSes in main loop */
     data = g_malloc(sizeof(*data));
     data->ret = ret;
Signature · 1 line
2.1.4
#8 lore raw

PATCH v4 4/8 block: Support streaming to an intermediate layer

In reply to Alberto Garcia

This makes sure that the image we are steaming 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.

---
 block.c        |  4 +++-
 block/stream.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
block.c +3 -1
index 25289ef..e892cb4 100644
@@ -1240,9 +1240,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
             backing_hd->drv ? backing_hd->drv->format_name : "");
 
     bdrv_op_block_all(bs->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(bs->backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
                     bs->backing_blocker);
+    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_STREAM,
+                    bs->backing_blocker);
 out:
     bdrv_refresh_limits(bs, NULL);
 }
block/stream.c +33 -0
index 37bfd8b..8ba5a24 100644
@@ -33,6 +33,8 @@ typedef struct StreamBlockJob {
     BlockDriverState *base;
     BlockdevOnError on_error;
     char *backing_file_str;
+    int bs_flags;
+    Error *blocker;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -89,6 +91,13 @@ static void stream_complete(BlockJob *job, void *opaque)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common);
     StreamCompleteData *data = opaque;
     BlockDriverState *base = s->base;
+    BlockDriverState *bs;
+
+    /* Remove all blockers set in stream_start() */
+    for (bs = job->bs->backing_hd; bs && bs != s->base; bs = bs->backing_hd) {
+        bdrv_op_unblock_all(bs, s->blocker);
+    }
+    error_free(s->blocker);
 
     if (!block_job_is_cancelled(&s->common) && data->reached_end &&
         data->ret == 0) {
@@ -103,6 +112,11 @@ static void stream_complete(BlockJob *job, void *opaque)
         close_unused_images(job->bs, base, base_id);
     }
 
+    /* Reopen the image back in read-only mode if necessary */
+    if (s->bs_flags != bdrv_get_flags(job->bs)) {
+        bdrv_reopen(job->bs, s->bs_flags, NULL);
+    }
+
     g_free(s->backing_file_str);
     block_job_completed(&s->common, data->ret);
     g_free(data);
@@ -246,7 +260,9 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
                   BlockCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
+    BlockDriverState *iter;
     StreamBlockJob *s;
+    int orig_bs_flags;
 
     if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
          on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
@@ -255,13 +271,30 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
         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) {
+            return;
+        }
+    }
+
     s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
 
+    /* Block all intermediate nodes between bs and base, because they
+     * will disappear from the chain after this operation */
+    error_setg(&s->blocker, "blocked by the block-stream operation in '%s'",
+               bdrv_get_device_or_node_name(bs));
+    for (iter = bs->backing_hd; iter && iter != base; iter = iter->backing_hd) {
+        bdrv_op_block_all(iter, s->blocker);
+    }
+
     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);
Signature · 1 line
2.1.4
#16 lore raw

PATCH v4 7/8 qemu-iotests: fix test_stream_partial()

In reply to Alberto Garcia

This test is streaming to the top layer using the intermediate image as the base. This is a mistake since block-stream never copies data from the base image and its backing chain, so this is effectively a no-op.

In addition to fixing the base parameter, this patch also writes some data to the intermediate image before the test, so there's anything to copy and the test is meaningful.

---
 tests/qemu-iotests/030 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tests/qemu-iotests/030 +2 -1
index 7ca9b25..6e6cb5a 100755
@@ -35,6 +35,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         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.launch()
 
@@ -93,7 +94,7 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_stream_partial(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
+        result = self.vm.qmp('block-stream', device='drive0', base=backing_img)
         self.assert_qmp(result, 'return', {})
 
         self.wait_until_completed()
Signature · 1 line
2.1.4
Reviewed-by on v4 2/8
#4 lore raw

Re: [Qemu-devel] [PATCH 2/8] block: allow block jobs in any arbitrary node

In reply to Alberto Garcia
Quoted 16 lines · Alberto Garcia

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

Quoted 8 lines
---
  block/mirror.c            |  5 +++--
  blockdev.c                | 16 ++++++++--------
  blockjob.c                | 18 ++++++++++--------
  docs/qmp/qmp-events.txt   |  8 ++++----
  include/qapi/qmp/qerror.h |  3 ---
  qapi/block-core.json      | 20 ++++++++++----------
  6 files changed, 35 insertions(+), 35 deletions(-)
Reviewed-by on v4 4/8
#9 lore raw

Re: [Qemu-devel] [PATCH 4/8] block: Support streaming to an intermediate layer

In reply to Alberto Garcia
Quoted 6 lines · Alberto Garcia

This makes sure that the image we are steaming 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.

Quoted 7 lines

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

---
  block.c        |  4 +++-
  block/stream.c | 33 +++++++++++++++++++++++++++++++++
  2 files changed, 36 insertions(+), 1 deletion(-)
Reviewed-by on v4 5/8
#11 lore raw

Re: [Qemu-devel] [PATCH 5/8] block: Add QMP support for streaming to an intermediate layer

In reply to Alberto Garcia
Quoted 10 lines · Alberto Garcia

This patch makes the 'device' paramater of the 'block-stream' command accept a node name as well as a device name.

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

Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and no longer returns DeviceNotFound, iotest 030 is updated to expect GenericError instead.

Quoted 6 lines
---
  blockdev.c             | 20 ++++++++++----------
  qapi/block-core.json   | 10 +++++++---
  tests/qemu-iotests/030 |  2 +-
  3 files changed, 18 insertions(+), 14 deletions(-)
Reviewed-by on v4 7/8
#17 lore raw

Re: [Qemu-devel] [PATCH 7/8] qemu-iotests: fix test_stream_partial()

In reply to Alberto Garcia
Quoted 5 lines · Alberto Garcia

This test is streaming to the top layer using the intermediate image as the base. This is a mistake since block-stream never copies data from the base image and its backing chain, so this is effectively a no-op.

Quoted 8 lines

In addition to fixing the base parameter, this patch also writes some data to the intermediate image before the test, so there's anything to copy and the test is meaningful.

---
  tests/qemu-iotests/030 | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by on v4 8/8
#21 lore raw

Re: [Qemu-devel] [PATCH 8/8] qemu-iotests: test streaming to an intermediate layer

In reply to Alberto Garcia
Quoted 8 lines · Alberto Garcia

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

---
  tests/qemu-iotests/030     | 18 +++++++++++++++++-
  tests/qemu-iotests/030.out |  4 ++--
  2 files changed, 19 insertions(+), 3 deletions(-)
Reviewed-by on v4 6/8
#15 lore raw

Re: [Qemu-devel] [PATCH 6/8] docs: Document how to stream to an intermediate layer

In reply to Alberto Garcia
Quoted 10 lines · Alberto Garcia
---
 docs/live-block-ops.txt | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
docs/live-block-ops.txt +0 -0
index a257087..3bf86be 100644

docs/live-block-ops.txt Snapshot live merge · line 13

Quoted 5 lines
docs/live-block-ops.txt +1 -1
@@ -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]

I think on the list we've mostly settled on a notation where we express this relation as:

[A] <- [B]

that is, the symbol "<-" makes more sense as "serves as a backing file of" than "->", so that you end up reading the relationship as "[A] serves as a backing of [B]" (or read right-to-left as "[B] has a backing file of [A]").

Of course, the direction of the arrows is pre-existing, so it doesn't hold up this patch, but we probably ought to scrub our examples to consistently use <-.

Quoted 11 lines
docs/live-block-ops.txt +1 -2
-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,13 @@ 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]

docs/live-block-ops.txt The snapshot live merge operation transforms such a chain into a · line 30

Quoted 10 lines
docs/live-block-ops.txt +6 -3
+[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, for instance from [B] into [D]:
+
+[A] -> [B] -> [D] -> [E]

Should you really describe that as moving data from [B] into [D], or would it be better to describe it as moving the data from [C] into [D] such that [D] can now be backed by [B] instead of [C]?

I don't know if it is worth a tweak, and modulo my question on this one sentence, I think you've done a good job documenting the feature addition, so:

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#18 lore raw

Re: [Qemu-devel] [PATCH 7/8] qemu-iotests: fix test_stream_partial()

In reply to Alberto Garcia
Quoted 7 lines · Alberto Garcia

This test is streaming to the top layer using the intermediate image as the base. This is a mistake since block-stream never copies data from the base image and its backing chain, so this is effectively a no-op.

In addition to fixing the base parameter, this patch also writes some data to the intermediate image before the test, so there's anything to

s/anything/something/

Quoted 6 lines

copy and the test is meaningful.

---
 tests/qemu-iotests/030 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

You know, we should _also_ test a no-op stream, to prove that we handle it correctly (we've had bugs in the past where 0-length active commit behaved differently in the events it generated than non-zero length), and your change is dropping that.

tests/qemu-iotests/030 class TestSingleDrive(iotests.QMPTestCase): · line 97

Quoted 6 lines
tests/qemu-iotests/030 +1 -1
@@ -93,7 +94,7 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_stream_partial(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
+        result = self.vm.qmp('block-stream', device='drive0', base=backing_img)

So, I'd actually prefer to keep _both_ of these block-streams (of course, checking the result between both cases), or even add a second self.vm.qmp('block-stream', device='drive0', base=backing_img) after the first one (the first one streams data from mid into active and rebases active; the second one is a no-op), for maximal coverage of our behavior.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#19 lore raw

Re: [Qemu-devel] [PATCH 7/8] qemu-iotests: fix test_stream_partial()

Quoted 4 lines · Eric Blake

You know, we should _also_ test a no-op stream, to prove that we handle it correctly (we've had bugs in the past where 0-length active commit behaved differently in the events it generated than non-zero length), and your change is dropping that.

Well, we were checking that by accident :-)

But yes, I get the point. I can add that test if a further patch.

I also think that it would be a good idea to compare the actual contents (using qemu-img compare) rather than the list of allocated areas, which is what we're doing now.

But that would require a more substantial modification of the test case.

Berto

#22 lore raw

Re: [Qemu-devel] [PATCH 8/8] qemu-iotests: test streaming to an intermediate layer

In reply to Alberto Garcia
Quoted 9 lines · Alberto Garcia

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

---
 tests/qemu-iotests/030     | 18 +++++++++++++++++-
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 19 insertions(+), 3 deletions(-)

I'm wondering if we should also test a multi-file stream. That is, starting from:

A <- B <- C <- D <- E

and rebasing so that A is the backing file of D (pulling both B and C into D):

A <- D <- E

we should be able to verify that B and C are unchanged, and that D picked up changes from both intermediate files (in particular, it's nice to test that we aren't being inefficient by breaking the operation into two steps: streaming B into C then C into D would give the same end result in D, but would waste time for any clusters in B that were not changed in C but were changed in D).

But such a test could be a separate patch, so I don't see anything wrong with this new test as-is.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Reviewed-by on v4 1/8
#2 lore raw

Re: [Qemu-devel] [PATCH 1/8] block: keep a list of block jobs

In reply to Alberto Garcia
Quoted 5 lines · Alberto Garcia

The current way to obtain the list of existing block jobs is to iterate over all root nodes and check which ones own a job.

Since we want to be able to support block jobs in other nodes as well, this patch keeps a list of jobs that is updated everytime one is

s/everytime/every time/

Quoted 4 lines

created or destroyed.

This also updates qmp_query_block_jobs() to use this new list.

Quoted 8 lines
---
 blockdev.c               | 19 ++++++++-----------
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 14 ++++++++++++++
 3 files changed, 35 insertions(+), 11 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Reviewed-by on v4 2/8
#5 lore raw

Re: [Qemu-devel] [PATCH 2/8] block: allow block jobs in any arbitrary node

In reply to Alberto Garcia
Quoted 28 lines · Alberto Garcia

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

---
 block/mirror.c            |  5 +++--
 blockdev.c                | 16 ++++++++--------
 blockjob.c                | 18 ++++++++++--------
 docs/qmp/qmp-events.txt   |  8 ++++----
 include/qapi/qmp/qerror.h |  3 ---
 qapi/block-core.json      | 20 ++++++++++----------
 6 files changed, 35 insertions(+), 35 deletions(-)

+++ b/docs/qmp/qmp-events.txt

docs/qmp/qmp-events.txt Data: · line 92

Quoted 5 lines
docs/qmp/qmp-events.txt +1 -1
@@ -89,7 +89,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Device name, or node name if not present (json-string)

You used this wording for all the events...

qapi/block-core.json line 544

Quoted 8 lines
qapi/block-core.json +2 -1
+++ b/qapi/block-core.json
@@ -540,7 +540,7 @@
 #
 # @type: the job type ('stream' for image streaming)
 #
-# @device: the block device name
+# @device: the block device name, or node name if not present
 #

and again here,

qapi/block-core.json line 1068

Quoted 8 lines
qapi/block-core.json +1 -1
 # @len: the maximum progress value
 #
@@ -1065,7 +1065,7 @@
 #
 # Throttling can be disabled by setting the speed to 0.
 #
-# @device: the device name
+# @device: the device or node name of the owner of the block job.

but this wording is different. I like it a bit better ("node name if not present" feels like it could be a bit ambiguous as to what is not present, and "device or node name" is shorter than "device name, or node name if not present").

I'm not sure if it's worth changing, and I could live with the patch as-is, so:

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Reviewed-by on v4 5/8
#12 lore raw

Re: [Qemu-devel] [PATCH 5/8] block: Add QMP support for streaming to an intermediate layer

In reply to Alberto Garcia
Quoted 1 line · Alberto Garcia

This patch makes the 'device' paramater of the 'block-stream' command

s/paramater/parameter/

Quoted 9 lines

accept a node name as well as a device name.

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

Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and no longer returns DeviceNotFound, iotest 030 is updated to expect GenericError instead.

Quoted 7 lines
---
 blockdev.c             | 20 ++++++++++----------
 qapi/block-core.json   | 10 +++++++---
 tests/qemu-iotests/030 |  2 +-
 3 files changed, 18 insertions(+), 14 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#6 lore raw

Re: [Qemu-devel] [PATCH 2/8] block: allow block jobs in any arbitrary node

In reply to Eric Blake
Quoted 5 lines · Eric Blake
Quoted 2 lines

-- "device": Device name (json-string)
+- "device": Device name, or node name if not present (json-string)

You used this wording for all the events...

Quoted 9 lines

Review

Quoted 4 lines

# Throttling can be disabled by setting the speed to 0. #

(quoted diff) +1 -1
-# @device: the device name
+# @device: the device or node name of the owner of the block job.

but this wording is different. I like it a bit better ("node name if not present" feels like it could be a bit ambiguous as to what is not present, and "device or node name" is shorter than "device name, or node name if not present").

There's a reason for that.

I used "Device name, or node name if not present" in return types, meaning that the field will always contain the device name first, and fall back to the node name only if the device name is not present. I think we should guarantee that the device name has precedence for compatibility reasons, and I want to make that explicit in the documentation.

I used "the device or node name of..." in command parameters, meaning that both are accepted and equally valid, and that the user can choose which one to pass.

Berto