Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 12.10.2016 um 16:33 hat Alberto Garcia geschrieben:
> On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
> >> +/* Block all intermediate nodes between bs and base, because they
> >> + * will disappear from the chain after this operation */
> >> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> >> backing_bs(iter)) {
> >> +block_job_add_bdrv(>common, iter);
> >> +}
> >
> > In patch 6, you had a comment that the top node must be blocked as
> > well because its backing file string will be updated. Isn't it the
> > same for streaming?
> 
> In the block-stream case, 'device' is always the top node, and creating
> the block job there already blocks all operations in it.
> 
> In the block-commit case, 'device' and 'top' are different parameters,
> that's why 'top' must be explicitly blocked. I actually don't think that
> we need to block 'device' at all, and we could even make the parameter
> optional. That would be for a future patch, though. Also, the backing
> file string is not updated in 'top', but in its overlay (unlike in
> block-stream, 'top' disappears after a block-commit job).

I see. So the block job for commit is owned by a node that is
potentially completely unrelated to the operation except that it holds
op blockers and because we use it to finde the overlay to change the
backing file pointers. This is _so_ broken. :-)

With your series (for the op blockers) and BdrvChild (for the operations
on the overlay) we should actually be much closer to finally remove
this. Good news.

Kevin

Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
>> +/* Block all intermediate nodes between bs and base, because they
>> + * will disappear from the chain after this operation */
>> +for (iter = backing_bs(bs); iter && iter != base; iter = 
>> backing_bs(iter)) {
>> +block_job_add_bdrv(>common, iter);
>> +}
>
> In patch 6, you had a comment that the top node must be blocked as
> well because its backing file string will be updated. Isn't it the
> same for streaming?

In the block-stream case, 'device' is always the top node, and creating
the block job there already blocks all operations in it.

In the block-commit case, 'device' and 'top' are different parameters,
that's why 'top' must be explicitly blocked. I actually don't think that
we need to block 'device' at all, and we could even make the parameter
optional. That would be for a future patch, though. Also, the backing
file string is not updated in 'top', but in its overlay (unlike in
block-stream, 'top' disappears after a block-commit job).

Berto

Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This makes sure that the image we are streaming 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 
> ---
>  block.c|  4 +++-
>  block/stream.c | 24 
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c80b528..8255d75 100644
> --- a/block.c
> +++ b/block.c
> @@ -1428,9 +1428,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);
>  /*
>   * We do backup in 3 ways:
>   * 1. drive backup
> diff --git a/block/stream.c b/block/stream.c
> index 3187481..b8ab89a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -37,6 +37,7 @@ typedef struct StreamBlockJob {
>  BlockDriverState *base;
>  BlockdevOnError on_error;
>  char *backing_file_str;
> +int bs_flags;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>  bdrv_set_backing_hd(bs, base);
>  }
>  
> +/* Reopen the image back in read-only mode if necessary */
> +if (s->bs_flags != bdrv_get_flags(bs)) {
> +bdrv_reopen(bs, s->bs_flags, NULL);
> +}
> +
>  g_free(s->backing_file_str);
>  block_job_completed(>common, data->ret);
>  g_free(data);
> @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>BlockCompletionFunc *cb, void *opaque, Error **errp)
>  {
>  StreamBlockJob *s;
> +BlockDriverState *iter;
> +int orig_bs_flags;
>  
>  s = block_job_create(job_id, _job_driver, bs, speed,
>   cb, opaque, errp);
> @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>  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) {
> +block_job_unref(>common);
> +return;
> +}
> +}
> +
> +/* Block all intermediate nodes between bs and base, because they
> + * will disappear from the chain after this operation */
> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> backing_bs(iter)) {
> +block_job_add_bdrv(>common, iter);
> +}

In patch 6, you had a comment that the top node must be blocked as well
because its backing file string will be updated. Isn't it the same for
streaming?

>  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, s);

Kevin