[RFC,COLO,v2,04/13] Add new block driver interfaces to control block replication

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>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 block.c                   | 39 +++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  4 ++++
 include/block/block_int.h | 11 +++++++++++
 qapi/block.json           | 16 ++++++++++++++++
 4 files changed, 70 insertions(+)

On 25/03/2015 10:36, 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>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  block.c                   | 39 +++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h | 11 +++++++++++
>  qapi/block.json           | 16 ++++++++++++++++
>  4 files changed, 70 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 0fe97de..0ff5cf8 100644
> --- a/block.c
> +++ b/block.c
> @@ -6196,3 +6196,42 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
>  {
>      return &bs->stats;
>  }
> +
> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_start_replication) {
> +        drv->bdrv_start_replication(bs, mode, errp);
> +    } else if (bs->file) {
> +        bdrv_start_replication(bs->file, mode, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_do_checkpoint) {
> +        drv->bdrv_do_checkpoint(bs, errp);
> +    } else if (bs->file) {
> +        bdrv_do_checkpoint(bs->file, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_stop_replication) {
> +        drv->bdrv_stop_replication(bs, errp);
> +    } else if (bs->file) {
> +        bdrv_stop_replication(bs->file, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 4c57d63..68f3b1a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -569,4 +569,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
>  
>  BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
>  
> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp);
> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp);
> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index dccb092..08dd8ba 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -290,6 +290,17 @@ struct BlockDriver {
>       */
>      int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
>  
> +
> +    void (*bdrv_start_replication)(BlockDriverState *bs, COLOMode mode,
> +                                   Error **errp);
> +    /* Drop Disk buffer when doing checkpoint. */
> +    void (*bdrv_do_checkpoint)(BlockDriverState *bs, Error **errp);
> +    /*
> +     * After failover, we should flush Disk buffer into secondary disk
> +     * and stop block replication.
> +     */
> +    void (*bdrv_stop_replication)(BlockDriverState *bs, Error **errp);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> diff --git a/qapi/block.json b/qapi/block.json
> index e313465..e640566 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -40,6 +40,22 @@
>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>  
>  ##
> +# @COLOMode
> +#
> +# An enumeration of COLO mode.
> +#
> +# @unprotected: COLO is not started or after failover
> +#
> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU.
> +#
> +# @secondary: Secondary mode, receive the vm's state from primary QEMU.
> +#
> +# Since: 2.4
> +##
> +{ 'enum' : 'COLOMode',
> +  'data' : ['unprotected', 'primary', 'secondary']}

Perhaps replace COLOMode with ReplicationMode or FaultToleranceMode?

> +##
>  # @BlockdevSnapshotInternal
>  #
>  # @device: the name of the device to generate the snapshot from
> 

Apart from this,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

On 03/25/2015 06:48 AM, Paolo Bonzini wrote:

>>  ##
>> +# @COLOMode
>> +#
>> +# An enumeration of COLO mode.

s/mode/modes/

>> +#
>> +# @unprotected: COLO is not started or after failover
>> +#
>> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU.

Inconsistent on whether you end with '.'

>> +#
>> +# @secondary: Secondary mode, receive the vm's state from primary QEMU.
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'enum' : 'COLOMode',
>> +  'data' : ['unprotected', 'primary', 'secondary']}
> 
> Perhaps replace COLOMode with ReplicationMode or FaultToleranceMode?

If you keep the name COLOMode, it would also be nice to spell out
'COurse-grain LOck-stepping' at least once somewhere in the .json file
(probably in the description of this enum).  A name like ReplicationMode
is a bit more self-documenting (I know what replication is, I have to
look up what COLO is).

On Wed, 03/25 17:36, 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>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  block.c                   | 39 +++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h | 11 +++++++++++
>  qapi/block.json           | 16 ++++++++++++++++
>  4 files changed, 70 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 0fe97de..0ff5cf8 100644
> --- a/block.c
> +++ b/block.c
> @@ -6196,3 +6196,42 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
>  {
>      return &bs->stats;
>  }
> +
> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_start_replication) {
> +        drv->bdrv_start_replication(bs, mode, errp);
> +    } else if (bs->file) {
> +        bdrv_start_replication(bs->file, mode, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);

I think we should use error_setg in new code? (The same to following ones)

> +    }
> +}
> +
> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_do_checkpoint) {
> +        drv->bdrv_do_checkpoint(bs, errp);
> +    } else if (bs->file) {
> +        bdrv_do_checkpoint(bs->file, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (drv && drv->bdrv_stop_replication) {
> +        drv->bdrv_stop_replication(bs, errp);
> +    } else if (bs->file) {
> +        bdrv_stop_replication(bs->file, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 4c57d63..68f3b1a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -569,4 +569,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
>  
>  BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
>  
> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp);
> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp);
> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index dccb092..08dd8ba 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -290,6 +290,17 @@ struct BlockDriver {
>       */
>      int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
>  
> +
> +    void (*bdrv_start_replication)(BlockDriverState *bs, COLOMode mode,
> +                                   Error **errp);

Need some documentation, but I have a generic question:

Why is a single interface with modes better than different functions for each
mode (bdrv_start_replication_{primary,secondary}? Asking because the behavior
is very different between them, and I don't see much sharing -- you implement
primary operation in quorum, and secondary in qcow2+colo.

> +    /* Drop Disk buffer when doing checkpoint. */
> +    void (*bdrv_do_checkpoint)(BlockDriverState *bs, Error **errp);
> +    /*
> +     * After failover, we should flush Disk buffer into secondary disk
> +     * and stop block replication.
> +     */
> +    void (*bdrv_stop_replication)(BlockDriverState *bs, Error **errp);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> diff --git a/qapi/block.json b/qapi/block.json
> index e313465..e640566 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -40,6 +40,22 @@
>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>  
>  ##
> +# @COLOMode
> +#
> +# An enumeration of COLO mode.
> +#
> +# @unprotected: COLO is not started or after failover
> +#
> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU.
> +#
> +# @secondary: Secondary mode, receive the vm's state from primary QEMU.
> +#
> +# Since: 2.4
> +##
> +{ 'enum' : 'COLOMode',
> +  'data' : ['unprotected', 'primary', 'secondary']}

If split bdrv_start_replication, do we still need an enum? I can't find the
usage in QMP interface, is it in some other series?

Fam

> +
> +##
>  # @BlockdevSnapshotInternal
>  #
>  # @device: the name of the device to generate the snapshot from
> -- 
> 2.1.0
>

On 03/26/2015 03:12 PM, Fam Zheng wrote:
> On Wed, 03/25 17:36, 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>
>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>  block.c                   | 39 +++++++++++++++++++++++++++++++++++++++
>>  include/block/block.h     |  4 ++++
>>  include/block/block_int.h | 11 +++++++++++
>>  qapi/block.json           | 16 ++++++++++++++++
>>  4 files changed, 70 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 0fe97de..0ff5cf8 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6196,3 +6196,42 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
>>  {
>>      return &bs->stats;
>>  }
>> +
>> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +
>> +    if (drv && drv->bdrv_start_replication) {
>> +        drv->bdrv_start_replication(bs, mode, errp);
>> +    } else if (bs->file) {
>> +        bdrv_start_replication(bs->file, mode, errp);
>> +    } else {
>> +        error_set(errp, QERR_UNSUPPORTED);
> 
> I think we should use error_setg in new code? (The same to following ones)

Hmm, do you mean that don't use QERR_UNSUPPORTED here?

> 
>> +    }
>> +}
>> +
>> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +
>> +    if (drv && drv->bdrv_do_checkpoint) {
>> +        drv->bdrv_do_checkpoint(bs, errp);
>> +    } else if (bs->file) {
>> +        bdrv_do_checkpoint(bs->file, errp);
>> +    } else {
>> +        error_set(errp, QERR_UNSUPPORTED);
>> +    }
>> +}
>> +
>> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +
>> +    if (drv && drv->bdrv_stop_replication) {
>> +        drv->bdrv_stop_replication(bs, errp);
>> +    } else if (bs->file) {
>> +        bdrv_stop_replication(bs->file, errp);
>> +    } else {
>> +        error_set(errp, QERR_UNSUPPORTED);
>> +    }
>> +}
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 4c57d63..68f3b1a 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -569,4 +569,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
>>  
>>  BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
>>  
>> +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp);
>> +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp);
>> +void bdrv_stop_replication(BlockDriverState *bs, Error **errp);
>> +
>>  #endif
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index dccb092..08dd8ba 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -290,6 +290,17 @@ struct BlockDriver {
>>       */
>>      int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
>>  
>> +
>> +    void (*bdrv_start_replication)(BlockDriverState *bs, COLOMode mode,
>> +                                   Error **errp);
> 
> Need some documentation, but I have a generic question:
> 
> Why is a single interface with modes better than different functions for each
> mode (bdrv_start_replication_{primary,secondary}? Asking because the behavior
> is very different between them, and I don't see much sharing -- you implement
> primary operation in quorum, and secondary in qcow2+colo.

No special reason.

> 
>> +    /* Drop Disk buffer when doing checkpoint. */
>> +    void (*bdrv_do_checkpoint)(BlockDriverState *bs, Error **errp);
>> +    /*
>> +     * After failover, we should flush Disk buffer into secondary disk
>> +     * and stop block replication.
>> +     */
>> +    void (*bdrv_stop_replication)(BlockDriverState *bs, Error **errp);
>> +
>>      QLIST_ENTRY(BlockDriver) list;
>>  };
>>  
>> diff --git a/qapi/block.json b/qapi/block.json
>> index e313465..e640566 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -40,6 +40,22 @@
>>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>>  
>>  ##
>> +# @COLOMode
>> +#
>> +# An enumeration of COLO mode.
>> +#
>> +# @unprotected: COLO is not started or after failover
>> +#
>> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU.
>> +#
>> +# @secondary: Secondary mode, receive the vm's state from primary QEMU.
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'enum' : 'COLOMode',
>> +  'data' : ['unprotected', 'primary', 'secondary']}
> 
> If split bdrv_start_replication, do we still need an enum? I can't find the
> usage in QMP interface, is it in some other series?

I will check it.

Thanks
Wen Congyang

> 
> Fam
> 
>> +
>> +##
>>  # @BlockdevSnapshotInternal
>>  #
>>  # @device: the name of the device to generate the snapshot from
>> -- 
>> 2.1.0
>>
> .
>