Re: [PATCH] vhost-user-scsi: support reconnect to backend
> 2023年7月31日 06:09，Raphael Norwitz 写道：
>
>
>
>> On Jul 28, 2023, at 3:48 AM, Li Feng wrote:
>>
>> Thanks for your reply.
>>
>>> 2023年7月28日 上午5:21，Raphael Norwitz 写道：
>>>
>>>
>>>
On Jul 25, 2023, at 6:19 AM, Li Feng wrote:
Thanks for your comments.
> 2023年7月25日 上午1:21，Raphael Norwitz 写道：
>
> Very excited to see this. High level looks good modulo a few small things.
>
> My major concern is around existing vhost-user-scsi backends which don’t
> support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the
> reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check.
> We may want to do the same for vhost-user-blk.
>
> The question is then what happens if the check is false. IIUC without an
> inflight FD, if a device processes requests out of order, it’s not safe
> to continue execution on reconnect, as there’s no way for the backend to
> know how to replay IO. Should we permanently wedge the device or have
> QEMU fail out? May be nice to have a toggle for this.
Based on what MST said, is there anything else I need to do?
>>>
>>> I don’t think so.
>>>
>
>> On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
>>
>> If the backend crashes and restarts, the device is broken.
>> This patch adds reconnect for vhost-user-scsi.
>>
>> Tested with spdk backend.
>>
>> Signed-off-by: Li Feng
>> ---
>> hw/block/vhost-user-blk.c | 2 -
>> hw/scsi/vhost-scsi-common.c | 27 ++---
>> hw/scsi/vhost-user-scsi.c | 163 +---
>> include/hw/virtio/vhost-user-scsi.h | 3 +
>> include/hw/virtio/vhost.h | 2 +
>> 5 files changed, 165 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index eecf3f7a81..f250c740b5 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -32,8 +32,6 @@
>> #include "sysemu/sysemu.h"
>> #include "sysemu/runstate.h"
>>
>> -#define REALIZE_CONNECTION_RETRIES 3
>> -
>> static const int user_feature_bits[] = {
>> VIRTIO_BLK_F_SIZE_MAX,
>> VIRTIO_BLK_F_SEG_MAX,
>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>
> Why can’t all the vhost-scsi-common stuff be moved to a separate change?
I will move this code to separate patch.
>
> Especially the stuff introduced for vhost-user-blk in
> 1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
OK.
>
>> index a06f01af26..08801886b8 100644
>> --- a/hw/scsi/vhost-scsi-common.c
>> +++ b/hw/scsi/vhost-scsi-common.c
>> @@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>
>> vsc->dev.acked_features = vdev->guest_features;
>>
>> -assert(vsc->inflight == NULL);
>> -vsc->inflight = g_new0(struct vhost_inflight, 1);
>> -ret = vhost_dev_get_inflight(&vsc->dev,
>> - vs->conf.virtqueue_size,
>> - vsc->inflight);
>> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
>> if (ret < 0) {
>> -error_report("Error get inflight: %d", -ret);
>> +error_report("Error setting inflight format: %d", -ret);
>> goto err_guest_notifiers;
>> }
>>
>> +if (!vsc->inflight->addr) {
>> +ret = vhost_dev_get_inflight(&vsc->dev,
>> +vs->conf.virtqueue_size,
>> +vsc->inflight);
>> +if (ret < 0) {
>> +error_report("Error get inflight: %d", -ret);
>> +goto err_guest_notifiers;
>> +}
>> +}
>> +
>> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
>> if (ret < 0) {
>> error_report("Error set inflight: %d", -ret);
>> @@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>> return ret;
>>
>> err_guest_notifiers:
>> -g_free(vsc->inflight);
>> -vsc->inflight = NULL;
>> -
>> k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> err_host_notifiers:
>> vhost_dev_disable_notifiers(&vsc->dev, vdev);
>> @@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>> }
>> assert(ret >= 0);
>>
>
> In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight
> now?
OK, we should check the vsc->inflight if it is null, the vhost-scsi
doesn’t allocate the
inflight object memory.
>>>
>>> Are you saying vhost-scsi never allocates inflight so we don’t need to
>>> check for it?
>> We have checked the vsc->inflight, and only if allocated, we send the
>> get/set_inflight_fd.
>> This works with vhost-user-scsi/vhost-scsi
Re: [PATCH] vhost-user-scsi: support reconnect to backend
> On Jul 28, 2023, at 3:48 AM, Li Feng wrote:
>
> Thanks for your reply.
>
>> 2023年7月28日 上午5:21，Raphael Norwitz 写道：
>>
>>
>>
>>> On Jul 25, 2023, at 6:19 AM, Li Feng wrote:
>>>
>>> Thanks for your comments.
>>>
2023年7月25日 上午1:21，Raphael Norwitz 写道：
Very excited to see this. High level looks good modulo a few small things.
My major concern is around existing vhost-user-scsi backends which don’t
support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the
reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We
may want to do the same for vhost-user-blk.
The question is then what happens if the check is false. IIUC without an
inflight FD, if a device processes requests out of order, it’s not safe to
continue execution on reconnect, as there’s no way for the backend to know
how to replay IO. Should we permanently wedge the device or have QEMU fail
out? May be nice to have a toggle for this.
>>>
>>> Based on what MST said, is there anything else I need to do?
>>
>> I don’t think so.
>>
> On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
>
> If the backend crashes and restarts, the device is broken.
> This patch adds reconnect for vhost-user-scsi.
>
> Tested with spdk backend.
>
> Signed-off-by: Li Feng
> ---
> hw/block/vhost-user-blk.c | 2 -
> hw/scsi/vhost-scsi-common.c | 27 ++---
> hw/scsi/vhost-user-scsi.c | 163 +---
> include/hw/virtio/vhost-user-scsi.h | 3 +
> include/hw/virtio/vhost.h | 2 +
> 5 files changed, 165 insertions(+), 32 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index eecf3f7a81..f250c740b5 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -32,8 +32,6 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/runstate.h"
>
> -#define REALIZE_CONNECTION_RETRIES 3
> -
> static const int user_feature_bits[] = {
> VIRTIO_BLK_F_SIZE_MAX,
> VIRTIO_BLK_F_SEG_MAX,
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
Why can’t all the vhost-scsi-common stuff be moved to a separate change?
>>>
>>> I will move this code to separate patch.
Especially the stuff introduced for vhost-user-blk in
1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
>>> OK.
>>>
> index a06f01af26..08801886b8 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>
> vsc->dev.acked_features = vdev->guest_features;
>
> -assert(vsc->inflight == NULL);
> -vsc->inflight = g_new0(struct vhost_inflight, 1);
> -ret = vhost_dev_get_inflight(&vsc->dev,
> - vs->conf.virtqueue_size,
> - vsc->inflight);
> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> -error_report("Error get inflight: %d", -ret);
> +error_report("Error setting inflight format: %d", -ret);
> goto err_guest_notifiers;
> }
>
> +if (!vsc->inflight->addr) {
> +ret = vhost_dev_get_inflight(&vsc->dev,
> +vs->conf.virtqueue_size,
> +vsc->inflight);
> +if (ret < 0) {
> +error_report("Error get inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> +}
> +
> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> if (ret < 0) {
> error_report("Error set inflight: %d", -ret);
> @@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> return ret;
>
> err_guest_notifiers:
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -
> k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> err_host_notifiers:
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> @@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> }
> assert(ret >= 0);
>
In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight
now?
>>> OK, we should check the vsc->inflight if it is null, the vhost-scsi doesn’t
>>> allocate the
>>> inflight object memory.
>>
>> Are you saying vhost-scsi never allocates inflight so we don’t need to check
>> for it?
> We have checked the vsc->inflight, and only if allocated, we send the
> get/set_inflight_fd.
> This works with vhost-user-scsi/vhost-scsi both.
So then it sounds like this code introduces a resource leak.
g_free(vsc->inflight) should be added to the vhost-scsi code in
vhost_scsi_stop().
>>
>>>
>
Re: [PATCH] vhost-user-scsi: support reconnect to backend
Thanks for your reply.
> 2023年7月28日 上午5:21，Raphael Norwitz 写道：
>
>
>
>> On Jul 25, 2023, at 6:19 AM, Li Feng wrote:
>>
>> Thanks for your comments.
>>
>>> 2023年7月25日 上午1:21，Raphael Norwitz 写道：
>>>
>>> Very excited to see this. High level looks good modulo a few small things.
>>>
>>> My major concern is around existing vhost-user-scsi backends which don’t
>>> support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the
>>> reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We
>>> may want to do the same for vhost-user-blk.
>>>
>>> The question is then what happens if the check is false. IIUC without an
>>> inflight FD, if a device processes requests out of order, it’s not safe to
>>> continue execution on reconnect, as there’s no way for the backend to know
>>> how to replay IO. Should we permanently wedge the device or have QEMU fail
>>> out? May be nice to have a toggle for this.
>>
>> Based on what MST said, is there anything else I need to do?
>
> I don’t think so.
>
>>>
On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
If the backend crashes and restarts, the device is broken.
This patch adds reconnect for vhost-user-scsi.
Tested with spdk backend.
Signed-off-by: Li Feng
---
hw/block/vhost-user-blk.c | 2 -
hw/scsi/vhost-scsi-common.c | 27 ++---
hw/scsi/vhost-user-scsi.c | 163 +---
include/hw/virtio/vhost-user-scsi.h | 3 +
include/hw/virtio/vhost.h | 2 +
5 files changed, 165 insertions(+), 32 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..f250c740b5 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -32,8 +32,6 @@
#include "sysemu/sysemu.h"
#include "sysemu/runstate.h"
-#define REALIZE_CONNECTION_RETRIES 3
-
static const int user_feature_bits[] = {
VIRTIO_BLK_F_SIZE_MAX,
VIRTIO_BLK_F_SEG_MAX,
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>>>
>>> Why can’t all the vhost-scsi-common stuff be moved to a separate change?
>>
>> I will move this code to separate patch.
>>>
>>> Especially the stuff introduced for vhost-user-blk in
>>> 1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
>> OK.
>>
>>>
index a06f01af26..08801886b8 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vsc->dev.acked_features = vdev->guest_features;
-assert(vsc->inflight == NULL);
-vsc->inflight = g_new0(struct vhost_inflight, 1);
-ret = vhost_dev_get_inflight(&vsc->dev,
- vs->conf.virtqueue_size,
- vsc->inflight);
+ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error get inflight: %d", -ret);
+error_report("Error setting inflight format: %d", -ret);
goto err_guest_notifiers;
}
+if (!vsc->inflight->addr) {
+ret = vhost_dev_get_inflight(&vsc->dev,
+vs->conf.virtqueue_size,
+vsc->inflight);
+if (ret < 0) {
+error_report("Error get inflight: %d", -ret);
+goto err_guest_notifiers;
+}
+}
+
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
error_report("Error set inflight: %d", -ret);
@@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
return ret;
err_guest_notifiers:
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-
k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
err_host_notifiers:
vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
}
assert(ret >= 0);
>>>
>>> In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight
>>> now?
>> OK, we should check the vsc->inflight if it is null, the vhost-scsi doesn’t
>> allocate the
>> inflight object memory.
>
> Are you saying vhost-scsi never allocates inflight so we don’t need to check
> for it?
We have checked the vsc->inflight, and only if allocated, we send the
get/set_inflight_fd.
This works with vhost-user-scsi/vhost-scsi both.
>
>>
>>>
-if (vsc->inflight) {
-vhost_dev_free_inflight(vsc->inflight);
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-}
-
vhost_dev_disable_notifiers(&vsc->dev, vdev);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index ee99b
Re: [PATCH] vhost-user-scsi: support reconnect to backend
> On Jul 25, 2023, at 6:19 AM, Li Feng wrote:
>
> Thanks for your comments.
>
>> 2023年7月25日 上午1:21，Raphael Norwitz 写道：
>>
>> Very excited to see this. High level looks good modulo a few small things.
>>
>> My major concern is around existing vhost-user-scsi backends which don’t
>> support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the
>> reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We
>> may want to do the same for vhost-user-blk.
>>
>> The question is then what happens if the check is false. IIUC without an
>> inflight FD, if a device processes requests out of order, it’s not safe to
>> continue execution on reconnect, as there’s no way for the backend to know
>> how to replay IO. Should we permanently wedge the device or have QEMU fail
>> out? May be nice to have a toggle for this.
>
> Based on what MST said, is there anything else I need to do?
I don’t think so.
>>
>>> On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
>>>
>>> If the backend crashes and restarts, the device is broken.
>>> This patch adds reconnect for vhost-user-scsi.
>>>
>>> Tested with spdk backend.
>>>
>>> Signed-off-by: Li Feng
>>> ---
>>> hw/block/vhost-user-blk.c | 2 -
>>> hw/scsi/vhost-scsi-common.c | 27 ++---
>>> hw/scsi/vhost-user-scsi.c | 163 +---
>>> include/hw/virtio/vhost-user-scsi.h | 3 +
>>> include/hw/virtio/vhost.h | 2 +
>>> 5 files changed, 165 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index eecf3f7a81..f250c740b5 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -32,8 +32,6 @@
>>> #include "sysemu/sysemu.h"
>>> #include "sysemu/runstate.h"
>>>
>>> -#define REALIZE_CONNECTION_RETRIES 3
>>> -
>>> static const int user_feature_bits[] = {
>>>VIRTIO_BLK_F_SIZE_MAX,
>>>VIRTIO_BLK_F_SEG_MAX,
>>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>>
>> Why can’t all the vhost-scsi-common stuff be moved to a separate change?
>
> I will move this code to separate patch.
>>
>> Especially the stuff introduced for vhost-user-blk in
>> 1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
> OK.
>
>>
>>> index a06f01af26..08801886b8 100644
>>> --- a/hw/scsi/vhost-scsi-common.c
>>> +++ b/hw/scsi/vhost-scsi-common.c
>>> @@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>>
>>>vsc->dev.acked_features = vdev->guest_features;
>>>
>>> -assert(vsc->inflight == NULL);
>>> -vsc->inflight = g_new0(struct vhost_inflight, 1);
>>> -ret = vhost_dev_get_inflight(&vsc->dev,
>>> - vs->conf.virtqueue_size,
>>> - vsc->inflight);
>>> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
>>>if (ret < 0) {
>>> -error_report("Error get inflight: %d", -ret);
>>> +error_report("Error setting inflight format: %d", -ret);
>>>goto err_guest_notifiers;
>>>}
>>>
>>> +if (!vsc->inflight->addr) {
>>> +ret = vhost_dev_get_inflight(&vsc->dev,
>>> +vs->conf.virtqueue_size,
>>> +vsc->inflight);
>>> +if (ret < 0) {
>>> +error_report("Error get inflight: %d", -ret);
>>> +goto err_guest_notifiers;
>>> +}
>>> +}
>>> +
>>>ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
>>>if (ret < 0) {
>>>error_report("Error set inflight: %d", -ret);
>>> @@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>>return ret;
>>>
>>> err_guest_notifiers:
>>> -g_free(vsc->inflight);
>>> -vsc->inflight = NULL;
>>> -
>>>k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>>> err_host_notifiers:
>>>vhost_dev_disable_notifiers(&vsc->dev, vdev);
>>> @@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>>>}
>>>assert(ret >= 0);
>>>
>>
>> In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight now?
> OK, we should check the vsc->inflight if it is null, the vhost-scsi doesn’t
> allocate the
> inflight object memory.
Are you saying vhost-scsi never allocates inflight so we don’t need to check
for it?
>
>>
>>> -if (vsc->inflight) {
>>> -vhost_dev_free_inflight(vsc->inflight);
>>> -g_free(vsc->inflight);
>>> -vsc->inflight = NULL;
>>> -}
>>> -
>>>vhost_dev_disable_notifiers(&vsc->dev, vdev);
>>> }
>>>
>>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>>> index ee99b19e7a..e0e88b0c42 100644
>>> --- a/hw/scsi/vhost-user-scsi.c
>>> +++ b/hw/scsi/vhost-user-scsi.c
>>> @@ -89,14 +89,126 @@ static void vhost_dummy_handle_output(VirtIODevice
>>> *vdev, VirtQueue *vq)
>>> {
>>> }
>>>
>>> +static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>>> +{
>>> +VirtIODevice *vdev = VIRTIO_DEVICE(de
Re: [PATCH] vhost-user-scsi: support reconnect to backend
Thanks for your comments.
> 2023年7月25日 上午1:21，Raphael Norwitz 写道：
>
> Very excited to see this. High level looks good modulo a few small things.
>
> My major concern is around existing vhost-user-scsi backends which don’t
> support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the
> reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We
> may want to do the same for vhost-user-blk.
>
> The question is then what happens if the check is false. IIUC without an
> inflight FD, if a device processes requests out of order, it’s not safe to
> continue execution on reconnect, as there’s no way for the backend to know
> how to replay IO. Should we permanently wedge the device or have QEMU fail
> out? May be nice to have a toggle for this.
Based on what MST said, is there anything else I need to do?
>
>> On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
>>
>> If the backend crashes and restarts, the device is broken.
>> This patch adds reconnect for vhost-user-scsi.
>>
>> Tested with spdk backend.
>>
>> Signed-off-by: Li Feng
>> ---
>> hw/block/vhost-user-blk.c | 2 -
>> hw/scsi/vhost-scsi-common.c | 27 ++---
>> hw/scsi/vhost-user-scsi.c | 163 +---
>> include/hw/virtio/vhost-user-scsi.h | 3 +
>> include/hw/virtio/vhost.h | 2 +
>> 5 files changed, 165 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index eecf3f7a81..f250c740b5 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -32,8 +32,6 @@
>> #include "sysemu/sysemu.h"
>> #include "sysemu/runstate.h"
>>
>> -#define REALIZE_CONNECTION_RETRIES 3
>> -
>> static const int user_feature_bits[] = {
>>VIRTIO_BLK_F_SIZE_MAX,
>>VIRTIO_BLK_F_SEG_MAX,
>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>
> Why can’t all the vhost-scsi-common stuff be moved to a separate change?
I will move this code to separate patch.
>
> Especially the stuff introduced for vhost-user-blk in
> 1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
OK.
>
>> index a06f01af26..08801886b8 100644
>> --- a/hw/scsi/vhost-scsi-common.c
>> +++ b/hw/scsi/vhost-scsi-common.c
>> @@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>
>>vsc->dev.acked_features = vdev->guest_features;
>>
>> -assert(vsc->inflight == NULL);
>> -vsc->inflight = g_new0(struct vhost_inflight, 1);
>> -ret = vhost_dev_get_inflight(&vsc->dev,
>> - vs->conf.virtqueue_size,
>> - vsc->inflight);
>> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
>>if (ret < 0) {
>> -error_report("Error get inflight: %d", -ret);
>> +error_report("Error setting inflight format: %d", -ret);
>>goto err_guest_notifiers;
>>}
>>
>> +if (!vsc->inflight->addr) {
>> +ret = vhost_dev_get_inflight(&vsc->dev,
>> +vs->conf.virtqueue_size,
>> +vsc->inflight);
>> +if (ret < 0) {
>> +error_report("Error get inflight: %d", -ret);
>> +goto err_guest_notifiers;
>> +}
>> +}
>> +
>>ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
>>if (ret < 0) {
>>error_report("Error set inflight: %d", -ret);
>> @@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>return ret;
>>
>> err_guest_notifiers:
>> -g_free(vsc->inflight);
>> -vsc->inflight = NULL;
>> -
>>k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> err_host_notifiers:
>>vhost_dev_disable_notifiers(&vsc->dev, vdev);
>> @@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>>}
>>assert(ret >= 0);
>>
>
> In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight now?
OK, we should check the vsc->inflight if it is null, the vhost-scsi doesn’t
allocate the
inflight object memory.
>
>> -if (vsc->inflight) {
>> -vhost_dev_free_inflight(vsc->inflight);
>> -g_free(vsc->inflight);
>> -vsc->inflight = NULL;
>> -}
>> -
>>vhost_dev_disable_notifiers(&vsc->dev, vdev);
>> }
>>
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index ee99b19e7a..e0e88b0c42 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -89,14 +89,126 @@ 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_q
Re: [PATCH] vhost-user-scsi: support reconnect to backend
On Mon, Jul 24, 2023 at 05:21:37PM +, Raphael Norwitz wrote: > Very excited to see this. High level looks good modulo a few small things. > > My major concern is around existing vhost-user-scsi backends which don’t > support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the > reconnect behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We > may want to do the same for vhost-user-blk. > > The question is then what happens if the check is false. IIUC without an > inflight FD, if a device processes requests out of order, it’s not safe to > continue execution on reconnect, as there’s no way for the backend to know > how to replay IO. Should we permanently wedge the device or have QEMU fail > out? May be nice to have a toggle for this. No, device itself can store the state somewhere. And if it wants to, it can check VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD and fail reconnect. -- MST
Re: [PATCH] vhost-user-scsi: support reconnect to backend
Very excited to see this. High level looks good modulo a few small things.
My major concern is around existing vhost-user-scsi backends which don’t
support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD. IMO we should hide the reconnect
behavior behind a VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD check. We may want to do
the same for vhost-user-blk.
The question is then what happens if the check is false. IIUC without an
inflight FD, if a device processes requests out of order, it’s not safe to
continue execution on reconnect, as there’s no way for the backend to know how
to replay IO. Should we permanently wedge the device or have QEMU fail out? May
be nice to have a toggle for this.
> On Jul 21, 2023, at 6:51 AM, Li Feng wrote:
>
> If the backend crashes and restarts, the device is broken.
> This patch adds reconnect for vhost-user-scsi.
>
> Tested with spdk backend.
>
> Signed-off-by: Li Feng
> ---
> hw/block/vhost-user-blk.c | 2 -
> hw/scsi/vhost-scsi-common.c | 27 ++---
> hw/scsi/vhost-user-scsi.c | 163 +---
> include/hw/virtio/vhost-user-scsi.h | 3 +
> include/hw/virtio/vhost.h | 2 +
> 5 files changed, 165 insertions(+), 32 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index eecf3f7a81..f250c740b5 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -32,8 +32,6 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/runstate.h"
>
> -#define REALIZE_CONNECTION_RETRIES 3
> -
> static const int user_feature_bits[] = {
> VIRTIO_BLK_F_SIZE_MAX,
> VIRTIO_BLK_F_SEG_MAX,
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
Why can’t all the vhost-scsi-common stuff be moved to a separate change?
Especially the stuff introduced for vhost-user-blk in
1b0063b3048af65dfaae6422a572c87db8575a92 should be moved out.
> index a06f01af26..08801886b8 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>
> vsc->dev.acked_features = vdev->guest_features;
>
> -assert(vsc->inflight == NULL);
> -vsc->inflight = g_new0(struct vhost_inflight, 1);
> -ret = vhost_dev_get_inflight(&vsc->dev,
> - vs->conf.virtqueue_size,
> - vsc->inflight);
> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> -error_report("Error get inflight: %d", -ret);
> +error_report("Error setting inflight format: %d", -ret);
> goto err_guest_notifiers;
> }
>
> +if (!vsc->inflight->addr) {
> +ret = vhost_dev_get_inflight(&vsc->dev,
> +vs->conf.virtqueue_size,
> +vsc->inflight);
> +if (ret < 0) {
> +error_report("Error get inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> +}
> +
> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> if (ret < 0) {
> error_report("Error set inflight: %d", -ret);
> @@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> return ret;
>
> err_guest_notifiers:
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -
> k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> err_host_notifiers:
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> @@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> }
> assert(ret >= 0);
>
In the vhost-scsi (kernel backend) path, what will cleanup vsc->inflight now?
> -if (vsc->inflight) {
> -vhost_dev_free_inflight(vsc->inflight);
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -}
> -
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> }
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index ee99b19e7a..e0e88b0c42 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -89,14 +89,126 @@ 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 */
Should this
[PATCH] 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.
Tested with spdk backend.
Signed-off-by: Li Feng
---
hw/block/vhost-user-blk.c | 2 -
hw/scsi/vhost-scsi-common.c | 27 ++---
hw/scsi/vhost-user-scsi.c | 163 +---
include/hw/virtio/vhost-user-scsi.h | 3 +
include/hw/virtio/vhost.h | 2 +
5 files changed, 165 insertions(+), 32 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..f250c740b5 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -32,8 +32,6 @@
#include "sysemu/sysemu.h"
#include "sysemu/runstate.h"
-#define REALIZE_CONNECTION_RETRIES 3
-
static const int user_feature_bits[] = {
VIRTIO_BLK_F_SIZE_MAX,
VIRTIO_BLK_F_SEG_MAX,
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a06f01af26..08801886b8 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,16 +52,22 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vsc->dev.acked_features = vdev->guest_features;
-assert(vsc->inflight == NULL);
-vsc->inflight = g_new0(struct vhost_inflight, 1);
-ret = vhost_dev_get_inflight(&vsc->dev,
- vs->conf.virtqueue_size,
- vsc->inflight);
+ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error get inflight: %d", -ret);
+error_report("Error setting inflight format: %d", -ret);
goto err_guest_notifiers;
}
+if (!vsc->inflight->addr) {
+ret = vhost_dev_get_inflight(&vsc->dev,
+vs->conf.virtqueue_size,
+vsc->inflight);
+if (ret < 0) {
+error_report("Error get inflight: %d", -ret);
+goto err_guest_notifiers;
+}
+}
+
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
error_report("Error set inflight: %d", -ret);
@@ -85,9 +91,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
return ret;
err_guest_notifiers:
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-
k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
err_host_notifiers:
vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +114,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
}
assert(ret >= 0);
-if (vsc->inflight) {
-vhost_dev_free_inflight(vsc->inflight);
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-}
-
vhost_dev_disable_notifiers(&vsc->dev, vdev);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index ee99b19e7a..e0e88b0c42 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -89,14 +89,126 @@ 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_scsi_common_start(vsc);
+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_scsi_common_stop(vsc);
+
+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)
