[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionali
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality |
Date: |
Tue, 25 Sep 2012 14:58:15 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0 |
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 <address@hidden>
>
>> +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
- [Qemu-devel] [PATCH v2 0/7] Live block commit, Jeff Cody, 2012/09/25
- [Qemu-devel] [PATCH v2 1/7] block: add support functions for live commit, to find and delete images., Jeff Cody, 2012/09/25
- [Qemu-devel] [PATCH v2 3/7] blockdev: rename block_stream_cb to a generic block_job_cb, Jeff Cody, 2012/09/25
- [Qemu-devel] [PATCH v2 4/7] block: helper function, to find the base image of a chain, Jeff Cody, 2012/09/25
- [Qemu-devel] [PATCH v2 5/7] QAPI: add command for live block commit, 'block-commit', Jeff Cody, 2012/09/25