Re: [Qemu-devel] [PATCH 2/3] COLO: Shutdown related socket fd while do failover

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2017/2/9 3:53, Dr. David Alan Gilbert wrote:
> * Hailiang Zhang (zhang.zhanghailiang@huawei.com) wrote:
>> On 2017/1/18 19:01, Dr. David Alan Gilbert wrote:
>>> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>>>> If the net connection between primary host and secondary host breaks
>>>> while COLO/COLO incoming threads are doing read() or write().
>>>> It will block until connection is timeout, and the failover process
>>>> will be blocked because of it.
>>>>
>>>> So it is necessary to shutdown all the socket fds used by COLO
>>>> to avoid this situation. Besides, we should close the corresponding
>>>> file descriptors after failvoer BH shutdown them,
>>>> Or there will be an error.
>>>
>>> Hi,
>>>     There are two parts to this patch:
>>>      a) Add some semaphores to sequence failover
>>>      b) Use shutdown()
>>>
>>>     At first I wondered if perhaps they should be split; but I see
>>> the reason for the semaphores is mostly to stop the race between
>>> the fd's getting closed and the shutdown() calls; so I think it's
>>> OK.
>>>
>>
>> Hi,
>>
>> Yes, you are right, maybe i should add some comments about that.
>> Will do in next version.
>>
>>> Do you have any problems with these semaphores during powering off the
>>> guest?
>>>
>>
>> No, we didn't encounter any problems or trigger any bugs in our test
>> with this semaphores. In what places do you doubt it may has problems ? :)
>
> I just wondered about other exit cases other than failover; e.g. what
> if the guest shutdown or something like that, would it get stuck
> waiting for the colo_incoming_sem.
>

Sorry for the late reply, too busy with our project these days ...

Your worry makes sense, we have processed the shutdown case specially,
Let qemu does a checkpoint process (It seems that, we should kick colo
thread which might be waiting for colo_checkpoint_sem.) before exit colo therad.
And for the secondary sides, if it receives shutdown request, it will exit
colo incoming thread after some cleanup works.


Thanks.
Hailiang


> Dave
>
>> Thanks,
>> Hailiang
>>
>>> Dave
>>>
>>>
>>>
>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> ---
>>>>    include/migration/migration.h |  3 +++
>>>>    migration/colo.c              | 43 +++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 46 insertions(+)
>>>>
>>>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>>>> index 487ac1e..7cac877 100644
>>>> --- a/include/migration/migration.h
>>>> +++ b/include/migration/migration.h
>>>> @@ -113,6 +113,7 @@ struct MigrationIncomingState {
>>>>        QemuThread colo_incoming_thread;
>>>>        /* The coroutine we should enter (back) after failover */
>>>>        Coroutine *migration_incoming_co;
>>>> +    QemuSemaphore colo_incoming_sem;
>>>>
>>>>        /* See savevm.c */
>>>>        LoadStateEntry_Head loadvm_handlers;
>>>> @@ -182,6 +183,8 @@ struct MigrationState
>>>>        QSIMPLEQ_HEAD(src_page_requests, MigrationSrcPageRequest) src_page_requests;
>>>>        /* The RAMBlock used in the last src_page_request */
>>>>        RAMBlock *last_req_rb;
>>>> +    /* The semaphore is used to notify COLO thread that failover is finished */
>>>> +    QemuSemaphore colo_exit_sem;
>>>>
>>>>        /* The semaphore is used to notify COLO thread to do checkpoint */
>>>>        QemuSemaphore colo_checkpoint_sem;
>>>> diff --git a/migration/colo.c b/migration/colo.c
>>>> index 08b2e46..3222812 100644
>>>> --- a/migration/colo.c
>>>> +++ b/migration/colo.c
>>>> @@ -59,6 +59,18 @@ static void secondary_vm_do_failover(void)
>>>>            /* recover runstate to normal migration finish state */
>>>>            autostart = true;
>>>>        }
>>>> +    /*
>>>> +     * Make sure COLO incoming thread not block in recv or send,
>>>> +     * If mis->from_src_file and mis->to_src_file use the same fd,
>>>> +     * The second shutdown() will return -1, we ignore this value,
>>>> +     * It is harmless.
>>>> +     */
>>>> +    if (mis->from_src_file) {
>>>> +        qemu_file_shutdown(mis->from_src_file);
>>>> +    }
>>>> +    if (mis->to_src_file) {
>>>> +        qemu_file_shutdown(mis->to_src_file);
>>>> +    }
>>>>
>>>>        old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,
>>>>                                       FAILOVER_STATUS_COMPLETED);
>>>> @@ -67,6 +79,8 @@ static void secondary_vm_do_failover(void)
>>>>                         "secondary VM", FailoverStatus_lookup[old_state]);
>>>>            return;
>>>>        }
>>>> +    /* Notify COLO incoming thread that failover work is finished */
>>>> +    qemu_sem_post(&mis->colo_incoming_sem);
>>>>        /* For Secondary VM, jump to incoming co */
>>>>        if (mis->migration_incoming_co) {
>>>>            qemu_coroutine_enter(mis->migration_incoming_co);
>>>> @@ -81,6 +95,18 @@ static void primary_vm_do_failover(void)
>>>>        migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
>>>>                          MIGRATION_STATUS_COMPLETED);
>>>>
>>>> +    /*
>>>> +     * Wake up COLO thread which may blocked in recv() or send(),
>>>> +     * The s->rp_state.from_dst_file and s->to_dst_file may use the
>>>> +     * same fd, but we still shutdown the fd for twice, it is harmless.
>>>> +     */
>>>> +    if (s->to_dst_file) {
>>>> +        qemu_file_shutdown(s->to_dst_file);
>>>> +    }
>>>> +    if (s->rp_state.from_dst_file) {
>>>> +        qemu_file_shutdown(s->rp_state.from_dst_file);
>>>> +    }
>>>> +
>>>>        old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,
>>>>                                       FAILOVER_STATUS_COMPLETED);
>>>>        if (old_state != FAILOVER_STATUS_ACTIVE) {
>>>> @@ -88,6 +114,8 @@ static void primary_vm_do_failover(void)
>>>>                         FailoverStatus_lookup[old_state]);
>>>>            return;
>>>>        }
>>>> +    /* Notify COLO thread that failover work is finished */
>>>> +    qemu_sem_post(&s->colo_exit_sem);
>>>>    }
>>>>
>>>>    void colo_do_failover(MigrationState *s)
>>>> @@ -361,6 +389,14 @@ out:
>>>>
>>>>        timer_del(s->colo_delay_timer);
>>>>
>>>> +    /* Hope this not to be too long to wait here */
>>>> +    qemu_sem_wait(&s->colo_exit_sem);
>>>> +    qemu_sem_destroy(&s->colo_exit_sem);
>>>> +    /*
>>>> +     * Must be called after failover BH is completed,
>>>> +     * Or the failover BH may shutdown the wrong fd that
>>>> +     * re-used by other threads after we release here.
>>>> +     */
>>>>        if (s->rp_state.from_dst_file) {
>>>>            qemu_fclose(s->rp_state.from_dst_file);
>>>>        }
>>>> @@ -385,6 +421,7 @@ void migrate_start_colo_process(MigrationState *s)
>>>>        s->colo_delay_timer =  timer_new_ms(QEMU_CLOCK_HOST,
>>>>                                    colo_checkpoint_notify, s);
>>>>
>>>> +    qemu_sem_init(&s->colo_exit_sem, 0);
>>>>        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>>>>                          MIGRATION_STATUS_COLO);
>>>>        colo_process_checkpoint(s);
>>>> @@ -423,6 +460,8 @@ void *colo_process_incoming_thread(void *opaque)
>>>>        uint64_t value;
>>>>        Error *local_err = NULL;
>>>>
>>>> +    qemu_sem_init(&mis->colo_incoming_sem, 0);
>>>> +
>>>>        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>>>>                          MIGRATION_STATUS_COLO);
>>>>
>>>> @@ -533,6 +572,10 @@ out:
>>>>            qemu_fclose(fb);
>>>>        }
>>>>
>>>> +    /* Hope this not to be too long to loop here */
>>>> +    qemu_sem_wait(&mis->colo_incoming_sem);
>>>> +    qemu_sem_destroy(&mis->colo_incoming_sem);
>>>> +    /* Must be called after failover BH is completed */
>>>>        if (mis->to_src_file) {
>>>>            qemu_fclose(mis->to_src_file);
>>>>        }
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>> .
>>>
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>