Re: [PATCH v7 3/5] vhost-user-scsi: support reconnect to backend

2023-10-08 Thread Li Feng 
Thanks for your comments, I will submit the v8.

> On 8 Oct 2023, at 6:46 PM, Manos Pitsidianakis 
>  wrote:
> 
> Hello Li, I have some trivial style comments you could possibly address in a 
> next version:
> 
> On Sun, 08 Oct 2023 12:12, Li Feng  wrote:
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index df6b66cc1a..5df24faff4 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
>>VHOST_INVALID_FEATURE_BIT
>> };
>> +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);
> 
> -+s->started_vu = (ret < 0 ? false : true);
> ++s->started_vu = !(ret < 0);
> 
>> static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>>VHostUserSCSI *s = (VHostUserSCSI *)vdev;
>> +DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
> 
> -+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
> ++DeviceState *dev = DEVICE(vdev);
> 
>> +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(>dev, >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;
>> +}
> 
> 
> -+if (virtio_device_started(vdev, vdev->status)) {
> -+ret = vhost_user_scsi_start(s, errp);
> -+if (ret < 0) {
> -+return ret;
> -+}
> -+}
> -+
> -+return 0;
> -+}
> ++if (virtio_device_started(vdev, vdev->status)) {
> ++ret = vhost_user_scsi_start(s, errp);
> ++}
> ++
> ++return ret;
> ++}
> 
> [skipping..]
> 
>> +static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
>> +{
>> +DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
> 
> 
> -+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
> ++DeviceState *dev = DEVICE(s);
> 
>> 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;
> 
> +bool connected;
> +bool started_vu;
> -+
> +struct vhost_virtqueue *vhost_vqs;
> 
> See 
> https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations
> 
> The definition should look like:
> 
> struct VHostUserSCSI {
>   VHostSCSICommon parent_obj;
> 
>   /* Properties */
>   bool connected;
>   bool started_vu;
> 
>   VhostUserState vhost_user;
>   struct vhost_virtqueue *vhost_vqs;
> }

Re: [PATCH v7 3/5] vhost-user-scsi: support reconnect to backend

2023-10-08 Thread Manos Pitsidianakis 
Hello Li, I have some trivial style comments you could possibly address 
in a next version:


On Sun, 08 Oct 2023 12:12, Li Feng  wrote:

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};

+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);


-+s->started_vu = (ret < 0 ? false : true);
++s->started_vu = !(ret < 0);

static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t 
status)

{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;


-+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(vdev);


+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(>dev, >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;
+}



-+if (virtio_device_started(vdev, vdev->status)) {
-+ret = vhost_user_scsi_start(s, errp);
-+if (ret < 0) {
-+return ret;
-+}
-+}
-+
-+return 0;
-+}
++if (virtio_device_started(vdev, vdev->status)) {
++ret = vhost_user_scsi_start(s, errp);
++}
++
++return ret;
++}

[skipping..]


+static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
+{
+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;



-+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(s);


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;


+bool connected;
+bool started_vu;
-+
+struct vhost_virtqueue *vhost_vqs;

See 
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations

The definition should look like:

struct VHostUserSCSI {
   VHostSCSICommon parent_obj;

   /* Properties */
   bool connected;
   bool started_vu;

   VhostUserState vhost_user;
   struct vhost_virtqueue *vhost_vqs;
}

[PATCH v7 3/5] vhost-user-scsi: support reconnect to backend

2023-10-08 Thread Li Feng 
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  |   6 +-
 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, 202 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..95cadb93e7 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,15 @@ 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) {
+error_reportf_err(local_err, "Error starting vhost-scsi");
 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 df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
 VHOST_INVALID_FEATURE_BIT
 };
 
+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,