| Message ID | 1457970292-12291-2-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 --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)
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(-)