diff mbox

[COLO-Frame,v14,37/40] COLO: enable buffer filters for PVM

Message ID 1454750932-7556-38-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhanghailiang Feb. 6, 2016, 9:28 a.m. UTC
Enable all buffer filters that added by COLO while
go into COLO process, and disable them while exit COLO.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Yang Hongyang <hongyang.yang@easystack.cn>
---
v14:
- New patch
---
 migration/colo.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Jason Wang Feb. 18, 2016, 3:31 a.m. UTC | #1
On 02/06/2016 05:28 PM, zhanghailiang wrote:
> Enable all buffer filters that added by COLO while
> go into COLO process, and disable them while exit COLO.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
> ---
> v14:
> - New patch
> ---
>  migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 235578b..86a7638 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>      }
>  }
>  
> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
> +                                   Error **errp)
> +{
> +    char colo_filter[128];
> +    char *name = object_get_canonical_path_component(OBJECT(nf));
> +    char *status = opaque;
> +
> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id);
> +    if (strcmp(colo_filter, name)) {
> +        return;
> +    }

Checking by name is not elegant. As we've discussed last time, why not
let filter-buffer track all filters with zero interval in a linked list
and just export a helper to disable and enable them all? Things will be
greatly simplified with this, and there's even no need for patch 36.
Zhanghailiang Feb. 18, 2016, 3:46 a.m. UTC | #2
On 2016/2/18 11:31, Jason Wang wrote:
>
>
> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>> Enable all buffer filters that added by COLO while
>> go into COLO process, and disable them while exit COLO.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>> v14:
>> - New patch
>> ---
>>   migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 235578b..86a7638 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>>       }
>>   }
>>
>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
>> +                                   Error **errp)
>> +{
>> +    char colo_filter[128];
>> +    char *name = object_get_canonical_path_component(OBJECT(nf));
>> +    char *status = opaque;
>> +
>> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id);
>> +    if (strcmp(colo_filter, name)) {
>> +        return;
>> +    }
>
> Checking by name is not elegant. As we've discussed last time, why not
> let filter-buffer track all filters with zero interval in a linked list
> and just export a helper to disable and enable them all? Things will be
> greatly simplified with this, and there's even no need for patch 36.
>
>

Yes, i know what you mean, but we have to add another
'QTAILQ_ENTRY() entry' like member into struct NetFilterState
if we do like that, is it acceptable ?

> .
>
Zhanghailiang Feb. 18, 2016, 7:30 a.m. UTC | #3
Hi Jason,

On 2016/2/18 11:46, Hailiang Zhang wrote:
> On 2016/2/18 11:31, Jason Wang wrote:
>>
>>
>> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>>> Enable all buffer filters that added by COLO while
>>> go into COLO process, and disable them while exit COLO.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>> ---
>>> v14:
>>> - New patch
>>> ---
>>>   migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/migration/colo.c b/migration/colo.c
>>> index 235578b..86a7638 100644
>>> --- a/migration/colo.c
>>> +++ b/migration/colo.c
>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>>>       }
>>>   }
>>>
>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
>>> +                                   Error **errp)
>>> +{
>>> +    char colo_filter[128];
>>> +    char *name = object_get_canonical_path_component(OBJECT(nf));
>>> +    char *status = opaque;
>>> +
>>> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id);
>>> +    if (strcmp(colo_filter, name)) {
>>> +        return;
>>> +    }
>>
>> Checking by name is not elegant. As we've discussed last time, why not
>> let filter-buffer track all filters with zero interval in a linked list
>> and just export a helper to disable and enable them all? Things will be
>> greatly simplified with this, and there's even no need for patch 36.
>>
>>
>
> Yes, i know what you mean, but we have to add another
> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState
> if we do like that, is it acceptable ?
>

Sorry for the hasty reply, ;)

We can use a list to store all the colo related buffer filters
without adding any member to struct NetFilterState.

The codes will be like:

struct COLOListNode{
     void *opaque;
     QLIST_ENTRY(COLOListNode) node;
};
static QLIST_HEAD(, COLOListNode) COLOBufferFilters =
    QLIST_HEAD_INITIALIZER(COLOBufferFilters);

void colo_add_buffer_filter()
{
     struct COLOListNode *filternode;
     ...

     filter = object_new_with_props();

     filternode = g_new0(struct COLOListNode, 1);
     filternode->opaque = NETFILTER(filter);
     QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node);
}

And we can track all the colo releated filters directly by
visiting the *COLOBufferFilters* list.

Thanks,
Hailiang

>> .
>>
>
Jason Wang Feb. 23, 2016, 8:37 a.m. UTC | #4
On 02/18/2016 11:46 AM, Hailiang Zhang wrote:
> On 2016/2/18 11:31, Jason Wang wrote:
>>
>>
>> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>>> Enable all buffer filters that added by COLO while
>>> go into COLO process, and disable them while exit COLO.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>> ---
>>> v14:
>>> - New patch
>>> ---
>>>   migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/migration/colo.c b/migration/colo.c
>>> index 235578b..86a7638 100644
>>> --- a/migration/colo.c
>>> +++ b/migration/colo.c
>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>>>       }
>>>   }
>>>
>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
>>> +                                   Error **errp)
>>> +{
>>> +    char colo_filter[128];
>>> +    char *name = object_get_canonical_path_component(OBJECT(nf));
>>> +    char *status = opaque;
>>> +
>>> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo",
>>> nf->netdev_id);
>>> +    if (strcmp(colo_filter, name)) {
>>> +        return;
>>> +    }
>>
>> Checking by name is not elegant. As we've discussed last time, why not
>> let filter-buffer track all filters with zero interval in a linked list
>> and just export a helper to disable and enable them all? Things will be
>> greatly simplified with this, and there's even no need for patch 36.
>>
>>
>
> Yes, i know what you mean, but we have to add another
> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState
> if we do like that, is it acceptable ? 

I think you can make it private to filter buffer itself.
Jason Wang Feb. 23, 2016, 8:38 a.m. UTC | #5
On 02/18/2016 03:30 PM, Hailiang Zhang wrote:
> Hi Jason,
>
> On 2016/2/18 11:46, Hailiang Zhang wrote:
>> On 2016/2/18 11:31, Jason Wang wrote:
>>>
>>>
>>> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>>>> Enable all buffer filters that added by COLO while
>>>> go into COLO process, and disable them while exit COLO.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>>> ---
>>>> v14:
>>>> - New patch
>>>> ---
>>>>   migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>>>>   1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/migration/colo.c b/migration/colo.c
>>>> index 235578b..86a7638 100644
>>>> --- a/migration/colo.c
>>>> +++ b/migration/colo.c
>>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>>>>       }
>>>>   }
>>>>
>>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
>>>> +                                   Error **errp)
>>>> +{
>>>> +    char colo_filter[128];
>>>> +    char *name = object_get_canonical_path_component(OBJECT(nf));
>>>> +    char *status = opaque;
>>>> +
>>>> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo",
>>>> nf->netdev_id);
>>>> +    if (strcmp(colo_filter, name)) {
>>>> +        return;
>>>> +    }
>>>
>>> Checking by name is not elegant. As we've discussed last time, why not
>>> let filter-buffer track all filters with zero interval in a linked list
>>> and just export a helper to disable and enable them all? Things will be
>>> greatly simplified with this, and there's even no need for patch 36.
>>>
>>>
>>
>> Yes, i know what you mean, but we have to add another
>> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState
>> if we do like that, is it acceptable ?
>>
>
> Sorry for the hasty reply, ;)
>
> We can use a list to store all the colo related buffer filters
> without adding any member to struct NetFilterState.
>
> The codes will be like:
>
> struct COLOListNode{
>     void *opaque;
>     QLIST_ENTRY(COLOListNode) node;
> };
> static QLIST_HEAD(, COLOListNode) COLOBufferFilters =
>    QLIST_HEAD_INITIALIZER(COLOBufferFilters);
>
> void colo_add_buffer_filter()
> {
>     struct COLOListNode *filternode;
>     ...
>
>     filter = object_new_with_props();
>
>     filternode = g_new0(struct COLOListNode, 1);
>     filternode->opaque = NETFILTER(filter);
>     QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node);
> }
>
> And we can track all the colo releated filters directly by
> visiting the *COLOBufferFilters* list.
>
> Thanks,
> Hailiang

Also fine, but looks suboptimal to my proposal (use a list private to
filter-buffer).

>
>>> .
>>>
>>
>
>
>
Zhanghailiang Feb. 23, 2016, 9:10 a.m. UTC | #6
Hi Jason,

Thanks for your patience.

On 2016/2/23 16:38, Jason Wang wrote:
>
>
> On 02/18/2016 03:30 PM, Hailiang Zhang wrote:
>> Hi Jason,
>>
>> On 2016/2/18 11:46, Hailiang Zhang wrote:
>>> On 2016/2/18 11:31, Jason Wang wrote:
>>>>
>>>>
>>>> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>>>>> Enable all buffer filters that added by COLO while
>>>>> go into COLO process, and disable them while exit COLO.
>>>>>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>>>> ---
>>>>> v14:
>>>>> - New patch
>>>>> ---
>>>>>    migration/colo.c | 32 ++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/migration/colo.c b/migration/colo.c
>>>>> index 235578b..86a7638 100644
>>>>> --- a/migration/colo.c
>>>>> +++ b/migration/colo.c
>>>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void)
>>>>>        }
>>>>>    }
>>>>>
>>>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque,
>>>>> +                                   Error **errp)
>>>>> +{
>>>>> +    char colo_filter[128];
>>>>> +    char *name = object_get_canonical_path_component(OBJECT(nf));
>>>>> +    char *status = opaque;
>>>>> +
>>>>> +    snprintf(colo_filter, sizeof(colo_filter), "%scolo",
>>>>> nf->netdev_id);
>>>>> +    if (strcmp(colo_filter, name)) {
>>>>> +        return;
>>>>> +    }
>>>>
>>>> Checking by name is not elegant. As we've discussed last time, why not
>>>> let filter-buffer track all filters with zero interval in a linked list
>>>> and just export a helper to disable and enable them all? Things will be
>>>> greatly simplified with this, and there's even no need for patch 36.
>>>>
>>>>
>>>
>>> Yes, i know what you mean, but we have to add another
>>> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState
>>> if we do like that, is it acceptable ?
>>>
>>
>> Sorry for the hasty reply, ;)
>>
>> We can use a list to store all the colo related buffer filters
>> without adding any member to struct NetFilterState.
>>
>> The codes will be like:
>>
>> struct COLOListNode{
>>      void *opaque;
>>      QLIST_ENTRY(COLOListNode) node;
>> };
>> static QLIST_HEAD(, COLOListNode) COLOBufferFilters =
>>     QLIST_HEAD_INITIALIZER(COLOBufferFilters);
>>
>> void colo_add_buffer_filter()
>> {
>>      struct COLOListNode *filternode;
>>      ...
>>
>>      filter = object_new_with_props();
>>
>>      filternode = g_new0(struct COLOListNode, 1);
>>      filternode->opaque = NETFILTER(filter);
>>      QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node);
>> }
>>
>> And we can track all the colo releated filters directly by
>> visiting the *COLOBufferFilters* list.
>>
>> Thanks,
>> Hailiang
>

I have updated these codes in 'PATCH COLO-Frame v15', which is sent yesterday. :)

> Also fine, but looks suboptimal to my proposal (use a list private to
> filter-buffer).
>

Yes, i have consider adding such a private member to filter-buffer, but
it seems to be only used by COLO, which looks strange, and we tried to avoid
dirtying the filter codes.
Besides, we still need to define the global QTAILQ_HEAD(,)COLOBufferFilters
in this scenario.

Thanks,
Hailiang

>>
>>>> .
>>>>
>>>
>>
>>
>>
>
>
> .
>
diff mbox

Patch

diff --git a/migration/colo.c b/migration/colo.c
index 235578b..86a7638 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -104,10 +104,26 @@  static void secondary_vm_do_failover(void)
     }
 }
 
+static void colo_set_filter_status(NetFilterState *nf, void *opaque,
+                                   Error **errp)
+{
+    char colo_filter[128];
+    char *name = object_get_canonical_path_component(OBJECT(nf));
+    char *status = opaque;
+
+    snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id);
+    if (strcmp(colo_filter, name)) {
+        return;
+    }
+    object_property_set_str(OBJECT(nf), status, "status", errp);
+}
+
 static void primary_vm_do_failover(void)
 {
     MigrationState *s = migrate_get_current();
     int old_state;
+    char *status;
+    Error *local_err = NULL;
 
     migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
                       MIGRATION_STATUS_COMPLETED);
@@ -131,6 +147,14 @@  static void primary_vm_do_failover(void)
                      old_state);
         return;
     }
+
+    status = g_strdup("disable");
+    qemu_foreach_netfilter(colo_set_filter_status, status, &local_err);
+    g_free(status);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+
     /* Notify COLO thread that failover work is finished */
     qemu_sem_post(&s->colo_sem);
 }
@@ -404,11 +428,19 @@  static void colo_process_checkpoint(MigrationState *s)
 {
     QEMUSizedBuffer *buffer = NULL;
     int64_t current_time, checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    char *status;
     Error *local_err = NULL;
     int ret;
 
     failover_init_state();
 
+    status = g_strdup("enable");
+    qemu_foreach_netfilter(colo_set_filter_status, status, &local_err);
+    g_free(status);
+    if (local_err) {
+        goto out;
+    }
+
     s->rp_state.from_dst_file = qemu_file_get_return_path(s->to_dst_file);
     if (!s->rp_state.from_dst_file) {
         error_report("Open QEMUFile from_dst_file failed");