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

[Fam Zheng]

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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>  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
> 
>