Re: [PATCH v6 3/5] vhost-user-scsi: support reconnect to backend
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
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 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
> > ---
> > 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(>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(>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(>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(>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(>dev, _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, _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");
> >
Re: [PATCH v6 3/5] vhost-user-scsi: support reconnect to backend
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 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
> ---
> 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(>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(>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(>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(>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(>dev, _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, _err);
> if (ret < 0) {
Why aren’t you reporting the error here?
> 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
[PATCH v6 3/5] vhost-user-scsi: support reconnect to backend
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
---
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(>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(>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(>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(>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(>dev, _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, _err);
if (ret < 0) {
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;
+
