| Message ID | 20230725104256.4861-3-fengli@smartx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement reconnect for vhost-user-scsi | expand |
On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote: > Get_inflight_fd is sent only once. When reconnecting to the backend, > qemu sent set_inflight_fd to the backend. I don't understand what you are trying to say here. Should be: Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ. > Signed-off-by: Li Feng <fengli@smartx.com> > --- > hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c > index a06f01af26..664adb15b4 100644 > --- a/hw/scsi/vhost-scsi-common.c > +++ b/hw/scsi/vhost-scsi-common.c > @@ -52,20 +52,28 @@ 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; > } > > - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); > - if (ret < 0) { > - error_report("Error set inflight: %d", -ret); > - goto err_guest_notifiers; > + if (vsc->inflight) { > + 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); As long as you are fixing this - should be "getting inflight". > + goto err_guest_notifiers; > + } > + } > + > + ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); > + if (ret < 0) { > + error_report("Error set inflight: %d", -ret); > + goto err_guest_notifiers; > + } > } > > ret = vhost_dev_start(&vsc->dev, vdev, true); > @@ -85,9 +93,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 +116,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); > } > > -- > 2.41.0
> 2023年7月28日 下午2:04，Michael S. Tsirkin <mst@redhat.com> 写道： > > On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote: >> Get_inflight_fd is sent only once. When reconnecting to the backend, >> qemu sent set_inflight_fd to the backend. > > I don't understand what you are trying to say here. > Should be: > Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ. Thanks, I will reorganize the commit message in v3. > >> Signed-off-by: Li Feng <fengli@smartx.com> >> --- >> hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- >> 1 file changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c >> index a06f01af26..664adb15b4 100644 >> --- a/hw/scsi/vhost-scsi-common.c >> +++ b/hw/scsi/vhost-scsi-common.c >> @@ -52,20 +52,28 @@ 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; >> } >> >> - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >> - if (ret < 0) { >> - error_report("Error set inflight: %d", -ret); >> - goto err_guest_notifiers; >> + if (vsc->inflight) { >> + 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); > > As long as you are fixing this - should be "getting inflight”. I will fix it in v3. > >> + goto err_guest_notifiers; >> + } >> + } >> + >> + ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >> + if (ret < 0) { >> + error_report("Error set inflight: %d", -ret); >> + goto err_guest_notifiers; >> + } >> } >> >> ret = vhost_dev_start(&vsc->dev, vdev, true); >> @@ -85,9 +93,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 +116,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); >> } >> >> -- >> 2.41.0
> On Jul 28, 2023, at 3:49 AM, Li Feng <fengli@smartx.com> wrote: > > > >> 2023年7月28日 下午2:04，Michael S. Tsirkin <mst@redhat.com> 写道： >> >> On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote: >>> Get_inflight_fd is sent only once. When reconnecting to the backend, >>> qemu sent set_inflight_fd to the backend. >> >> I don't understand what you are trying to say here. >> Should be: >> Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ. > > Thanks, I will reorganize the commit message in v3. >> >>> Signed-off-by: Li Feng <fengli@smartx.com> >>> --- >>> hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- >>> 1 file changed, 18 insertions(+), 19 deletions(-) >>> >>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c >>> index a06f01af26..664adb15b4 100644 >>> --- a/hw/scsi/vhost-scsi-common.c >>> +++ b/hw/scsi/vhost-scsi-common.c >>> @@ -52,20 +52,28 @@ 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; >>> } >>> >>> - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>> - if (ret < 0) { >>> - error_report("Error set inflight: %d", -ret); >>> - goto err_guest_notifiers; >>> + if (vsc->inflight) { >>> + 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); >> >> As long as you are fixing this - should be "getting inflight”. > I will fix it in v3. >> >>> + goto err_guest_notifiers; >>> + } >>> + } >>> + Looks like you reworked this a bit so to avoid a potential crash if vsc->inflight is NULL Should we fix it for vhost-user-blk too? >>> + ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>> + if (ret < 0) { >>> + error_report("Error set inflight: %d", -ret); >>> + goto err_guest_notifiers; >>> + } >>> } >>> >>> ret = vhost_dev_start(&vsc->dev, vdev, true); >>> @@ -85,9 +93,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 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc) >>> } >>> assert(ret >= 0); >>> As I said before, I think this introduces a leak. >>> - if (vsc->inflight) { >>> - vhost_dev_free_inflight(vsc->inflight); >>> - g_free(vsc->inflight); >>> - vsc->inflight = NULL; >>> - } >>> - >>> vhost_dev_disable_notifiers(&vsc->dev, vdev); >>> } >>> >>> -- >>> 2.41.0
> 2023年7月31日 06:13，Raphael Norwitz <raphael.norwitz@nutanix.com> 写道： > >> >> On Jul 28, 2023, at 3:49 AM, Li Feng <fengli@smartx.com> wrote: >> >> >> >>> 2023年7月28日 下午2:04，Michael S. Tsirkin <mst@redhat.com> 写道： >>> >>> On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote: >>>> Get_inflight_fd is sent only once. When reconnecting to the backend, >>>> qemu sent set_inflight_fd to the backend. >>> >>> I don't understand what you are trying to say here. >>> Should be: >>> Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ. >> >> Thanks, I will reorganize the commit message in v3. >>> >>>> Signed-off-by: Li Feng <fengli@smartx.com> >>>> --- >>>> hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- >>>> 1 file changed, 18 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c >>>> index a06f01af26..664adb15b4 100644 >>>> --- a/hw/scsi/vhost-scsi-common.c >>>> +++ b/hw/scsi/vhost-scsi-common.c >>>> @@ -52,20 +52,28 @@ 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; >>>> } >>>> >>>> - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>>> - if (ret < 0) { >>>> - error_report("Error set inflight: %d", -ret); >>>> - goto err_guest_notifiers; >>>> + if (vsc->inflight) { >>>> + 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); >>> >>> As long as you are fixing this - should be "getting inflight”. >> I will fix it in v3. >>> >>>> + goto err_guest_notifiers; >>>> + } >>>> + } >>>> + > > Looks like you reworked this a bit so to avoid a potential crash if vsc->inflight is NULL > > Should we fix it for vhost-user-blk too? > This check is mainly for the vhost-scsi code, that doesn’t need allocate the inflight memory. The vhost-user-blk doesn’t need this check, because there isn't a vhost-blk device that reuse the code. >>>> + ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>>> + if (ret < 0) { >>>> + error_report("Error set inflight: %d", -ret); >>>> + goto err_guest_notifiers; >>>> + } >>>> } >>>> >>>> ret = vhost_dev_start(&vsc->dev, vdev, true); >>>> @@ -85,9 +93,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 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc) >>>> } >>>> assert(ret >= 0); >>>> > > As I said before, I think this introduces a leak. I have answered in the previous mail. > >>>> - if (vsc->inflight) { >>>> - vhost_dev_free_inflight(vsc->inflight); >>>> - g_free(vsc->inflight); >>>> - vsc->inflight = NULL; >>>> - } >>>> - >>>> vhost_dev_disable_notifiers(&vsc->dev, vdev); >>>> } >>>> >>>> -- >>>> 2.41.0
> On Jul 31, 2023, at 7:38 AM, Li Feng <fengli@smartx.com> wrote: > > > >> 2023年7月31日 06:13，Raphael Norwitz <raphael.norwitz@nutanix.com> 写道： >> >>> >>> On Jul 28, 2023, at 3:49 AM, Li Feng <fengli@smartx.com> wrote: >>> >>> >>> >>>> 2023年7月28日 下午2:04，Michael S. Tsirkin <mst@redhat.com> 写道： >>>> >>>> On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote: >>>>> Get_inflight_fd is sent only once. When reconnecting to the backend, >>>>> qemu sent set_inflight_fd to the backend. >>>> >>>> I don't understand what you are trying to say here. >>>> Should be: >>>> Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ. >>> >>> Thanks, I will reorganize the commit message in v3. >>>> >>>>> Signed-off-by: Li Feng <fengli@smartx.com> >>>>> --- >>>>> hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- >>>>> 1 file changed, 18 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c >>>>> index a06f01af26..664adb15b4 100644 >>>>> --- a/hw/scsi/vhost-scsi-common.c >>>>> +++ b/hw/scsi/vhost-scsi-common.c >>>>> @@ -52,20 +52,28 @@ 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; >>>>> } >>>>> >>>>> - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>>>> - if (ret < 0) { >>>>> - error_report("Error set inflight: %d", -ret); >>>>> - goto err_guest_notifiers; >>>>> + if (vsc->inflight) { >>>>> + 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); >>>> >>>> As long as you are fixing this - should be "getting inflight”. >>> I will fix it in v3. >>>> >>>>> + goto err_guest_notifiers; >>>>> + } >>>>> + } >>>>> + >> >> Looks like you reworked this a bit so to avoid a potential crash if vsc->inflight is NULL >> >> Should we fix it for vhost-user-blk too? >> > This check is mainly for the vhost-scsi code, that doesn’t need allocate the inflight memory. > > The vhost-user-blk doesn’t need this check, because there isn't a vhost-blk device that reuse the code. > Makes sense. >>>>> + ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); >>>>> + if (ret < 0) { >>>>> + error_report("Error set inflight: %d", -ret); >>>>> + goto err_guest_notifiers; >>>>> + } >>>>> } >>>>> >>>>> ret = vhost_dev_start(&vsc->dev, vdev, true); >>>>> @@ -85,9 +93,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 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc) >>>>> } >>>>> assert(ret >= 0); >>>>> >> >> As I said before, I think this introduces a leak. > I have answered in the previous mail. > On re-review I agree it’s fine since vac-inflight isn’t set. >> >>>>> - if (vsc->inflight) { >>>>> - vhost_dev_free_inflight(vsc->inflight); >>>>> - g_free(vsc->inflight); >>>>> - vsc->inflight = NULL; >>>>> - } >>>>> - >>>>> vhost_dev_disable_notifiers(&vsc->dev, vdev); >>>>> } >>>>> >>>>> -- >>>>> 2.41.0
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c index a06f01af26..664adb15b4 100644 --- a/hw/scsi/vhost-scsi-common.c +++ b/hw/scsi/vhost-scsi-common.c @@ -52,20 +52,28 @@ 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; } - ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight); - if (ret < 0) { - error_report("Error set inflight: %d", -ret); - goto err_guest_notifiers; + if (vsc->inflight) { + 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); + goto err_guest_notifiers; + } } ret = vhost_dev_start(&vsc->dev, vdev, true); @@ -85,9 +93,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 +116,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); }
Get_inflight_fd is sent only once. When reconnecting to the backend, qemu sent set_inflight_fd to the backend. Signed-off-by: Li Feng <fengli@smartx.com> --- hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)