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

[Cathy Avery <cavery@redhat.com>]

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>