qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v15 7/9] Introduce new APIs to do replication op


From: Wen Congyang
Subject: Re: [Qemu-devel] [PATCH v15 7/9] Introduce new APIs to do replication operation
Date: Fri, 19 Feb 2016 16:55:18 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 02/19/2016 04:41 PM, Hailiang Zhang wrote:
> Hi,
> 
> On 2016/2/15 9:13, Wen Congyang wrote:
>> On 02/15/2016 08:57 AM, Hailiang Zhang wrote:
>>> On 2016/2/5 12:18, Changlong Xie wrote:
>>>> Signed-off-by: Wen Congyang <address@hidden>
>>>> Signed-off-by: zhanghailiang <address@hidden>
>>>> Signed-off-by: Gonglei <address@hidden>
>>>> Signed-off-by: Changlong Xie <address@hidden>
>>>> ---
>>>>    Makefile.objs        |  1 +
>>>>    qapi/block-core.json | 13 ++++++++
>>>>    replication.c        | 94 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    replication.h        | 53 +++++++++++++++++++++++++++++
>>>>    4 files changed, 161 insertions(+)
>>>>    create mode 100644 replication.c
>>>>    create mode 100644 replication.h
>>>>
>>>> diff --git a/Makefile.objs b/Makefile.objs
>>>> index 06b95c7..a8c74b7 100644
>>>> --- a/Makefile.objs
>>>> +++ b/Makefile.objs
>>>> @@ -15,6 +15,7 @@ block-obj-$(CONFIG_POSIX) += aio-posix.o
>>>>    block-obj-$(CONFIG_WIN32) += aio-win32.o
>>>>    block-obj-y += block/
>>>>    block-obj-y += qemu-io-cmds.o
>>>> +block-obj-y += replication.o
>>>>
>>>>    block-obj-m = block/
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 7e9e8fe..12362b8 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2002,6 +2002,19 @@
>>>>                '*read-pattern': 'QuorumReadPattern' } }
>>>>
>>>>    ##
>>>> +# @ReplicationMode
>>>> +#
>>>> +# An enumeration of replication modes.
>>>> +#
>>>> +# @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.6
>>>> +##
>>>> +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] }
>>>> +
>>>> +##
>>>>    # @BlockdevOptions
>>>>    #
>>>>    # Options for creating a block device.
>>>> diff --git a/replication.c b/replication.c
>>>> new file mode 100644
>>>> index 0000000..e8ac2f0
>>>> --- /dev/null
>>>> +++ b/replication.c
>>>> @@ -0,0 +1,94 @@
>>>> +/*
>>>> + * Replication filter
>>>> + *
>>>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
>>>> + * Copyright (c) 2016 Intel Corporation
>>>> + * Copyright (c) 2016 FUJITSU LIMITED
>>>> + *
>>>> + * Author:
>>>> + *   Wen Congyang <address@hidden>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "replication.h"
>>>> +
>>>> +static QLIST_HEAD(, ReplicationState) replication_states;
>>>> +
>>>> +ReplicationState *replication_new(void *opaque, ReplicationOps *ops)
>>>> +{
>>>> +    ReplicationState *rs;
>>>> +
>>>> +    rs = g_new0(ReplicationState, 1);
>>>> +    rs->opaque = opaque;
>>>> +    rs->ops = ops;
>>>> +    QLIST_INSERT_HEAD(&replication_states, rs, node);
>>>> +
>>>> +    return rs;
>>>> +}
>>>> +
>>>> +void replication_remove(ReplicationState *rs)
>>>> +{
>>>> +    QLIST_REMOVE(rs, node);
>>>> +    g_free(rs);
>>>> +}
>>>> +
>>>> +/*
>>>> + * The caller of the function *MUST* make sure vm stopped
>>>> + */
>>>> +void replication_start_all(ReplicationMode mode, Error **errp)
>>>> +{
>>>
>>> Is this public API is only used for block ?
>>> If yes, I'd like it with a 'block_' prefix.
>>
>> No, we hope it can be used for nic too.
>>
> 
> OK, i got why you designed these APIs, I like this idea that
> use the callback/notifier to notify the status of COLO for block/nic.
> 
> But let's do something more graceful.
> For COLO, we can consider it has four states:
> Prepare/start checkpoint(with VM stopped)/finish checkpoint (with VM 
> resumed)/failover.
> So here we need four operation functions according to these four states.

Where is do_checkpoint?

> I'd like these public APIs with a 'colo_' prefix,

I hope this API can also be used by other solution such as microcheckpoint.

> 
> What about colo_replication_prepare()/colo_replication_start_checkpoint()/
> 
> colo_replication_finish_checkpoint()/colo_replication_failover() ?
> 
> Besides, It's better to rename the replicaton.c, and move it to the 
> 'migration' folder.
> Because it seems to be only used by COLO, maybe colo_ops.c or maybe another 
> choice
> move all these codes to colo-comm.c. (If we do this, we need to merge 
> colo-comm.c
> first as prerequisite).

This api is used by block driver, so qemu-nbd, qemu-img also needs it.

Thanks
Wen Congyang

> 
> Thanks,
> Hailiang
> 
>>
>>>
>>>> +    ReplicationState *rs, *next;
>>>> +
>>>> +    QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
>>>> +        if (rs->ops && rs->ops->start) {
>>>> +            rs->ops->start(rs, mode, errp);
>>>> +        }
>>>> +        if (*errp != NULL) {
>>>
>>> This is incorrect, you miss checking if errp is NULL,
>>> if errp is NULL, there will be an error that accessing memory at address 
>>> 0x0.
>>> Same with other places in this patch.
>>>
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +void replication_do_checkpoint_all(Error **errp)
>>>> +{
>>>> +    ReplicationState *rs, *next;
>>>> +
>>>> +    QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
>>>> +        if (rs->ops && rs->ops->checkpoint) {
>>>> +            rs->ops->checkpoint(rs, errp);
>>>> +        }
>>>> +        if (*errp != NULL) {
>>>> +            return;
>>>
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +void replication_get_error_all(Error **errp)
>>>> +{
>>>> +    ReplicationState *rs, *next;
>>>> +
>>>> +    QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
>>>> +        if (rs->ops && rs->ops->get_error) {
>>>> +            rs->ops->get_error(rs, errp);
>>>> +        }
>>>> +        if (*errp != NULL) {
>>>
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +void replication_stop_all(bool failover, Error **errp)
>>>> +{
>>>> +    ReplicationState *rs, *next;
>>>> +
>>>> +    QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
>>>> +        if (rs->ops && rs->ops->stop) {
>>>> +            rs->ops->stop(rs, failover, errp);
>>>> +        }
>>>> +        if (*errp != NULL) {
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +}
>>>> diff --git a/replication.h b/replication.h
>>>> new file mode 100644
>>>> index 0000000..faea649
>>>> --- /dev/null
>>>> +++ b/replication.h
>>>> @@ -0,0 +1,53 @@
>>>> +/*
>>>> + * Replication filter
>>>> + *
>>>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
>>>> + * Copyright (c) 2016 Intel Corporation
>>>> + * Copyright (c) 2016 FUJITSU LIMITED
>>>> + *
>>>> + * Author:
>>>> + *   Wen Congyang <address@hidden>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#ifndef REPLICATION_H
>>>> +#define REPLICATION_H
>>>> +
>>>> +#include "sysemu/sysemu.h"
>>>> +
>>>> +typedef struct ReplicationOps ReplicationOps;
>>>> +typedef struct ReplicationState ReplicationState;
>>>> +typedef void (*Start)(ReplicationState *rs, ReplicationMode mode, Error 
>>>> **errp);
>>>> +typedef void (*Stop)(ReplicationState *rs, bool failover, Error **errp);
>>>> +typedef void (*Checkpoint)(ReplicationState *rs, Error **errp);
>>>> +typedef void (*GetError)(ReplicationState *rs, Error **errp);
>>>> +
>>>> +struct ReplicationState {
>>>> +    void *opaque;
>>>> +    ReplicationOps *ops;
>>>> +    QLIST_ENTRY(ReplicationState) node;
>>>> +};
>>>> +
>>>> +struct ReplicationOps{
>>>> +    Start start;
>>>> +    Checkpoint checkpoint;
>>>> +    GetError get_error;
>>>> +    Stop stop;
>>>> +};
>>>> +
>>>> +
>>>> +ReplicationState *replication_new(void *opaque, ReplicationOps *ops);
>>>> +
>>>> +void replication_remove(ReplicationState *rs);
>>>> +
>>>> +void replication_start_all(ReplicationMode mode, Error **errp);
>>>> +
>>>> +void replication_do_checkpoint_all(Error **errp);
>>>> +
>>>> +void replication_get_error_all(Error **errp);
>>>> +
>>>> +void replication_stop_all(bool failover, Error **errp);
>>>> +
>>>> +#endif /* REPLICATION_H */
>>>>
>>>
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>>
>> .
>>
> 
> 
> 
> 
> .
> 






reply via email to

[Prev in Thread] Current Thread [Next in Thread]