Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit'

[Jeff Cody <jcody@redhat.com>]

On 09/06/2012 10:29 AM, Kevin Wolf wrote:
> Am 30.08.2012 20:47, schrieb Jeff Cody:
>> The command for live block commit is added, which has the following
>> arguments:
>>
>> device: the block device to perform the commit on (mandatory)
>> base:   the base image to commit into; optional (if not specified,
>>         it is the underlying original image)
>> top:    the top image of the commit - all data from inside top down
>>         to base will be committed into base. optional (if not specified,
>>         it is the active image) - see note below
>> speed:  maximum speed, in bytes/sec
>>
>> note: eventually this will support merging down the active layer,
>>       but that code is not yet complete.  If the active layer is passed
>>       in currently as top, or top is left to the default, then the error
>>       QERR_TOP_NOT_FOUND will be returned.
>>
>> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will
>> be emitted.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  blockdev.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi-schema.json | 30 ++++++++++++++++++++
>>  qmp-commands.hx  |  6 ++++
>>  3 files changed, 119 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 68d65fb..e0d6ca0 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -827,6 +827,89 @@ exit:
>>      return;
>>  }
>>  
>> +void qmp_block_commit(const char *device,
>> +                      bool has_base, const char *base,
>> +                      bool has_top, const char *top,
>> +                      bool has_speed, int64_t speed,
>> +                      Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    BlockDriverState *base_bs, *top_bs, *child_bs;
>> +    Error *local_err = NULL;
>> +    int orig_base_flags, orig_top_flags;
>> +    BlockReopenQueue *reopen_queue = NULL;
>> +    /* This will be part of the QMP command, if/when the
>> +     * BlockdevOnError change for blkmirror makes it in
>> +     */
>> +    BlockErrorAction on_error = BLOCK_ERR_REPORT;
>> +
>> +    /* drain all i/o before commits */
>> +    bdrv_drain_all();
>> +
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> +        return;
>> +    }
>> +    if (base && has_base) {
>> +        base_bs = bdrv_find_backing_image(bs, base);
>> +    } else {
>> +        base_bs = bdrv_find_base(bs);
>> +    }
>> +
>> +    if (base_bs == NULL) {
>> +        error_set(errp, QERR_BASE_NOT_FOUND, "NULL");
> 
> Shouldn't it be base rather than "NULL"?
>

Yes

>> +        return;
>> +    }
>> +
>> +    if (top && has_top) {
>> +        /* if we want to allow the active layer,
>> +         * use 'bdrv_find_image()' here */
>> +        top_bs = bdrv_find_backing_image(bs, top);
>> +        if (top_bs == NULL) {
>> +            error_set(errp, QERR_TOP_NOT_FOUND, top);
>> +            return;
>> +        }
>> +    } else {
>> +        /* we will eventually default to the top layer,i.e. top_bs = bs */
>> +        error_set(errp, QERR_TOP_NOT_FOUND, top);
>> +        return;
>> +    }
>> +
>> +    child_bs = bdrv_find_child(bs, top_bs);
>> +
>> +    orig_base_flags = bdrv_get_flags(base_bs);  /* what we are writing into   */
>> +    orig_top_flags = bdrv_get_flags(child_bs);  /* to change the backing file */
>> +
>> +    /* convert base_bs to r/w, if necessary */
>> +    if (!(orig_base_flags & BDRV_O_RDWR)) {
>> +        reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs,
>> +                                         orig_base_flags | BDRV_O_RDWR);
>> +    }
>> +    if (!(orig_top_flags & BDRV_O_RDWR)) {
>> +        reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs,
>> +                                         orig_base_flags | BDRV_O_RDWR);
> 
> I think you mean child_bs and orig_top_flags. (Why isn't it called
> orig_child_flags?)
> 

Yes, thanks - and (per your comments on the previous patch), I'll move
the reopen bits into commit_start().

>> +    }
>> +    if (reopen_queue) {
>> +        bdrv_reopen_multiple(reopen_queue, &local_err);
>> +        if (local_err != NULL) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +
>> +    commit_start(bs, base_bs, top_bs, speed, on_error,
>> +                 block_job_cb, bs, orig_base_flags, orig_top_flags,
>> +                 &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +    /* Grab a reference so hotplug does not delete the BlockDriverState from
>> +     * underneath us.
>> +     */
>> +    drive_get_ref(drive_get_by_blockdev(bs));
>> +}
>>  
>>  static void eject_device(BlockDriverState *bs, int force, Error **errp)
>>  {
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index bd8ad74..45feda6 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1401,6 +1401,36 @@
>>    'returns': 'str' }
>>  
>>  ##
>> +# @block-commit
>> +#
>> +# Live commit of data from child image nodes into parent nodes - i.e.,
>> +# writes data between 'top' and 'base' into 'base'.
>> +#
>> +# @device:  the name of the device
>> +#
>> +# @base:   #optional The parent image of the device to write data into.
>> +#                    If not specified, this is the original parent image.
> 
> "The file name of the parent image", actually.
> 
> Which I'm afraid means that this isn't an API for the long term, but
> there's little we can do about it now...
> 
>> +#
>> +# @top:    #optional The child image, above which data will not be committed
>> +#                    down.  If not specified, this is the active layer.
> 
> Again, the file name.
> 
> Kevin
>