[Qemu-devel] [PATCH v3 0/6] Support streaming to an intermediate layer

[Qemu-devel] [PATCH v3 0/6] Support streaming to an intermediate layer

[Alberto Garcia]

There's only one change in this series: there's now a list of block
jobs that is updated everytime a new one is created or destroyed.

This way we can iterate the list directly, rather than searching all
BlockDriverStates for the ones that own a block job.

Otherwise please check the summary I wrote for the previous series,
because the rest of the changes are detailed there:

   https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html

v3:
- 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 (6):
  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

 block.c                   |  4 +++-
 block/mirror.c            |  5 +++--
 block/stream.c            | 47 ++++++++++++++++++++++++++++++++++++-----
 blockdev.c                | 53 ++++++++++++++++++++++-------------------------
 blockjob.c                | 30 ++++++++++++++++++++-------
 docs/live-block-ops.txt   | 32 +++++++++++++++++-----------
 docs/qmp/qmp-events.txt   |  8 +++----
 include/block/blockjob.h  | 14 +++++++++++++
 include/qapi/qmp/qerror.h |  3 ---
 qapi/block-core.json      | 28 ++++++++++++++-----------
 10 files changed, 149 insertions(+), 75 deletions(-)

-- 
2.1.4

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

[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.

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

diff --git a/blockdev.c b/blockdev.c
index 30dc9d2..567d5e3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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;
diff --git a/blockjob.c b/blockjob.c
index ba2255d..562362b 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -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);
 }
 
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index b6d4ebb..636204b 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -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
-- 
2.1.4

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

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> 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.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   blockdev.c               | 19 ++++++++-----------
>   blockjob.c               | 13 +++++++++++++
>   include/block/blockjob.h | 14 ++++++++++++++
>   3 files changed, 35 insertions(+), 11 deletions(-)

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

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

[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.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c            |  5 +++--
 blockdev.c                | 14 +++++++-------
 blockjob.c                | 17 +++++++++--------
 docs/qmp/qmp-events.txt   |  8 ++++----
 include/qapi/qmp/qerror.h |  3 ---
 qapi/block-core.json      | 18 +++++++++---------
 6 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 4056164..189e8f8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -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;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 567d5e3..dc5d931 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2643,18 +2643,18 @@ 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, NULL);
+    if (!bs) {
         goto notfound;
     }
-    bs = blk_bs(blk);
 
     *aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(*aio_context);
@@ -2668,7 +2668,7 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
 notfound:
     error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
-              "No active block job on device '%s'", device);
+              "No active block job on node '%s'", device_or_node);
     *aio_context = NULL;
     return NULL;
 }
diff --git a/blockjob.c b/blockjob.c
index 562362b..b2b6cc9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -52,7 +52,7 @@ 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_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_or_node_name(bs));
         return NULL;
     }
     bdrv_ref(bs);
@@ -121,8 +121,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 +269,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 +291,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 +301,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 +315,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 +344,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);
diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index b19e490..db90c61 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -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.
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index e567339..4ba442e 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -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"
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 21e6cb5..3639454 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -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
 #
-- 
2.1.4

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

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> 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.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/mirror.c            |  5 +++--
>   blockdev.c                | 14 +++++++-------
>   blockjob.c                | 17 +++++++++--------
>   docs/qmp/qmp-events.txt   |  8 ++++----
>   include/qapi/qmp/qerror.h |  3 ---
>   qapi/block-core.json      | 18 +++++++++---------
>   6 files changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 4056164..189e8f8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -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));

I know there is no way right now that bdrv_get_device_name() returns an 
empty string, but somehow I'd feel more consistent for this to be 
bdrv_get_device_or_node_name() and the string saying "node" instead of 
"device". Your choice, though, since it's completely correct for now.

>           return;
>       }
>   
> diff --git a/blockdev.c b/blockdev.c
> index 567d5e3..dc5d931 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2643,18 +2643,18 @@ 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, NULL);
> +    if (!bs) {
>           goto notfound;

It would be possible to pass errp to bdrv_lookup_bs() and move the 
error_set() under notfound to the if (!bs->job) block (I'd find the 
error message confusing if there just is no node with that name; but on 
the other hand, it wouldn't be a regression).

>       }
> -    bs = blk_bs(blk);
>   
>       *aio_context = bdrv_get_aio_context(bs);
>       aio_context_acquire(*aio_context);
> @@ -2668,7 +2668,7 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
>   
>   notfound:
>       error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> -              "No active block job on device '%s'", device);
> +              "No active block job on node '%s'", device_or_node);
>       *aio_context = NULL;
>       return NULL;
>   }
> diff --git a/blockjob.c b/blockjob.c
> index 562362b..b2b6cc9 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -52,7 +52,7 @@ 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_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_or_node_name(bs));

Hm, the error message only mentions "device" now... Not too nice.

>           return NULL;
>       }
>       bdrv_ref(bs);
> @@ -121,8 +121,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 +269,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 +291,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 +301,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 +315,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 +344,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);
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index b19e490..db90c61 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -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.
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index e567339..4ba442e 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -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"
>   
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 21e6cb5..3639454 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -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
>   #

I think you need to apply the same treatment for the comment of 
BlockJobInfo, too.

That last thing is the only thing preventing me from giving an R-b, the 
rest is optional.

Max

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

[Alberto Garcia]

On Wed 15 Apr 2015 05:22:13 PM CEST, Max Reitz <mreitz@redhat.com> wrote:

>> diff --git a/block/mirror.c b/block/mirror.c
>> index 4056164..189e8f8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -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));
>
> I know there is no way right now that bdrv_get_device_name() returns
> an empty string, but somehow I'd feel more consistent for this to be
> bdrv_get_device_or_node_name() and the string saying "node" instead of
> "device". Your choice, though, since it's completely correct for now.

Yeah, I decided to leave it like that while the mirror operation can
only work on root nodes. In general in all these patches I'm only using
bdrv_get_device_or_node_name() in the cases where it's actually possible
that the device name is empty.

>> +/* 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, NULL);
>> +    if (!bs) {
>>           goto notfound;
>
> It would be possible to pass errp to bdrv_lookup_bs() and move the
> error_set() under notfound to the if (!bs->job) block (I'd find the
> error message confusing if there just is no node with that name; but
> on the other hand, it wouldn't be a regression).

Sounds reasonable, I'll change that.

>> diff --git a/blockjob.c b/blockjob.c
>> index 562362b..b2b6cc9 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -52,7 +52,7 @@ 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_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_or_node_name(bs));
>
> Hm, the error message only mentions "device" now... Not too nice.

Errr... I overlooked that, I'll fix it.

>> @@ -1895,7 +1895,7 @@
>>   #
>>   # @type: job type
>>   #
>> -# @device: device name
>> +# @device: device name, or node name if not present
>>   #
>>   # @len: maximum progress value
>>   #
>
> I think you need to apply the same treatment for the comment of 
> BlockJobInfo, too.

You're right again :)

Berto

[Qemu-devel] [PATCH 3/6] block: never cancel a streaming job without running stream_complete()

[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.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/stream.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index a628901..37bfd8b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -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;
-- 
2.1.4

Re: [Qemu-devel] [Qemu-block] [PATCH 3/6] block: never cancel a streaming job without running stream_complete()

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> 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,

Which is important enough on its own.

> but it
> will become more important as we introduce support for streaming to
> any intermediate node.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/stream.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)

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

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

[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.

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

diff --git a/block.c b/block.c
index 25289ef..e892cb4 100644
--- a/block.c
+++ b/block.c
@@ -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);
 }
diff --git a/block/stream.c b/block/stream.c
index 37bfd8b..327d964 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -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,
@@ -88,8 +90,15 @@ static void stream_complete(BlockJob *job, void *opaque)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common);
     StreamCompleteData *data = opaque;
+    BlockDriverState *i;
     BlockDriverState *base = s->base;
 
+    /* Remove all blockers set in stream_start() */
+    for (i = job->bs->backing_hd; i && i != s->base; i = i->backing_hd) {
+        bdrv_op_unblock_all(i, s->blocker);
+    }
+    error_free(s->blocker);
+
     if (!block_job_is_cancelled(&s->common) && data->reached_end &&
         data->ret == 0) {
         const char *base_id = NULL, *base_fmt = NULL;
@@ -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 *i;
     StreamBlockJob *s;
+    int orig_bs_flags;
 
     if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
          on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
@@ -255,13 +271,33 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
+    /* Make sure that the image in opened in read-write mode */
+    orig_bs_flags = bdrv_get_flags(bs);
+    if (!(orig_bs_flags & BDRV_O_RDWR)) {
+        Error *local_err = NULL;
+        bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            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_node_name(bs));
+    for (i = bs->backing_hd; i != base && i != NULL; i = i->backing_hd) {
+        bdrv_op_block_all(i, 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);
-- 
2.1.4

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

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> 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.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block.c        |  4 +++-
>   block/stream.c | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 25289ef..e892cb4 100644
> --- a/block.c
> +++ b/block.c
> @@ -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);
>   }
> diff --git a/block/stream.c b/block/stream.c
> index 37bfd8b..327d964 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -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,
> @@ -88,8 +90,15 @@ static void stream_complete(BlockJob *job, void *opaque)
>   {
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common);
>       StreamCompleteData *data = opaque;
> +    BlockDriverState *i;

I'd prefer another name. "i" is generally used for integers used as indices.

>       BlockDriverState *base = s->base;
>   
> +    /* Remove all blockers set in stream_start() */
> +    for (i = job->bs->backing_hd; i && i != s->base; i = i->backing_hd) {
> +        bdrv_op_unblock_all(i, s->blocker);
> +    }
> +    error_free(s->blocker);
> +
>       if (!block_job_is_cancelled(&s->common) && data->reached_end &&
>           data->ret == 0) {
>           const char *base_id = NULL, *base_fmt = NULL;
> @@ -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 *i;

Again, just "i" is a bit strange to read...

>       StreamBlockJob *s;
> +    int orig_bs_flags;
>   
>       if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
>            on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
> @@ -255,13 +271,33 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
>           return;
>       }
>   
> +    /* Make sure that the image in opened in read-write mode */

s/in/is/

> +    orig_bs_flags = bdrv_get_flags(bs);
> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {

I feel like we don't want to do this if we're not streaming to an 
intermediate layer but to the top layer (because that means there is 
some reason for the BDS to be read-only beyond it just being a backing BDS).

> +        Error *local_err = NULL;
> +        bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);

Shorter alternative: "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 */

Hm, do we really need to? There's a difference between "it doesn't make 
sense, but it works if you want to" and "it will break". Shouldn't it be 
enough that the intermediate nodes are all read-only anyway (hopefully)?

But then again, it probably won't hurt and I don't really want to think 
about the implications of trying to run a block-commit or a separate 
block-stream on the chain...

> +    error_setg(&s->blocker, "blocked by the block-stream operation in '%s'",
> +               bdrv_get_node_name(bs));

Why not bdrv_get_device_or_node_name()?

> +    for (i = bs->backing_hd; i != base && i != NULL; i = i->backing_hd) {
> +        bdrv_op_block_all(i, 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);

So, in general this patch looks OK to me. I had some nitpicks 
(..._device_or_node_name, "i", ...), and I think we should not try to 
re-open the target BDS if streaming to the top level.

Max

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

[Alberto Garcia]

On Wed 15 Apr 2015 06:09:18 PM CEST, Max Reitz wrote:

>> +    orig_bs_flags = bdrv_get_flags(bs);
>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
>
> I feel like we don't want to do this if we're not streaming to an
> intermediate layer but to the top layer (because that means there is
> some reason for the BDS to be read-only beyond it just being a backing
> BDS).

I didn't think about this... that I can fix easily, but I wonder what's
the scenario where the top layer is read-only.

>> +    /* Block all intermediate nodes between bs and base, because they
>> +     * will disappear from the chain after this operation */
>
> Hm, do we really need to? There's a difference between "it doesn't
> make sense, but it works if you want to" and "it will
> break". Shouldn't it be enough that the intermediate nodes are all
> read-only anyway (hopefully)?

I don't think that the fact that intermediate nodes are read-only
changes anything, because some operations (this one, block-commit)
reopens them in r/w mode.

> But then again, it probably won't hurt and I don't really want to
> think about the implications of trying to run a block-commit or a
> separate block-stream on the chain...

I actually tried with scenarios such as A>B>C>D>E, streaming from B to E
and from A to C simultaneously, and it seems to work (at least I didn't
see any obvious problems), but I don't think we want to support that
because a) I don't see the use case and b) we're likely opening a can of
worms.

What we can do is stream from A to C and from D to E at the same time,
that would be allowed with these patches and I don't see any obvious
reason why it would fail.

Berto

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

[Eric Blake]

On 04/16/2015 03:36 AM, Alberto Garcia wrote:
> On Wed 15 Apr 2015 06:09:18 PM CEST, Max Reitz wrote:
> 
>>> +    orig_bs_flags = bdrv_get_flags(bs);
>>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
>>
>> I feel like we don't want to do this if we're not streaming to an
>> intermediate layer but to the top layer (because that means there is
>> some reason for the BDS to be read-only beyond it just being a backing
>> BDS).
> 
> I didn't think about this... that I can fix easily, but I wonder what's
> the scenario where the top layer is read-only.

Suppose I tell qemu to open the chain 'base <- mid <- top' read-only,
and then later decide to stream base into mid or top. It should be fine
from the guest's perspective to keep the guest in read-only mode, while
still using streaming to reduce the chain.


> 
>> But then again, it probably won't hurt and I don't really want to
>> think about the implications of trying to run a block-commit or a
>> separate block-stream on the chain...
> 
> I actually tried with scenarios such as A>B>C>D>E, streaming from B to E
> and from A to C simultaneously, and it seems to work (at least I didn't
> see any obvious problems), but I don't think we want to support that
> because a) I don't see the use case and b) we're likely opening a can of
> worms.

Streaming doesn't corrupt any part of the chain, so I think you are safe
doing overlapping streams like that.  However, I also agree that it is
not worth supporting, and that we are safer not allowing overlapping
streams.

> 
> What we can do is stream from A to C and from D to E at the same time,
> that would be allowed with these patches and I don't see any obvious
> reason why it would fail.

Correct - that's not overlapping, so it should not be forbidden, as long
as parallel jobs are working well.

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

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

[Alberto Garcia]

On Thu 16 Apr 2015 02:27:39 PM CEST, Eric Blake wrote:

>>>> +    orig_bs_flags = bdrv_get_flags(bs);
>>>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
>>>
>>> I feel like we don't want to do this if we're not streaming to an
>>> intermediate layer but to the top layer (because that means there is
>>> some reason for the BDS to be read-only beyond it just being a
>>> backing BDS).
>> 
>> I didn't think about this... that I can fix easily, but I wonder
>> what's the scenario where the top layer is read-only.
>
> Suppose I tell qemu to open the chain 'base <- mid <- top' read-only,
> and then later decide to stream base into mid or top. It should be
> fine from the guest's perspective to keep the guest in read-only mode,
> while still using streaming to reduce the chain.

Ok, I'll add the check for the top image then.

>> I actually tried with scenarios such as A>B>C>D>E, streaming from B
>> to E and from A to C simultaneously, and it seems to work (at least I
>> didn't see any obvious problems), but I don't think we want to
>> support that because a) I don't see the use case and b) we're likely
>> opening a can of worms.
>
> Streaming doesn't corrupt any part of the chain, so I think you are
> safe doing overlapping streams like that.  However, I also agree that
> it is not worth supporting, and that we are safer not allowing
> overlapping streams.

I'm thinking about cases where streaming from B to E finishes (and all
the intermediate nodes are removed from the chain) while A to C is still
ongoing. At the very least it doesn't seem to make sense to allow that
kind of things.

Berto

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

[Kevin Wolf]

Am 16.04.2015 um 14:34 hat Alberto Garcia geschrieben:
> On Thu 16 Apr 2015 02:27:39 PM CEST, Eric Blake wrote:
> 
> >>>> +    orig_bs_flags = bdrv_get_flags(bs);
> >>>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
> >>>
> >>> I feel like we don't want to do this if we're not streaming to an
> >>> intermediate layer but to the top layer (because that means there is
> >>> some reason for the BDS to be read-only beyond it just being a
> >>> backing BDS).
> >> 
> >> I didn't think about this... that I can fix easily, but I wonder
> >> what's the scenario where the top layer is read-only.
> >
> > Suppose I tell qemu to open the chain 'base <- mid <- top' read-only,
> > and then later decide to stream base into mid or top. It should be
> > fine from the guest's perspective to keep the guest in read-only mode,
> > while still using streaming to reduce the chain.
> 
> Ok, I'll add the check for the top image then.
> 
> >> I actually tried with scenarios such as A>B>C>D>E, streaming from B
> >> to E and from A to C simultaneously, and it seems to work (at least I
> >> didn't see any obvious problems), but I don't think we want to
> >> support that because a) I don't see the use case and b) we're likely
> >> opening a can of worms.
> >
> > Streaming doesn't corrupt any part of the chain, so I think you are
> > safe doing overlapping streams like that.  However, I also agree that
> > it is not worth supporting, and that we are safer not allowing
> > overlapping streams.
> 
> I'm thinking about cases where streaming from B to E finishes (and all
> the intermediate nodes are removed from the chain) while A to C is still
> ongoing. At the very least it doesn't seem to make sense to allow that
> kind of things.

Let's stay conservative with what we allow as long as we don't have the
new op blockers yet. As soon as we have them, it might be a lot easier
and less error prone to accurately describe the real requirements.

Kevin

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

[Alberto Garcia]

On Wed 15 Apr 2015 06:09:18 PM CEST, Max Reitz <mreitz@redhat.com> wrote:

>> +    orig_bs_flags = bdrv_get_flags(bs);
>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
>
> I feel like we don't want to do this if we're not streaming to an
> intermediate layer but to the top layer (because that means there is
> some reason for the BDS to be read-only beyond it just being a backing
> BDS).

Looks like we don't actually need to do anything, bdrv_reopen_prepare()
already takes care of checking the BDRV_O_ALLOW_RDWR flag, which is not
set if the image was opened in read-only mode.

Berto

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

[Max Reitz]

On 16.04.2015 16:30, Alberto Garcia wrote:
> On Wed 15 Apr 2015 06:09:18 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>
>>> +    orig_bs_flags = bdrv_get_flags(bs);
>>> +    if (!(orig_bs_flags & BDRV_O_RDWR)) {
>> I feel like we don't want to do this if we're not streaming to an
>> intermediate layer but to the top layer (because that means there is
>> some reason for the BDS to be read-only beyond it just being a backing
>> BDS).
> Looks like we don't actually need to do anything, bdrv_reopen_prepare()
> already takes care of checking the BDRV_O_ALLOW_RDWR flag, which is not
> set if the image was opened in read-only mode.

Oh, that's nice. :-)

Max

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

[Alberto Garcia]

This patch makes the 'device' paramater of the 'block-stream' command
allow 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.

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

diff --git a/blockdev.c b/blockdev.c
index dc5d931..f24cf2d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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, *i;
     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 (i = bs; i != NULL && i != base_bs; i = i->backing_hd) {
+        if (bdrv_op_is_blocked(i, 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 3639454..60b9664 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -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
-- 
2.1.4

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

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> This patch makes the 'device' paramater of the 'block-stream' command
> allow a node name as well as a device name.

s/allow/accept/

> 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           | 20 ++++++++++----------
>   qapi/block-core.json | 10 +++++++---
>   2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index dc5d931..f24cf2d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -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, *i;

Hmmmm... ;-)

Looks good otherwise.

Max

>       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 (i = bs; i != NULL && i != base_bs; i = i->backing_hd) {
> +        if (bdrv_op_is_blocked(i, 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 3639454..60b9664 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -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

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

[Alberto Garcia]

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

diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
index a257087..5e969fd 100644
--- a/docs/live-block-ops.txt
+++ b/docs/live-block-ops.txt
@@ -10,9 +10,9 @@ Snapshot live merge
 Given a snapshot chain, described in this document in the following
 format:
 
-[A] -> [B] -> [C] -> [D]
+[A] -> [B] -> [C] -> [D] -> [E]
 
-Where the rightmost object ([D] in the example) described is the current
+Where the rightmost object ([E] in the example) described is the current
 image which the guest OS has write access to. To the left of it is its base
 image, and so on accordingly until the leftmost image, which has no
 base.
@@ -21,28 +21,36 @@ 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, [D] in this example:
+
+[A] -> [B] -> [D] -> [E]
 
 The operation is implemented in QEMU through image streaming facilities.
 
 The basic idea is to execute 'block_stream virtio0' while the guest is
 running. Progress can be monitored using 'info block-jobs'. When the
 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.
+copies data from the backing file(s) into the destination 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
 
+In order to specify a destination image different from the active
+(rightmost) one we can use its (previously set) node name instead.
+
+In the second example above, the command would be:
+
+(qemu) block_stream node-D A
 
 Live block copy
 ===============
-- 
2.1.4

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

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   docs/live-block-ops.txt | 32 ++++++++++++++++++++------------
>   1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
> index a257087..5e969fd 100644
> --- a/docs/live-block-ops.txt
> +++ b/docs/live-block-ops.txt
> @@ -10,9 +10,9 @@ Snapshot live merge
>   Given a snapshot chain, described in this document in the following
>   format:
>   
> -[A] -> [B] -> [C] -> [D]
> +[A] -> [B] -> [C] -> [D] -> [E]
>   
> -Where the rightmost object ([D] in the example) described is the current
> +Where the rightmost object ([E] in the example) described is the current
>   image which the guest OS has write access to. To the left of it is its base
>   image, and so on accordingly until the leftmost image, which has no
>   base.
> @@ -21,28 +21,36 @@ 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, [D] in this example:

"for instance from [B] into/to [D]"?

> +
> +[A] -> [B] -> [D] -> [E]
>   
>   The operation is implemented in QEMU through image streaming facilities.
>   
>   The basic idea is to execute 'block_stream virtio0' while the guest is
>   running. Progress can be monitored using 'info block-jobs'. When the
>   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.
> +copies data from the backing file(s) into the destination image.
> +When finished, it adjusts the backing file pointer.

Well, the example given here ("block_stream virtio0") would actually 
copy the data into the active image. It's not that "destination image" 
is wrong, but in this case, it really is the active image, so I don't 
think this change is necessary.

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

Here, the change is fine, because this paragraph is detached from the 
example (which does not make use of the @base parameter).

>   
> -In the example above, the command would be:
> +In the first example above, the command would be:
>   
>   (qemu) block_stream virtio0 A

I'd like it if you could change the "A" to "A-filename", "filename-A", 
or something like that, to make clear it's a filename and not a node 
name. (I'd personally expect a node name, but I guess block-stream is 
simply older than node names are).

> +In order to specify a destination image different from the active
> +(rightmost) one we can use its (previously set) node name instead.
> +
> +In the second example above, the command would be:
> +
> +(qemu) block_stream node-D A

Hm, wouldn't this yield the chain "A -> D -> E" instead of "A -> B -> D 
-> E"?

Max

>   
>   Live block copy
>   ===============

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/6] Support streaming to an intermediate layer

[Max Reitz]

On 08.04.2015 16:43, Alberto Garcia wrote:
> There's only one change in this series: there's now a list of block
> jobs that is updated everytime a new one is created or destroyed.
>
> This way we can iterate the list directly, rather than searching all
> BlockDriverStates for the ones that own a block job.
>
> Otherwise please check the summary I wrote for the previous series,
> because the rest of the changes are detailed there:
>
>     https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html
>
> v3:
> - 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 (6):
>    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
>
>   block.c                   |  4 +++-
>   block/mirror.c            |  5 +++--
>   block/stream.c            | 47 ++++++++++++++++++++++++++++++++++++-----
>   blockdev.c                | 53 ++++++++++++++++++++++-------------------------
>   blockjob.c                | 30 ++++++++++++++++++++-------
>   docs/live-block-ops.txt   | 32 +++++++++++++++++-----------
>   docs/qmp/qmp-events.txt   |  8 +++----
>   include/block/blockjob.h  | 14 +++++++++++++
>   include/qapi/qmp/qerror.h |  3 ---
>   qapi/block-core.json      | 28 ++++++++++++++-----------
>   10 files changed, 149 insertions(+), 75 deletions(-)

The concept of the series looks good to me. Would you mind adding some 
test cases, though?

Max