Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality
Am 25.09.2012 18:29, schrieb Jeff Cody: This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself. The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion. Signed-off-by: Jeff Cody jc...@redhat.com Reviewed-by: Kevin Wolf kw...@redhat.com
Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality
On 09/25/2012 10:29 AM, Jeff Cody wrote:
This adds the live commit coroutine. This iteration focuses on the
commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked
through, and anything that exists above 'base' is committed back down
into base. At the end, intermediate images are deleted, and the
chain stitched together. Images are restored to their original open
flags upon completion.
Signed-off-by: Jeff Cody jc...@redhat.com
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+ BlockDriverState *top, int64_t speed,
+ BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp)
+{
+CommitBlockJob *s;
+BlockReopenQueue *reopen_queue = NULL;
+int orig_overlay_flags;
+int orig_base_flags;
+BlockDriverState *overlay_bs;
+Error *local_err = NULL;
+
+if ((on_error == BLOCK_ERR_STOP_ANY ||
+ on_error == BLOCK_ERR_STOP_ENOSPC)
+!bdrv_iostatus_is_enabled(bs)) {
+error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+return;
+}
+
+overlay_bs = bdrv_find_overlay(bs, top);
+
+if (overlay_bs == NULL) {
+error_setg(errp, Could not find overlay image for %s:,
top-filename);
+return;
+}
+
+orig_base_flags= bdrv_get_flags(base);
+orig_overlay_flags = bdrv_get_flags(overlay_bs);
I think you are missing a check here that base is on the backing chain
of top. See also my comments to 5/7.
+
+/* convert base_bs overlay_bs to r/w, if necessary */
+if (!(orig_base_flags BDRV_O_RDWR)) {
+reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+ orig_base_flags | BDRV_O_RDWR);
+}
+if (!(orig_overlay_flags BDRV_O_RDWR)) {
+reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+ orig_overlay_flags | BDRV_O_RDWR);
+}
+if (reopen_queue) {
+bdrv_reopen_multiple(reopen_queue, local_err);
Is it valid to make a no-op call, such as:
{ execute:block-commit, arguments:{
device:drive0, top:base, base:base }}
If so, should we do an early exit here, rather than temporarily changing
base to R/W just to change it back to R/O?
If not, should we be rejecting it up front (again, back to the question
of failing if 'base' is not a backing file of 'top', even if both 'top'
and 'base' are backing files of 'device').
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality
On 09/25/2012 02:12 PM, Eric Blake wrote:
On 09/25/2012 10:29 AM, Jeff Cody wrote:
This adds the live commit coroutine. This iteration focuses on the
commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked
through, and anything that exists above 'base' is committed back down
into base. At the end, intermediate images are deleted, and the
chain stitched together. Images are restored to their original open
flags upon completion.
Signed-off-by: Jeff Cody jc...@redhat.com
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+ BlockDriverState *top, int64_t speed,
+ BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp)
+{
+CommitBlockJob *s;
+BlockReopenQueue *reopen_queue = NULL;
+int orig_overlay_flags;
+int orig_base_flags;
+BlockDriverState *overlay_bs;
+Error *local_err = NULL;
+
+if ((on_error == BLOCK_ERR_STOP_ANY ||
+ on_error == BLOCK_ERR_STOP_ENOSPC)
+!bdrv_iostatus_is_enabled(bs)) {
+error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+return;
+}
+
+overlay_bs = bdrv_find_overlay(bs, top);
+
+if (overlay_bs == NULL) {
+error_setg(errp, Could not find overlay image for %s:,
top-filename);
+return;
+}
+
+orig_base_flags= bdrv_get_flags(base);
+orig_overlay_flags = bdrv_get_flags(overlay_bs);
I think you are missing a check here that base is on the backing chain
of top. See also my comments to 5/7.
Did you mean your comments on 6/7 (or am I missing an email)?
This does get partially validated in patch 5/7, in the
qmp_block_commit() handler - both base and top are verified to be in the
chain 'bs'. What is not validated, however, is that you did not swap
your 'top' and 'base' arguments. I'll add a check here for that, to
make sure that base is reachable from overlay_bs.
+
+/* convert base_bs overlay_bs to r/w, if necessary */
+if (!(orig_base_flags BDRV_O_RDWR)) {
+reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+ orig_base_flags | BDRV_O_RDWR);
+}
+if (!(orig_overlay_flags BDRV_O_RDWR)) {
+reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+ orig_overlay_flags | BDRV_O_RDWR);
+}
+if (reopen_queue) {
+bdrv_reopen_multiple(reopen_queue, local_err);
Is it valid to make a no-op call, such as:
{ execute:block-commit, arguments:{
device:drive0, top:base, base:base }}
If so, should we do an early exit here, rather than temporarily changing
base to R/W just to change it back to R/O?
If not, should we be rejecting it up front (again, back to the question
of failing if 'base' is not a backing file of 'top', even if both 'top'
and 'base' are backing files of 'device').
I'll add a quick check for that as well. Patch 5 checks for it in one
scenario (default 'top'), and returns a generic error of:
Invalid files for merge: top and base are the same
I'll make sure to check for that in all scenarios (or just make the test
mentioned earlier incorporate top == base as well)
Thanks,
Jeff
Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality
On 09/25/2012 12:58 PM, Jeff Cody wrote: On 09/25/2012 02:12 PM, Eric Blake wrote: On 09/25/2012 10:29 AM, Jeff Cody wrote: This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself. I think you are missing a check here that base is on the backing chain of top. See also my comments to 5/7. Did you mean your comments on 6/7 (or am I missing an email)? Shoot. You're right, and I messed myself up by reviewing out of order :) This does get partially validated in patch 5/7, in the qmp_block_commit() handler - both base and top are verified to be in the chain 'bs'. What is not validated, however, is that you did not swap your 'top' and 'base' arguments. I'll add a check here for that, to make sure that base is reachable from overlay_bs. At any rate, it sounds like I got my point across, in spite of myself. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
