> 2024年5月14日 21:58，Raphael Norwitz <raph...@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 <fen...@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 <fen...@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
>>
