[Qemu-devel] [PULL 02/48] block: Remove copy-on-read from bdrv_move_feature_fields()

From: Kevin Wolf <kwolf@redhat.com>
To: qemu-block@nongnu.org
Cc: kwolf@redhat.com, qemu-devel@nongnu.org
Subject: [Qemu-devel] [PULL 02/48] block: Remove copy-on-read from bdrv_move_feature_fields()
Date: Tue, 29 Mar 2016 17:08:02 +0200	[thread overview]
Message-ID: <1459264128-12761-3-git-send-email-kwolf@redhat.com> (raw)
In-Reply-To: <1459264128-12761-1-git-send-email-kwolf@redhat.com>

Ever since we first introduced bdrv_append() in commit 8802d1fd ('qapi:
Introduce blockdev-group-snapshot-sync command'), the copy-on-read flag
was moved to the new top layer when taking a snapshot. The only problem
is that it doesn't make a whole lot of sense.

The use case for manually enabled CoR is to avoid reading data twice
from a slow remote image, so we want to save it to a local overlay, say
an ISO image accessed via HTTP to a local qcow2 overlay. When taking a
snapshot, we end up with a backing chain like this:

    http <- local.qcow2 <- snap_overlay.qcow2

There is no point in doing CoR from local.qcow2 into snap_overlay.qcow2,
we just want to keep copying data from the remote source into
local.qcow2.

The other use case of CoR is in the context of streaming, which isn't
very interesting for bdrv_move_feature_fields() because op blockers
prevent this combination.

This patch makes the copy-on-read flag stay on the image for which it
was originally set and prevents it from being propagated to the new
overlay. It is no longer intended to move CoR to the BlockBackend level.
In order for this to make sense, we also need to keep the respective
image read-write.

As a side effect of these changes, creating a live snapshot image (as
opposed to using an existing externally created one) on top of a COR
block device works now. It used to fail because it tried to open its
backing file both read-only and with COR.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c    | 2 --
 blockdev.c | 7 +++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index e0b280b..ee34c8c 100644
--- a/block.c
+++ b/block.c
@@ -2248,8 +2248,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* move some fields that need to stay attached to the device */
 
     /* dev info */
-    bs_dest->copy_on_read       = bs_src->copy_on_read;
-
     bs_dest->enable_write_cache = bs_src->enable_write_cache;
 
     /* dirty bitmap */
diff --git a/blockdev.c b/blockdev.c
index 35f8515..914b526 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1741,6 +1741,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         }
 
         flags = state->old_bs->open_flags;
+        flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
 
         /* create new image w/backing file */
         mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -1811,8 +1812,10 @@ static void external_snapshot_commit(BlkActionState *common)
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
-    bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-                NULL);
+    if (!state->old_bs->copy_on_read) {
+        bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
+                    NULL);
+    }
 }
 
 static void external_snapshot_abort(BlkActionState *common)
-- 
1.8.3.1
