date:20230928

Re: [PATCH v3 0/7] Steps towards enabling -Wshadow=local

2023-09-28 Thread Markus Armbruster

Markus Armbruster  writes:

> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Bugs love to hide in such code.
> Evidence: PATCH 1.
>
> Enabling -Wshadow would prevent bugs like this one.  But we'd have to
> clean up all the offenders first.  We got a lot of them.
>
> Enabling -Wshadow=local should be less work for almost as much gain.
> I took a stab at it.  There's a small, exciting part, and a large,
> boring part.
>
> The exciting part is dark preprocessor sorcery to let us nest macro
> calls without shadowing: PATCH 7.

[...]

Queued.

Re: [PATCH] test-throttle: don't shadow 'index' variable in do_test_accounting()

2023-09-28 Thread Markus Armbruster

Alberto Garcia  writes:

> Fixes build with -Wshadow=local
>
> Signed-off-by: Alberto Garcia 
> ---
>  tests/unit/test-throttle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
> index cb587e33e7..ac35d65d19 100644
> --- a/tests/unit/test-throttle.c
> +++ b/tests/unit/test-throttle.c
> @@ -625,7 +625,7 @@ static bool do_test_accounting(bool is_ops, /* are we 
> testing bps or ops */
>  throttle_config_init();
>  
>  for (i = 0; i < 3; i++) {
> -BucketType index = to_test[is_ops][i];
> +index = to_test[is_ops][i];
>  cfg.buckets[index].avg = avg;
>  }

Reviewed-by: Markus Armbruster 
and queued, thanks!

Re: [PATCH 0/3] (few more) Steps towards enabling -Wshadow [3 more]

2023-09-28 Thread Markus Armbruster

Philippe Mathieu-Daudé  writes:

> Just missed while posting v2 eh :/
> (https://lore.kernel.org/qemu-devel/20230904161235.84651-1-phi...@linaro.org/)

PATCH 3 has become commit 82fdcd3e140c8d4c63f177ece554f90f2bccdf68.

Remainder queued.  Thanks!

Re: [PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

2023-09-28 Thread Markus Armbruster

Philippe Mathieu-Daudé  writes:

> Since v1:
> - Addressed review comments
> - Added R-b tags
> - More patches
>
> For rational see Markus cover on
> https://lore.kernel.org/qemu-devel/20230831132546.3525721-1-arm...@redhat.com/
>
> This series contains few more, my take.
>
> Based-on: <20230831132546.3525721-1-arm...@redhat.com>

Queued except for:

PATCH 11: I asked for John Snow's opinion on a matter of taste.
PATCH 18: Review comment from Peter Maydell is pending.

Thanks!

Re: [PATCH v2 11/22] hw/ide/ahci: Clean up local variable shadowing

2023-09-28 Thread Markus Armbruster

Philippe Mathieu-Daudé  writes:

>   hw/ide/ahci.c:1577:23: error: declaration shadows a local variable 
> [-Werror,-Wshadow]
> IDEState *s = >port.ifs[j];
>   ^
>   hw/ide/ahci.c:1569:29: note: previous declaration is here
> void ahci_uninit(AHCIState *s)
> ^
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/ahci.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 48d550f633..8c9a7c2117 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1573,10 +1573,8 @@ void ahci_uninit(AHCIState *s)
>  for (i = 0; i < s->ports; i++) {
>  AHCIDevice *ad = >dev[i];
>  
> -for (j = 0; j < 2; j++) {
> -IDEState *s = >port.ifs[j];
> -
> -ide_exit(s);
> +for (j = 0; j < ARRAY_SIZE(ad->port.ifs); j++) {

This line's change is unrelated.

When we're dealing with at a huge changeset like the tree-wide
-Wshadow=local cleanup, I prefer to keep changes minimal to ease review
as much as possible.  But it's sunk cost now.

ad->port.ifs is IDEBus member IDEState ifs[2].  .ifs[0] corresponds to
.master, and .ifs[1] corresponds to .slave.  Size 2 is fundamental, and
will not ever change.  Whether we count to 2 or to ARRAY_SIZE(.ifs) is a
matter of taste.  Taste is up to the maintainer, not me.  John?

> +ide_exit(>port.ifs[j]);
>  }
>  object_unparent(OBJECT(>port));
>  }

Re: [PATCH v2 20/22] sysemu/device_tree: Clean up local variable shadowing

2023-09-28 Thread Markus Armbruster

Philippe Mathieu-Daudé  writes:

> Fix:
>
>   hw/mips/boston.c:472:5: error: declaration shadows a local variable 
> [-Werror,-Wshadow]
> qemu_fdt_setprop_cells(fdt, name, "reg", reg_base, reg_size);
> ^
>   include/sysemu/device_tree.h:129:13: note: expanded from macro 
> 'qemu_fdt_setprop_cells'
> int i;
> ^
>   hw/mips/boston.c:461:9: note: previous declaration is here
> int i;
> ^
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/device_tree.h | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ca5339beae..8eab395934 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -126,10 +126,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)
>  \
>  do { 
>  \
>  uint32_t qdt_tmp[] = { __VA_ARGS__ };
>  \
> -int i;   
>  \
> - 
>  \
> -for (i = 0; i < ARRAY_SIZE(qdt_tmp); i++) {  
>  \
> -qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);
>  \
> +for (unsigned i_ = 0; i_ < ARRAY_SIZE(qdt_tmp); i_++) {  
>  \
> +qdt_tmp[i_] = cpu_to_be32(qdt_tmp[i_]);  
>  \
>  }
>  \
>  qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,  
>  \
>   sizeof(qdt_tmp));   
>  \

You don't just rename the variable, but also adjust its type.  When
we're dealing with at a huge changeset like the tree-wide -Wshadow=local
cleanup, I prefer to keep changes minimal to ease review as much as
possible.  But it's sunk cost now, so

Reviewed-by: Markus Armbruster

Re: [PATCH v6 4/5] vhost-user-scsi: start vhost when guest kicks

2023-09-28 Thread Raphael Norwitz




> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> Let's keep the same behavior as vhost-user-blk.
> 
> Some old guests kick virtqueue before setting VIRTIO_CONFIG_S_DRIVER_OK.
> 

Reviewed-by: Raphael Norwitz 

> Signed-off-by: Li Feng 
> ---
> hw/scsi/vhost-user-scsi.c | 48 +++
> 1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index dc109154ad..53a62c3170 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -115,8 +115,48 @@ static void vhost_user_scsi_reset(VirtIODevice *vdev)
> }
> }
> 
> -static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> +static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> {
> +VHostUserSCSI *s = (VHostUserSCSI *)vdev;
> +DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
> +VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> +VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> +
> +Error *local_err = NULL;
> +int i, ret;
> +
> +if (!vdev->start_on_kick) {
> +return;
> +}
> +
> +if (!s->connected) {
> +return;
> +}
> +
> +if (vhost_dev_is_started(>dev)) {
> +return;
> +}
> +
> +/*
> + * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> + * vhost here instead of waiting for .set_status().
> + */
> +ret = vhost_user_scsi_start(s, _err);
> +if (ret < 0) {
> +error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: 
> ");
> +qemu_chr_fe_disconnect(>conf.chardev);
> +return;
> +}
> +
> +/* Kick right away to begin processing requests already in vring */
> +for (i = 0; i < vsc->dev.nvqs; i++) {
> +VirtQueue *kick_vq = virtio_get_queue(vdev, i);
> +
> +if (!virtio_queue_get_desc_addr(vdev, i)) {
> +continue;
> +}
> +event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
> +}
> }
> 
> static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
> @@ -246,9 +286,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, 
> Error **errp)
> return;
> }
> 
> -virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
> -   vhost_dummy_handle_output,
> -   vhost_dummy_handle_output, );
> +virtio_scsi_common_realize(dev, vhost_user_scsi_handle_output,
> +   vhost_user_scsi_handle_output,
> +   vhost_user_scsi_handle_output, );
> if (err != NULL) {
> error_propagate(errp, err);
> return;
> -- 
> 2.41.0
>

Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

2023-09-28 Thread Raphael Norwitz



> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> Currently the get_inflight_fd will be sent every time the device is started, 
> and
> the backend will allocate shared memory to save the inflight state. If the
> backend finds that it receives the second get_inflight_fd, it will release the
> previous shared memory, which breaks inflight working logic.
> 
> This patch is a preparation for the following patches.

This looks identical to the v3 patch I reviewed? If I’ve missed something can 
you please point it out?


> Signed-off-by: Li Feng 
> ---
> hw/scsi/vhost-scsi-common.c | 37 ++---
> 1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a06f01af26..a61cd0e907 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> 
> vsc->dev.acked_features = vdev->guest_features;
> 
> -assert(vsc->inflight == NULL);
> -vsc->inflight = g_new0(struct vhost_inflight, 1);
> -ret = vhost_dev_get_inflight(>dev,
> - vs->conf.virtqueue_size,
> - vsc->inflight);
> +ret = vhost_dev_prepare_inflight(>dev, vdev);
> if (ret < 0) {
> -error_report("Error get inflight: %d", -ret);
> +error_report("Error setting inflight format: %d", -ret);
> goto err_guest_notifiers;
> }
> 
> -ret = vhost_dev_set_inflight(>dev, vsc->inflight);
> -if (ret < 0) {
> -error_report("Error set inflight: %d", -ret);
> -goto err_guest_notifiers;
> +if (vsc->inflight) {
> +if (!vsc->inflight->addr) {
> +ret = vhost_dev_get_inflight(>dev,
> +vs->conf.virtqueue_size,
> +vsc->inflight);
> +if (ret < 0) {
> +error_report("Error getting inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> +}
> +
> +ret = vhost_dev_set_inflight(>dev, vsc->inflight);
> +if (ret < 0) {
> +error_report("Error setting inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> }
> 
> ret = vhost_dev_start(>dev, vdev, true);
> @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> return ret;
> 
> err_guest_notifiers:
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -
> k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> err_host_notifiers:
> vhost_dev_disable_notifiers(>dev, vdev);
> @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> }
> assert(ret >= 0);
> 
> -if (vsc->inflight) {
> -vhost_dev_free_inflight(vsc->inflight);
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -}
> -
> vhost_dev_disable_notifiers(>dev, vdev);
> }
> 
> -- 
> 2.41.0
>

Re: [PATCH v6 5/5] vhost-user: fix lost reconnect

2023-09-28 Thread Raphael Norwitz




> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> When the vhost-user is reconnecting to the backend, and if the vhost-user 
> fails
> at the get_features in vhost_dev_init(), then the reconnect will fail
> and it will not be retriggered forever.
> 
> The reason is:
> When the vhost-user fails at get_features, the vhost_dev_cleanup will be 
> called
> immediately.
> 
> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
> 
> The reconnect path is:
> vhost_user_blk_event
>   vhost_user_async_close(.. vhost_user_blk_disconnect ..)
> qemu_chr_fe_set_handlers <- clear the notifier callback
>   schedule vhost_user_async_close_bh
> 
> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
> called, then the event fd callback will not be reinstalled.
> 
> All vhost-user devices have this issue, including vhost-user-blk/scsi.
> 
> With this patch, if the vdev->vdev is null, the fd callback will still
> be reinstalled.
> 
> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
> 

Reviewed-by: Raphael Norwitz 

> Signed-off-by: Li Feng 
> ---
> hw/block/vhost-user-blk.c  |  2 +-
> hw/scsi/vhost-user-scsi.c  |  3 ++-
> hw/virtio/vhost-user-gpio.c|  2 +-
> hw/virtio/vhost-user.c | 10 --
> include/hw/virtio/vhost-user.h |  3 ++-
> 5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 3c69fa47d5..95c758200d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -391,7 +391,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, >chardev, >dev,
> -   vhost_user_blk_disconnect);
> +   vhost_user_blk_disconnect, 
> vhost_user_blk_event);
> 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 53a62c3170..0effbb4787 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -238,7 +238,8 @@ 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, >conf.chardev, >dev,
> -   vhost_user_scsi_disconnect);
> +   vhost_user_scsi_disconnect,
> +   vhost_user_scsi_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d9979aa5db..04c2cc79f4 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent 
> event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, >chardev, >vhost_dev,
> -   vu_gpio_disconnect);
> +   vu_gpio_disconnect, vu_gpio_event);
> 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 8dcf049d42..7344f57ba7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2643,6 +2643,7 @@ typedef struct {
> DeviceState *dev;
> CharBackend *cd;
> struct vhost_dev *vhost;
> +IOEventHandler *event_cb;
> } VhostAsyncCallback;
> 
> static void vhost_user_async_close_bh(void *opaque)
> @@ -2657,7 +2658,10 @@ 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);
> }
> @@ -2669,7 +2673,8 @@ 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)
> +vu_async_close_fn cb,
> +IOEventHandler *event_cb)
> {
> if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> /*
> @@ -2685,6 +2690,7 @@ 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 191216a74f..649e9dd54f 100644
> --- a/include/hw/virtio/vhost-user.h
> +++

Re: [PATCH v6 3/5] vhost-user-scsi: support reconnect to backend

2023-09-28 Thread Raphael Norwitz

One comment on the logging stuff in vhost-scsi. As far as I can tell the 
logging in vhost-user-scsi looks good.

Markus - does this look better to you? Otherwise do you think we should also 
fix up the vhost-user-blk realize function?

> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> If the backend crashes and restarts, the device is broken.
> This patch adds reconnect for vhost-user-scsi.
> 
> This patch also improves the error messages, and reports some silent errors.
> 
> Tested with spdk backend.
> 
> Signed-off-by: Li Feng 
> ---
> hw/scsi/vhost-scsi-common.c   |  16 +-
> hw/scsi/vhost-scsi.c  |   5 +-
> hw/scsi/vhost-user-scsi.c | 204 +++---
> include/hw/virtio/vhost-scsi-common.h |   2 +-
> include/hw/virtio/vhost-user-scsi.h   |   4 +
> 5 files changed, 201 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a61cd0e907..4c8637045d 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -16,6 +16,7 @@
>  */
> 
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> #include "hw/virtio/vhost.h"
> @@ -25,7 +26,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "hw/fw-path-provider.h"
> 
> -int vhost_scsi_common_start(VHostSCSICommon *vsc)
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
> {
> int ret, i;
> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
> 
> if (!k->set_guest_notifiers) {
> -error_report("binding does not support guest notifiers");
> +error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> }
> 
> ret = vhost_dev_enable_notifiers(>dev, vdev);
> if (ret < 0) {
> +error_setg_errno(errp, -ret, "Error enabling host notifiers");
> return ret;
> }
> 
> ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
> if (ret < 0) {
> -error_report("Error binding guest notifier");
> +error_setg_errno(errp, -ret, "Error binding guest notifier");
> goto err_host_notifiers;
> }
> 
> @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> 
> ret = vhost_dev_prepare_inflight(>dev, vdev);
> if (ret < 0) {
> -error_report("Error setting inflight format: %d", -ret);
> +error_setg_errno(errp, -ret, "Error setting inflight format");
> goto err_guest_notifiers;
> }
> 
> @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> vs->conf.virtqueue_size,
> vsc->inflight);
> if (ret < 0) {
> -error_report("Error getting inflight: %d", -ret);
> +error_setg_errno(errp, -ret, "Error getting inflight");
> goto err_guest_notifiers;
> }
> }
> 
> ret = vhost_dev_set_inflight(>dev, vsc->inflight);
> if (ret < 0) {
> -error_report("Error setting inflight: %d", -ret);
> +error_setg_errno(errp, -ret, "Error setting inflight");
> goto err_guest_notifiers;
> }
> }
> 
> ret = vhost_dev_start(>dev, vdev, true);
> if (ret < 0) {
> -error_report("Error start vhost dev");
> +error_setg_errno(errp, -ret, "Error starting vhost dev");
> goto err_guest_notifiers;
> }
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 443f67daa4..01a3ab4277 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
> int ret, abi_version;
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> const VhostOps *vhost_ops = vsc->dev.vhost_ops;
> +Error *local_err = NULL;
> 
> ret = vhost_ops->vhost_scsi_get_abi_version(>dev, _version);
> if (ret < 0) {
> @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
> return -ENOSYS;
> }
> 
> -ret = vhost_scsi_common_start(vsc);
> +ret = vhost_scsi_common_start(vsc, _err);
> if (ret < 0) {

Why aren’t you reporting the error here?

> return ret;
> }
> 
> ret = vhost_scsi_set_endpoint(s);
> if (ret < 0) {
> -error_report("Error setting vhost-scsi endpoint");
> +error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
> vhost_scsi_common_stop(vsc);
> }
> 
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index ee99b19e7a..dc109154ad 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -43,26 +43,56 @@ enum VhostUserProtocolFeature {
> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
> };
> 
> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
> +{
> +VHostSCSICommon

Re: [PATCH v6 5/5] vhost-user: fix lost reconnect

2023-09-28 Thread Raphael Norwitz




> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> When the vhost-user is reconnecting to the backend, and if the vhost-user 
> fails
> at the get_features in vhost_dev_init(), then the reconnect will fail
> and it will not be retriggered forever.
> 
> The reason is:
> When the vhost-user fails at get_features, the vhost_dev_cleanup will be 
> called
> immediately.
> 
> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
> 
> The reconnect path is:
> vhost_user_blk_event
>   vhost_user_async_close(.. vhost_user_blk_disconnect ..)
> qemu_chr_fe_set_handlers <- clear the notifier callback
>   schedule vhost_user_async_close_bh
> 
> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
> called, then the event fd callback will not be reinstalled.
> 
> All vhost-user devices have this issue, including vhost-user-blk/scsi.
> 
> With this patch, if the vdev->vdev is null, the fd callback will still
> be reinstalled.
> 
> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
> 

Reviewed-by: Raphael Norwitz 

> Signed-off-by: Li Feng 
> ---
> hw/block/vhost-user-blk.c  |  2 +-
> hw/scsi/vhost-user-scsi.c  |  3 ++-
> hw/virtio/vhost-user-gpio.c|  2 +-
> hw/virtio/vhost-user.c | 10 --
> include/hw/virtio/vhost-user.h |  3 ++-
> 5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 3c69fa47d5..95c758200d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -391,7 +391,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, >chardev, >dev,
> -   vhost_user_blk_disconnect);
> +   vhost_user_blk_disconnect, 
> vhost_user_blk_event);
> 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 53a62c3170..0effbb4787 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -238,7 +238,8 @@ 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, >conf.chardev, >dev,
> -   vhost_user_scsi_disconnect);
> +   vhost_user_scsi_disconnect,
> +   vhost_user_scsi_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d9979aa5db..04c2cc79f4 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent 
> event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, >chardev, >vhost_dev,
> -   vu_gpio_disconnect);
> +   vu_gpio_disconnect, vu_gpio_event);
> 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 8dcf049d42..7344f57ba7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2643,6 +2643,7 @@ typedef struct {
> DeviceState *dev;
> CharBackend *cd;
> struct vhost_dev *vhost;
> +IOEventHandler *event_cb;
> } VhostAsyncCallback;
> 
> static void vhost_user_async_close_bh(void *opaque)
> @@ -2657,7 +2658,10 @@ 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);
> }
> @@ -2669,7 +2673,8 @@ 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)
> +vu_async_close_fn cb,
> +IOEventHandler *event_cb)
> {
> if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> /*
> @@ -2685,6 +2690,7 @@ 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 191216a74f..649e9dd54f 100644
> --- a/include/hw/virtio/vhost-user.h
> +++

Re: [PATCH v7 01/12] nbd/server: Support a request payload

2023-09-28 Thread Vladimir Sementsov-Ogievskiy


On 28.09.23 17:33, Eric Blake wrote:

On Thu, Sep 28, 2023 at 12:09:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:

On 27.09.23 18:59, Eric Blake wrote:

We could also try to be a bit more complicated by peeking at the next
few bytes: if they look like a magic number of the next request,
assume the client set the bit accidentally but didn't send a payload
after all; for anything else, assume the client did pass a payload.
But adding in machinery to peek at a prefix is more complex than
either assuming a payload is always present (as done in this patch) or
assuming the bit was in error (and dropping the connection
unconditionally).  Preferences?



Ohh, you are right, thanks for comprehensive explanation. I really missed some things you 
are saying about. Yes, now I agree that "payload always exist when flag is set" 
is the best effort. Finally, that was our aim of the protocol design: make it more 
context independent. Probably, we may fix that in specification as preferable or at least 
possible server behavior about non-compliant client.


One other possibility I just thought of: have a heuristic where the
flag set with h->request_length less than 512 bytes is likely to
indicate an intentional payload (even if for a command where we
weren't expecting payload, so still a client error); while the flag
set wtih h->request_length >= 512 bytes is likely to be a mistaken
setting of the flag (but also still a client error).  NBD_CMD_WRITE is
probably the only command that will ever need to send a payload larger
than one sector, but that command already has handling to accept
payloads of all sizes because we know what to do with them and where
the client is not in error.



I'd prefer to avoid extra logic for optimizing handling of bad client, better 
keep code simpler.


--
Best regards,
Vladimir

Re: [PATCH v4 3/4] qcow2: add zoned emulation capability

2023-09-28 Thread Eric Blake

On Mon, Sep 18, 2023 at 05:53:12PM +0800, Sam Li wrote:
> By adding zone operations and zoned metadata, the zoned emulation
> capability enables full emulation support of zoned device using
> a qcow2 file. The zoned device metadata includes zone type,
> zoned device state and write pointer of each zone, which is stored
> to an array of unsigned integers.
> 
> Each zone of a zoned device makes state transitions following
> the zone state machine. The zone state machine mainly describes
> five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
> READ ONLY and OFFLINE states will generally be affected by device
> internal events. The operations on zones cause corresponding state
> changing.
> 
> Zoned devices have a limit on zone resources, which puts constraints on
> write operations into zones.
> 
> Signed-off-by: Sam Li 
> ---
>  block/qcow2.c  | 709 -
>  block/qcow2.h  |   2 +
>  block/trace-events |   2 +
>  docs/interop/qcow2.txt |   6 +
>  4 files changed, 717 insertions(+), 2 deletions(-)

You may want to look at scripts/git.orderfile; putting spec changes
(docs/*) first in your output before implementation is generally
beneficial to reviewers.

> +++ b/docs/interop/qcow2.txt
> @@ -367,6 +367,12 @@ The fields of the zoned extension are:
>  The maximal number of 512-byte sectors of a zone
>  append request that can be issued to the device.
>  
> +  36 - 43:  zonedmeta_offset
> +The offset of zoned metadata structure in the file in 
> bytes.

For the spec to be useful, you also need to add a section describing
the layout of the zoned metadata structure actually is.

> +
> +  44 - 51:  zonedmeta_size
> +The size of zoned metadata in bytes.
> +

Can the zoned metadata structure ever occupy more than 4G, or can this
field be sized at 4 bytes instead of 8?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v4 2/4] qcow2: add configurations for zoned format extension

2023-09-28 Thread Eric Blake

On Mon, Sep 18, 2023 at 05:53:11PM +0800, Sam Li wrote:
> To configure the zoned format feature on the qcow2 driver, it
> requires settings as: the device size, zone model, zone size,
> zone capacity, number of conventional zones, limits on zone
> resources (max append sectors, max open zones, and max_active_zones).
> 
> To create a qcow2 file with zoned format, use command like this:
> $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> zone_size=64M -o zone_capacity=64M -o nr_conv_zones=0 -o
> max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> -o zone_model=1
> 
> Signed-off-by: Sam Li 
> ---
>  block/qcow2.c| 186 ++-
>  block/qcow2.h|  28 +
>  docs/interop/qcow2.txt   |  36 ++
>  include/block/block_int-common.h |  13 +++
>  qapi/block-core.json |  30 -
>  5 files changed, 291 insertions(+), 2 deletions(-)

Below, I'll focus only on the spec change, not the implementation:

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b48cd9ce63..521276fc51 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -73,6 +73,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
>  #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>  #define  QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
> +#define  QCOW2_EXT_MAGIC_ZONED_FORMAT 0x7a6264

Why not spell it 0x007a6264 with 8 hex digits, like the others?  (I
get why you choose that constant, though - ascii 'zbd')

> +++ b/docs/interop/qcow2.txt
> @@ -331,6 +331,42 @@ The fields of the bitmaps extension are:
> Offset into the image file at which the bitmap directory
> starts. Must be aligned to a cluster boundary.
>  
> +== Zoned extension ==

Where is the magic number for this extension called out?  That's
missing, and MUST be part of the spec.

Back-compatibility constraints: you should consider what happens in
both of the following cases:

a program that intends to do read-only access to the qcow2 file but
which does not understand this extension header (for example, an older
version of 'qemu-img convert' being used to extract data from a newer
.qcow2 file with this header present - but also the new 'nbdkit
qcow2dec' decoder plugin just released in nbdkit 1.36).  Is it safe to
read the data as-is, by basically ignoring zone informations?  Or will
that ever produce wrong data (for example, if operations on a
particular zone imply that the guest should read all zeroes after the
current zone offset within that zone, regardless of whether non-zero
content was previously stored at those offsets - then not honoring the
existence of the extension header would require you to add and
document an incompatible feature bit so that reader apps fail to open
the file rather than reading wrong data).

a program that intends to edit the qcow2 file but which does not
understand this extension header (again, consider access by an older
version of qemu).  Is it safe to just write data anywhere in the disk,
but where failure to update the zone metadata means that all
subsequent use of the file MUST behave as if it is now a non-zeoned
device?  If so, then it is sufficient to document an autoclear feature
bit: any time a newer qcow2 writer creates a file with a zoned
extension, it also sets the autoclear feature bit; any time an older
qcow2 writer edits a file with the autoclear bit, it clears the bit
(because it has no idea if its edits invalidated the unknown
extension).  Then when the new qcow2 program again accesses the file,
it knows that the zone information is no longer reliable, and can fall
back to forcing the image to behave as flat.

> +
> +The zoned extension is an optional header extension. It contains fields for
> +emulating the zoned stroage model (https://zonedstorage.io/).

Assuming that you'll need to add one or two feature bits (most likely
an autoclear bit, to prevent editing the file without keeping the zone
information up-to-data, and possibly also an incompatible feature bit,
if interpreting the file even without editing it is impossible without
understanding zones), you'll want to mention this header's relation to
those feature bit(s).

> +
> +The fields of the zoned extension are:
> +Byte0:  zoned
> +Zoned model, 1 for host-managed and 0 for non-zoned 
> devices.

This tells me nothing about what those two models mean.  You'll need
to describe them better for an independent implementation of a qcow2
reader (such as 'nbdkit qcow2dec') to be able to properly read such a
file with either value, even if it doesn't plan on editing it.

If we do add feature bits, what happens when reading a file when the
feature bits are set but this extension header is missing?

> +
> +1 - 3:  Reserved, must be zero.
> +
> +4 - 7:  zone_size
> +Total number of logical blocks within the zones in bytes.

This is confusing.  It is a number of

Re: [PATCH v7 01/12] nbd/server: Support a request payload

2023-09-28 Thread Eric Blake

On Thu, Sep 28, 2023 at 12:09:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 27.09.23 18:59, Eric Blake wrote:
> > We could also try to be a bit more complicated by peeking at the next
> > few bytes: if they look like a magic number of the next request,
> > assume the client set the bit accidentally but didn't send a payload
> > after all; for anything else, assume the client did pass a payload.
> > But adding in machinery to peek at a prefix is more complex than
> > either assuming a payload is always present (as done in this patch) or
> > assuming the bit was in error (and dropping the connection
> > unconditionally).  Preferences?
> 
> 
> Ohh, you are right, thanks for comprehensive explanation. I really missed 
> some things you are saying about. Yes, now I agree that "payload always exist 
> when flag is set" is the best effort. Finally, that was our aim of the 
> protocol design: make it more context independent. Probably, we may fix that 
> in specification as preferable or at least possible server behavior about 
> non-compliant client.

One other possibility I just thought of: have a heuristic where the
flag set with h->request_length less than 512 bytes is likely to
indicate an intentional payload (even if for a command where we
weren't expecting payload, so still a client error); while the flag
set wtih h->request_length >= 512 bytes is likely to be a mistaken
setting of the flag (but also still a client error).  NBD_CMD_WRITE is
probably the only command that will ever need to send a payload larger
than one sector, but that command already has handling to accept
payloads of all sizes because we know what to do with them and where
the client is not in error.

> 
> r-b coming soon, I just need to take another look with corrected picture in 
> mind.
> 
> -- 
> Best regards,
> Vladimir
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v3 0/6] python/machine: use socketpair() for console socket

2023-09-28 Thread John Snow

On Thu, Sep 28, 2023, 4:12 AM Daniel P. Berrangé 
wrote:

> On Thu, Sep 28, 2023 at 12:49:37AM -0400, John Snow wrote:
> > Like we did for the QMP socket, use socketpair() for the console socket
> > so that hopefully there isn't a race condition during early boot where
> > data might get dropped on the floor.
> >
> > May or may not help with various race conditions where early console
> > output is not showing up in the logs and/or potentially being missed by
> > wait_for_console_pattern.
> >
> > V3:
> >   - Rebased.
>
> V3 has R-B on every single patch already. Should this
> just have been a PULL instead ?
>

I guess just erring on the side of caution with some fresh patches for
patchew since it had been a while since V2 and I did have to rebase this.

I'll send the PR soon if there's no objections.


> >
> > V2:
> >   - Fixed some Socket ownership/garbage collection problems
> >   - Fixed callers of now-dropped VM arguments/properties
> >   - added a dedicated sock_fd arg to ConsoleSocket()
> >   - now using socketpair() for qtest console, too.
> >   - dropped sock_dir arg from *all* machine.py classes
> >   - Tested quite a bit more thoroughly ...
> >
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/1019123030
> >
> > John Snow (6):
> >   python/machine: move socket setup out of _base_args property
> >   python/machine: close sock_pair in cleanup path
> >   python/console_socket: accept existing FD in initializer
> >   python/machine: use socketpair() for console connections
> >   python/machine: use socketpair() for qtest connection
> >   python/machine: remove unused sock_dir argument
> >
> >  python/qemu/machine/console_socket.py  | 29 ---
> >  python/qemu/machine/machine.py | 58 +-
> >  python/qemu/machine/qtest.py   | 54 +++-
> >  tests/avocado/acpi-bits.py |  5 +-
> >  tests/avocado/avocado_qemu/__init__.py |  2 +-
> >  tests/avocado/machine_aspeed.py|  5 +-
> >  tests/qemu-iotests/iotests.py  |  2 +-
> >  tests/qemu-iotests/tests/copy-before-write |  3 +-
> >  8 files changed, 104 insertions(+), 54 deletions(-)
> >
> > --
> > 2.41.0
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>

Re: [PATCH 1/1] hw/ide/core: terminate in-flight DMA on IDE bus reset

2023-09-28 Thread Fiona Ebner

Am 26.09.23 um 16:45 schrieb John Snow:
> 
> 
> On Tue, Sep 26, 2023, 3:11 AM Fiona Ebner  > wrote:
> 
> Am 25.09.23 um 21:53 schrieb John Snow:
> > On Thu, Sep 21, 2023 at 12:07 PM Simon Rowe
> mailto:simon.r...@nutanix.com>> wrote:
> >>
> >> When an IDE controller is reset, its internal state is being cleared
> >> before any outstanding I/O is cancelled. If a response to DMA is
> >> received in this window, the aio callback will incorrectly continue
> >> with the next part of the transfer (now using sector 0 from
> >> the cleared controller state).
> >
> > Eugh, yikes. It feels like we should fix the cancellation ...
> Please note that there already is a patch for that on the list:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01011.html 
> 
> 
> Best Regards,
> Fiona
> 
> 
> Gotcha, thanks for the pointer. I wonder if that's sufficient to fix the
> CVE here? I don't have the reproducer in my hands (that I know of ...
> it's genuinely possible I missed it, apologies)
> 

AFAICT, yes, because the DMA callback is invoked before resetting the
state now. But not 100% sure if it can't be triggered in some other way,
maybe Simon knows more? I don't have a reproducer for the CVE either,
but the second patch after the one linked above adds a qtest for the
reset scenario.

Best Regards,
Fiona

Re: [PATCH v7 01/12] nbd/server: Support a request payload

2023-09-28 Thread Vladimir Sementsov-Ogievskiy


On 27.09.23 18:59, Eric Blake wrote:

We could also try to be a bit more complicated by peeking at the next
few bytes: if they look like a magic number of the next request,
assume the client set the bit accidentally but didn't send a payload
after all; for anything else, assume the client did pass a payload.
But adding in machinery to peek at a prefix is more complex than
either assuming a payload is always present (as done in this patch) or
assuming the bit was in error (and dropping the connection
unconditionally).  Preferences?



Ohh, you are right, thanks for comprehensive explanation. I really missed some things you 
are saying about. Yes, now I agree that "payload always exist when flag is set" 
is the best effort. Finally, that was our aim of the protocol design: make it more 
context independent. Probably, we may fix that in specification as preferable or at least 
possible server behavior about non-compliant client.

r-b coming soon, I just need to take another look with corrected picture in 
mind.

--
Best regards,
Vladimir

Re: [PATCH v3 0/6] python/machine: use socketpair() for console socket

2023-09-28 Thread Daniel P . Berrangé

On Thu, Sep 28, 2023 at 12:49:37AM -0400, John Snow wrote:
> Like we did for the QMP socket, use socketpair() for the console socket
> so that hopefully there isn't a race condition during early boot where
> data might get dropped on the floor.
> 
> May or may not help with various race conditions where early console
> output is not showing up in the logs and/or potentially being missed by
> wait_for_console_pattern.
> 
> V3:
>   - Rebased.

V3 has R-B on every single patch already. Should this
just have been a PULL instead ?

> 
> V2:
>   - Fixed some Socket ownership/garbage collection problems
>   - Fixed callers of now-dropped VM arguments/properties
>   - added a dedicated sock_fd arg to ConsoleSocket()
>   - now using socketpair() for qtest console, too.
>   - dropped sock_dir arg from *all* machine.py classes
>   - Tested quite a bit more thoroughly ...
> 
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/1019123030
> 
> John Snow (6):
>   python/machine: move socket setup out of _base_args property
>   python/machine: close sock_pair in cleanup path
>   python/console_socket: accept existing FD in initializer
>   python/machine: use socketpair() for console connections
>   python/machine: use socketpair() for qtest connection
>   python/machine: remove unused sock_dir argument
> 
>  python/qemu/machine/console_socket.py  | 29 ---
>  python/qemu/machine/machine.py | 58 +-
>  python/qemu/machine/qtest.py   | 54 +++-
>  tests/avocado/acpi-bits.py |  5 +-
>  tests/avocado/avocado_qemu/__init__.py |  2 +-
>  tests/avocado/machine_aspeed.py|  5 +-
>  tests/qemu-iotests/iotests.py  |  2 +-
>  tests/qemu-iotests/tests/copy-before-write |  3 +-
>  8 files changed, 104 insertions(+), 54 deletions(-)
> 
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: deadlock when using iothread during backup_clean()

2023-09-28 Thread Fiona Ebner

Am 05.09.23 um 13:42 schrieb Paolo Bonzini:
> On 9/5/23 12:01, Fiona Ebner wrote:
>> Can we assume block_job_remove_all_bdrv() to always hold the job's
>> AioContext?
> 
> I think so, see job_unref_locked(), job_prepare_locked() and
> job_finalize_single_locked().  These call the callbacks that ultimately
> get to block_job_remove_all_bdrv().
> 
>> And if yes, can we just tell bdrv_graph_wrlock() that it
>> needs to release that before polling to fix the deadlock?
> 
> No, but I think it should be released and re-acquired in
> block_job_remove_all_bdrv() itself.
> 

For fixing the backup cancel deadlock, I tried the following:

> diff --git a/blockjob.c b/blockjob.c
> index 58c5d64539..fd6132ebfe 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -198,7 +198,9 @@ void block_job_remove_all_bdrv(BlockJob *job)
>   * one to make sure that such a concurrent access does not attempt
>   * to process an already freed BdrvChild.
>   */
> +aio_context_release(job->job.aio_context);
>  bdrv_graph_wrlock(NULL);
> +aio_context_acquire(job->job.aio_context);
>  while (job->nodes) {
>  GSList *l = job->nodes;
>  BdrvChild *c = l->data;

but it's not enough unfortunately. And I don't just mean with the later
deadlock during bdrv_close() (via bdrv_cbw_drop()) as mentioned in the
other mail.

Even when I got lucky and that deadlock didn't trigger by chance or with
an additional change to try and avoid that one

> diff --git a/block.c b/block.c
> index e7f349b25c..02d2c4e777 100644
> --- a/block.c
> +++ b/block.c
> @@ -5165,7 +5165,7 @@ static void bdrv_close(BlockDriverState *bs)
>  bs->drv = NULL;
>  }
>  
> -bdrv_graph_wrlock(NULL);
> +bdrv_graph_wrlock(bs);
>  QLIST_FOREACH_SAFE(child, >children, next, next) {
>  bdrv_unref_child(bs, child);
>  }

often guest IO would get completely stuck after canceling the backup.
There's nothing obvious to me in the backtraces at that point, but it
seems the vCPU and main threads running like usual, while the IO thread
is stuck in aio_poll(), i.e. never returns from the __ppoll() call. This
would happen with both, a VirtIO SCSI and a VirtIO block disk and with
both aio=io_uring and aio=threads.

I should also mention I'm using

> fio --name=file --size=4k --direct=1 --rw=randwrite --bs=4k --ioengine=psync 
> --numjobs=5 --runtime=6000 --time_based

inside the guest during canceling of the backup.

I'd be glad for any pointers what to look for and happy to provide more
information.

Best Regards,
Fiona

Re: [PATCH v3 0/7] Steps towards enabling -Wshadow=local

Re: [PATCH] test-throttle: don't shadow 'index' variable in do_test_accounting()

Re: [PATCH 0/3] (few more) Steps towards enabling -Wshadow [3 more]

Re: [PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

Re: [PATCH v2 11/22] hw/ide/ahci: Clean up local variable shadowing

Re: [PATCH v2 20/22] sysemu/device_tree: Clean up local variable shadowing

Re: [PATCH v6 4/5] vhost-user-scsi: start vhost when guest kicks

Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

Re: [PATCH v6 5/5] vhost-user: fix lost reconnect

Re: [PATCH v6 3/5] vhost-user-scsi: support reconnect to backend

Re: [PATCH v6 5/5] vhost-user: fix lost reconnect

Re: [PATCH v7 01/12] nbd/server: Support a request payload

Re: [PATCH v4 3/4] qcow2: add zoned emulation capability

Re: [PATCH v4 2/4] qcow2: add configurations for zoned format extension

Re: [PATCH v7 01/12] nbd/server: Support a request payload

Re: [PATCH v3 0/6] python/machine: use socketpair() for console socket

Re: [PATCH 1/1] hw/ide/core: terminate in-flight DMA on IDE bus reset

Re: [PATCH v7 01/12] nbd/server: Support a request payload

Re: [PATCH v3 0/6] python/machine: use socketpair() for console socket

Re: deadlock when using iothread during backup_clean()

20 matches

Site Navigation

Mail list logo

Footer information