Re: [PATCH v3 1/2] Revert "vhost-user: fix lost reconnect"

[Raphael Norwitz]

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
> >>
>