Re: [Qemu-devel] [PATCH v9 06/11] block: Support streaming to an intermediate layer

[Max Reitz]

On 04.04.2016 15:43, Alberto Garcia wrote:
> This makes sure that the image we are steaming into is open in
> read-write mode during the operation.
> 
> The block job is created on the destination image, but operation
> blockers are also set on the active layer. We do this in order to
> prevent other block jobs from running in parallel in the same chain.
> See here for details on why that is currently not supported:
> 
> [Qemu-block] RFC: Status of the intermediate block streaming work
> https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html
> 
> Finally, this also unblocks the stream operation in backing files.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block.c                   |  4 +++-
>  block/stream.c            | 39 ++++++++++++++++++++++++++++++++++++++-
>  blockdev.c                |  2 +-
>  include/block/block_int.h |  5 ++++-
>  4 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 48638c9..f7698a1 100644
> --- a/block.c
> +++ b/block.c
> @@ -1252,9 +1252,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>              backing_hd->drv ? backing_hd->drv->format_name : "");
>  
>      bdrv_op_block_all(backing_hd, bs->backing_blocker);
> -    /* Otherwise we won't be able to commit due to check in bdrv_commit */
> +    /* Otherwise we won't be able to commit or stream */
>      bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>                      bs->backing_blocker);
> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
> +                    bs->backing_blocker);
>  out:
>      bdrv_refresh_limits(bs, NULL);
>  }
> diff --git a/block/stream.c b/block/stream.c
> index 332b9a1..ac02ddf 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -35,8 +35,10 @@ typedef struct StreamBlockJob {
>      BlockJob common;
>      RateLimit limit;
>      BlockDriverState *base;
> +    BlockDriverState *active;
>      BlockdevOnError on_error;
>      char *backing_file_str;
> +    int bs_flags;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockDriverState *bs,
> @@ -66,6 +68,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>      StreamCompleteData *data = opaque;
>      BlockDriverState *base = s->base;
>  
> +    if (s->active) {
> +        bdrv_op_unblock_all(s->active, s->common.blocker);
> +        bdrv_unref(s->active);
> +    }
> +
>      if (!block_job_is_cancelled(&s->common) && data->reached_end &&
>          data->ret == 0) {
>          const char *base_id = NULL, *base_fmt = NULL;
> @@ -79,6 +86,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>          bdrv_set_backing_hd(job->bs, base);
>      }
>  
> +    /* 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);
> @@ -216,13 +228,15 @@ static const BlockJobDriver stream_job_driver = {
>      .set_speed     = stream_set_speed,
>  };
>  
> -void stream_start(BlockDriverState *bs, BlockDriverState *base,
> +void stream_start(BlockDriverState *bs, BlockDriverState *active,
> +                  BlockDriverState *base,
>                    const char *backing_file_str, int64_t speed,
>                    BlockdevOnError on_error,
>                    BlockCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
> +    int orig_bs_flags;
>  
>      if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
>           on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
> @@ -231,13 +245,36 @@ 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;
>      }
>  
> +    /* If we are streaming to an intermediate image, we need to block
> +     * the active layer. Due to a race condition, having several block
> +     * jobs running in the same chain is broken and we currently don't
> +     * support it. See here for details:
> +     * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html
> +     */
> +    if (active) {
> +        bdrv_op_block_all(active, s->common.blocker);

block_job_create() unblocks BLOCK_OP_TYPE_DATAPLANE. Maybe this should
do the same?

No other objections from me.

Max

> +        /* Hold a reference to the active layer, otherwise
> +         * bdrv_close_all() will destroy it if we shut down the VM */
> +        bdrv_ref(active);
> +    }
> +
>      s->base = base;
> +    s->active = active;
>      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);
> diff --git a/blockdev.c b/blockdev.c
> index d1f5dfb..2e7712e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3038,7 +3038,7 @@ void qmp_block_stream(const char *device,
>      /* backing_file string overrides base bs filename */
>      base_name = has_backing_file ? backing_file : base_name;
>  
> -    stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
> +    stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
>                   on_error, block_job_cb, bs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 10d8759..0a53d5f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -597,6 +597,8 @@ int is_windows_drive(const char *filename);
>  /**
>   * stream_start:
>   * @bs: Block device to operate on.
> + * @active: The active layer of the chain where @bs belongs, or %NULL
> + *          if @bs is already the topmost device.
>   * @base: Block device that will become the new base, or %NULL to
>   * flatten the whole backing file chain onto @bs.
>   * @base_id: The file name that will be written to @bs as the new
> @@ -613,7 +615,8 @@ int is_windows_drive(const char *filename);
>   * streaming job, the backing file of @bs will be changed to
>   * @base_id in the written image and to @base in the live BlockDriverState.
>   */
> -void stream_start(BlockDriverState *bs, BlockDriverState *base,
> +void stream_start(BlockDriverState *bs, BlockDriverState *active,
> +                  BlockDriverState *base,
>                    const char *base_id, int64_t speed, BlockdevOnError 
> on_error,
>                    BlockCompletionFunc *cb,
>                    void *opaque, Error **errp);
>

signature.asc
Description: OpenPGP digital signature