Re: [PATCH] resend slave fd to vhost when reconnect to vhost

[Raphael Norwitz]

On Fri, Apr 17, 2020 at 01:14:00PM +0800, address@hidden wrote:
> 
> From: Ni Xun <address@hidden>
> 
> when reconnecting to vhost server, it doesn't send slave fd to vhost
> as the slave fd is only sent in vhost_user_init. also resend the slave fd
> in vhost reconnect.
> 

I don’t think that’s correct. See vhost_user_init() here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost-user.c;h=08e7e63790e5bcfae6cd31bf9ccd32c3a7347f4e;hb=HEAD#l1898

Rather, vhost_setup_slave_channel() is called by vhost_user_backend_init(),
which is called on every reconnect inside vhost_dev_init().

see vhost_user_blk_connect(): 
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/block/vhost-user-blk.c;h=17df5338e77c684175a86e882b508849c246e78a;hb=HEAD#l297
and 
vhost_dev_init():https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost.c;h=01ebe12f28e9d7e3150375dda6f55b6b8f04a42a;hb=HEAD#l1224

Note that the dev->vq_index is also set to 0 right before the
vhost_dev_init() call.

I tested myself and saw a VHOST_USER_SET_SLAVE_REQ_FD message sent on each
reconnect with the vhost-user-blk sample.

Are you seeing different behavior? If so, please provide more details on
your setup?

> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 17df5338e7..59650a570b 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -138,6 +138,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
>              error_report("Error get inflight: %d", -ret);
>              goto err_guest_notifiers;
>          }

What's the justification for sending the slave fd here? Please elaborate.

> +    } else {
> +        ret = vhost_setup_slave_channel(&s->dev);
> +        if (ret < 0) {
> +            error_report("Error setting vhost slave channel: %d", -ret);
> +            return ret;
> +        }
>      }
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 085450c6f8..cad60ad521 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h

Why expose this through vhost.h? This is vhost-user specific.

> @@ -100,6 +100,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice 
> *vdev);
>  void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>  int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_setup_slave_channel(struct vhost_dev *hdev);
>  
>  /* Test and clear masked event pending status.
>   * Should be called after unmask to avoid losing events.
> -- 
> 2.24.1 (Apple Git-126)
> 
>