Sorry, the reply is late due to being on vacation for half a month. On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz <raphael.norw...@nutanix.com> wrote: > > One comment on the logging stuff in vhost-scsi. As far as I can tell the > logging in vhost-user-scsi looks good. > > Markus - does this look better to you? Otherwise do you think we should also > fix up the vhost-user-blk realize function? > > > On Sep 22, 2023, at 7:46 AM, Li Feng <fen...@smartx.com> wrote: > > > > If the backend crashes and restarts, the device is broken. > > This patch adds reconnect for vhost-user-scsi. > > > > This patch also improves the error messages, and reports some silent errors. > > > > Tested with spdk backend. > > > > Signed-off-by: Li Feng <fen...@smartx.com> > > --- > > hw/scsi/vhost-scsi-common.c | 16 +- > > hw/scsi/vhost-scsi.c | 5 +- > > hw/scsi/vhost-user-scsi.c | 204 +++++++++++++++++++++++--- > > include/hw/virtio/vhost-scsi-common.h | 2 +- > > include/hw/virtio/vhost-user-scsi.h | 4 + > > 5 files changed, 201 insertions(+), 30 deletions(-) > > > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c > > index a61cd0e907..4c8637045d 100644 > > --- a/hw/scsi/vhost-scsi-common.c > > +++ b/hw/scsi/vhost-scsi-common.c > > @@ -16,6 +16,7 @@ > > */ > > > > #include "qemu/osdep.h" > > +#include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "qemu/module.h" > > #include "hw/virtio/vhost.h" > > @@ -25,7 +26,7 @@ > > #include "hw/virtio/virtio-access.h" > > #include "hw/fw-path-provider.h" > > > > -int vhost_scsi_common_start(VHostSCSICommon *vsc) > > +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp) > > { > > int ret, i; > > VirtIODevice *vdev = VIRTIO_DEVICE(vsc); > > @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc) > > VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc; > > > > if (!k->set_guest_notifiers) { > > - error_report("binding does not support guest notifiers"); > > + error_setg(errp, "binding does not support guest notifiers"); > > return -ENOSYS; > > } > > > > ret = vhost_dev_enable_notifiers(&vsc->dev, vdev); > > if (ret < 0) { > > + error_setg_errno(errp, -ret, "Error enabling host notifiers"); > > return ret; > > } > > > > ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true); > > if (ret < 0) { > > - error_report("Error binding guest notifier"); > > + error_setg_errno(errp, -ret, "Error binding guest notifier"); > > goto err_host_notifiers; > > } > > > > @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc) > > > > ret = vhost_dev_prepare_inflight(&vsc->dev, vdev); > > if (ret < 0) { > > - error_report("Error setting inflight format: %d", -ret); > > + error_setg_errno(errp, -ret, "Error setting inflight format"); > > goto err_guest_notifiers; > > } > > > > @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc) > > vs->conf.virtqueue_size, > > vsc->inflight); > > if (ret < 0) { > > - error_report("Error getting inflight: %d", -ret); > > + error_setg_errno(errp, -ret, "Error getting inflight"); > > goto err_guest_notifiers; > > } > > } > > > > ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); > > if (ret < 0) { > > - error_report("Error setting inflight: %d", -ret); > > + error_setg_errno(errp, -ret, "Error setting inflight"); > > goto err_guest_notifiers; > > } > > } > > > > ret = vhost_dev_start(&vsc->dev, vdev, true); > > if (ret < 0) { > > - error_report("Error start vhost dev"); > > + error_setg_errno(errp, -ret, "Error starting vhost dev"); > > goto err_guest_notifiers; > > } > > > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > > index 443f67daa4..01a3ab4277 100644 > > --- a/hw/scsi/vhost-scsi.c > > +++ b/hw/scsi/vhost-scsi.c > > @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s) > > int ret, abi_version; > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > const VhostOps *vhost_ops = vsc->dev.vhost_ops; > > + Error *local_err = NULL; > > > > ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version); > > if (ret < 0) { > > @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s) > > return -ENOSYS; > > } > > > > - ret = vhost_scsi_common_start(vsc); > > + ret = vhost_scsi_common_start(vsc, &local_err); > > if (ret < 0) { > > Why aren’t you reporting the error here? I will add reporting the error in the next version.
> > > return ret; > > } > > > > ret = vhost_scsi_set_endpoint(s); > > if (ret < 0) { > > - error_report("Error setting vhost-scsi endpoint"); > > + error_reportf_err(local_err, "Error setting vhost-scsi endpoint"); > > vhost_scsi_common_stop(vsc); > > } > > > > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > > index ee99b19e7a..dc109154ad 100644 > > --- a/hw/scsi/vhost-user-scsi.c > > +++ b/hw/scsi/vhost-user-scsi.c > > @@ -43,26 +43,56 @@ enum VhostUserProtocolFeature { > > VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, > > }; > > > > +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp) > > +{ > > + VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > + int ret; > > + > > + ret = vhost_scsi_common_start(vsc, errp); > > + s->started_vu = (ret < 0 ? false : true); > > + > > + return ret; > > +} > > + > > +static void vhost_user_scsi_stop(VHostUserSCSI *s) > > +{ > > + VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > + > > + if (!s->started_vu) { > > + return; > > + } > > + s->started_vu = false; > > + > > + vhost_scsi_common_stop(vsc); > > +} > > + > > static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) > > { > > VHostUserSCSI *s = (VHostUserSCSI *)vdev; > > + DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj; > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > - bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running; > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > + bool should_start = virtio_device_should_start(vdev, status); > > + Error *local_err = NULL; > > + int ret; > > > > - if (vhost_dev_is_started(&vsc->dev) == start) { > > + if (!s->connected) { > > return; > > } > > > > - if (start) { > > - int ret; > > + if (vhost_dev_is_started(&vsc->dev) == should_start) { > > + return; > > + } > > > > - ret = vhost_scsi_common_start(vsc); > > + if (should_start) { > > + ret = vhost_user_scsi_start(s, &local_err); > > if (ret < 0) { > > - error_report("unable to start vhost-user-scsi: %s", > > strerror(-ret)); > > - exit(1); > > + error_reportf_err(local_err, "unable to start vhost-user-scsi: > > %s", > > + strerror(-ret)); > > + qemu_chr_fe_disconnect(&vs->conf.chardev); > > } > > } else { > > - vhost_scsi_common_stop(vsc); > > + vhost_user_scsi_stop(s); > > } > > } > > > > @@ -89,14 +119,127 @@ static void vhost_dummy_handle_output(VirtIODevice > > *vdev, VirtQueue *vq) > > { > > } > > > > +static int vhost_user_scsi_connect(DeviceState *dev, Error **errp) > > +{ > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + VHostUserSCSI *s = VHOST_USER_SCSI(vdev); > > + VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > + int ret = 0; > > + > > + if (s->connected) { > > + return 0; > > + } > > + s->connected = true; > > + > > + vsc->dev.num_queues = vs->conf.num_queues; > > + vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > > + vsc->dev.vqs = s->vhost_vqs; > > + vsc->dev.vq_index = 0; > > + vsc->dev.backend_features = 0; > > + > > + ret = vhost_dev_init(&vsc->dev, &s->vhost_user, > > VHOST_BACKEND_TYPE_USER, 0, > > + errp); > > + if (ret < 0) { > > + return ret; > > + } > > + > > + /* restore vhost state */ > > + if (virtio_device_started(vdev, vdev->status)) { > > + ret = vhost_user_scsi_start(s, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event); > > + > > +static void vhost_user_scsi_disconnect(DeviceState *dev) > > +{ > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + VHostUserSCSI *s = VHOST_USER_SCSI(vdev); > > + VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > + > > + if (!s->connected) { > > + return; > > + } > > + s->connected = false; > > + > > + vhost_user_scsi_stop(s); > > + > > + vhost_dev_cleanup(&vsc->dev); > > + > > + /* Re-instate the event handler for new connections */ > > + qemu_chr_fe_set_handlers(&vs->conf.chardev, NULL, NULL, > > + vhost_user_scsi_event, NULL, dev, NULL, true); > > +} > > + > > +static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event) > > +{ > > + DeviceState *dev = opaque; > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + VHostUserSCSI *s = VHOST_USER_SCSI(vdev); > > + VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > + Error *local_err = NULL; > > + > > + switch (event) { > > + case CHR_EVENT_OPENED: > > + if (vhost_user_scsi_connect(dev, &local_err) < 0) { > > + error_report_err(local_err); > > + qemu_chr_fe_disconnect(&vs->conf.chardev); > > + return; > > + } > > + break; > > + case CHR_EVENT_CLOSED: > > + /* defer close until later to avoid circular close */ > > + vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev, > > + vhost_user_scsi_disconnect); > > + break; > > + case CHR_EVENT_BREAK: > > + case CHR_EVENT_MUX_IN: > > + case CHR_EVENT_MUX_OUT: > > + /* Ignore */ > > + break; > > + } > > +} > > + > > +static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp) > > +{ > > + DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj; > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > + int ret; > > + > > + s->connected = false; > > + > > + ret = qemu_chr_fe_wait_connected(&vs->conf.chardev, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + > > + ret = vhost_user_scsi_connect(dev, errp); > > + if (ret < 0) { > > + qemu_chr_fe_disconnect(&vs->conf.chardev); > > + return ret; > > + } > > + assert(s->connected); > > + > > + return 0; > > +} > > + > > static void vhost_user_scsi_realize(DeviceState *dev, Error **errp) > > { > > + ERRP_GUARD(); > > VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > VHostUserSCSI *s = VHOST_USER_SCSI(dev); > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > - struct vhost_virtqueue *vqs = NULL; > > Error *err = NULL; > > int ret; > > + int retries = VU_REALIZE_CONN_RETRIES; > > > > if (!vs->conf.chardev.chr) { > > error_setg(errp, "vhost-user-scsi: missing chardev"); > > @@ -115,18 +258,28 @@ static void vhost_user_scsi_realize(DeviceState *dev, > > Error **errp) > > goto free_virtio; > > } > > > > - vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > > - vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs); > > - vsc->dev.vq_index = 0; > > - vsc->dev.backend_features = 0; > > - vqs = vsc->dev.vqs; > > + vsc->inflight = g_new0(struct vhost_inflight, 1); > > + s->vhost_vqs = g_new0(struct vhost_virtqueue, > > + VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues); > > + > > + assert(!*errp); > > + do { > > + if (*errp) { > > + error_prepend(errp, "Reconnecting after error: "); > > + error_report_err(*errp); > > + *errp = NULL; > > + } > > + ret = vhost_user_scsi_realize_connect(s, errp); > > + } while (ret < 0 && retries--); > > > > - ret = vhost_dev_init(&vsc->dev, &s->vhost_user, > > - VHOST_BACKEND_TYPE_USER, 0, errp); > > if (ret < 0) { > > goto free_vhost; > > } > > > > + /* we're fully initialized, now we can operate, so add the handler */ > > + qemu_chr_fe_set_handlers(&vs->conf.chardev, NULL, NULL, > > + vhost_user_scsi_event, NULL, (void *)dev, > > + NULL, true); > > /* Channel and lun both are 0 for bootable vhost-user-scsi disk */ > > vsc->channel = 0; > > vsc->lun = 0; > > @@ -135,8 +288,12 @@ static void vhost_user_scsi_realize(DeviceState *dev, > > Error **errp) > > return; > > > > free_vhost: > > + g_free(s->vhost_vqs); > > + s->vhost_vqs = NULL; > > + g_free(vsc->inflight); > > + vsc->inflight = NULL; > > vhost_user_cleanup(&s->vhost_user); > > - g_free(vqs); > > + > > free_virtio: > > virtio_scsi_common_unrealize(dev); > > } > > @@ -146,16 +303,23 @@ static void vhost_user_scsi_unrealize(DeviceState > > *dev) > > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > VHostUserSCSI *s = VHOST_USER_SCSI(dev); > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > - struct vhost_virtqueue *vqs = vsc->dev.vqs; > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev); > > > > /* This will stop the vhost backend. */ > > vhost_user_scsi_set_status(vdev, 0); > > + qemu_chr_fe_set_handlers(&vs->conf.chardev, NULL, NULL, NULL, NULL, > > NULL, > > + NULL, false); > > > > vhost_dev_cleanup(&vsc->dev); > > - g_free(vqs); > > + g_free(s->vhost_vqs); > > + s->vhost_vqs = NULL; > > + > > + vhost_dev_free_inflight(vsc->inflight); > > + g_free(vsc->inflight); > > + vsc->inflight = NULL; > > > > - virtio_scsi_common_unrealize(dev); > > vhost_user_cleanup(&s->vhost_user); > > + virtio_scsi_common_unrealize(dev); > > } > > > > static Property vhost_user_scsi_properties[] = { > > diff --git a/include/hw/virtio/vhost-scsi-common.h > > b/include/hw/virtio/vhost-scsi-common.h > > index 18f115527c..c5d2c09455 100644 > > --- a/include/hw/virtio/vhost-scsi-common.h > > +++ b/include/hw/virtio/vhost-scsi-common.h > > @@ -39,7 +39,7 @@ struct VHostSCSICommon { > > struct vhost_inflight *inflight; > > }; > > > > -int vhost_scsi_common_start(VHostSCSICommon *vsc); > > +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp); > > void vhost_scsi_common_stop(VHostSCSICommon *vsc); > > char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus, > > DeviceState *dev); > > diff --git a/include/hw/virtio/vhost-user-scsi.h > > b/include/hw/virtio/vhost-user-scsi.h > > index 521b08e559..b405ec952a 100644 > > --- a/include/hw/virtio/vhost-user-scsi.h > > +++ b/include/hw/virtio/vhost-user-scsi.h > > @@ -29,6 +29,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserSCSI, > > VHOST_USER_SCSI) > > struct VHostUserSCSI { > > VHostSCSICommon parent_obj; > > VhostUserState vhost_user; > > + bool connected; > > + bool started_vu; > > + > > + struct vhost_virtqueue *vhost_vqs; > > }; > > > > #endif /* VHOST_USER_SCSI_H */ > > -- > > 2.41.0 > > >