Re: [PATCH v2] Drivers: hv: kvp: fix IP Failover

[Cathy Avery <cavery@redhat.com>]

Sorry acking wrong email.

Thanks,

Cathy

On 03/30/2016 08:21 AM, Cathy Avery wrote:
>
>
> On 03/29/2016 08:30 AM, Vitaly Kuznetsov wrote:
>> Hyper-V VMs can be replicated to another hosts and there is a feature to
>> set different IP for replicas, it is called 'Failover TCP/IP'. When
>> such guest starts Hyper-V host sends it KVP_OP_SET_IP_INFO message as 
>> soon
>> as we finish negotiation procedure. The problem is that it can happen 
>> (and
>> it actually happens) before userspace daemon connects and we reply with
>> HV_E_FAIL to the message. As there are no repetitions we fail to set the
>> requested IP.
>>
>> Solve the issue by postponing our reply to the negotiation message till
>> userspace daemon is connected. We can't wait too long as there is a
>> host-side timeout (cca. 75 seconds) and if we fail to reply in this time
>> frame the whole KVP service will become inactive. The solution is not
>> ideal - if it takes userspace daemon more than 60 seconds to connect
>> IP Failover will still fail but I don't see a solution with our current
>> separation between kernel and userspace parts.
>>
>> Other two modules (VSS and FCOPY) don't require such delay, leave them
>> untouched.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> Changes since v1:
>> - Cancel handshake timeout work on module unload.
>> ---
>>   drivers/hv/hv_kvp.c       | 31 +++++++++++++++++++++++++++++++
>>   drivers/hv/hyperv_vmbus.h |  5 +++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
>> index 9b9b370..cb1a916 100644
>> --- a/drivers/hv/hv_kvp.c
>> +++ b/drivers/hv/hv_kvp.c
>> @@ -78,9 +78,11 @@ static void kvp_send_key(struct work_struct *dummy);
>>     static void kvp_respond_to_host(struct hv_kvp_msg *msg, int error);
>>   static void kvp_timeout_func(struct work_struct *dummy);
>> +static void kvp_host_handshake_func(struct work_struct *dummy);
>>   static void kvp_register(int);
>>     static DECLARE_DELAYED_WORK(kvp_timeout_work, kvp_timeout_func);
>> +static DECLARE_DELAYED_WORK(kvp_host_handshake_work, 
>> kvp_host_handshake_func);
>>   static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);
>>     static const char kvp_devname[] = "vmbus/hv_kvp";
>> @@ -130,6 +132,11 @@ static void kvp_timeout_func(struct work_struct 
>> *dummy)
>>       hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
>>   }
>>   +static void kvp_host_handshake_func(struct work_struct *dummy)
>> +{
>> +    hv_poll_channel(kvp_transaction.recv_channel, 
>> hv_kvp_onchannelcallback);
>> +}
>> +
>>   static int kvp_handle_handshake(struct hv_kvp_msg *msg)
>>   {
>>       switch (msg->kvp_hdr.operation) {
>> @@ -154,6 +161,12 @@ static int kvp_handle_handshake(struct 
>> hv_kvp_msg *msg)
>>       pr_debug("KVP: userspace daemon ver. %d registered\n",
>>            KVP_OP_REGISTER);
>>       kvp_register(dm_reg_value);
>> +
>> +    /*
>> +     * If we're still negotiating with the host cancel the timeout
>> +     * work to not poll the channel twice.
>> +     */
>> +    cancel_delayed_work_sync(&kvp_host_handshake_work);
>>       hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
>>         return 0;
>> @@ -594,7 +607,22 @@ void hv_kvp_onchannelcallback(void *context)
>>       struct icmsg_negotiate *negop = NULL;
>>       int util_fw_version;
>>       int kvp_srv_version;
>> +    static enum {NEGO_NOT_STARTED,
>> +             NEGO_IN_PROGRESS,
>> +             NEGO_FINISHED} host_negotiatied = NEGO_NOT_STARTED;
>>   +    if (host_negotiatied == NEGO_NOT_STARTED &&
>> +        kvp_transaction.state < HVUTIL_READY) {
>> +        /*
>> +         * If userspace daemon is not connected and host is asking
>> +         * us to negotiate we need to delay to not lose messages.
>> +         * This is important for Failover IP setting.
>> +         */
>> +        host_negotiatied = NEGO_IN_PROGRESS;
>> +        schedule_delayed_work(&kvp_host_handshake_work,
>> +                      HV_UTIL_NEGO_TIMEOUT * HZ);
>> +        return;
>> +    }
>>       if (kvp_transaction.state > HVUTIL_READY)
>>           return;
>>   @@ -672,6 +700,8 @@ void hv_kvp_onchannelcallback(void *context)
>>           vmbus_sendpacket(channel, recv_buffer,
>>                          recvlen, requestid,
>>                          VM_PKT_DATA_INBAND, 0);
>> +
>> +        host_negotiatied = NEGO_FINISHED;
>>       }
>>     }
>> @@ -708,6 +738,7 @@ hv_kvp_init(struct hv_util_service *srv)
>>   void hv_kvp_deinit(void)
>>   {
>>       kvp_transaction.state = HVUTIL_DEVICE_DYING;
>> +    cancel_delayed_work_sync(&kvp_host_handshake_work);
>>       cancel_delayed_work_sync(&kvp_timeout_work);
>>       cancel_work_sync(&kvp_sendkey_work);
>>       hvutil_transport_destroy(hvt);
>> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
>> index 12321b9..8b07f9c 100644
>> --- a/drivers/hv/hyperv_vmbus.h
>> +++ b/drivers/hv/hyperv_vmbus.h
>> @@ -36,6 +36,11 @@
>>   #define HV_UTIL_TIMEOUT 30
>>     /*
>> + * Timeout for guest-host handshake for services.
>> + */
>> +#define HV_UTIL_NEGO_TIMEOUT 60
>> +
>> +/*
>>    * The below CPUID leaves are present if 
>> VersionAndFeatures.HypervisorPresent
>>    * is set by CPUID(HVCPUID_VERSION_FEATURES).
>>    */
> Acked-by: Cathy Avery <cavery@redhat.com>