Re: [Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf  wrote:
>>  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>>  goto out;
>>  }
>
> Added a bit more context.
>
> This check is redundant now...
>
>>  if (has_base) {
>>  base_bs = bdrv_find_backing_image(bs, base);
>>  if (base_bs == NULL) {
>>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>>  goto out;
>>  }
>>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>>  base_name = base;
>>  }
>>  
>> +/* Check for op blockers in the whole chain between bs and base */
>> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
>> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>> +goto out;
>> +}
>> +}
>
> ...because you start with iter = bs here.

You're right, I'll fix it.

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

I thought about that too. In some cases I don't know if it's a good idea
because the qmp_foo_bar() functions do a bit more than simply checking
blockers (e.g. blockdev-mirror creates the target image), so you would
want to have the checks before that.

However doing it in the actual block job code could allow us to do other
things. For example: at the moment when we call block-stream we check
whether a number of BDSs are blocked (in qmp_block_stream()), and if
they're not we proceed to block them (in stream_start()). We could make
block_job_add_bdrv() do both things. On the other hand, since the plan
is to move to a new block job API maybe it's better not to overdo things
now.

It's worth considering for the future anyway.

Berto

Re: [Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c   | 11 +--
>  qapi/block-core.json | 10 +++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f13fc69..57c8329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>bool has_on_error, BlockdevOnError on_error,
>Error **errp)
>  {
> -BlockDriverState *bs;
> +BlockDriverState *bs, *iter;
>  BlockDriverState *base_bs = NULL;
>  AioContext *aio_context;
>  Error *local_err = NULL;
> @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>  on_error = BLOCKDEV_ON_ERROR_REPORT;
>  }
>  
> -bs = qmp_get_root_bs(device, errp);
> +bs = bdrv_lookup_bs(device, device, errp);
>  if (!bs) {
>  return;
>  }
>  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;
>  }

Added a bit more context.

This check is redundant now...

>  if (has_base) {
>  base_bs = bdrv_find_backing_image(bs, base);
>  if (base_bs == NULL) {
>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>  goto out;
>  }
>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>  base_name = base;
>  }
>  
> +/* Check for op blockers in the whole chain between bs and base */
> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
> +goto out;
> +}
> +}

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

Another thought I had while looking at the previous few patches is
whether all the op blocker checks wouldn't be better moved to the actual
block job code (i.e. stream_start in this case). In this case it doesn't
matter much because this is the only caller of stream_start(), but for
other jobs the situation is more complicated and it would be easy for a
caller to forget the checks.

Probably also matter for another series, but I wanted to mention it.

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

Kevin

[Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

This patch makes the 'device' parameter of the 'block-stream' command
accept a node name that is not a root node.

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

Signed-off-by: Alberto Garcia 
---
 blockdev.c   | 11 +--
 qapi/block-core.json | 10 +++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f13fc69..57c8329 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
   bool has_on_error, BlockdevOnError on_error,
   Error **errp)
 {
-BlockDriverState *bs;
+BlockDriverState *bs, *iter;
 BlockDriverState *base_bs = NULL;
 AioContext *aio_context;
 Error *local_err = NULL;
@@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 on_error = BLOCKDEV_ON_ERROR_REPORT;
 }
 
-bs = qmp_get_root_bs(device, errp);
+bs = bdrv_lookup_bs(device, device, errp);
 if (!bs) {
 return;
 }
@@ -2969,6 +2969,13 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 base_name = base;
 }
 
+/* Check for op blockers in the whole chain between bs and base */
+for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
+if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
+goto out;
+}
+}
+
 /* if we are streaming the entire chain, the result will have no backing
  * file, and specifying one is therefore an error */
 if (base_bs == NULL && has_backing_file) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..88f4c55 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1469,6 +1469,10 @@
 # with query-block-jobs.  The operation can be stopped before it has completed
 # using the block-job-cancel command.
 #
+# The node that receives the data is called the top image, can be located in
+# any part of the chain (but always above the base image; see below) and can be
+# specified using its device or node name.
+#
 # If a base file is specified then sectors are not copied from that base file 
and
 # its backing chain.  When streaming completes the image file will have the 
base
 # file as its backing file.  This can be used to stream a subset of the backing
@@ -1480,12 +1484,12 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #  omitted, the device name will be used. (Since 2.7)
 #
-# @device: the device name or node-name of a root node
+# @device: the device or node name of the top image
 #
 # @base:   #optional the common backing file name
 #
-# @backing-file: #optional The backing file string to write into the active
-#  layer. This filename is not validated.
+# @backing-file: #optional The backing file string to write into the top
+#  image. This filename is not validated.
 #
 #  If a pathname string is such that it cannot be
 #  resolved by QEMU, that means that subsequent QMP or
-- 
2.9.3