Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> 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>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>

An issue was found with this patch: we don't cancel
kvp_host_handshake_work on module unload and if the work is still
pending we end up crashing. As far as I can see this series didn't make
it to char-misc tree so instead of sending a patch to this patch I'll
send v2 for the original issue.

Sorry for the inconvenience.

> ---
>  drivers/hv/hv_kvp.c       |   30 ++++++++++++++++++++++++++++++
>  drivers/hv/hyperv_vmbus.h |    5 +++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 9b9b370..0d3fcd6 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;
>  	}
>
>  }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index a64b176..28e9df9 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).
>   */

-- 
  Vitaly