Re: [Qemu-devel] [PATCH v3 4/5] qmp: Add blockdev-mirror command

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4407 bytes --]

On 24.12.2015 04:25, Fam Zheng wrote:
> On Thu, 12/24 01:53, Max Reitz wrote:
>> On 23.12.2015 06:59, Fam Zheng wrote:
>>> This will start a mirror job from a named device to another named
>>> device, its relation with drive-mirror is similar with blockdev-backup
>>> to drive-backup.
>>>
>>> In blockdev-mirror, the target node should be prepared by blockdev-add,
>>> which will be responsible for assigning a name to the new node, so
>>> we don't have 'node-name' parameter.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  blockdev.c           | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++
>>>  qmp-commands.hx      | 48 ++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 157 insertions(+)
>>
>> It appears you haven't addressed the comments for v2. I only had a
>> single one (regarding documentation), but Markus had a couple ones, so
>> those may be worth addressing.
> 
> Will look into that.
> 
>>
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index f42e171..2df0c6d 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3345,6 +3345,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
>>>      if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
>>>          return;
>>>      }
>>> +    if (target->blk) {
>>> +        error_setg(errp, "Cannot mirror to an attached block device");
>>> +        return;
>>> +    }
>>>  
>>>      if (!bs->backing && sync == MIRROR_SYNC_MODE_TOP) {
>>>          sync = MIRROR_SYNC_MODE_FULL;
>>> @@ -3518,6 +3522,64 @@ out:
>>>      aio_context_release(aio_context);
>>>  }
>>>  
>>> +void qmp_blockdev_mirror(const char *device, const char *target,
>>> +                         bool has_replaces, const char *replaces,
>>> +                         MirrorSyncMode sync,
>>> +                         bool has_speed, int64_t speed,
>>> +                         bool has_granularity, uint32_t granularity,
>>> +                         bool has_buf_size, int64_t buf_size,
>>> +                         bool has_on_source_error,
>>> +                         BlockdevOnError on_source_error,
>>> +                         bool has_on_target_error,
>>> +                         BlockdevOnError on_target_error,
>>> +                         Error **errp)
>>> +{
>>> +    BlockDriverState *bs;
>>> +    BlockBackend *blk;
>>> +    BlockDriverState *target_bs;
>>> +    AioContext *aio_context;
>>> +    Error *local_err = NULL;
>>> +
>>> +    blk = blk_by_name(device);
>>> +    if (!blk) {
>>> +        error_setg(errp, "Device '%s' not found", device);
>>> +        return;
>>> +    }
>>> +    bs = blk_bs(blk);
>>> +
>>> +    if (!bs) {
>>> +        error_setg(errp, "Device '%s' has no media", device);
>>> +        return;
>>> +    }
>>> +
>>> +    target_bs = bdrv_lookup_bs(target, target, errp);
>>> +    if (!target_bs) {
>>> +        return;
>>> +    }
>>> +
>>> +    aio_context = bdrv_get_aio_context(bs);
>>> +    aio_context_acquire(aio_context);
>>> +
>>> +    bdrv_ref(target_bs);
>>> +    bdrv_set_aio_context(target_bs, aio_context);
>>> +
>>> +    blockdev_mirror_common(bs, target_bs,
>>> +                           has_replaces, replaces, sync,
>>> +                           has_speed, speed,
>>> +                           has_granularity, granularity,
>>> +                           has_buf_size, buf_size,
>>> +                           has_on_source_error, on_source_error,
>>> +                           has_on_target_error, on_target_error,
>>> +                           true, true,
>>
>> Shouldn't this be "false, false,", or, ideally, set by the user?
> 
> I think true is correct here because then it will be effectively controlled by
> open flags of target. I.e. mirror.c always sets BDRV_REQ_MAY_UNMAP, and
> bdrv_co_write_zeroes has:
> 
>     if (!(bs->open_flags & BDRV_O_UNMAP)) {
>         flags &= ~BDRV_REQ_MAY_UNMAP;
>     }

I was asking because it differs from what drive-mirror does - but that
is probably a good thing (drive-mirror takes this flag from the user
(defaulting to false, which is why I was asking), but it takes the
open_flags for the new image from the mirror source, which is...
Interesting.

So it's probably better this way, right.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]