qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] block: Support streaming to an


From: Max Reitz
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] block: Support streaming to an intermediate layer
Date: Wed, 15 Apr 2015 18:09:18 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 08.04.2015 16:43, 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>
---
  block.c        |  4 +++-
  block/stream.c | 36 ++++++++++++++++++++++++++++++++++++
  2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 25289ef..e892cb4 100644
--- a/block.c
+++ b/block.c
@@ -1240,9 +1240,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..327d964 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,
@@ -88,8 +90,15 @@ static void stream_complete(BlockJob *job, void *opaque)
  {
      StreamBlockJob *s = container_of(job, StreamBlockJob, common);
      StreamCompleteData *data = opaque;
+    BlockDriverState *i;

I'd prefer another name. "i" is generally used for integers used as indices.

      BlockDriverState *base = s->base;
+ /* Remove all blockers set in stream_start() */
+    for (i = job->bs->backing_hd; i && i != s->base; i = i->backing_hd) {
+        bdrv_op_unblock_all(i, s->blocker);
+    }
+    error_free(s->blocker);
+
      if (!block_job_is_cancelled(&s->common) && data->reached_end &&
          data->ret == 0) {
          const char *base_id = NULL, *base_fmt = NULL;
@@ -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 *i;

Again, just "i" is a bit strange to read...

      StreamBlockJob *s;
+    int orig_bs_flags;
if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
           on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
@@ -255,13 +271,33 @@ void stream_start(BlockDriverState *bs, BlockDriverState 
*base,
          return;
      }
+ /* Make sure that the image in opened in read-write mode */

s/in/is/

+    orig_bs_flags = bdrv_get_flags(bs);
+    if (!(orig_bs_flags & BDRV_O_RDWR)) {

I feel like we don't want to do this if we're not streaming to an intermediate layer but to the top layer (because that means there is some reason for the BDS to be read-only beyond it just being a backing BDS).

+        Error *local_err = NULL;
+        bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);

Shorter alternative: "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 */

Hm, do we really need to? There's a difference between "it doesn't make sense, but it works if you want to" and "it will break". Shouldn't it be enough that the intermediate nodes are all read-only anyway (hopefully)?

But then again, it probably won't hurt and I don't really want to think about the implications of trying to run a block-commit or a separate block-stream on the chain...

+    error_setg(&s->blocker, "blocked by the block-stream operation in '%s'",
+               bdrv_get_node_name(bs));

Why not bdrv_get_device_or_node_name()?

+    for (i = bs->backing_hd; i != base && i != NULL; i = i->backing_hd) {
+        bdrv_op_block_all(i, 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);

So, in general this patch looks OK to me. I had some nitpicks (..._device_or_node_name, "i", ...), and I think we should not try to re-open the target BDS if streaming to the top level.

Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]