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

[Li Feng]

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