diff mbox

[1/5] block: Remove copy-on-read from bdrv_move_feature_fields()

Message ID 1457970292-12291-2-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf March 14, 2016, 3:44 p.m. UTC
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>
---
 block.c    | 2 --
 blockdev.c | 7 +++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Eric Blake March 14, 2016, 3:51 p.m. UTC | #1
On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> 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.

Your arguments make sense.

> 
> 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>
> ---
>  block.c    | 2 --
>  blockdev.c | 7 +++++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/block.c b/block.c
index cf5eb34..a75c4b3 100644
--- a/block.c
+++ b/block.c
@@ -2283,8 +2283,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 322ca03..a22b444 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1728,6 +1728,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;
@@ -1798,8 +1799,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)