* [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:11 ` Fam Zheng
2015-05-18 16:39 ` Max Reitz
2015-05-13 13:27 ` [Qemu-devel] [PATCH 02/11] block: allow block jobs in any arbitrary node Alberto Garcia
` (11 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz
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 every time one is
created or destroyed.
This also updates qmp_query_block_jobs() and bdrv_drain_all() to use
this new list.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
---
block/io.c | 21 ++++++++-------------
blockdev.c | 19 ++++++++-----------
blockjob.c | 13 +++++++++++++
include/block/blockjob.h | 14 ++++++++++++++
4 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/block/io.c b/block/io.c
index 1ce62c4..c38d776 100644
--- a/block/io.c
+++ b/block/io.c
@@ -310,21 +310,19 @@ void bdrv_drain_all(void)
{
/* Always run first iteration so any pending completion BHs run */
bool busy = true;
- BlockDriverState *bs = NULL;
+ BlockJob *job = NULL;
- while ((bs = bdrv_next(bs))) {
- AioContext *aio_context = bdrv_get_aio_context(bs);
+ while ((job = block_job_next(job))) {
+ AioContext *aio_context = bdrv_get_aio_context(job->bs);
aio_context_acquire(aio_context);
- if (bs->job) {
- block_job_pause(bs->job);
- }
+ block_job_pause(job);
aio_context_release(aio_context);
}
while (busy) {
+ BlockDriverState *bs = NULL;
busy = false;
- bs = NULL;
while ((bs = bdrv_next(bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -335,14 +333,11 @@ void bdrv_drain_all(void)
}
}
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- AioContext *aio_context = bdrv_get_aio_context(bs);
+ while ((job = block_job_next(job))) {
+ AioContext *aio_context = bdrv_get_aio_context(job->bs);
aio_context_acquire(aio_context);
- if (bs->job) {
- block_job_resume(bs->job);
- }
+ block_job_resume(job);
aio_context_release(aio_context);
}
}
diff --git a/blockdev.c b/blockdev.c
index 5eaf77e..bf36a0e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3073,21 +3073,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 2755465..c46984d 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 57d8ef1..5431dd2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -102,6 +102,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;
@@ -125,6 +128,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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs
2015-05-13 13:27 ` [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs Alberto Garcia
@ 2015-05-15 2:11 ` Fam Zheng
2015-05-18 16:39 ` Max Reitz
1 sibling, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:11 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
On Wed, 05/13 16:27, 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 every time one is
> created or destroyed.
>
> This also updates qmp_query_block_jobs() and bdrv_drain_all() to use
> this new list.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> block/io.c | 21 ++++++++-------------
> blockdev.c | 19 ++++++++-----------
> blockjob.c | 13 +++++++++++++
> include/block/blockjob.h | 14 ++++++++++++++
> 4 files changed, 43 insertions(+), 24 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 1ce62c4..c38d776 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -310,21 +310,19 @@ void bdrv_drain_all(void)
> {
> /* Always run first iteration so any pending completion BHs run */
> bool busy = true;
> - BlockDriverState *bs = NULL;
> + BlockJob *job = NULL;
>
> - while ((bs = bdrv_next(bs))) {
> - AioContext *aio_context = bdrv_get_aio_context(bs);
> + while ((job = block_job_next(job))) {
> + AioContext *aio_context = bdrv_get_aio_context(job->bs);
>
> aio_context_acquire(aio_context);
> - if (bs->job) {
> - block_job_pause(bs->job);
> - }
> + block_job_pause(job);
> aio_context_release(aio_context);
> }
>
> while (busy) {
> + BlockDriverState *bs = NULL;
> busy = false;
> - bs = NULL;
>
> while ((bs = bdrv_next(bs))) {
> AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -335,14 +333,11 @@ void bdrv_drain_all(void)
> }
> }
>
> - bs = NULL;
> - while ((bs = bdrv_next(bs))) {
> - AioContext *aio_context = bdrv_get_aio_context(bs);
> + while ((job = block_job_next(job))) {
> + AioContext *aio_context = bdrv_get_aio_context(job->bs);
>
> aio_context_acquire(aio_context);
> - if (bs->job) {
> - block_job_resume(bs->job);
> - }
> + block_job_resume(job);
> aio_context_release(aio_context);
> }
> }
> diff --git a/blockdev.c b/blockdev.c
> index 5eaf77e..bf36a0e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3073,21 +3073,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 2755465..c46984d 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 57d8ef1..5431dd2 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -102,6 +102,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;
>
> @@ -125,6 +128,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
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs
2015-05-13 13:27 ` [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs Alberto Garcia
2015-05-15 2:11 ` Fam Zheng
@ 2015-05-18 16:39 ` Max Reitz
1 sibling, 0 replies; 39+ messages in thread
From: Max Reitz @ 2015-05-18 16:39 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: qemu-block
On 13.05.2015 15:27, 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 every time one is
> created or destroyed.
>
> This also updates qmp_query_block_jobs() and bdrv_drain_all() to use
> this new list.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> block/io.c | 21 ++++++++-------------
> blockdev.c | 19 ++++++++-----------
> blockjob.c | 13 +++++++++++++
> include/block/blockjob.h | 14 ++++++++++++++
> 4 files changed, 43 insertions(+), 24 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 02/11] block: allow block jobs in any arbitrary node
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
2015-05-13 13:27 ` [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:19 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 03/11] block: never cancel a streaming job without running stream_complete() Alberto Garcia
` (10 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
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(-)
diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..4b36e0b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -598,8 +598,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 bf36a0e..1fcf466 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2813,32 +2813,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;
}
diff --git a/blockjob.c b/blockjob.c
index c46984d..96471c6 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -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->pause_count || 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;
}
@@ -277,7 +279,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->pause_count > 0;
@@ -299,7 +301,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,
@@ -309,7 +311,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,
@@ -323,7 +325,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);
@@ -352,7 +354,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 6dc2cca..791f9dd 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -90,7 +90,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.
@@ -114,7 +114,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.
@@ -141,7 +141,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
@@ -165,7 +165,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 863ffea..9b5cddd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -547,7 +547,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
#
@@ -1146,7 +1146,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.
@@ -1177,7 +1177,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.
@@ -1203,7 +1203,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
@@ -1223,7 +1223,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
@@ -1249,7 +1249,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
@@ -1903,7 +1903,7 @@
#
# @type: job type
#
-# @device: device name
+# @device: device name, or node name if not present
#
# @len: maximum progress value
#
@@ -1934,7 +1934,7 @@
#
# @type: job type
#
-# @device: device name
+# @device: device name, or node name if not present
#
# @len: maximum progress value
#
@@ -1957,7 +1957,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
#
@@ -1977,7 +1977,7 @@
#
# @type: job type
#
-# @device: device name
+# @device: device name, or node name if not present
#
# @len: maximum progress value
#
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 02/11] block: allow block jobs in any arbitrary node
2015-05-13 13:27 ` [Qemu-devel] [PATCH 02/11] block: allow block jobs in any arbitrary node Alberto Garcia
@ 2015-05-15 2:19 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:19 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> 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(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 58f391a..4b36e0b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -598,8 +598,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 bf36a0e..1fcf466 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2813,32 +2813,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;
> }
> diff --git a/blockjob.c b/blockjob.c
> index c46984d..96471c6 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -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->pause_count || 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;
> }
>
> @@ -277,7 +279,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->pause_count > 0;
> @@ -299,7 +301,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,
> @@ -309,7 +311,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,
> @@ -323,7 +325,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);
> @@ -352,7 +354,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 6dc2cca..791f9dd 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -90,7 +90,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.
> @@ -114,7 +114,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.
> @@ -141,7 +141,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
> @@ -165,7 +165,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 863ffea..9b5cddd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -547,7 +547,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
> #
> @@ -1146,7 +1146,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.
> @@ -1177,7 +1177,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.
> @@ -1203,7 +1203,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
> @@ -1223,7 +1223,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
> @@ -1249,7 +1249,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
> @@ -1903,7 +1903,7 @@
> #
> # @type: job type
> #
> -# @device: device name
> +# @device: device name, or node name if not present
> #
> # @len: maximum progress value
> #
> @@ -1934,7 +1934,7 @@
> #
> # @type: job type
> #
> -# @device: device name
> +# @device: device name, or node name if not present
> #
> # @len: maximum progress value
> #
> @@ -1957,7 +1957,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
> #
> @@ -1977,7 +1977,7 @@
> #
> # @type: job type
> #
> -# @device: device name
> +# @device: device name, or node name if not present
> #
> # @len: maximum progress value
> #
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 03/11] block: never cancel a streaming job without running stream_complete()
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
2015-05-13 13:27 ` [Qemu-devel] [PATCH 01/11] block: keep a list of block jobs Alberto Garcia
2015-05-13 13:27 ` [Qemu-devel] [PATCH 02/11] block: allow block jobs in any arbitrary node Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:23 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer Alberto Garcia
` (9 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
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>
Reviewed-by: Max Reitz <mreitz@redhat.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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 03/11] block: never cancel a streaming job without running stream_complete()
2015-05-13 13:27 ` [Qemu-devel] [PATCH 03/11] block: never cancel a streaming job without running stream_complete() Alberto Garcia
@ 2015-05-15 2:23 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:23 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, 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, but it
> will become more important as we introduce support for streaming to
> any intermediate node.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.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
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (2 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 03/11] block: never cancel a streaming job without running stream_complete() Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:33 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 05/11] block: Add QMP support for " Alberto Garcia
` (8 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 4 +++-
block/stream.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 7904098..8fab7e4 100644
--- a/block.c
+++ b/block.c
@@ -1044,9 +1044,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..8ba5a24 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,
@@ -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);
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer
2015-05-13 13:27 ` [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer Alberto Garcia
@ 2015-05-15 2:33 ` Fam Zheng
2015-05-15 9:04 ` Alberto Garcia
0 siblings, 1 reply; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:33 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 4 +++-
> block/stream.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 7904098..8fab7e4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1044,9 +1044,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..8ba5a24 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,
> @@ -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) {
I don't think all bdrv_reopen implementation will guarantee to set errp
on error :(. For example:
static int qcow2_reopen_prepare(BDRVReopenState *state,
BlockReopenQueue *queue, Error **errp)
{
int ret;
if ((state->flags & BDRV_O_RDWR) == 0) {
ret = bdrv_flush(state->bs);
if (ret < 0) {
return ret;
}
ret = qcow2_mark_clean(state->bs);
if (ret < 0) {
return ret;
}
}
return 0;
}
Maybe we should set one here so the user sees the failure?
Fam
> + 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);
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer
2015-05-15 2:33 ` Fam Zheng
@ 2015-05-15 9:04 ` Alberto Garcia
2015-05-15 9:14 ` Fam Zheng
0 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-15 9:04 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, qemu-block
On Fri 15 May 2015 04:33:19 AM CEST, Fam Zheng wrote:
>> + /* 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) {
>
> I don't think all bdrv_reopen implementation will guarantee to set errp
> on error :(. For example:
>
> static int qcow2_reopen_prepare(BDRVReopenState *state,
> BlockReopenQueue *queue, Error **errp)
> {
bdrv_reopen_prepare() already sets a generic error if the implementation
(qcow2_reopen_prepare() in this case) doesn't provide one:
ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
if (ret) {
if (local_err != NULL) {
error_propagate(errp, local_err);
} else {
error_setg(errp, "failed while preparing to reopen image '%s'",
reopen_state->bs->filename);
}
goto error;
}
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer
2015-05-15 9:04 ` Alberto Garcia
@ 2015-05-15 9:14 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 9:14 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Fri, 05/15 11:04, Alberto Garcia wrote:
> On Fri 15 May 2015 04:33:19 AM CEST, Fam Zheng wrote:
>
> >> + /* 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) {
> >
> > I don't think all bdrv_reopen implementation will guarantee to set errp
> > on error :(. For example:
> >
> > static int qcow2_reopen_prepare(BDRVReopenState *state,
> > BlockReopenQueue *queue, Error **errp)
> > {
>
> bdrv_reopen_prepare() already sets a generic error if the implementation
> (qcow2_reopen_prepare() in this case) doesn't provide one:
>
> ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
> if (ret) {
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> } else {
> error_setg(errp, "failed while preparing to reopen image '%s'",
> reopen_state->bs->filename);
> }
> goto error;
> }
>
You're right, I missed that. So,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 05/11] block: Add QMP support for streaming to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (3 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:38 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 06/11] docs: Document how to stream " Alberto Garcia
` (7 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
This patch makes the 'device' parameter 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.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
blockdev.c | 20 ++++++++++----------
qapi/block-core.json | 10 +++++++---
tests/qemu-iotests/030 | 2 +-
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 1fcf466..85d2d5e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2263,8 +2263,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;
@@ -2274,20 +2273,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) {
@@ -2298,6 +2291,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) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9b5cddd..769b6c7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1095,6 +1095,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
@@ -1103,12 +1107,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
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 952a524..7ca9b25 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -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):
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 05/11] block: Add QMP support for streaming to an intermediate layer
2015-05-13 13:27 ` [Qemu-devel] [PATCH 05/11] block: Add QMP support for " Alberto Garcia
@ 2015-05-15 2:38 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:38 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> This patch makes the 'device' parameter 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.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 06/11] docs: Document how to stream to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (4 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 05/11] block: Add QMP support for " Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:42 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 07/11] qemu-iotests: fix test_stream_partial() Alberto Garcia
` (6 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
docs/live-block-ops.txt | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
index a257087..a05d869 100644
--- a/docs/live-block-ops.txt
+++ b/docs/live-block-ops.txt
@@ -10,9 +10,9 @@ Snapshot live merge
Given a snapshot chain, described in this document in the following
format:
-[A] -> [B] -> [C] -> [D]
+[A] <- [B] <- [C] <- [D] <- [E]
-Where the rightmost object ([D] in the example) described is the current
+Where the rightmost object ([E] in the example) described is the current
image which the guest OS has write access to. To the left of it is its base
image, and so on accordingly until the leftmost image, which has no
base.
@@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain into a
smaller one with fewer elements, such as this transformation relative
to the first example:
-[A] -> [D]
+[A] <- [E]
-Currently only forward merge with target being the active image is
-supported, that is, data copy is performed in the right direction with
-destination being the rightmost image.
+Data is copied in the right direction with destination being the
+rightmost image, but any other intermediate image can be specified
+instead. In this example data is copied from [C] into [D], so [D] can
+be backed by [B]:
+
+[A] <- [B] <- [D] <- [E]
The operation is implemented in QEMU through image streaming facilities.
@@ -35,14 +38,20 @@ streaming operation completes it raises a QMP event. 'block_stream'
copies data from the backing file(s) into the active image. When finished,
it adjusts the backing file pointer.
-The 'base' parameter specifies an image which data need not be streamed from.
-This image will be used as the backing file for the active image when the
-operation is finished.
+The 'base' parameter specifies an image which data need not be
+streamed from. This image will be used as the backing file for the
+destination image when the operation is finished.
+
+In the 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
===============
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 06/11] docs: Document how to stream to an intermediate layer
2015-05-13 13:27 ` [Qemu-devel] [PATCH 06/11] docs: Document how to stream " Alberto Garcia
@ 2015-05-15 2:42 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:42 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> docs/live-block-ops.txt | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
> index a257087..a05d869 100644
> --- a/docs/live-block-ops.txt
> +++ b/docs/live-block-ops.txt
> @@ -10,9 +10,9 @@ Snapshot live merge
> Given a snapshot chain, described in this document in the following
> format:
>
> -[A] -> [B] -> [C] -> [D]
> +[A] <- [B] <- [C] <- [D] <- [E]
>
> -Where the rightmost object ([D] in the example) described is the current
> +Where the rightmost object ([E] in the example) described is the current
> image which the guest OS has write access to. To the left of it is its base
> image, and so on accordingly until the leftmost image, which has no
> base.
> @@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain into a
> smaller one with fewer elements, such as this transformation relative
> to the first example:
>
> -[A] -> [D]
> +[A] <- [E]
>
> -Currently only forward merge with target being the active image is
> -supported, that is, data copy is performed in the right direction with
> -destination being the rightmost image.
> +Data is copied in the right direction with destination being the
> +rightmost image, but any other intermediate image can be specified
> +instead. In this example data is copied from [C] into [D], so [D] can
> +be backed by [B]:
> +
> +[A] <- [B] <- [D] <- [E]
>
> The operation is implemented in QEMU through image streaming facilities.
>
> @@ -35,14 +38,20 @@ streaming operation completes it raises a QMP event. 'block_stream'
> copies data from the backing file(s) into the active image. When finished,
> it adjusts the backing file pointer.
>
> -The 'base' parameter specifies an image which data need not be streamed from.
> -This image will be used as the backing file for the active image when the
> -operation is finished.
> +The 'base' parameter specifies an image which data need not be
> +streamed from. This image will be used as the backing file for the
> +destination image when the operation is finished.
> +
> +In the 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
> ===============
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 07/11] qemu-iotests: fix test_stream_partial()
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (5 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 06/11] docs: Document how to stream " Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:43 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 08/11] qemu-iotests: add no-op streaming test Alberto Garcia
` (5 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
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 something
to copy and the test is meaningful.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 7ca9b25..6e6cb5a 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -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()
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 07/11] qemu-iotests: fix test_stream_partial()
2015-05-13 13:27 ` [Qemu-devel] [PATCH 07/11] qemu-iotests: fix test_stream_partial() Alberto Garcia
@ 2015-05-15 2:43 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:43 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> 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 something
> to copy and the test is meaningful.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> tests/qemu-iotests/030 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 7ca9b25..6e6cb5a 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -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()
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 08/11] qemu-iotests: add no-op streaming test
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (6 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 07/11] qemu-iotests: fix test_stream_partial() Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:44 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 09/11] qemu-iotests: test streaming to an intermediate layer Alberto Garcia
` (4 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
This patch tests that in a partial block-stream operation, no data is
ever copied from the base image.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 18 ++++++++++++++++++
tests/qemu-iotests/030.out | 4 ++--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 6e6cb5a..bc53885 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -91,6 +91,24 @@ 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_no_op(self):
+ self.assert_no_active_block_jobs()
+
+ # The image map is empty before the operation
+ empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+
+ # This is a no-op: no data should ever be copied from the base image
+ result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
+ self.assert_qmp(result, 'return', {})
+
+ self.wait_until_completed()
+
+ self.assert_no_active_block_jobs()
+ self.vm.shutdown()
+
+ self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
+ empty_map, 'image file map changed after a no-op')
+
def test_stream_partial(self):
self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index fa16b5c..6323079 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-.............
+..............
----------------------------------------------------------------------
-Ran 13 tests
+Ran 14 tests
OK
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 08/11] qemu-iotests: add no-op streaming test
2015-05-13 13:27 ` [Qemu-devel] [PATCH 08/11] qemu-iotests: add no-op streaming test Alberto Garcia
@ 2015-05-15 2:44 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:44 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> This patch tests that in a partial block-stream operation, no data is
> ever copied from the base image.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> tests/qemu-iotests/030 | 18 ++++++++++++++++++
> tests/qemu-iotests/030.out | 4 ++--
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 6e6cb5a..bc53885 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -91,6 +91,24 @@ 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_no_op(self):
> + self.assert_no_active_block_jobs()
> +
> + # The image map is empty before the operation
> + empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
> +
> + # This is a no-op: no data should ever be copied from the base image
> + result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
> + self.assert_qmp(result, 'return', {})
> +
> + self.wait_until_completed()
> +
> + self.assert_no_active_block_jobs()
> + self.vm.shutdown()
> +
> + self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
> + empty_map, 'image file map changed after a no-op')
> +
> def test_stream_partial(self):
> self.assert_no_active_block_jobs()
>
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index fa16b5c..6323079 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -.............
> +..............
> ----------------------------------------------------------------------
> -Ran 13 tests
> +Ran 14 tests
>
> OK
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 09/11] qemu-iotests: test streaming to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (7 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 08/11] qemu-iotests: add no-op streaming test Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:45 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 10/11] qemu-iotests: test block-stream operations in parallel Alberto Garcia
` (3 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
This adds test_stream_intermediate(), similar to test_stream() but
streams to the intermediate image instead.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 18 +++++++++++++++++-
tests/qemu-iotests/030.out | 4 ++--
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index bc53885..0927457 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -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()
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 6323079..96961ed 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-..............
+...............
----------------------------------------------------------------------
-Ran 14 tests
+Ran 15 tests
OK
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 09/11] qemu-iotests: test streaming to an intermediate layer
2015-05-13 13:27 ` [Qemu-devel] [PATCH 09/11] qemu-iotests: test streaming to an intermediate layer Alberto Garcia
@ 2015-05-15 2:45 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:45 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> This adds test_stream_intermediate(), similar to test_stream() but
> streams to the intermediate image instead.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> tests/qemu-iotests/030 | 18 +++++++++++++++++-
> tests/qemu-iotests/030.out | 4 ++--
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index bc53885..0927457 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -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()
>
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 6323079..96961ed 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -..............
> +...............
> ----------------------------------------------------------------------
> -Ran 14 tests
> +Ran 15 tests
>
> OK
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 10/11] qemu-iotests: test block-stream operations in parallel
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (8 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 09/11] qemu-iotests: test streaming to an intermediate layer Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:53 ` Fam Zheng
2015-05-13 13:27 ` [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations Alberto Garcia
` (2 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
This test case checks that it's possible to launch several stream
operations in parallel in the same snapshot chain, each one involving
a different set of nodes.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 80 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/030.out | 4 +--
2 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 0927457..c199cef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -145,6 +145,86 @@ class TestSingleDrive(iotests.QMPTestCase):
self.assert_qmp(result, 'error/class', 'GenericError')
+class TestParallelOps(iotests.QMPTestCase):
+ image_len = 2 * 1024 * 1024 # MB
+ num_ops = 4 # Number of parallel block-stream operations
+ num_imgs = num_ops * 2 + 1
+ imgs = []
+
+ def setUp(self):
+ opts = []
+ self.imgs = []
+
+ # Initialize file names and command-line options
+ for i in range(self.num_imgs):
+ img_depth = self.num_imgs - i - 1
+ opts.append("backing." * img_depth + "node-name=node%d" % i)
+ self.imgs.append(os.path.join(iotests.test_dir, 'img-%d.img' % i))
+
+ # Create all images
+ iotests.create_image(self.imgs[0], self.image_len)
+ for i in range(1, self.num_imgs):
+ qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i])
+
+ # Put data into the images we are copying data from
+ for i in range(1, self.num_imgs, 2):
+ qemu_io('-f', iotests.imgfmt,
+ '-c', 'write -P %d %d 128K' % (i, i*128*1024), self.imgs[i])
+
+ # Attach the drive to the VM
+ self.vm = iotests.VM()
+ self.vm.add_drive("blkdebug::" + self.imgs[-1], ','.join(opts))
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for img in self.imgs:
+ os.remove(img)
+
+ # Test that it's possible to run several block-stream operations
+ # in parallel in the same snapshot chain
+ def test_stream_parallel(self):
+ self.assert_no_active_block_jobs()
+
+ # Check that the maps don't match before the streaming operations
+ for i in range(2, self.num_imgs, 2):
+ self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
+ qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
+ 'image file map matches backing file before streaming')
+
+ # Create all streaming jobs
+ pending_jobs = []
+ for i in range(2, self.num_imgs, 2):
+ node_name = 'node%d' % i
+ pending_jobs.append(node_name)
+ result = self.vm.qmp('block-stream', device=node_name, base=self.imgs[i-2], speed=32768)
+ self.assert_qmp(result, 'return', {})
+
+ # The block job on the active image is always referenced by
+ # its device name. Therefore we have to replace the node name
+ # with the device name in the list of pending jobs
+ pending_jobs.pop()
+ pending_jobs.append("drive0")
+
+ # Wait for all jobs to be finished.
+ while len(pending_jobs) > 0:
+ for event in self.vm.get_qmp_events(wait=True):
+ if event['event'] == 'BLOCK_JOB_COMPLETED':
+ node_name = self.dictpath(event, 'data/device')
+ self.assertTrue(node_name in pending_jobs)
+ self.assert_qmp_absent(event, 'data/error')
+ pending_jobs.remove(node_name)
+
+ self.assert_no_active_block_jobs()
+ self.vm.shutdown()
+
+ # Check that all maps match now
+ for i in range(2, self.num_imgs, 2):
+ self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
+ qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
+ 'image file map does not match backing file after streaming')
+
class TestSmallerBackingFile(iotests.QMPTestCase):
backing_len = 1 * 1024 * 1024 # MB
image_len = 2 * backing_len
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 96961ed..b6f2576 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...............
+................
----------------------------------------------------------------------
-Ran 15 tests
+Ran 16 tests
OK
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 10/11] qemu-iotests: test block-stream operations in parallel
2015-05-13 13:27 ` [Qemu-devel] [PATCH 10/11] qemu-iotests: test block-stream operations in parallel Alberto Garcia
@ 2015-05-15 2:53 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:53 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> This test case checks that it's possible to launch several stream
> operations in parallel in the same snapshot chain, each one involving
> a different set of nodes.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> tests/qemu-iotests/030 | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/030.out | 4 +--
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 0927457..c199cef 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -145,6 +145,86 @@ class TestSingleDrive(iotests.QMPTestCase):
> self.assert_qmp(result, 'error/class', 'GenericError')
>
>
> +class TestParallelOps(iotests.QMPTestCase):
> + image_len = 2 * 1024 * 1024 # MB
> + num_ops = 4 # Number of parallel block-stream operations
> + num_imgs = num_ops * 2 + 1
> + imgs = []
> +
> + def setUp(self):
> + opts = []
> + self.imgs = []
> +
> + # Initialize file names and command-line options
> + for i in range(self.num_imgs):
> + img_depth = self.num_imgs - i - 1
> + opts.append("backing." * img_depth + "node-name=node%d" % i)
> + self.imgs.append(os.path.join(iotests.test_dir, 'img-%d.img' % i))
> +
> + # Create all images
> + iotests.create_image(self.imgs[0], self.image_len)
> + for i in range(1, self.num_imgs):
> + qemu_img('create', '-f', iotests.imgfmt,
> + '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i])
> +
> + # Put data into the images we are copying data from
> + for i in range(1, self.num_imgs, 2):
> + qemu_io('-f', iotests.imgfmt,
> + '-c', 'write -P %d %d 128K' % (i, i*128*1024), self.imgs[i])
> +
> + # Attach the drive to the VM
> + self.vm = iotests.VM()
> + self.vm.add_drive("blkdebug::" + self.imgs[-1], ','.join(opts))
> + self.vm.launch()
> +
> + def tearDown(self):
> + self.vm.shutdown()
> + for img in self.imgs:
> + os.remove(img)
> +
> + # Test that it's possible to run several block-stream operations
> + # in parallel in the same snapshot chain
> + def test_stream_parallel(self):
> + self.assert_no_active_block_jobs()
> +
> + # Check that the maps don't match before the streaming operations
> + for i in range(2, self.num_imgs, 2):
> + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
> + qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
> + 'image file map matches backing file before streaming')
> +
> + # Create all streaming jobs
> + pending_jobs = []
> + for i in range(2, self.num_imgs, 2):
> + node_name = 'node%d' % i
> + pending_jobs.append(node_name)
> + result = self.vm.qmp('block-stream', device=node_name, base=self.imgs[i-2], speed=32768)
> + self.assert_qmp(result, 'return', {})
> +
> + # The block job on the active image is always referenced by
> + # its device name. Therefore we have to replace the node name
> + # with the device name in the list of pending jobs
> + pending_jobs.pop()
> + pending_jobs.append("drive0")
> +
> + # Wait for all jobs to be finished.
> + while len(pending_jobs) > 0:
> + for event in self.vm.get_qmp_events(wait=True):
> + if event['event'] == 'BLOCK_JOB_COMPLETED':
> + node_name = self.dictpath(event, 'data/device')
> + self.assertTrue(node_name in pending_jobs)
> + self.assert_qmp_absent(event, 'data/error')
> + pending_jobs.remove(node_name)
> +
> + self.assert_no_active_block_jobs()
> + self.vm.shutdown()
> +
> + # Check that all maps match now
> + for i in range(2, self.num_imgs, 2):
> + self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
> + qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
> + 'image file map does not match backing file after streaming')
> +
> class TestSmallerBackingFile(iotests.QMPTestCase):
> backing_len = 1 * 1024 * 1024 # MB
> image_len = 2 * backing_len
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 96961ed..b6f2576 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -...............
> +................
> ----------------------------------------------------------------------
> -Ran 15 tests
> +Ran 16 tests
>
> OK
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (9 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 10/11] qemu-iotests: test block-stream operations in parallel Alberto Garcia
@ 2015-05-13 13:27 ` Alberto Garcia
2015-05-15 2:56 ` Fam Zheng
2015-06-16 8:51 ` [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
2015-06-22 16:17 ` [Qemu-devel] " Stefan Hajnoczi
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-13 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block
This test case checks that it's not possible to perform two
block-stream operations if there are nodes involved in both.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 27 +++++++++++++++++++++++++++
tests/qemu-iotests/030.out | 4 ++--
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index c199cef..f898c92 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -225,6 +225,33 @@ class TestParallelOps(iotests.QMPTestCase):
qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
'image file map does not match backing file after streaming')
+ # Test that it's not possible to perform two block-stream
+ # operations if there are nodes involved in both.
+ def test_stream_overlapping(self):
+ self.assert_no_active_block_jobs()
+
+ # Set a speed limit to make sure that this job blocks the rest
+ result = self.vm.qmp('block-stream', device='node4', base=self.imgs[0], speed=32768)
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('block-stream', device='node5', base=self.imgs[1])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ result = self.vm.qmp('block-stream', device='node3', base=self.imgs[2])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ result = self.vm.qmp('block-stream', device='node4')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ # If node4 is the active node, the id of the block job is drive0
+ if self.num_imgs == 5:
+ self.wait_until_completed(drive='drive0')
+ else:
+ self.wait_until_completed(drive='node4')
+ self.assert_no_active_block_jobs()
+
+ self.vm.shutdown()
+
class TestSmallerBackingFile(iotests.QMPTestCase):
backing_len = 1 * 1024 * 1024 # MB
image_len = 2 * backing_len
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index b6f2576..52d796e 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-................
+.................
----------------------------------------------------------------------
-Ran 16 tests
+Ran 17 tests
OK
--
2.1.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations
2015-05-13 13:27 ` [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations Alberto Garcia
@ 2015-05-15 2:56 ` Fam Zheng
2015-05-15 8:58 ` Alberto Garcia
0 siblings, 1 reply; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 2:56 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Wed, 05/13 16:27, Alberto Garcia wrote:
> This test case checks that it's not possible to perform two
> block-stream operations if there are nodes involved in both.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/030 | 27 +++++++++++++++++++++++++++
> tests/qemu-iotests/030.out | 4 ++--
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index c199cef..f898c92 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -225,6 +225,33 @@ class TestParallelOps(iotests.QMPTestCase):
> qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
> 'image file map does not match backing file after streaming')
>
> + # Test that it's not possible to perform two block-stream
> + # operations if there are nodes involved in both.
> + def test_stream_overlapping(self):
> + self.assert_no_active_block_jobs()
> +
> + # Set a speed limit to make sure that this job blocks the rest
> + result = self.vm.qmp('block-stream', device='node4', base=self.imgs[0], speed=32768)
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('block-stream', device='node5', base=self.imgs[1])
> + self.assert_qmp(result, 'error/class', 'GenericError')
> +
> + result = self.vm.qmp('block-stream', device='node3', base=self.imgs[2])
> + self.assert_qmp(result, 'error/class', 'GenericError')
> +
> + result = self.vm.qmp('block-stream', device='node4')
> + self.assert_qmp(result, 'error/class', 'GenericError')
> +
> + # If node4 is the active node, the id of the block job is drive0
> + if self.num_imgs == 5:
Isn't self.num_imgs a constant (9) ?
Fam
> + self.wait_until_completed(drive='drive0')
> + else:
> + self.wait_until_completed(drive='node4')
> + self.assert_no_active_block_jobs()
> +
> + self.vm.shutdown()
> +
> class TestSmallerBackingFile(iotests.QMPTestCase):
> backing_len = 1 * 1024 * 1024 # MB
> image_len = 2 * backing_len
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index b6f2576..52d796e 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -................
> +.................
> ----------------------------------------------------------------------
> -Ran 16 tests
> +Ran 17 tests
>
> OK
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations
2015-05-15 2:56 ` Fam Zheng
@ 2015-05-15 8:58 ` Alberto Garcia
2015-05-15 9:18 ` Fam Zheng
0 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-05-15 8:58 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, qemu-block
On Fri 15 May 2015 04:56:09 AM CEST, Fam Zheng wrote:
>> + # If node4 is the active node, the id of the block job is drive0
>> + if self.num_imgs == 5:
>
> Isn't self.num_imgs a constant (9) ?
Yes, but the number is arbitrary. The idea is to let users change it if
they want to, so the test case must be prepared for that.
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations
2015-05-15 8:58 ` Alberto Garcia
@ 2015-05-15 9:18 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2015-05-15 9:18 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
On Fri, 05/15 10:58, Alberto Garcia wrote:
> On Fri 15 May 2015 04:56:09 AM CEST, Fam Zheng wrote:
>
> >> + # If node4 is the active node, the id of the block job is drive0
> >> + if self.num_imgs == 5:
> >
> > Isn't self.num_imgs a constant (9) ?
>
> Yes, but the number is arbitrary. The idea is to let users change it if
> they want to, so the test case must be prepared for that.
>
They have to change the code to get a different value, no? This is unreachable
code as is and IMO even if you're not as considerate on this, whoever changes
num_ops should make sure code that uses it still works. This is bikeshedding,
this is a great series, so even if you insist,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (10 preceding siblings ...)
2015-05-13 13:27 ` [Qemu-devel] [PATCH 11/11] qemu-iotests: test overlapping block-stream operations Alberto Garcia
@ 2015-06-16 8:51 ` Alberto Garcia
2015-06-18 10:45 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2015-06-22 16:17 ` [Qemu-devel] " Stefan Hajnoczi
12 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-06-16 8:51 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, Max Reitz
Ping...
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html
On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote:
> v7:
> - Rebased against the current master
> - Updated bdrv_drain_all() to use the new block_job_next() API.
>
> v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html
> - fix the no-op test following Max's suggestions
>
> v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html
> - Fix a few typos
> - Minor documentation updates
> - Update test_stream_partial() to test no-ops
> - New test case: test_stream_parallel()
> - New test case: test_stream_overlapping()
>
> v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html
> - Refactor find_block_job to use the error from bdrv_lookup_bs()
> - Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be
> dealing with nodes now.
> - Fix @device comment in the BlockJobInfo documentation
> - stream_start(): simplify the bdrv_reopen() call and use
> bdrv_get_device_or_node_name() for error messages.
> - Use a different variable name for BlockDriverState *i
> - Documentation fixes in docs/live-block-ops.txt
> - Update iotest 30 since now test_device_not_found() returns
> GenericError
> - Fix test case test_stream_partial()
> - Add new test case test_stream_intermediate()
> - Fix typos
>
> v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html
> - Keep a list of block jobs and make qmp_query_block_jobs() iterate
> over it.
>
> v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html
> - The 'block-stream' command does not have a 'node-name' parameter
> anymore and reuses 'device' for that purpose.
> - Block jobs can now be owned by any intermediate node, and not just
> by the ones at the root. query-block-jobs is updated to reflect that
> change.
> - The 'device' parameter of all 'block-job-*' commands can now take a
> node name.
> - The BlockJobInfo type and all BLOCK_JOB_* events report the node
> name in the 'device' field if the node does not have a device name.
> - All intermediate nodes are blocked (and checked for blockers) during
> the streaming operation.
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html
>
> Regards,
>
> Berto
>
> Alberto Garcia (11):
> 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: add no-op streaming test
> qemu-iotests: test streaming to an intermediate layer
> qemu-iotests: test block-stream operations in parallel
> qemu-iotests: test overlapping block-stream operations
>
> block.c | 4 +-
> block/io.c | 21 +++----
> block/mirror.c | 5 +-
> block/stream.c | 44 ++++++++++++--
> blockdev.c | 55 ++++++++---------
> blockjob.c | 31 +++++++---
> docs/live-block-ops.txt | 31 ++++++----
> 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 | 148 ++++++++++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/030.out | 4 +-
> 13 files changed, 304 insertions(+), 94 deletions(-)
>
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-16 8:51 ` [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
@ 2015-06-18 10:45 ` Kevin Wolf
2015-06-18 11:41 ` Alberto Garcia
0 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2015-06-18 10:45 UTC (permalink / raw)
To: Alberto Garcia; +Cc: armbru, stefanha, qemu-devel, qemu-block, Max Reitz
Am 16.06.2015 um 10:51 hat Alberto Garcia geschrieben:
> Ping...
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html
I believe our conclusion from an earlier version of the series was that
we need QAPI introspection so that libvirt can detect the presence of
the feature.
Markus is planning to have the introspection code mergable when the 2.5
development phase starts. That's what I would wait for before I merge
your series.
Markus also offered to write a preliminary version of the introspection
code for 2.4. However, I will be offline from tomorrow until the day
before the hard freeze, so I won't be able to manage this if Markus and
you still wanted to try it for 2.4.
If you guys can come up with patches in time and convince Stefan and/or
Max to send a pull request for it, that's fine with me.
Kevin
> On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote:
> > v7:
> > - Rebased against the current master
> > - Updated bdrv_drain_all() to use the new block_job_next() API.
> >
> > v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html
> > - fix the no-op test following Max's suggestions
> >
> > v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html
> > - Fix a few typos
> > - Minor documentation updates
> > - Update test_stream_partial() to test no-ops
> > - New test case: test_stream_parallel()
> > - New test case: test_stream_overlapping()
> >
> > v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html
> > - Refactor find_block_job to use the error from bdrv_lookup_bs()
> > - Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be
> > dealing with nodes now.
> > - Fix @device comment in the BlockJobInfo documentation
> > - stream_start(): simplify the bdrv_reopen() call and use
> > bdrv_get_device_or_node_name() for error messages.
> > - Use a different variable name for BlockDriverState *i
> > - Documentation fixes in docs/live-block-ops.txt
> > - Update iotest 30 since now test_device_not_found() returns
> > GenericError
> > - Fix test case test_stream_partial()
> > - Add new test case test_stream_intermediate()
> > - Fix typos
> >
> > v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html
> > - Keep a list of block jobs and make qmp_query_block_jobs() iterate
> > over it.
> >
> > v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html
> > - The 'block-stream' command does not have a 'node-name' parameter
> > anymore and reuses 'device' for that purpose.
> > - Block jobs can now be owned by any intermediate node, and not just
> > by the ones at the root. query-block-jobs is updated to reflect that
> > change.
> > - The 'device' parameter of all 'block-job-*' commands can now take a
> > node name.
> > - The BlockJobInfo type and all BLOCK_JOB_* events report the node
> > name in the 'device' field if the node does not have a device name.
> > - All intermediate nodes are blocked (and checked for blockers) during
> > the streaming operation.
> >
> > v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html
> >
> > Regards,
> >
> > Berto
> >
> > Alberto Garcia (11):
> > 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: add no-op streaming test
> > qemu-iotests: test streaming to an intermediate layer
> > qemu-iotests: test block-stream operations in parallel
> > qemu-iotests: test overlapping block-stream operations
> >
> > block.c | 4 +-
> > block/io.c | 21 +++----
> > block/mirror.c | 5 +-
> > block/stream.c | 44 ++++++++++++--
> > blockdev.c | 55 ++++++++---------
> > blockjob.c | 31 +++++++---
> > docs/live-block-ops.txt | 31 ++++++----
> > 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 | 148 ++++++++++++++++++++++++++++++++++++++++++++-
> > tests/qemu-iotests/030.out | 4 +-
> > 13 files changed, 304 insertions(+), 94 deletions(-)
> >
> > --
> > 2.1.4
> >
> >
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 10:45 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2015-06-18 11:41 ` Alberto Garcia
2015-06-18 11:47 ` Kevin Wolf
0 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-06-18 11:41 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, armbru, qemu-devel, stefanha, Max Reitz
On Thu 18 Jun 2015 12:45:35 PM CEST, Kevin Wolf wrote:
> I believe our conclusion from an earlier version of the series was
> that we need QAPI introspection so that libvirt can detect the
> presence of the feature.
The initial version of this series had an extra 'top' parameter to
decide what image to stream data into. That's what prompted the
discussion:
https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04182.html
That was later removed when we agreed that we could just reuse the
'device' parameter to refer to either a device or a node name.
I don't think that introspection support would make a difference in this
case, or am I missing anything?
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 11:41 ` Alberto Garcia
@ 2015-06-18 11:47 ` Kevin Wolf
2015-06-18 12:07 ` Alberto Garcia
0 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2015-06-18 11:47 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-block, armbru, qemu-devel, stefanha, Max Reitz
Am 18.06.2015 um 13:41 hat Alberto Garcia geschrieben:
> On Thu 18 Jun 2015 12:45:35 PM CEST, Kevin Wolf wrote:
>
> > I believe our conclusion from an earlier version of the series was
> > that we need QAPI introspection so that libvirt can detect the
> > presence of the feature.
>
> The initial version of this series had an extra 'top' parameter to
> decide what image to stream data into. That's what prompted the
> discussion:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04182.html
>
> That was later removed when we agreed that we could just reuse the
> 'device' parameter to refer to either a device or a node name.
>
> I don't think that introspection support would make a difference in this
> case, or am I missing anything?
Hm, yes, probably not. But how would libvirt detect the feature then?
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 11:47 ` Kevin Wolf
@ 2015-06-18 12:07 ` Alberto Garcia
2015-06-18 12:36 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2015-06-18 12:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, armbru, qemu-devel, stefanha, Max Reitz
On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote:
>> > I believe our conclusion from an earlier version of the series was
>> > that we need QAPI introspection so that libvirt can detect the
>> > presence of the feature.
>>
>> The initial version of this series had an extra 'top' parameter to
>> decide what image to stream data into. [...] That was later removed
>> when we agreed that we could just reuse the 'device' parameter to
>> refer to either a device or a node name.
>>
>> I don't think that introspection support would make a difference in
>> this case, or am I missing anything?
>
> Hm, yes, probably not. But how would libvirt detect the feature then?
One possibility is to try to stream to an intermediate node and see if
it fails.
Example: in a chain like [A] <- [B] <- [C], streaming to [B] using [A]
as the 'base' parameter is a no-op (there's even a test for that in
iotest 030).
If QEMU does support streaming to [B], the operation will succeed but do
nothing. Otherwise the operation will fail with a DeviceNotFound error.
That said, I would prefer a way to detect the feature that does not
involve testing commands for their error codes, but is there any? What
does libvirt generally do in order to detect new features that don't
depend on API changes?
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 12:07 ` Alberto Garcia
@ 2015-06-18 12:36 ` Eric Blake
2015-06-18 12:55 ` Eric Blake
2015-06-19 10:09 ` Alberto Garcia
0 siblings, 2 replies; 39+ messages in thread
From: Eric Blake @ 2015-06-18 12:36 UTC (permalink / raw)
To: Alberto Garcia, Kevin Wolf
Cc: armbru, stefanha, qemu-devel, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]
On 06/18/2015 06:07 AM, Alberto Garcia wrote:
> On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote:
>
>>>> I believe our conclusion from an earlier version of the series was
>>>> that we need QAPI introspection so that libvirt can detect the
>>>> presence of the feature.
Detecting the presence of a feature allows libvirt the luxury of giving
its own error message, rather than relying on the qemu message. But
that's not to say libvirt HAS to use its own error message, and
therefore being unable to detect the feature may not be the end of the
world.
>>>
>>> The initial version of this series had an extra 'top' parameter to
>>> decide what image to stream data into. [...] That was later removed
>>> when we agreed that we could just reuse the 'device' parameter to
>>> refer to either a device or a node name.
>>>
>>> I don't think that introspection support would make a difference in
>>> this case, or am I missing anything?
>>
>> Hm, yes, probably not. But how would libvirt detect the feature then?
>
> One possibility is to try to stream to an intermediate node and see if
> it fails.
>
> Example: in a chain like [A] <- [B] <- [C], streaming to [B] using [A]
> as the 'base' parameter is a no-op (there's even a test for that in
> iotest 030).
>
> If QEMU does support streaming to [B], the operation will succeed but do
> nothing. Otherwise the operation will fail with a DeviceNotFound error.
>
> That said, I would prefer a way to detect the feature that does not
> involve testing commands for their error codes, but is there any? What
> does libvirt generally do in order to detect new features that don't
> depend on API changes?
But libvirt has not yet set up node name management (I'm about to revive
Jeff's patch for auto-node-naming simultaneously with a libvirt patch
series that proves that it helps libvirt), and libvirt will need a new
API to allow users a way to request streams to an intermediate image.
So anything libvirt does to interact with the new stream-to-intermediate
will have to be new code, and I can worry about whether the qemu error
message is good enough, or whether I have to contrive some probing test
to see if it even works; but my initial thought is that merely probing
to see if auto-node-naming is in place is a good approximation filter
(if libvirt isn't managing its own node names, then the only way to use
stream-to-intermediate is via a node name automatically supplied by
qemu, especially nice if both features land in 2.4).
In general, if a feature addition doesn't change API, but merely
converts what was previously an error into something that works, then
libvirt is probably okay with just trying the feature, and reporting the
error message if it fails (assuming the qemu error message is sane).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 12:36 ` Eric Blake
@ 2015-06-18 12:55 ` Eric Blake
2015-06-19 10:09 ` Alberto Garcia
1 sibling, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-06-18 12:55 UTC (permalink / raw)
To: Alberto Garcia, Kevin Wolf
Cc: qemu-block, libvir-list, armbru, qemu-devel, stefanha, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]
[adding libvirt, to make sure I capture a design idea]
On 06/18/2015 06:36 AM, Eric Blake wrote:
> On 06/18/2015 06:07 AM, Alberto Garcia wrote:
>> On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote:
>>
>>>>> I believe our conclusion from an earlier version of the series was
>>>>> that we need QAPI introspection so that libvirt can detect the
>>>>> presence of the feature.
>
> Detecting the presence of a feature allows libvirt the luxury of giving
> its own error message, rather than relying on the qemu message. But
> that's not to say libvirt HAS to use its own error message, and
> therefore being unable to detect the feature may not be the end of the
> world.
>
>> That said, I would prefer a way to detect the feature that does not
>> involve testing commands for their error codes, but is there any? What
>> does libvirt generally do in order to detect new features that don't
>> depend on API changes?
>
> But libvirt has not yet set up node name management (I'm about to revive
> Jeff's patch for auto-node-naming simultaneously with a libvirt patch
> series that proves that it helps libvirt), and libvirt will need a new
> API to allow users a way to request streams to an intermediate image.
> So anything libvirt does to interact with the new stream-to-intermediate
> will have to be new code, and I can worry about whether the qemu error
> message is good enough, or whether I have to contrive some probing test
> to see if it even works; but my initial thought is that merely probing
> to see if auto-node-naming is in place is a good approximation filter
> (if libvirt isn't managing its own node names, then the only way to use
> stream-to-intermediate is via a node name automatically supplied by
> qemu, especially nice if both features land in 2.4).
Actually, in thinking more about it, libvirt won't need a new API; the
existing virDomainBlockPull() and virDomainBlockRebase() are sufficient,
if I allow libvirt to treat "vda[1]" as a destination (which is the
first backing image of disk vda; pretty much similar to how qemu is
adding node names rather than device names as a way to make the existing
block-stream now stream to intermediate). And that is consistent with
the way we have been retrofitting other existing libvirt API to refer to
specific backing images. On libvirt's front, I may want to add a new
flag (where the flag must be present to make it clear that
stream-to-intermediate is desired, so that upper level applications can
use the absence of the libvirt flag as their feature probe), but that
has no bearing on what qemu has to do to turn on the feature.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-06-18 12:36 ` Eric Blake
2015-06-18 12:55 ` Eric Blake
@ 2015-06-19 10:09 ` Alberto Garcia
1 sibling, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2015-06-19 10:09 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf
Cc: armbru, stefanha, qemu-devel, qemu-block, Max Reitz
On Thu 18 Jun 2015 02:36:13 PM CEST, Eric Blake wrote:
[Detecting support for intermediate block streaming]
>> One possibility is to try to stream to an intermediate node and see
>> if it fails.
>>
>> Example: in a chain like [A] <- [B] <- [C], streaming to [B] using
>> [A] as the 'base' parameter is a no-op (there's even a test for that
>> in iotest 030).
>>
>> If QEMU does support streaming to [B], the operation will succeed but
>> do nothing. Otherwise the operation will fail with a DeviceNotFound
>> error.
> In general, if a feature addition doesn't change API, but merely
> converts what was previously an error into something that works, then
> libvirt is probably okay with just trying the feature, and reporting
> the error message if it fails (assuming the qemu error message is
> sane).
With the example I gave above you should be able to detect easily if the
feature is present, but from what you say I understand that strictly
speaking you wouldn't even need to do that.
If you try to stream to a node and QEMU does not support that you will
get a 'device not found' error, which is I guess the kind of error that
you would expect. Since the only thing that changes with this feature is
the 'device' parameter I don't think there's any room for ambiguity.
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
2015-05-13 13:27 [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
` (11 preceding siblings ...)
2015-06-16 8:51 ` [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer Alberto Garcia
@ 2015-06-22 16:17 ` Stefan Hajnoczi
12 siblings, 0 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2015-06-22 16:17 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 366 bytes --]
On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote:
> v7:
> - Rebased against the current master
> - Updated bdrv_drain_all() to use the new block_job_next() API.
Please rebase onto https://github.com/stefanha/qemu 'block' branch and
check that qemu-iotests 015 030 032 038 046 pass:
make
cd tests/qemu-iotests
./check -qcow2 015 030 032 038 046
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread