Re: [Qemu-devel] [RFC PATCH 04/14] Add new block driver interfaces to control disk replication

[Max Reitz <mreitz@redhat.com>]

On 2015-02-11 at 22:07, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>   block.c                   | 36 ++++++++++++++++++++++++++++++++++++
>   include/block/block.h     | 10 ++++++++++
>   include/block/block_int.h | 12 ++++++++++++
>   3 files changed, 58 insertions(+)
>
> diff --git a/block.c b/block.c
> index 210fd5f..2335af1 100644
> --- a/block.c
> +++ b/block.c
> @@ -6156,3 +6156,39 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
>   {
>       return &bs->stats;
>   }
> +
> +int bdrv_start_replication(BlockDriverState *bs, int mode)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (drv && drv->bdrv_start_replication) {
> +        return drv->bdrv_start_replication(bs, mode);
> +    } else if (bs->file) {
> +        return bdrv_start_replication(bs->file, mode);
> +    }
> +
> +    return -1;

I'd prefer returning -errno here (like -ENOTSUP). Alternatively, you may 
want to use Error objects (which would probably actually be the 
preferable way).

> +}
> +
> +int bdrv_do_checkpoint(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (drv && drv->bdrv_do_checkpoint) {
> +        return drv->bdrv_do_checkpoint(bs);
> +    } else if (bs->file) {
> +        return bdrv_do_checkpoint(bs->file);
> +    }
> +
> +    return -1;

Same here.

> +}
> +
> +int bdrv_stop_replication(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (drv && drv->bdrv_stop_replication) {
> +        return drv->bdrv_stop_replication(bs);
> +    } else if (bs->file) {
> +        return bdrv_stop_replication(bs->file);
> +    }
> +
> +    return -1;

And here.

> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 321295e..632b9fc 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -557,4 +557,14 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
>   
>   BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
>   
> +/* Checkpoint control, called in migration/checkpoint thread */
> +enum {
> +    COLO_UNPROTECTED_MODE = 0,
> +    COLO_PRIMARY_MODE,
> +    COLO_SECONDARY_MODE,
> +};

I have a feeling that you may want to define these values through QAPI...

There's nothing wrong with this patch, but I don't yet really know what 
you want to do with these functions (the doc didn't really help me with 
them), so I'll have to look into the rest of the series before I can 
really say something useful about it.

Max

> +int bdrv_start_replication(BlockDriverState *bs, int mode);
> +int bdrv_do_checkpoint(BlockDriverState *bs);
> +int bdrv_stop_replication(BlockDriverState *bs);
> +
>   #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7ad1950..603f704 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -273,6 +273,18 @@ struct BlockDriver {
>       void (*bdrv_io_unplug)(BlockDriverState *bs);
>       void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>   
> +
> +    /* Checkpoint control, called in migration/checkpoint thread */
> +    int (*bdrv_start_replication)(BlockDriverState *bs, int mode);
> +    /*
> +     * Drop Disk buffer when doing checkpoint.
> +     */
> +    int (*bdrv_do_checkpoint)(BlockDriverState *bs);
> +    /* After failover, we should flush Disk buffer into secondary disk
> +     * and stop block replication.
> +     */
> +    int (*bdrv_stop_replication)(BlockDriverState *bs);
> +
>       QLIST_ENTRY(BlockDriver) list;
>   };
>