hw/block/vhost-user-blk.c | 88 ++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 34 deletions(-)
Commit a1a20d06b73e "vhost-user-blk: delay vhost_user_blk_disconnect"
introduced postponing vhost_dev cleanup aiming to eliminate qemu aborts
because of connection problems with vhost-blk daemon.
However, it introdues a new problem. Now, any communication errors
during execution of vhost_dev_init() called by vhost_user_blk_device_realize()
lead to qemu abort on assert in vhost_dev_get_config().
This happens because vhost_user_blk_disconnect() is postponed but
it should have dropped s->connected flag by the time
vhost_user_blk_device_realize() performs a new connection opening.
On the connection opening, vhost_dev initialization in
vhost_user_blk_connect() relies on s->connection flag and
if it's not dropped, it skips vhost_dev initialization and returns
with success. Then, vhost_user_blk_device_realize()'s execution flow
goes to vhost_dev_get_config() where it's aborted on the assert.
It seems connection/disconnection processing should happen
differently on initialization and operation of vhost-user-blk.
On initialization (in vhost_user_blk_device_realize()) we fully
control the initialization process. At that point, nobody can use the
device since it isn't initialized and we don't need to postpone any
cleanups, so we can do cleanup right away when there is communication
problems with the vhost-blk daemon.
On operation the disconnect may happen when the device is in use, so
the device users may want to use vhost_dev's data to do rollback before
vhost_dev is re-initialized (e.g. in vhost_dev_set_log()), so we
postpone the cleanup.
The patch splits those two cases, and performs the cleanup immediately on
initialization, and postpones cleanup when the device is initialized and
in use.
Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
---
hw/block/vhost-user-blk.c | 88 ++++++++++++++++++++++++---------------
1 file changed, 54 insertions(+), 34 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index b870a50e6b20..84940122b8ca 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -362,7 +362,17 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
vhost_dev_cleanup(&s->dev);
}
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init);
+
+static void vhost_user_blk_event_init(void *opaque, QEMUChrEvent event)
+{
+ vhost_user_blk_event(opaque, event, true);
+}
+
+static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
+{
+ vhost_user_blk_event(opaque, event, false);
+}
static void vhost_user_blk_chr_closed_bh(void *opaque)
{
@@ -371,11 +381,11 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
VHostUserBlk *s = VHOST_USER_BLK(vdev);
vhost_user_blk_disconnect(dev);
- qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
- NULL, opaque, NULL, true);
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+ vhost_user_blk_event_oper, NULL, opaque, NULL, true);
}
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init)
{
DeviceState *dev = opaque;
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -390,38 +400,42 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
break;
case CHR_EVENT_CLOSED:
/*
- * A close event may happen during a read/write, but vhost
- * code assumes the vhost_dev remains setup, so delay the
- * stop & clear. There are two possible paths to hit this
- * disconnect event:
- * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
- * vhost_user_blk_device_realize() is a caller.
- * 2. In tha main loop phase after VM start.
- *
- * For p2 the disconnect event will be delayed. We can't
- * do the same for p1, because we are not running the loop
- * at this moment. So just skip this step and perform
- * disconnect in the caller function.
- *
- * TODO: maybe it is a good idea to make the same fix
- * for other vhost-user devices.
+ * Closing the connection should happen differently on device
+ * initialization and operation stages.
+ * On initalization, we want to re-start vhost_dev initialization
+ * from the very beginning right away when the connection is closed,
+ * so we clean up vhost_dev on each connection closing.
+ * On operation, we want to postpone vhost_dev cleanup to let the
+ * other code perform its own cleanup sequence using vhost_dev data
+ * (e.g. vhost_dev_set_log).
*/
- if (runstate_is_running()) {
+ if (init) {
+ vhost_user_blk_disconnect(dev);
+ } else {
+ /*
+ * A close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear.
+ */
AioContext *ctx = qemu_get_current_aio_context();
- qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
- NULL, NULL, false);
- aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
+ /*
+ * Prevent any re-connection until cleanup is done in
+ * vhost_user_blk_chr_closed_bh
+ */
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
+ NULL, NULL, false);
+ aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
+
+ /*
+ * Move vhost device to the stopped state. The vhost-user device
+ * will be clean up and disconnected in BH. This can be useful in
+ * the vhost migration code. If disconnect was caught there is an
+ * option for the general vhost code to get the dev state without
+ * knowing its type (in this case vhost-user).
+ */
+ s->dev.started = false;
}
-
- /*
- * Move vhost device to the stopped state. The vhost-user device
- * will be clean up and disconnected in BH. This can be useful in
- * the vhost migration code. If disconnect was caught there is an
- * option for the general vhost code to get the dev state without
- * knowing its type (in this case vhost-user).
- */
- s->dev.started = false;
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
@@ -473,8 +487,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
s->connected = false;
- qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
- NULL, (void *)dev, NULL, true);
+ /* set the handler performing immediate cleanup on each disconnect */
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+ vhost_user_blk_event_init, NULL, (void *)dev,
+ NULL, true);
reconnect:
if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
@@ -494,6 +510,10 @@ reconnect:
goto reconnect;
}
+ /* we're fully initialized, now we can operate, so change the handler */
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+ vhost_user_blk_event_oper, NULL, (void *)dev,
+ NULL, true);
return;
virtio_err:
--
2.25.1
ping!
On 11.03.2021 11:10, Denis Plotnikov wrote:
> Commit a1a20d06b73e "vhost-user-blk: delay vhost_user_blk_disconnect"
> introduced postponing vhost_dev cleanup aiming to eliminate qemu aborts
> because of connection problems with vhost-blk daemon.
>
> However, it introdues a new problem. Now, any communication errors
> during execution of vhost_dev_init() called by vhost_user_blk_device_realize()
> lead to qemu abort on assert in vhost_dev_get_config().
>
> This happens because vhost_user_blk_disconnect() is postponed but
> it should have dropped s->connected flag by the time
> vhost_user_blk_device_realize() performs a new connection opening.
> On the connection opening, vhost_dev initialization in
> vhost_user_blk_connect() relies on s->connection flag and
> if it's not dropped, it skips vhost_dev initialization and returns
> with success. Then, vhost_user_blk_device_realize()'s execution flow
> goes to vhost_dev_get_config() where it's aborted on the assert.
>
> It seems connection/disconnection processing should happen
> differently on initialization and operation of vhost-user-blk.
> On initialization (in vhost_user_blk_device_realize()) we fully
> control the initialization process. At that point, nobody can use the
> device since it isn't initialized and we don't need to postpone any
> cleanups, so we can do cleanup right away when there is communication
> problems with the vhost-blk daemon.
> On operation the disconnect may happen when the device is in use, so
> the device users may want to use vhost_dev's data to do rollback before
> vhost_dev is re-initialized (e.g. in vhost_dev_set_log()), so we
> postpone the cleanup.
>
> The patch splits those two cases, and performs the cleanup immediately on
> initialization, and postpones cleanup when the device is initialized and
> in use.
>
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
> hw/block/vhost-user-blk.c | 88 ++++++++++++++++++++++++---------------
> 1 file changed, 54 insertions(+), 34 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index b870a50e6b20..84940122b8ca 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -362,7 +362,17 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> vhost_dev_cleanup(&s->dev);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init);
> +
> +static void vhost_user_blk_event_init(void *opaque, QEMUChrEvent event)
> +{
> + vhost_user_blk_event(opaque, event, true);
> +}
> +
> +static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
> +{
> + vhost_user_blk_event(opaque, event, false);
> +}
>
> static void vhost_user_blk_chr_closed_bh(void *opaque)
> {
> @@ -371,11 +381,11 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>
> vhost_user_blk_disconnect(dev);
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> - NULL, opaque, NULL, true);
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_oper, NULL, opaque, NULL, true);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init)
> {
> DeviceState *dev = opaque;
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -390,38 +400,42 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> break;
> case CHR_EVENT_CLOSED:
> /*
> - * A close event may happen during a read/write, but vhost
> - * code assumes the vhost_dev remains setup, so delay the
> - * stop & clear. There are two possible paths to hit this
> - * disconnect event:
> - * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
> - * vhost_user_blk_device_realize() is a caller.
> - * 2. In tha main loop phase after VM start.
> - *
> - * For p2 the disconnect event will be delayed. We can't
> - * do the same for p1, because we are not running the loop
> - * at this moment. So just skip this step and perform
> - * disconnect in the caller function.
> - *
> - * TODO: maybe it is a good idea to make the same fix
> - * for other vhost-user devices.
> + * Closing the connection should happen differently on device
> + * initialization and operation stages.
> + * On initalization, we want to re-start vhost_dev initialization
> + * from the very beginning right away when the connection is closed,
> + * so we clean up vhost_dev on each connection closing.
> + * On operation, we want to postpone vhost_dev cleanup to let the
> + * other code perform its own cleanup sequence using vhost_dev data
> + * (e.g. vhost_dev_set_log).
> */
> - if (runstate_is_running()) {
> + if (init) {
> + vhost_user_blk_disconnect(dev);
> + } else {
> + /*
> + * A close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear.
> + */
> AioContext *ctx = qemu_get_current_aio_context();
>
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> - NULL, NULL, false);
> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> + /*
> + * Prevent any re-connection until cleanup is done in
> + * vhost_user_blk_chr_closed_bh
> + */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> + NULL, NULL, false);
> + aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> +
> + /*
> + * Move vhost device to the stopped state. The vhost-user device
> + * will be clean up and disconnected in BH. This can be useful in
> + * the vhost migration code. If disconnect was caught there is an
> + * option for the general vhost code to get the dev state without
> + * knowing its type (in this case vhost-user).
> + */
> + s->dev.started = false;
> }
> -
> - /*
> - * Move vhost device to the stopped state. The vhost-user device
> - * will be clean up and disconnected in BH. This can be useful in
> - * the vhost migration code. If disconnect was caught there is an
> - * option for the general vhost code to get the dev state without
> - * knowing its type (in this case vhost-user).
> - */
> - s->dev.started = false;
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> @@ -473,8 +487,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
> s->connected = false;
>
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> - NULL, (void *)dev, NULL, true);
> + /* set the handler performing immediate cleanup on each disconnect */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_init, NULL, (void *)dev,
> + NULL, true);
>
> reconnect:
> if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
> @@ -494,6 +510,10 @@ reconnect:
> goto reconnect;
> }
>
> + /* we're fully initialized, now we can operate, so change the handler */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_oper, NULL, (void *)dev,
> + NULL, true);
> return;
>
> virtio_err:
I'm mostly happy with this. My biggest overall comment is that I think
this should be split into two, as your refactor using different event
handlers for init is a standalone improvement over and above the bugfix.
I would have the first commit split out vhost_user_blk_event_init() and
vhost_user_blk_event_oper(), replace the runstate_is_running() check
...etc and the second commit immidiately call
vhost_user_blk_disconnect() during device realization.
A couple other comments mixed in bellow:
On Thu, Mar 11, 2021 at 11:10:45AM +0300, Denis Plotnikov wrote:
> Commit a1a20d06b73e "vhost-user-blk: delay vhost_user_blk_disconnect"
For the hash above can we rather use the first digits of the commit hash
instead of the last?
> introduced postponing vhost_dev cleanup aiming to eliminate qemu aborts
> because of connection problems with vhost-blk daemon.
>
> However, it introdues a new problem. Now, any communication errors
> during execution of vhost_dev_init() called by vhost_user_blk_device_realize()
> lead to qemu abort on assert in vhost_dev_get_config().
>
> This happens because vhost_user_blk_disconnect() is postponed but
> it should have dropped s->connected flag by the time
> vhost_user_blk_device_realize() performs a new connection opening.
> On the connection opening, vhost_dev initialization in
> vhost_user_blk_connect() relies on s->connection flag and
> if it's not dropped, it skips vhost_dev initialization and returns
> with success. Then, vhost_user_blk_device_realize()'s execution flow
> goes to vhost_dev_get_config() where it's aborted on the assert.
>
> It seems connection/disconnection processing should happen
> differently on initialization and operation of vhost-user-blk.
> On initialization (in vhost_user_blk_device_realize()) we fully
> control the initialization process. At that point, nobody can use the
> device since it isn't initialized and we don't need to postpone any
> cleanups, so we can do cleanup right away when there is communication
> problems with the vhost-blk daemon.
> On operation the disconnect may happen when the device is in use, so
> the device users may want to use vhost_dev's data to do rollback before
> vhost_dev is re-initialized (e.g. in vhost_dev_set_log()), so we
> postpone the cleanup.
>
> The patch splits those two cases, and performs the cleanup immediately on
> initialization, and postpones cleanup when the device is initialized and
> in use.
>
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
> hw/block/vhost-user-blk.c | 88 ++++++++++++++++++++++++---------------
> 1 file changed, 54 insertions(+), 34 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index b870a50e6b20..84940122b8ca 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -362,7 +362,17 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> vhost_dev_cleanup(&s->dev);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init);
The parameter name "init" feels a little unclear. Maybe "realized"
would be better? I would also change the vhost_user_blk_event_init
function name accordingly.
> +
> +static void vhost_user_blk_event_init(void *opaque, QEMUChrEvent event)
> +{
> + vhost_user_blk_event(opaque, event, true);
> +}
> +
> +static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
> +{
> + vhost_user_blk_event(opaque, event, false);
> +}
>
> static void vhost_user_blk_chr_closed_bh(void *opaque)
> {
> @@ -371,11 +381,11 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>
> vhost_user_blk_disconnect(dev);
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> - NULL, opaque, NULL, true);
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_oper, NULL, opaque, NULL, true);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, bool init)
> {
> DeviceState *dev = opaque;
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -390,38 +400,42 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> break;
> case CHR_EVENT_CLOSED:
> /*
> - * A close event may happen during a read/write, but vhost
> - * code assumes the vhost_dev remains setup, so delay the
> - * stop & clear. There are two possible paths to hit this
> - * disconnect event:
> - * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
> - * vhost_user_blk_device_realize() is a caller.
> - * 2. In tha main loop phase after VM start.
> - *
> - * For p2 the disconnect event will be delayed. We can't
> - * do the same for p1, because we are not running the loop
> - * at this moment. So just skip this step and perform
> - * disconnect in the caller function.
> - *
> - * TODO: maybe it is a good idea to make the same fix
> - * for other vhost-user devices.
> + * Closing the connection should happen differently on device
> + * initialization and operation stages.
> + * On initalization, we want to re-start vhost_dev initialization
> + * from the very beginning right away when the connection is closed,
> + * so we clean up vhost_dev on each connection closing.
> + * On operation, we want to postpone vhost_dev cleanup to let the
> + * other code perform its own cleanup sequence using vhost_dev data
> + * (e.g. vhost_dev_set_log).
> */
> - if (runstate_is_running()) {
> + if (init) {
> + vhost_user_blk_disconnect(dev);
> + } else {
> + /*
> + * A close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear.
> + */
> AioContext *ctx = qemu_get_current_aio_context();
>
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> - NULL, NULL, false);
> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
This comment itself is a standalone improvement - maybe add it as another separate
commit?
> + /*
> + * Prevent any re-connection until cleanup is done in
> + * vhost_user_blk_chr_closed_bh
> + */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> + NULL, NULL, false);
> + aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> +
> + /*
> + * Move vhost device to the stopped state. The vhost-user device
> + * will be clean up and disconnected in BH. This can be useful in
> + * the vhost migration code. If disconnect was caught there is an
> + * option for the general vhost code to get the dev state without
> + * knowing its type (in this case vhost-user).
> + */
> + s->dev.started = false;
> }
> -
> - /*
> - * Move vhost device to the stopped state. The vhost-user device
> - * will be clean up and disconnected in BH. This can be useful in
> - * the vhost migration code. If disconnect was caught there is an
> - * option for the general vhost code to get the dev state without
> - * knowing its type (in this case vhost-user).
> - */
> - s->dev.started = false;
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> @@ -473,8 +487,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
> s->connected = false;
>
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> - NULL, (void *)dev, NULL, true);
> + /* set the handler performing immediate cleanup on each disconnect */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_init, NULL, (void *)dev,
> + NULL, true);
>
> reconnect:
> if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
> @@ -494,6 +510,10 @@ reconnect:
> goto reconnect;
> }
>
> + /* we're fully initialized, now we can operate, so change the handler */
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> + vhost_user_blk_event_oper, NULL, (void *)dev,
> + NULL, true);
> return;
>
> virtio_err:
> --
> 2.25.1
>
© 2016 - 2024 Red Hat, Inc.