| Message ID | 20240514061239.86461-2-fengli@smartx.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] Revert "vhost-user: fix lost reconnect" | expand |
The code for these two patches looks fine. Just some questions on the failure case you're trying to fix. On Tue, May 14, 2024 at 2:12 AM Li Feng <fengli@smartx.com> wrote: > > This reverts commit f02a4b8e6431598612466f76aac64ab492849abf. > > Since the current patch cannot completely fix the lost reconnect > problem, there is a scenario that is not considered: > - When the virtio-blk driver is removed from the guest os, > s->connected has no chance to be set to false, resulting in Why would the virtio-blk driver being removed (unloaded?) in the guest effect s->connected? Isn't this variable just tracking whether Qemu is connected to the backend process? What does it have to do with the guest driver state? > subsequent reconnection not being executed. > > The next patch will completely fix this issue with a better approach. > > Signed-off-by: Li Feng <fengli@smartx.com> > --- > hw/block/vhost-user-blk.c | 2 +- > hw/scsi/vhost-user-scsi.c | 3 +-- > hw/virtio/vhost-user-base.c | 2 +- > hw/virtio/vhost-user.c | 10 ++-------- > include/hw/virtio/vhost-user.h | 3 +-- > 5 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index 9e6bbc6950..41d1ac3a5a 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -384,7 +384,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) > case CHR_EVENT_CLOSED: > /* defer close until later to avoid circular close */ > vhost_user_async_close(dev, &s->chardev, &s->dev, > - vhost_user_blk_disconnect, vhost_user_blk_event); > + vhost_user_blk_disconnect); > break; > case CHR_EVENT_BREAK: > case CHR_EVENT_MUX_IN: > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > index a63b1f4948..48a59e020e 100644 > --- a/hw/scsi/vhost-user-scsi.c > +++ b/hw/scsi/vhost-user-scsi.c > @@ -214,8 +214,7 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event) > 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, > - vhost_user_scsi_event); > + vhost_user_scsi_disconnect); > break; > case CHR_EVENT_BREAK: > case CHR_EVENT_MUX_IN: > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c > index a83167191e..4b54255682 100644 > --- a/hw/virtio/vhost-user-base.c > +++ b/hw/virtio/vhost-user-base.c > @@ -254,7 +254,7 @@ static void vub_event(void *opaque, QEMUChrEvent event) > case CHR_EVENT_CLOSED: > /* defer close until later to avoid circular close */ > vhost_user_async_close(dev, &vub->chardev, &vub->vhost_dev, > - vub_disconnect, vub_event); > + vub_disconnect); > break; > case CHR_EVENT_BREAK: > case CHR_EVENT_MUX_IN: > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index cdf9af4a4b..c929097e87 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -2776,7 +2776,6 @@ typedef struct { > DeviceState *dev; > CharBackend *cd; > struct vhost_dev *vhost; > - IOEventHandler *event_cb; > } VhostAsyncCallback; > > static void vhost_user_async_close_bh(void *opaque) > @@ -2791,10 +2790,7 @@ static void vhost_user_async_close_bh(void *opaque) > */ > if (vhost->vdev) { > data->cb(data->dev); > - } else if (data->event_cb) { > - qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, > - NULL, data->dev, NULL, true); > - } > + } > > g_free(data); > } > @@ -2806,8 +2802,7 @@ static void vhost_user_async_close_bh(void *opaque) > */ > void vhost_user_async_close(DeviceState *d, > CharBackend *chardev, struct vhost_dev *vhost, > - vu_async_close_fn cb, > - IOEventHandler *event_cb) > + vu_async_close_fn cb) > { > if (!runstate_check(RUN_STATE_SHUTDOWN)) { > /* > @@ -2823,7 +2818,6 @@ void vhost_user_async_close(DeviceState *d, > data->dev = d; > data->cd = chardev; > data->vhost = vhost; > - data->event_cb = event_cb; > > /* Disable any further notifications on the chardev */ > qemu_chr_fe_set_handlers(chardev, > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h > index d7c09ffd34..324cd8663a 100644 > --- a/include/hw/virtio/vhost-user.h > +++ b/include/hw/virtio/vhost-user.h > @@ -108,7 +108,6 @@ typedef void (*vu_async_close_fn)(DeviceState *cb); > > void vhost_user_async_close(DeviceState *d, > CharBackend *chardev, struct vhost_dev *vhost, > - vu_async_close_fn cb, > - IOEventHandler *event_cb); > + vu_async_close_fn cb); > > #endif > -- > 2.45.0 >
> 2024年5月14日 21:58，Raphael Norwitz <raphael@enfabrica.net> 写道： > > The code for these two patches looks fine. Just some questions on the > failure case you're trying to fix. > > > On Tue, May 14, 2024 at 2:12 AM Li Feng <fengli@smartx.com> wrote: >> >> This reverts commit f02a4b8e6431598612466f76aac64ab492849abf. >> >> Since the current patch cannot completely fix the lost reconnect >> problem, there is a scenario that is not considered: >> - When the virtio-blk driver is removed from the guest os, >> s->connected has no chance to be set to false, resulting in > > Why would the virtio-blk driver being removed (unloaded?) in the guest > effect s->connected? Isn't this variable just tracking whether Qemu is > connected to the backend process? What does it have to do with the > guest driver state? Unload the virtio-blk, it will trigger ‘vhost_user_blk_stop’, and in `vhost_dev_stop` it will set the `hdev->vdev = NULL;`. Next if kill the backend, the CLOSE event will be triggered, and the `vhost->vdev` has been set to null before, then the `vhost_user_blk_disconnect` will not have a chance to execute.So that he s->connected is still true. static void vhost_user_async_close_bh(void *opaque) { VhostAsyncCallback *data = opaque; struct vhost_dev *vhost = data->vhost; /* * If the vhost_dev has been cleared in the meantime there is * nothing left to do as some other path has completed the * cleanup. */ if (vhost->vdev) { <============================ HERE vdev is null. data->cb(data->dev); } else if (data->event_cb) { qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, NULL, data->dev, NULL, true); } g_free(data); } Thanks, Li > >> subsequent reconnection not being executed. >> >> The next patch will completely fix this issue with a better approach. >> >> Signed-off-by: Li Feng <fengli@smartx.com> >> --- >> hw/block/vhost-user-blk.c | 2 +- >> hw/scsi/vhost-user-scsi.c | 3 +-- >> hw/virtio/vhost-user-base.c | 2 +- >> hw/virtio/vhost-user.c | 10 ++-------- >> include/hw/virtio/vhost-user.h | 3 +-- >> 5 files changed, 6 insertions(+), 14 deletions(-) >> >> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c >> index 9e6bbc6950..41d1ac3a5a 100644 >> --- a/hw/block/vhost-user-blk.c >> +++ b/hw/block/vhost-user-blk.c >> @@ -384,7 +384,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) >> case CHR_EVENT_CLOSED: >> /* defer close until later to avoid circular close */ >> vhost_user_async_close(dev, &s->chardev, &s->dev, >> - vhost_user_blk_disconnect, vhost_user_blk_event); >> + vhost_user_blk_disconnect); >> break; >> case CHR_EVENT_BREAK: >> case CHR_EVENT_MUX_IN: >> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c >> index a63b1f4948..48a59e020e 100644 >> --- a/hw/scsi/vhost-user-scsi.c >> +++ b/hw/scsi/vhost-user-scsi.c >> @@ -214,8 +214,7 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event) >> 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, >> - vhost_user_scsi_event); >> + vhost_user_scsi_disconnect); >> break; >> case CHR_EVENT_BREAK: >> case CHR_EVENT_MUX_IN: >> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c >> index a83167191e..4b54255682 100644 >> --- a/hw/virtio/vhost-user-base.c >> +++ b/hw/virtio/vhost-user-base.c >> @@ -254,7 +254,7 @@ static void vub_event(void *opaque, QEMUChrEvent event) >> case CHR_EVENT_CLOSED: >> /* defer close until later to avoid circular close */ >> vhost_user_async_close(dev, &vub->chardev, &vub->vhost_dev, >> - vub_disconnect, vub_event); >> + vub_disconnect); >> break; >> case CHR_EVENT_BREAK: >> case CHR_EVENT_MUX_IN: >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >> index cdf9af4a4b..c929097e87 100644 >> --- a/hw/virtio/vhost-user.c >> +++ b/hw/virtio/vhost-user.c >> @@ -2776,7 +2776,6 @@ typedef struct { >> DeviceState *dev; >> CharBackend *cd; >> struct vhost_dev *vhost; >> - IOEventHandler *event_cb; >> } VhostAsyncCallback; >> >> static void vhost_user_async_close_bh(void *opaque) >> @@ -2791,10 +2790,7 @@ static void vhost_user_async_close_bh(void *opaque) >> */ >> if (vhost->vdev) { >> data->cb(data->dev); >> - } else if (data->event_cb) { >> - qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, >> - NULL, data->dev, NULL, true); >> - } >> + } >> >> g_free(data); >> } >> @@ -2806,8 +2802,7 @@ static void vhost_user_async_close_bh(void *opaque) >> */ >> void vhost_user_async_close(DeviceState *d, >> CharBackend *chardev, struct vhost_dev *vhost, >> - vu_async_close_fn cb, >> - IOEventHandler *event_cb) >> + vu_async_close_fn cb) >> { >> if (!runstate_check(RUN_STATE_SHUTDOWN)) { >> /* >> @@ -2823,7 +2818,6 @@ void vhost_user_async_close(DeviceState *d, >> data->dev = d; >> data->cd = chardev; >> data->vhost = vhost; >> - data->event_cb = event_cb; >> >> /* Disable any further notifications on the chardev */ >> qemu_chr_fe_set_handlers(chardev, >> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h >> index d7c09ffd34..324cd8663a 100644 >> --- a/include/hw/virtio/vhost-user.h >> +++ b/include/hw/virtio/vhost-user.h >> @@ -108,7 +108,6 @@ typedef void (*vu_async_close_fn)(DeviceState *cb); >> >> void vhost_user_async_close(DeviceState *d, >> CharBackend *chardev, struct vhost_dev *vhost, >> - vu_async_close_fn cb, >> - IOEventHandler *event_cb); >> + vu_async_close_fn cb); >> >> #endif >> -- >> 2.45.0 >>
On Wed, May 15, 2024 at 1:47 AM Li Feng <fengli@smartx.com> wrote: > > > > > 2024年5月14日 21:58，Raphael Norwitz <raphael@enfabrica.net> 写道： > > > > The code for these two patches looks fine. Just some questions on the > > failure case you're trying to fix. > > > > > > On Tue, May 14, 2024 at 2:12 AM Li Feng <fengli@smartx.com> wrote: > >> > >> This reverts commit f02a4b8e6431598612466f76aac64ab492849abf. > >> > >> Since the current patch cannot completely fix the lost reconnect > >> problem, there is a scenario that is not considered: > >> - When the virtio-blk driver is removed from the guest os, > >> s->connected has no chance to be set to false, resulting in > > > > Why would the virtio-blk driver being removed (unloaded?) in the guest > > effect s->connected? Isn't this variable just tracking whether Qemu is > > connected to the backend process? What does it have to do with the > > guest driver state? > > Unload the virtio-blk, it will trigger ‘vhost_user_blk_stop’, and in `vhost_dev_stop` > it will set the `hdev->vdev = NULL;`. > > Next if kill the backend, the CLOSE event will be triggered, and the `vhost->vdev` > has been set to null before, then the `vhost_user_blk_disconnect` will not have a > chance to execute.So that he s->connected is still true. Makes sense - basically if the driver is unloaded and then the device is brought down s->connected will remain true when it should be false, which will mess up a subsequent reconnect. See my comments on the following patch though. > > static void vhost_user_async_close_bh(void *opaque) > { > VhostAsyncCallback *data = opaque; > struct vhost_dev *vhost = data->vhost; > > /* > * If the vhost_dev has been cleared in the meantime there is > * nothing left to do as some other path has completed the > * cleanup. > */ > if (vhost->vdev) { <============================ HERE vdev is null. > data->cb(data->dev); > } else if (data->event_cb) { > qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, > NULL, data->dev, NULL, true); > } > > g_free(data); > } > > Thanks, > Li > > > > >> subsequent reconnection not being executed. > >> > >> The next patch will completely fix this issue with a better approach. > >> > >> Signed-off-by: Li Feng <fengli@smartx.com> > >> --- > >> hw/block/vhost-user-blk.c | 2 +- > >> hw/scsi/vhost-user-scsi.c | 3 +-- > >> hw/virtio/vhost-user-base.c | 2 +- > >> hw/virtio/vhost-user.c | 10 ++-------- > >> include/hw/virtio/vhost-user.h | 3 +-- > >> 5 files changed, 6 insertions(+), 14 deletions(-) > >> > >> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > >> index 9e6bbc6950..41d1ac3a5a 100644 > >> --- a/hw/block/vhost-user-blk.c > >> +++ b/hw/block/vhost-user-blk.c > >> @@ -384,7 +384,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) > >> case CHR_EVENT_CLOSED: > >> /* defer close until later to avoid circular close */ > >> vhost_user_async_close(dev, &s->chardev, &s->dev, > >> - vhost_user_blk_disconnect, vhost_user_blk_event); > >> + vhost_user_blk_disconnect); > >> break; > >> case CHR_EVENT_BREAK: > >> case CHR_EVENT_MUX_IN: > >> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > >> index a63b1f4948..48a59e020e 100644 > >> --- a/hw/scsi/vhost-user-scsi.c > >> +++ b/hw/scsi/vhost-user-scsi.c > >> @@ -214,8 +214,7 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event) > >> 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, > >> - vhost_user_scsi_event); > >> + vhost_user_scsi_disconnect); > >> break; > >> case CHR_EVENT_BREAK: > >> case CHR_EVENT_MUX_IN: > >> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c > >> index a83167191e..4b54255682 100644 > >> --- a/hw/virtio/vhost-user-base.c > >> +++ b/hw/virtio/vhost-user-base.c > >> @@ -254,7 +254,7 @@ static void vub_event(void *opaque, QEMUChrEvent event) > >> case CHR_EVENT_CLOSED: > >> /* defer close until later to avoid circular close */ > >> vhost_user_async_close(dev, &vub->chardev, &vub->vhost_dev, > >> - vub_disconnect, vub_event); > >> + vub_disconnect); > >> break; > >> case CHR_EVENT_BREAK: > >> case CHR_EVENT_MUX_IN: > >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >> index cdf9af4a4b..c929097e87 100644 > >> --- a/hw/virtio/vhost-user.c > >> +++ b/hw/virtio/vhost-user.c > >> @@ -2776,7 +2776,6 @@ typedef struct { > >> DeviceState *dev; > >> CharBackend *cd; > >> struct vhost_dev *vhost; > >> - IOEventHandler *event_cb; > >> } VhostAsyncCallback; > >> > >> static void vhost_user_async_close_bh(void *opaque) > >> @@ -2791,10 +2790,7 @@ static void vhost_user_async_close_bh(void *opaque) > >> */ > >> if (vhost->vdev) { > >> data->cb(data->dev); > >> - } else if (data->event_cb) { > >> - qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, > >> - NULL, data->dev, NULL, true); > >> - } > >> + } > >> > >> g_free(data); > >> } > >> @@ -2806,8 +2802,7 @@ static void vhost_user_async_close_bh(void *opaque) > >> */ > >> void vhost_user_async_close(DeviceState *d, > >> CharBackend *chardev, struct vhost_dev *vhost, > >> - vu_async_close_fn cb, > >> - IOEventHandler *event_cb) > >> + vu_async_close_fn cb) > >> { > >> if (!runstate_check(RUN_STATE_SHUTDOWN)) { > >> /* > >> @@ -2823,7 +2818,6 @@ void vhost_user_async_close(DeviceState *d, > >> data->dev = d; > >> data->cd = chardev; > >> data->vhost = vhost; > >> - data->event_cb = event_cb; > >> > >> /* Disable any further notifications on the chardev */ > >> qemu_chr_fe_set_handlers(chardev, > >> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h > >> index d7c09ffd34..324cd8663a 100644 > >> --- a/include/hw/virtio/vhost-user.h > >> +++ b/include/hw/virtio/vhost-user.h > >> @@ -108,7 +108,6 @@ typedef void (*vu_async_close_fn)(DeviceState *cb); > >> > >> void vhost_user_async_close(DeviceState *d, > >> CharBackend *chardev, struct vhost_dev *vhost, > >> - vu_async_close_fn cb, > >> - IOEventHandler *event_cb); > >> + vu_async_close_fn cb); > >> > >> #endif > >> -- > >> 2.45.0 > >> >
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 9e6bbc6950..41d1ac3a5a 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -384,7 +384,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) case CHR_EVENT_CLOSED: /* defer close until later to avoid circular close */ vhost_user_async_close(dev, &s->chardev, &s->dev, - vhost_user_blk_disconnect, vhost_user_blk_event); + vhost_user_blk_disconnect); break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN: diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index a63b1f4948..48a59e020e 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -214,8 +214,7 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event) 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, - vhost_user_scsi_event); + vhost_user_scsi_disconnect); break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN: diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c index a83167191e..4b54255682 100644 --- a/hw/virtio/vhost-user-base.c +++ b/hw/virtio/vhost-user-base.c @@ -254,7 +254,7 @@ static void vub_event(void *opaque, QEMUChrEvent event) case CHR_EVENT_CLOSED: /* defer close until later to avoid circular close */ vhost_user_async_close(dev, &vub->chardev, &vub->vhost_dev, - vub_disconnect, vub_event); + vub_disconnect); break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN: diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index cdf9af4a4b..c929097e87 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2776,7 +2776,6 @@ typedef struct { DeviceState *dev; CharBackend *cd; struct vhost_dev *vhost; - IOEventHandler *event_cb; } VhostAsyncCallback; static void vhost_user_async_close_bh(void *opaque) @@ -2791,10 +2790,7 @@ static void vhost_user_async_close_bh(void *opaque) */ if (vhost->vdev) { data->cb(data->dev); - } else if (data->event_cb) { - qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb, - NULL, data->dev, NULL, true); - } + } g_free(data); } @@ -2806,8 +2802,7 @@ static void vhost_user_async_close_bh(void *opaque) */ void vhost_user_async_close(DeviceState *d, CharBackend *chardev, struct vhost_dev *vhost, - vu_async_close_fn cb, - IOEventHandler *event_cb) + vu_async_close_fn cb) { if (!runstate_check(RUN_STATE_SHUTDOWN)) { /* @@ -2823,7 +2818,6 @@ void vhost_user_async_close(DeviceState *d, data->dev = d; data->cd = chardev; data->vhost = vhost; - data->event_cb = event_cb; /* Disable any further notifications on the chardev */ qemu_chr_fe_set_handlers(chardev, diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h index d7c09ffd34..324cd8663a 100644 --- a/include/hw/virtio/vhost-user.h +++ b/include/hw/virtio/vhost-user.h @@ -108,7 +108,6 @@ typedef void (*vu_async_close_fn)(DeviceState *cb); void vhost_user_async_close(DeviceState *d, CharBackend *chardev, struct vhost_dev *vhost, - vu_async_close_fn cb, - IOEventHandler *event_cb); + vu_async_close_fn cb); #endif
This reverts commit f02a4b8e6431598612466f76aac64ab492849abf. Since the current patch cannot completely fix the lost reconnect problem, there is a scenario that is not considered: - When the virtio-blk driver is removed from the guest os, s->connected has no chance to be set to false, resulting in subsequent reconnection not being executed. The next patch will completely fix this issue with a better approach. Signed-off-by: Li Feng <fengli@smartx.com> --- hw/block/vhost-user-blk.c | 2 +- hw/scsi/vhost-user-scsi.c | 3 +-- hw/virtio/vhost-user-base.c | 2 +- hw/virtio/vhost-user.c | 10 ++-------- include/hw/virtio/vhost-user.h | 3 +-- 5 files changed, 6 insertions(+), 14 deletions(-)