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

[Fam Zheng <famz@redhat.com>]

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;
    }

Fam

> 
> > +                           &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        bdrv_unref(target_bs);
> > +    }
> > +
> > +    aio_context_release(aio_context);
> > +}
> > +
> 
> Max
>