Re: [PATCH v5 5/7] migration: Deprecate old compression method
Juan Quintela writes: > Markus Armbruster wrote: >> Juan Quintela writes: >> >>> Signed-off-by: Juan Quintela >>> Acked-by: Stefan Hajnoczi >>> Acked-by: Peter Xu > > >>> # @deprecated: Member @disk is deprecated because block migration is. >>> +# Member @compression is deprecated because it is unreliable and >>> +# untested. It is recommended to use multifd migration, which >>> +# offers an alternative compression implementation that is >>> +# reliable and tested. >> >> Two spaces between sentences for consistency, please. > > I have reviewed all the patches again. Let's hope that I didn't miss > one. > >>> # @announce-step: Increase in delay (in milliseconds) between >>> # subsequent packets in the announcement (Since 4.0) >>> # >>> -# @compress-level: compression level >>> +# @compress-level: compression level. >>> # >>> -# @compress-threads: compression thread count >>> +# @compress-threads: compression thread count. >>> # >>> # @compress-wait-thread: Controls behavior when all compression >>> # threads are currently busy. If true (default), wait for a free >>> # compression thread to become available; otherwise, send the page >>> -# uncompressed. (Since 3.1) >>> +# uncompressed. (Since 3.1) >>> # >>> -# @decompress-threads: decompression thread count >>> +# @decompress-threads: decompression thread count. >>> # >>> # @throttle-trigger-threshold: The ratio of bytes_dirty_period and >>> # bytes_xfer_period to trigger throttling. It is expressed as >> >> Unrelated. > > Rebases are bad for you O:-) > >>> @@ -1023,7 +1036,9 @@ >>> # Features: >>> # >>> # @deprecated: Member @block-incremental is deprecated. Use >>> -# blockdev-mirror with NBD instead. >>> +# blockdev-mirror with NBD instead. Members @compress-level, >>> +# @compress-threads, @decompress-threads and @compress-wait-thread >>> +# are deprecated because @compression is deprecated. >> >> Two spaces between sentences for consistency, please. > > Done. >>> @@ -1078,7 +1097,7 @@ >>> # Example: >>> # >>> # -> { "execute": "migrate-set-parameters" , >>> -# "arguments": { "compress-level": 1 } } >>> +# "arguments": { "multifd-channels": 5 } } >>> # <- { "return": {} } >>> ## >> >> Thanks for taking care of updating the example! > > You are welcome. grep for all occurences of compress-level and friends > has its advantages. > >>> # @compress-wait-thread: Controls behavior when all compression >>> # threads are currently busy. If true (default), wait for a free >>> # compression thread to become available; otherwise, send the page >>> -# uncompressed. (Since 3.1) >>> +# uncompressed. (Since 3.1) >>> # >>> -# @decompress-threads: decompression thread count >>> +# @decompress-threads: decompression thread count. >>> # >>> # @throttle-trigger-threshold: The ratio of bytes_dirty_period and >>> # bytes_xfer_period to trigger throttling. It is expressed as >> >> Unrelated. > > I have removed the periods. > > But I have a question, why the descriptions that are less than one line > don't have period and the other have it. When the description consists of multiple sentences, we obviously need to end them with punctuation. Sometimes the description isn't a sentence, just a phrase, e.g. "decompression thread count". No punctuation then. Sometimes it's a single sentence. Then we roll dice. >>> if (params->has_compress_level) { >>> +warn_report("Old compression is deprecated. " >>> +"Use multifd compression methods instead."); >>> s->parameters.compress_level = params->compress_level; >>> } >>> >>> if (params->has_compress_threads) { >>> +warn_report("Old compression is deprecated. " >>> +"Use multifd compression methods instead."); >>> s->parameters.compress_threads = params->compress_threads; >>> } >>> >>> if (params->has_compress_wait_thread) { >>> +warn_report("Old compression is deprecated. " >>> +"Use multifd compression methods instead."); >>> s->parameters.compress_wait_thread = params->compress_wait_thread; >>> } >>> >>> if (params->has_decompress_threads) { >>> +warn_report("Old compression is deprecated. " > > Once here, I did s/Old/old/ > > as all your examples of description start with lowercase. Yes, please. >>> +"Use multifd compression methods instead."); >>> s->parameters.decompress_threads = params->decompress_threads; >>> } >> >> Other than that >> Reviewed-by: Markus Armbruster > > Thanks for your patience.
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
On Mon, 16 Oct 2023 16:19:08 +0100 David Woodhouse wrote: > From: David Woodhouse > is this index a user (guest) visible? > There's no need to force the user to assign a vdev. We can automatically > assign one, starting at xvda and searching until we find the first disk > name that's unused. > > This means we can now allow '-drive if=xen,file=xxx' to work without an > explicit separate -driver argument, just like if=virtio. > > Signed-off-by: David Woodhouse > --- > blockdev.c | 15 --- > hw/block/xen-block.c | 25 + > 2 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 325b7a3bef..9dec4c9c74 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -255,13 +255,13 @@ void drive_check_orphaned(void) > * Ignore default drives, because we create certain default > * drives unconditionally, then leave them unclaimed. Not the > * users fault. > - * Ignore IF_VIRTIO, because it gets desugared into -device, > - * so we can leave failing to -device. > + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into > + * -device, so we can leave failing to -device. > * Ignore IF_NONE, because leaving unclaimed IF_NONE remains > * available for device_add is a feature. > */ > if (dinfo->is_default || dinfo->type == IF_VIRTIO > -|| dinfo->type == IF_NONE) { > +|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) { > continue; > } > if (!blk_get_attached_dev(blk)) { > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, > BlockInterfaceType block_default_type, > qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort); > qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"), > &error_abort); > +} else if (type == IF_XEN) { > +QemuOpts *devopts; > +devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, > + &error_abort); > +qemu_opt_set(devopts, "driver", > + (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk", > + &error_abort); > +qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"), > + &error_abort); > } > > filename = qemu_opt_get(legacy_opts, "file"); > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index 9262338535..20fa783cbe 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error > **errp) > XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev); > XenBlockVdev *vdev = &blockdev->props.vdev; > > +if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) { > +char name[11]; > +int disk = 0; > +unsigned long idx; > + > +/* Find an unoccupied device name */ > +while (disk < (1 << 20)) { > +if (disk < (1 << 4)) { > +idx = (202 << 8) | (disk << 4); > +} else { > +idx = (1 << 28) | (disk << 8); > +} > +snprintf(name, sizeof(name), "%lu", idx); > +if (!xen_backend_exists("qdisk", name)) { > +vdev->type = XEN_BLOCK_VDEV_TYPE_XVD; > +vdev->partition = 0; > +vdev->disk = disk; > +vdev->number = idx; > +return g_strdup(name); > +} > +disk++; > +} > +error_setg(errp, "cannot find device vdev for block device"); > +return NULL; > +} > return g_strdup_printf("%lu", vdev->number); > } >
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote: > On Mon, 16 Oct 2023 16:19:08 +0100 > David Woodhouse wrote: > > > From: David Woodhouse > > > > is this index a user (guest) visible? Yes. It defines what block device (e.g. /dev/xvda) the disk appears as in the guest. In the common case, it literally encodes the Linux major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc. Previously we had to explicitly set it for each disk in Qemu: -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb Now we can just do -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen (We could go further and make if=xen the default for Xen guests too, but that doesn't work right now because xen-block will barf on the default provided CD-ROM when it's empty. It doesn't handle empty devices. And if I work around that, then `-hda disk1.img` would work on the command line... but would make it appear as /dev/xvda instead of /dev/hda, and I don't know how I feel about that.) [root@localhost ~]# xenstore-ls -f device/vbd device/vbd = "" device/vbd/51712 = "" device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712" device/vbd/51712/backend-id = "0" device/vbd/51712/device-type = "disk" device/vbd/51712/event-channel = "8" device/vbd/51712/feature-persistent = "1" device/vbd/51712/protocol = "x86_64-abi" device/vbd/51712/ring-ref = "8" device/vbd/51712/state = "4" device/vbd/51712/virtual-device = "51712" > > > There's no need to force the user to assign a vdev. We can automatically > > assign one, starting at xvda and searching until we find the first disk > > name that's unused. > > > > This means we can now allow '-drive if=xen,file=xxx' to work without an > > explicit separate -driver argument, just like if=virtio. > > > > Signed-off-by: David Woodhouse > > --- > > blockdev.c | 15 --- > > hw/block/xen-block.c | 25 + > > 2 files changed, 37 insertions(+), 3 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 325b7a3bef..9dec4c9c74 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -255,13 +255,13 @@ void drive_check_orphaned(void) > > * Ignore default drives, because we create certain default > > * drives unconditionally, then leave them unclaimed. Not the > > * users fault. > > - * Ignore IF_VIRTIO, because it gets desugared into -device, > > - * so we can leave failing to -device. > > + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into > > + * -device, so we can leave failing to -device. > > * Ignore IF_NONE, because leaving unclaimed IF_NONE remains > > * available for device_add is a feature. > > */ > > if (dinfo->is_default || dinfo->type == IF_VIRTIO > > - || dinfo->type == IF_NONE) { > > + || dinfo->type == IF_XEN || dinfo->type == IF_NONE) { > > continue; > > } > > if (!blk_get_attached_dev(blk)) { > > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, > > BlockInterfaceType block_default_type, > > qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort); > > qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"), > > &error_abort); > > + } else if (type == IF_XEN) { > > + QemuOpts *devopts; > > + devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, > > + &error_abort); > > + qemu_opt_set(devopts, "driver", > > + (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk", > > + &error_abort); > > + qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"), > > + &error_abort); > > } > > > > filename = qemu_opt_get(legacy_opts, "file"); > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > > index 9262338535..20fa783cbe 100644 > > --- a/hw/block/xen-block.c > > +++ b/hw/block/xen-block.c > > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error > > **errp) > > XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev); > > XenBlockVdev *vdev = &blockdev->props.vdev; > > > > + if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) { > > + char name[11]; > > + int disk = 0; > > + unsigned long idx; > > + > > + /* Find an unoccupied device name */ > > + while (disk < (1 << 20)) { > > + if (disk < (1 << 4)) { > > + idx = (202 << 8) | (disk << 4); > > + } else { > > + idx = (1 << 28) | (disk << 8); > > + } > > + snprintf(name, sizeof(name), "%lu", idx); > > + if (!xen_backend_exists("qdisk", name)) { > > + vdev->type = XEN_BLOCK_VDEV_TYPE_XVD; > > + vdev->partition = 0; > >
Re: [PATCH v8 3/5] vhost-user-scsi: support reconnect to backend
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 --- Reviewed-by: Manos Pitsidianakis
Re: [PATCH v8 0/5] Implement reconnect for vhost-user-scsi
Hello Guys, Ping… These patches have been waiting for a long time. Can they be merged? Best Regards, li > On 9 Oct 2023, at 12:46 PM, Li Feng wrote: > > Changes for v8: > - [PATCH 3/5] vhost-user-scsi: support reconnect to backend > - Fix code style suggested by Manos Pitsidianakis > - [PATCH 4/5] vhost-user-scsi: start vhost when guest kicks > - Use 'DEVICE()' macro in vhost_user_scsi_handle_output to replace the >'parent_obj.parent_obj.parent_obj.parent_obj'. > > Changes for v7: > - [PATCH 3/5] vhost-user-scsi: support reconnect to backend > - Add reporting the error in vhost-scsi; > - Rebase to master and fix the conflict. > - Add "Reviewed-by" tags. > > Changes for v6: > - [PATCH] vhost-user: fix lost reconnect > - Fix missing assign event_cb. > > Changes for v5: > - No logic has been changed, just move part of the code from patch 4 to patch > 5. > > Changes for v4: > - Merge > https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to > this series. > - Add ERRP_GUARD in vhost_user_scsi_realize; > - Reword the commit messages. > > Changes for v3: > - Split the vhost_user_scsi_handle_output to a separate patch; > - Move the started_vu from vhost scsi common header to vhost-user-scsi header; > - Fix a log print error; > > Changes for v2: > - Split the v1 patch to small separate patchset; > - New patch for fixing fd leak, which has sent to reviewers in another > mail; > - Implement the `vhost_user_scsi_handle_output`; > - Add the started_vu safe check; > - Fix error handler; > - Check the inflight before set/get inflight fd. > > Li Feng (5): > vhost-user-common: send get_inflight_fd once > vhost: move and rename the conn retry times > vhost-user-scsi: support reconnect to backend > vhost-user-scsi: start vhost when guest kicks > vhost-user: fix lost reconnect > > hw/block/vhost-user-blk.c | 6 +- > hw/scsi/vhost-scsi-common.c | 47 ++--- > hw/scsi/vhost-scsi.c | 6 +- > hw/scsi/vhost-user-scsi.c | 250 +++--- > hw/virtio/vhost-user-gpio.c | 5 +- > hw/virtio/vhost-user.c| 10 +- > include/hw/virtio/vhost-scsi-common.h | 2 +- > include/hw/virtio/vhost-user-scsi.h | 6 + > include/hw/virtio/vhost-user.h| 3 +- > include/hw/virtio/vhost.h | 2 + > 10 files changed, 277 insertions(+), 60 deletions(-) > > -- > 2.41.0 >
Re: [PATCH v6 2/5] migration: migrate 'inc' command option is deprecated.
Juan Quintela writes: > Use blockdev-mirror with NBD instead. > > Reviewed-by: Thomas Huth > Acked-by: Stefan Hajnoczi > Reviewed-by: Markus Armbruster > Signed-off-by: Juan Quintela > --- > docs/about/deprecated.rst | 9 + > qapi/migration.json| 8 +++- > migration/migration-hmp-cmds.c | 5 + > migration/migration.c | 5 + > 4 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 2febd2d12f..b51136f50a 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -461,3 +461,12 @@ Migration > ``skipped`` field in Migration stats has been deprecated. It hasn't > been used for more than 10 years. > > +``inc`` migrate command option (since 8.2) > +'' > + > +Use blockdev-mirror with NBD instead. > + > +As an intermediate step the ``inc`` functionality can be achieved by > +setting the ``block-incremental`` migration parameter to ``true``. > +But this parameter is also deprecated. > + If you need to respin for some other reason, drop the blank line at end of file. Same in later patches. [...]
Re: [PATCH v8 0/5] Implement reconnect for vhost-user-scsi
Queued. Thanks! On Wed, Oct 18, 2023 at 04:26:10PM +0800, Li Feng wrote: > Hello Guys, > > Ping… > > These patches have been waiting for a long time. Can they be merged? > > > Best Regards, > > li > > > On 9 Oct 2023, at 12:46 PM, Li Feng wrote: > > Changes for v8: > - [PATCH 3/5] vhost-user-scsi: support reconnect to backend > - Fix code style suggested by Manos Pitsidianakis > - [PATCH 4/5] vhost-user-scsi: start vhost when guest kicks > - Use 'DEVICE()' macro in vhost_user_scsi_handle_output to replace the >'parent_obj.parent_obj.parent_obj.parent_obj'. > > Changes for v7: > - [PATCH 3/5] vhost-user-scsi: support reconnect to backend > - Add reporting the error in vhost-scsi; > - Rebase to master and fix the conflict. > - Add "Reviewed-by" tags. > > Changes for v6: > - [PATCH] vhost-user: fix lost reconnect > - Fix missing assign event_cb. > > Changes for v5: > - No logic has been changed, just move part of the code from patch 4 to > patch 5. > > Changes for v4: > - Merge > https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to > this series. > - Add ERRP_GUARD in vhost_user_scsi_realize; > - Reword the commit messages. > > Changes for v3: > - Split the vhost_user_scsi_handle_output to a separate patch; > - Move the started_vu from vhost scsi common header to vhost-user-scsi > header; > - Fix a log print error; > > Changes for v2: > - Split the v1 patch to small separate patchset; > - New patch for fixing fd leak, which has sent to reviewers in another > mail; > - Implement the `vhost_user_scsi_handle_output`; > - Add the started_vu safe check; > - Fix error handler; > - Check the inflight before set/get inflight fd. > > Li Feng (5): > vhost-user-common: send get_inflight_fd once > vhost: move and rename the conn retry times > vhost-user-scsi: support reconnect to backend > vhost-user-scsi: start vhost when guest kicks > vhost-user: fix lost reconnect > > hw/block/vhost-user-blk.c | 6 +- > hw/scsi/vhost-scsi-common.c | 47 ++--- > hw/scsi/vhost-scsi.c | 6 +- > hw/scsi/vhost-user-scsi.c | 250 +++--- > hw/virtio/vhost-user-gpio.c | 5 +- > hw/virtio/vhost-user.c| 10 +- > include/hw/virtio/vhost-scsi-common.h | 2 +- > include/hw/virtio/vhost-user-scsi.h | 6 + > include/hw/virtio/vhost-user.h| 3 +- > include/hw/virtio/vhost.h | 2 + > 10 files changed, 277 insertions(+), 60 deletions(-) > > -- > 2.41.0 > > >
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
On 18.10.23 09:47, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: On 17.10.23 18:00, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: Send a new event when guest reads virtio-pci config after virtio_notify_config() call. That's useful to check that guest fetched modified config, for example after resizing disk backend. Signed-off-by: Vladimir Sementsov-Ogievskiy [...] diff --git a/qapi/qdev.json b/qapi/qdev.json index 2468f8bddf..37a8785b81 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -329,3 +329,25 @@ # Since: 8.2 ## { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } + +## +# @X_CONFIG_READ: +# +# Emitted whenever guest reads virtio device config after config change. +# +# @device: device name +# +# @path: device path +# +# Since: 5.0.1-24 +# +# Example: +# +# <- { "event": "X_CONFIG_READ", +# "data": { "device": "virtio-net-pci-0", +#"path": "/machine/peripheral/virtio-net-pci-0" }, +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +# +## +{ 'event': 'X_CONFIG_READ', + 'data': { '*device': 'str', 'path': 'str' } } The commit message talks about event CONFIG_READ, but you actually name it x-device-sync-config. will fix I figure you use x- to signify "unstable". Please use feature flag 'unstable' for that. See docs/devel/qapi-code-gen.rst section "Features", in particular "Special features", and also the note on x- in section "Naming rules and reserved names". OK, will do. Hmm, it say Names beginning with ``x-`` used to signify "experimental". This convention has been replaced by special feature "unstable". "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems "unstable" always used together with "x-". True. The "x-" prefix originated with qdev properties. First use might be commit f0c07c7c7b4. The convention wasn't documented then, but QOM/qdev properties have always been a documentation wasteland. It then spread to other places, and eventually to the QAPI schema. Where we try pretty hard to document things properly. We documented the "x-" prefix in commit e790e666518: Any name (command, event, type, field, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed incompatibly in a future release. Minor pain point: when things grow up from experimental to stable, we have to rename. The convention didn't stop us from naming non-experimental things x-FOO, e.g. QOM property "x-origin" in commit 6105683da35. Made it to the QAPI schema in commit 8825587b53c. Point is: the prefix isn't a reliable marker for "unstable". Since I needed a reliable marker for my "set policy for unstable interfaces" feature (see CLI option -compat), I created special feature flag "unstable", and dropped the "x-" convention for the QAPI schema. Renaming existing "x-" names felt like pointless churn, so I didn't. I'm not objecting to new names starting with "x-". Nor am I objecting to feature 'unstable' on names that don't start with "x-". I guess "x-" remains just fine for things we don't intend to make stable at some point. The "x-" can remind humans "this is unstable" better than a feature flag can (for machines, it's the other way round). For things we do intend (hope?) to make stable, I wouldn't bother with the "x-". Clearer now? Yes, thanks. x- seems safer for management tool that doesn't know about "unstable" properties.. But on the other hand, changing from x- to no-prefix is already done when the feature is stable, and thouse who use it already use the latest version of interface, so, removing the prefix is just extra work. So, I think, I'd go without prefix. Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead? Since this is the first unstable event, there is no precedent. Let's use no prefix, and move on. The name CONFIG_READ feels overly generic for something that makes sense only with virtio devices. Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. That one came to be as a generalization of existing MEM_UNPLUG_ERROR and a concrete need to signal CPU unplug errors. Demonstrates "unplug guest errors" can happen for different kinds of devices. So we went with a generic event we can use for all of them. This doesn't seem to be the case for this patch's event. Thoughts? Right.. VIRTIO_CONFIG_READ maybe? So, what about DEVICE_GUEST_READ_CONFIG ? diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index b485375049..d0f022e925 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) dev->device_on_event_sent = true; qapi_event_send_x_device_on(dev->id, dev->canonical_path); } + +void qdev_config_read_event(DeviceState *dev) +{ +qapi_event_send_x_config_read(dev->id, dev->ca
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben: > From: David Woodhouse > > There's no need to force the user to assign a vdev. We can automatically > assign one, starting at xvda and searching until we find the first disk > name that's unused. > > This means we can now allow '-drive if=xen,file=xxx' to work without an > explicit separate -driver argument, just like if=virtio. > > Signed-off-by: David Woodhouse Actually, how does this play together with xen_config_dev_blk()? This looks like it tried to implement a very similar thing (which is IF_XEN even already existed). Are we now trying to attach each if=xen disk twice in the 'xenpv' machine? Or if something prevents this, is it dead code? I think in both cases, we would want to delete that function and remove the loop that calls it in xen_init_pv()? Kevin
Re: [PATCH v3 6/9] qapi/block-core: use JobType for BlockJobInfo's type
Fiona Ebner writes: > In preparation to turn BlockJobInfo into a union with @type as the > discriminator. That requires it to be an enum. > > No functional change is intended. > > Signed-off-by: Fiona Ebner > Reviewed-by: Vladimir Sementsov-Ogievskiy Replacing str by enum makes sense whether we need enum for a union or not. Reviewed-by: Markus Armbruster
Re: [PATCH v3 5/9] mirror: implement mirror_change method
Fiona Ebner writes: > which allows switching the @copy-mode from 'background' to > 'write-blocking'. > > This is useful for management applications, so they can start out in > background mode to avoid limiting guest write speed and switch to > active mode when certain criteria are fulfilled. > > In presence of an iothread, the copy_mode member is now shared between > the iothread and the main thread, so turn accesses to it atomic. > > Signed-off-by: Fiona Ebner [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index c6f31a9399..01427c259a 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3044,6 +3044,17 @@ > { 'command': 'block-job-finalize', 'data': { 'id': 'str' }, >'allow-preconfig': true } > > +## > +# @BlockJobChangeOptionsMirror: > +# > +# @copy-mode: Switch to this copy mode. Currenlty, only the switch Typo: Currently Also, two spaces between sentences for consistency, please. > +# from 'background' to 'write-blocking' is implemented. > +# > +# Since: 8.2 > +## > +{ 'struct': 'BlockJobChangeOptionsMirror', > + 'data': { 'copy-mode' : 'MirrorCopyMode' } } > + > ## > # @BlockJobChangeOptions: > # > @@ -3058,7 +3069,7 @@ > { 'union': 'BlockJobChangeOptions', >'base': { 'id': 'str', 'type': 'JobType' }, >'discriminator': 'type', > - 'data': {} } > + 'data': { 'mirror': 'BlockJobChangeOptionsMirror' } } > > ## > # @block-job-change:
Re: [PATCH v3 0/9] mirror: allow switching from background to active mode
Fiona Ebner writes: > Changes in v3: > * unlock the job mutex when calling the new block job driver > 'query' handler > * squash patch adapting iotest output into patch that changes the > output > * turn accesses to copy_mode and actively_synced atomic > * slightly rework error handling in mirror_change > > Changes in v2: > * move bitmap to filter which allows to avoid draining when > changing the copy mode > * add patch to determine copy_to_target only once > * drop patches returning redundant information upon query > * update QEMU version in QAPI > * update indentation in QAPI > * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat > doc comments to conform to current conventions")) > * add patch to adapt iotest output > > Discussion of v2: > https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02290.html > > Discussion of v1: > https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html > > With active mode, the guest write speed is limited by the synchronous > writes to the mirror target. For this reason, management applications > might want to start out in background mode and only switch to active > mode later, when certain conditions are met. This series adds a > block-job-change QMP command to achieve that, as well as > job-type-specific information when querying block jobs, which > can be used to decide when the switch should happen. > > For now, only the direction background -> active is supported. > > The information added upon querying is whether the target is actively > synced, the total data sent, and the remaining dirty bytes. > > Initially, I tried to go for a more general 'job-change' command, but > to avoid mutual inclusion of block-core.json and job.json, more > preparation would be required. Can you elaborate a bit? A more generic command is preferable, and we need to understand what it would take.
Re: deadlock when using iothread during backup_clean()
Am 17.10.23 um 16:20 schrieb Kevin Wolf: > Am 17.10.2023 um 15:37 hat Fiona Ebner geschrieben: >> Am 17.10.23 um 14:12 schrieb Kevin Wolf: >>> Am 17.10.2023 um 12:18 hat Fiona Ebner geschrieben: I ran into similar issues now with mirror, (both deadlocks and stuck guest IO at other times), and interestingly, also during job start. Also had a backtrace similar to [0] once, so I took a closer look. Probably was obvious to others already, but for the record: 1. the graph is locked by the main thread 2. the iothread holds the AioContext lock 3. the main thread waits on the AioContext lock 4. the iothread waits for coroutine spawned by blk_is_available() >>> >>> Where does this blk_is_available() in the iothread come from? Having it >>> wait without dropping the AioContext lock sounds like something that >>> we'd want to avoid. Ideally, devices using iothreads shouldn't use >>> synchronous requests at all, but I think scsi-disk might have some of >>> them. >>> >> >> It's part of the request handling in virtio-scsi: >> >>> #0 0x7ff7f5f55136 in __ppoll (fds=0x7ff7e40030c0, nfds=8, >>> timeout=, sigmask=0x0) at >>> ../sysdeps/unix/sysv/linux/ppoll.c:42 >>> #1 0x5587132615ab in qemu_poll_ns (fds=0x7ff7e40030c0, nfds=8, >>> timeout=-1) at ../util/qemu-timer.c:339 >>> #2 0x55871323e8b1 in fdmon_poll_wait (ctx=0x55871598d5e0, >>> ready_list=0x7ff7f288ebe0, timeout=-1) at ../util/fdmon-poll.c:79 >>> #3 0x55871323e1ed in aio_poll (ctx=0x55871598d5e0, blocking=true) at >>> ../util/aio-posix.c:670 >>> #4 0x558713089efa in bdrv_poll_co (s=0x7ff7f288ec90) at >>> /home/febner/repos/qemu/block/block-gen.h:43 >>> #5 0x55871308c362 in blk_is_available (blk=0x55871599e2f0) at >>> block/block-gen.c:1426 >>> #6 0x558712f6843b in virtio_scsi_ctx_check (s=0x558716c049c0, >>> d=0x55871581cd30) at ../hw/scsi/virtio-scsi.c:290 > > Oh... So essentially for an assertion. > > I wonder if the blk_is_available() check introduced in 2a2d69f490c is > even necessary any more, because BlockBackend has its own AioContext > now. And if blk_bs(blk) != NULL isn't what we actually want to check if > the check is necessary, because calling bdrv_is_inserted() doesn't seem > to have been intended. blk_bs() wouldn't have to poll. > Could virtio_scsi_hotunplug() be an issue with removing or modifying the check? There's a call there which sets the blk's AioContext to qemu_get_aio_context(). Or are we sure that the assert in virtio_scsi_ctx_check() can't be reached after that? >>> #7 0x558712f697e4 in virtio_scsi_handle_cmd_req_prepare >>> (s=0x558716c049c0, req=0x7ff7e400b650) at ../hw/scsi/virtio-scsi.c:788 >>> #8 0x558712f699b0 in virtio_scsi_handle_cmd_vq (s=0x558716c049c0, >>> vq=0x558716c0d2a8) at ../hw/scsi/virtio-scsi.c:831 >>> #9 0x558712f69bcb in virtio_scsi_handle_cmd (vdev=0x558716c049c0, >>> vq=0x558716c0d2a8) at ../hw/scsi/virtio-scsi.c:867 >>> #10 0x558712f96812 in virtio_queue_notify_vq (vq=0x558716c0d2a8) at >>> ../hw/virtio/virtio.c:2263 >>> #11 0x558712f99b75 in virtio_queue_host_notifier_read >>> (n=0x558716c0d31c) at ../hw/virtio/virtio.c:3575 >>> #12 0x55871323d8b5 in aio_dispatch_handler (ctx=0x55871598d5e0, >>> node=0x558716771000) at ../util/aio-posix.c:372 >>> #13 0x55871323d988 in aio_dispatch_ready_handlers (ctx=0x55871598d5e0, >>> ready_list=0x7ff7f288eeb0) at ../util/aio-posix.c:401 >> >> As for why it doesn't progress, blk_co_is_available_entry() uses bdrv_graph_co_rdlock() and can't get it, because the main thread has the write lock. Should be fixed once the AioContext locks are gone, but not sure what should be done to avoid it until then. >>> >>> Then the nested event loop in blk_is_available() would probably be >>> enough to make progress, yes. >>> >>> Maybe we could actually drop the lock (and immediately reacquire it) in >>> AIO_WAIT_WHILE() even if we're in the home thread? That should give the >>> main thread a chance to make progress. >> >> Seems to work :) I haven't run into the issue with the following change >> anymore, but I have to say, running into that specific deadlock only >> happened every 10-15 tries or so before. Did 30 tests now. But >> unfortunately, the stuck IO issue is still there. >> >>> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h >>> index 5449b6d742..da159501ca 100644 >>> --- a/include/block/aio-wait.h >>> +++ b/include/block/aio-wait.h >>> @@ -88,7 +88,13 @@ extern AioWait global_aio_wait; >>> smp_mb__after_rmw(); \ >>> if (ctx_ && in_aio_context_home_thread(ctx_)) {\ >>> while ((cond)) { \ >>> +if (unlock && ctx_) { \ >>> +aio_context_release(ctx_); \ >>> +}
Re: [PATCH v3 0/9] mirror: allow switching from background to active mode
Am 18.10.23 um 11:41 schrieb Markus Armbruster: > Fiona Ebner writes: >> >> Initially, I tried to go for a more general 'job-change' command, but >> to avoid mutual inclusion of block-core.json and job.json, more >> preparation would be required. > > Can you elaborate a bit? A more generic command is preferable, and we > need to understand what it would take. > Yes, I did so during the discussion of v2: https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02993.html Best Regards, Fiona
Re: [PATCH v6 2/5] migration: migrate 'inc' command option is deprecated.
Markus Armbruster wrote: > Juan Quintela writes: > >> Use blockdev-mirror with NBD instead. >> >> Reviewed-by: Thomas Huth >> Acked-by: Stefan Hajnoczi >> Reviewed-by: Markus Armbruster >> Signed-off-by: Juan Quintela >> --- >> docs/about/deprecated.rst | 9 + >> qapi/migration.json| 8 +++- >> migration/migration-hmp-cmds.c | 5 + >> migration/migration.c | 5 + >> 4 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index 2febd2d12f..b51136f50a 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -461,3 +461,12 @@ Migration >> ``skipped`` field in Migration stats has been deprecated. It hasn't >> been used for more than 10 years. >> >> +``inc`` migrate command option (since 8.2) >> +'' >> + >> +Use blockdev-mirror with NBD instead. >> + >> +As an intermediate step the ``inc`` functionality can be achieved by >> +setting the ``block-incremental`` migration parameter to ``true``. >> +But this parameter is also deprecated. >> + > > If you need to respin for some other reason, drop the blank line at end > of file. Same in later patches. > > [...] Done. There is a tool, git-am maybe, that complains if files don't end in a blank line. You can't have happy everybody. Later, Juan.
[PATCH v7 4/4] migration: Deprecate old compression method
Acked-by: Stefan Hajnoczi Acked-by: Peter Xu Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 8 + qapi/migration.json | 63 ++- migration/options.c | 13 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 7ae872162d..e7f17827d3 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -488,3 +488,11 @@ devices or none. Please see "QMP invocation for live storage migration with ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst for a detailed explanation. + +old compression method (since 8.2) +'' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they fail randomly. If you need +compression, use multifd compression methods. diff --git a/qapi/migration.json b/qapi/migration.json index e3b00a215b..e6610af428 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -272,6 +272,10 @@ # Features: # # @deprecated: Member @disk is deprecated because block migration is. +# Member @compression is deprecated because it is unreliable and +# untested. It is recommended to use multifd migration, which +# offers an alternative compression implementation that is +# reliable and tested. # # Since: 0.14 ## @@ -289,7 +293,7 @@ '*blocked-reasons': ['str'], '*postcopy-blocktime': 'uint32', '*postcopy-vcpu-blocktime': ['uint32'], - '*compression': 'CompressionStats', + '*compression': { 'type': 'CompressionStats', 'features': [ 'deprecated' ] }, '*socket-address': ['SocketAddress'], '*dirty-limit-throttle-time-per-round': 'uint64', '*dirty-limit-ring-full-time': 'uint64'} } @@ -530,7 +534,10 @@ # Features: # # @deprecated: Member @block is deprecated. Use blockdev-mirror with -# NBD instead. +# NBD instead. Member @compression is deprecated because it is +# unreliable and untested. It is recommended to use multifd +# migration, which offers an alternative compression +# implementation that is reliable and tested. # # @unstable: Members @x-colo and @x-ignore-shared are experimental. # @@ -538,7 +545,8 @@ ## { 'enum': 'MigrationCapability', 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', - 'compress', 'events', 'postcopy-ram', + { 'name': 'compress', 'features': [ 'deprecated' ] }, + 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', { 'name': 'block', 'features': [ 'deprecated' ] }, @@ -844,7 +852,9 @@ # Features: # # @deprecated: Member @block-incremental is deprecated. Use -# blockdev-mirror with NBD instead. +# blockdev-mirror with NBD instead. Members @compress-level, +# @compress-threads, @decompress-threads and @compress-wait-thread +# are deprecated because @compression is deprecated. # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. @@ -854,8 +864,11 @@ { 'enum': 'MigrationParameter', 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', + { 'name': 'compress-level', 'features': [ 'deprecated' ] }, + { 'name': 'compress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'decompress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] }, + 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', @@ -1023,7 +1036,9 @@ # Features: # # @deprecated: Member @block-incremental is deprecated. Use -# blockdev-mirror with NBD instead. +# blockdev-mirror with NBD instead. Members @compress-level, +# @compress-threads, @decompress-threads and @compress-wait-thread +# are deprecated because @compression is deprecated. # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. @@ -1038,10 +1053,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', -'*compress-level': 'uint8', -'*compress-threads': 'uint8', -'*compress-wait-thread': 'bool', -'*decompress-threads': 'uint8', +'*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, +'*compress-threads':
[PATCH v7 0/4] Migration deprecated parts
Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com> [PULL 00/11] Migration 20231018 patches And here we are, at v7: - drop black line at the end of deprecated.rst - change qemu-iotest output due to warnings for deprecation. The only real change is the output of the qemu-iotest. That is the reason why I maintained the reviewed-by. But will be happy if anyone of the block people ack the changes. Thanks, Juan. On this v6: - Fixed Markus comments - 1st patch is reviewed - dropped the RFC ones. Later, Juan. On this v5: - Rebased on top of last migration pull requesnt: - address markus comments. Basically we recommend always blockdev-mirror + NBD. In deprecated.rst we also put the posiblity of using block-incremental and block, but we state that they are also deprecated. I know, I know, I deprecated them in the following patch. - Dropped the removal of block-migration and block-incremental I am only interested in showing why I want to remove the -b/-i options. Please review. Later, Juan. On this v4: - addressed all markus comments. - rebased on latest. - improve formatting of migration.json - print block migration status when needed. - patches 7-10 are not mean to merge, they just show why we want to deprecate block migration and remove its support. - Patch 7 just drop support for -i/-b and qmp equivalents. - Patch 8 shows all the helpers and convolutions we need to have to support that -i and -d. - patch 9 drops block-incremental migration support. - patch 9 drops block migration support. Please review. Thanks, Juan. On this v3: - Rebase on top of upstream. - Changed v8.1 to 8.2 (I left the reviewed by anyways) - missing the block deprecation code, please. Please, review. Later, Juan. On this v2: - dropped -incoming deprecation Paolo came with a better solution using keyvalues. - skipped field is already ready for next pull request, so dropped. - dropped the RFC bits, nermal PATCH. - Assessed all the review comments. - Added indentation of migration.json. - Used the documentation pointer to substitute block migration. Please review. [v1] Hi this series describe the migration parts that have to be deprecated. - It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-) - skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero. - inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options. - block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it? - old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more. I don't know what to do with this code, really. * Remove it for this release? It don't work, and haven't work reliabely in quite a few time. * Deprecate it and remove in another couple of releases, i.e. normal deprecation. * Ideas? - -incoming if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here? This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit. What do you think? Later, Juan. Juan Quintela (4): migration: migrate 'inc' command option is deprecated. migration: migrate 'blk' command option is deprecated. migration: Deprecate block migration migration: Deprecate old compression method docs/about/deprecated.rst | 35 + qapi/migration.json| 93 ++ migration/block.c | 3 ++ migration/migration-hmp-cmds.c | 10 migration/migration.c | 10 migration/options.c| 22 +++- tests/qemu-iotests/183.out | 2 + 7 files changed, 152 insertions(+), 23 deletions(-) -- 2.41.0
[PATCH v7 3/4] migration: Deprecate block migration
It is obsolete. It is better to use driver-mirror with NBD instead. CC: Kevin Wolf CC: Eric Blake CC: Stefan Hajnoczi CC: Hanna Czenczek Acked-by: Stefan Hajnoczi Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 10 ++ qapi/migration.json| 29 - migration/block.c | 3 +++ migration/options.c| 9 - tests/qemu-iotests/183.out | 1 + 5 files changed, 46 insertions(+), 6 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 98b0f14e69..7ae872162d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -478,3 +478,13 @@ Use blockdev-mirror with NBD instead. As an intermediate step the ``blk`` functionality can be achieved by setting the ``block`` migration capability to ``true``. But this capability is also deprecated. + +block migration (since 8.2) +''' + +Block migration is too inflexible. It needs to migrate all block +devices or none. + +Please see "QMP invocation for live storage migration with +``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst +for a detailed explanation. diff --git a/qapi/migration.json b/qapi/migration.json index 3765c2b662..e3b00a215b 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -269,11 +269,15 @@ # average memory load of the virtual CPU indirectly. Note that # zero means guest doesn't dirty memory. (Since 8.1) # +# Features: +# +# @deprecated: Member @disk is deprecated because block migration is. +# # Since: 0.14 ## { 'struct': 'MigrationInfo', 'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats', - '*disk': 'MigrationStats', + '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] }, '*vfio': 'VfioStats', '*xbzrle-cache': 'XBZRLECacheStats', '*total-time': 'int', @@ -525,6 +529,9 @@ # # Features: # +# @deprecated: Member @block is deprecated. Use blockdev-mirror with +# NBD instead. +# # @unstable: Members @x-colo and @x-ignore-shared are experimental. # # Since: 1.2 @@ -534,7 +541,8 @@ 'compress', 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', - 'block', 'return-path', 'pause-before-switchover', 'multifd', + { 'name': 'block', 'features': [ 'deprecated' ] }, + 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, 'validate-uuid', 'background-snapshot', @@ -835,6 +843,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -850,7 +861,7 @@ 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'avail-switchover-bandwidth', 'downtime-limit', { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, - 'block-incremental', + { 'name': 'block-incremental', 'features': [ 'deprecated' ] }, 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', @@ -1011,6 +1022,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -1040,7 +1054,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, -'*block-incremental': 'bool', +'*block-incremental': { 'type': 'bool', +'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1225,6 +1240,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -1251,7 +1269,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, -'*block-incremental': 'bool', +'*block-incremental': { 'type': 'bool', +'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', diff --git a/migration/block.c b/migration/block.c index b60698d6e2..acff
[PATCH v7 2/4] migration: migrate 'blk' command option is deprecated.
Use blocked-mirror with NBD instead. Acked-by: Stefan Hajnoczi Reviewed-by: Thomas Huth Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 9 + qapi/migration.json| 7 --- migration/migration-hmp-cmds.c | 5 + migration/migration.c | 5 + tests/qemu-iotests/183.out | 1 + 5 files changed, 24 insertions(+), 3 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index fc6adf1dea..98b0f14e69 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -469,3 +469,12 @@ Use blockdev-mirror with NBD instead. As an intermediate step the ``inc`` functionality can be achieved by setting the ``block-incremental`` migration parameter to ``true``. But this parameter is also deprecated. + +``blk`` migrate command option (since 8.2) +'' + +Use blockdev-mirror with NBD instead. + +As an intermediate step the ``blk`` functionality can be achieved by +setting the ``block`` migration capability to ``true``. But this +capability is also deprecated. diff --git a/qapi/migration.json b/qapi/migration.json index fa7f4f2575..3765c2b662 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1526,8 +1526,8 @@ # # Features: # -# @deprecated: Member @inc is deprecated. Use blockdev-mirror with -# NBD instead. +# @deprecated: Members @inc and @blk are deprecated. Use +# blockdev-mirror with NBD instead. # # Returns: nothing on success # @@ -1550,7 +1550,8 @@ # <- { "return": {} } ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', + 'data': {'uri': 'str', + '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*detach': 'bool', '*resume': 'bool' } } diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 83176f5bae..dfe98da355 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) " use blockdev-mirror with NBD instead"); } +if (blk) { +warn_report("option '-b' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, true, resume, &err); if (hmp_handle_error(mon, err)) { diff --git a/migration/migration.c b/migration/migration.c index 27145cd99e..79b742b98b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1609,6 +1609,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, " use blockdev-mirror with NBD instead"); } +if (blk) { +warn_report("parameter 'blk' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " diff --git a/tests/qemu-iotests/183.out b/tests/qemu-iotests/183.out index fd9c2e52a5..b49f491854 100644 --- a/tests/qemu-iotests/183.out +++ b/tests/qemu-iotests/183.out @@ -28,6 +28,7 @@ read 65536/65536 bytes at offset 0 { 'execute': 'migrate', 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } } +warning: parameter 'blk' is deprecated; use blockdev-mirror with NBD instead {"return": {}} { 'execute': 'query-status' } {"return": {"status": "postmigrate", "singlestep": false, "running": false}} -- 2.41.0
[PATCH v7 1/4] migration: migrate 'inc' command option is deprecated.
Use blockdev-mirror with NBD instead. Reviewed-by: Thomas Huth Acked-by: Stefan Hajnoczi Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 8 qapi/migration.json| 8 +++- migration/migration-hmp-cmds.c | 5 + migration/migration.c | 5 + 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 2febd2d12f..fc6adf1dea 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -461,3 +461,11 @@ Migration ``skipped`` field in Migration stats has been deprecated. It hasn't been used for more than 10 years. +``inc`` migrate command option (since 8.2) +'' + +Use blockdev-mirror with NBD instead. + +As an intermediate step the ``inc`` functionality can be achieved by +setting the ``block-incremental`` migration parameter to ``true``. +But this parameter is also deprecated. diff --git a/qapi/migration.json b/qapi/migration.json index db3df12d6c..fa7f4f2575 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1524,6 +1524,11 @@ # # @resume: resume one paused migration, default "off". (since 3.0) # +# Features: +# +# @deprecated: Member @inc is deprecated. Use blockdev-mirror with +# NBD instead. +# # Returns: nothing on success # # Since: 0.14 @@ -1545,7 +1550,8 @@ # <- { "return": {} } ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', + 'data': {'uri': 'str', '*blk': 'bool', + '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*detach': 'bool', '*resume': 'bool' } } ## diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index a82597f18e..83176f5bae 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) const char *uri = qdict_get_str(qdict, "uri"); Error *err = NULL; +if (inc) { +warn_report("option '-i' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, true, resume, &err); if (hmp_handle_error(mon, err)) { diff --git a/migration/migration.c b/migration/migration.c index 05c0b801ba..27145cd99e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1604,6 +1604,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, { Error *local_err = NULL; +if (blk_inc) { +warn_report("parameter 'inc' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " -- 2.41.0
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
Vladimir Sementsov-Ogievskiy writes: > On 18.10.23 09:47, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy writes: >> >>> On 17.10.23 18:00, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: > Send a new event when guest reads virtio-pci config after > virtio_notify_config() call. > > That's useful to check that guest fetched modified config, for example > after resizing disk backend. > > Signed-off-by: Vladimir Sementsov-Ogievskiy >> [...] >> > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 2468f8bddf..37a8785b81 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -329,3 +329,25 @@ ># Since: 8.2 >## >{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} } > + > +## > +# @X_CONFIG_READ: > +# > +# Emitted whenever guest reads virtio device config after config change. > +# > +# @device: device name > +# > +# @path: device path > +# > +# Since: 5.0.1-24 > +# > +# Example: > +# > +# <- { "event": "X_CONFIG_READ", > +# "data": { "device": "virtio-net-pci-0", > +#"path": "/machine/peripheral/virtio-net-pci-0" }, > +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > +# > +## > +{ 'event': 'X_CONFIG_READ', > + 'data': { '*device': 'str', 'path': 'str' } } The commit message talks about event CONFIG_READ, but you actually name it x-device-sync-config. >>> >>> will fix >>> I figure you use x- to signify "unstable". Please use feature flag 'unstable' for that. See docs/devel/qapi-code-gen.rst section "Features", in particular "Special features", and also the note on x- in section "Naming rules and reserved names". >>> >>> OK, will do. >>> >>> Hmm, it say >>> >>> Names beginning with ``x-`` used to signify "experimental". This >>> convention has been replaced by special feature "unstable". >>> >>> "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't >>> find an example. Seems "unstable" always used together with "x-". >> >> True. >> >> The "x-" prefix originated with qdev properties. First use might be >> commit f0c07c7c7b4. The convention wasn't documented then, but QOM/qdev >> properties have always been a documentation wasteland. It then spread >> to other places, and eventually to the QAPI schema. Where we try pretty >> hard to document things properly. We documented the "x-" prefix in >> commit e790e666518: >> >> Any name (command, event, type, field, or enum value) beginning with >> "x-" is marked experimental, and may be withdrawn or changed >> incompatibly in a future release. >> >> Minor pain point: when things grow up from experimental to stable, we >> have to rename. >> >> The convention didn't stop us from naming non-experimental things x-FOO, >> e.g. QOM property "x-origin" in commit 6105683da35. Made it to the QAPI >> schema in commit 8825587b53c. Point is: the prefix isn't a reliable >> marker for "unstable". >> >> Since I needed a reliable marker for my "set policy for unstable >> interfaces" feature (see CLI option -compat), I created special feature >> flag "unstable", and dropped the "x-" convention for the QAPI schema. >> >> Renaming existing "x-" names felt like pointless churn, so I didn't. >> >> I'm not objecting to new names starting with "x-". Nor am I objecting >> to feature 'unstable' on names that don't start with "x-". >> >> I guess "x-" remains just fine for things we don't intend to make stable >> at some point. The "x-" can remind humans "this is unstable" better >> than a feature flag can (for machines, it's the other way round). >> >> For things we do intend (hope?) to make stable, I wouldn't bother with >> the "x-". >> >> Clearer now? > > Yes, thanks. > > x- seems safer for management tool that doesn't know about "unstable" > properties.. Easy, traditional, and unreliable :) > But on the other hand, changing from x- to no-prefix is already done when the > feature is stable, and thouse who use it already use the latest version of > interface, so, removing the prefix is just extra work. Exactly. > So, I think, I'd go without prefix. Makes sense. >>> Also, nothing said about events. Is using "X_" wrong idea? Should it be >>> x-SOME_EVENT instead? >> >> Since this is the first unstable event, there is no precedent. Let's >> use no prefix, and move on. >> The name CONFIG_READ feels overly generic for something that makes sense only with virtio devices. >>> >>> Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. >> >> That one came to be as a generalization of existing MEM_UNPLUG_ERROR and >> a concrete need to signal CPU unplug errors. Demonstrates "unplug guest >> errors" can happen for different kinds of devices. So we went with a >> generic event we can use for all of them. >> This doesn't seem
Re: [PATCH v7 0/4] Migration deprecated parts
Juan Quintela wrote: > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com> > [PULL 00/11] Migration 20231018 patches > > And here we are, at v7: > - drop black line at the end of deprecated.rst > - change qemu-iotest output due to warnings for deprecation. > > The only real change is the output of the qemu-iotest. That is the > reason why I maintained the reviewed-by. But will be happy if anyone > of the block people ack the changes. I forgot to include the link to the CI of the previous failure. https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229 tput mismatch (see /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad) --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out +++ /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad @@ -28,6 +28,8 @@ { 'execute': 'migrate', 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } } +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead +warning: block migration is deprecated; use blockdev-mirror with NBD instead {"return": {}} { 'execute': 'query-status' } {"return": {"status": "postmigrate", "singlestep": false, "running": false}} > > Thanks, Juan. > > On this v6: > - Fixed Markus comments > - 1st patch is reviewed > - dropped the RFC ones. > > Later, Juan. > > On this v5: > - Rebased on top of last migration pull requesnt: > > - address markus comments. Basically we recommend always > blockdev-mirror + NBD. In deprecated.rst we also put the posiblity > of using block-incremental and block, but we state that they are > also deprecated. > I know, I know, I deprecated them in the following patch. > > - Dropped the removal of block-migration and block-incremental I am > only interested in showing why I want to remove the -b/-i options. > > Please review. > > Later, Juan. > > On this v4: > - addressed all markus comments. > - rebased on latest. > - improve formatting of migration.json > - print block migration status when needed. > - patches 7-10 are not mean to merge, they just show why we want to > deprecate block migration and remove its support. > - Patch 7 just drop support for -i/-b and qmp equivalents. > - Patch 8 shows all the helpers and convolutions we need to have to > support that -i and -d. > - patch 9 drops block-incremental migration support. > - patch 9 drops block migration support. > > Please review. > > Thanks, Juan. > > On this v3: > > - Rebase on top of upstream. > - Changed v8.1 to 8.2 (I left the reviewed by anyways) > - missing the block deprecation code, please. > > Please, review. > > Later, Juan. > > On this v2: > > - dropped -incoming deprecation > Paolo came with a better solution using keyvalues. > > - skipped field is already ready for next pull request, so dropped. > > - dropped the RFC bits, nermal PATCH. > > - Assessed all the review comments. > > - Added indentation of migration.json. > > - Used the documentation pointer to substitute block migration. > > Please review. > > [v1] > Hi this series describe the migration parts that have to be deprecated. > > - It is an rfc because I doubt that I did the deprecation process right. > Hello Markus O:-) > > - skipped field: It is older than me, I have never know what it stands > for. As far as I know it has always been zero. > > - inc/blk migrate command options. They are only used by block > migration (that I deprecate on the following patch). And they are really > bad. > grep must_remove_block_options. > > - block migration. block jobs, whatever they are called this week are > way more flexible. Current code works, but we broke it here and > there, and really nobody has stand up to maintain it. It is quite > contained and can be left there. Is anyone really using it? > > - old compression method. It don't work. See last try from Lukas to > make a test that works reliabely. I failed with the same task years > ago. It is really slow, and if compression is good for you, multifd > + zlib is going to perform/compress way more. > > I don't know what to do with this code, really. > > * Remove it for this release? It don't work, and haven't work > reliabely in quite a few time. > > * Deprecate it and remove in another couple of releases, i.e. normal > deprecation. > > * Ideas? > > - -incoming > > if you need to set parameters (multifd cames to mind, and pree
Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command
Vladimir Sementsov-Ogievskiy writes: > Add a command that can replace bs in following BdrvChild structures: > > - qdev blk root child > - block-export blk root child > - any child of BlockDriverState selected by child-name > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 89751d81f2..cf92e0df8b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -6054,3 +6054,86 @@ > ## > { 'struct': 'DummyBlockCoreForceArrays', >'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } } > + > +## > +# @BlockParentType: > +# > +# @qdev: block device, such as created by device_add, and denoted by > +# qdev-id > +# > +# @driver: block driver node, such as created by blockdev-add, and > +# denoted by node-name > +# > +# @export: block export, such created by block-export-add, and > +# denoted by export-id > +# > +# Since 8.2 > +## > +{ 'enum': 'BlockParentType', > + 'data': ['qdev', 'driver', 'export'] } > + > +## > +# @BdrvChildRefQdev: > +# > +# @qdev-id: the device's ID or QOM path > +# > +# Since 8.2 > +## > +{ 'struct': 'BdrvChildRefQdev', > + 'data': { 'qdev-id': 'str' } } > + > +## > +# @BdrvChildRefExport: > +# > +# @export-id: block export identifier > +# > +# Since 8.2 > +## > +{ 'struct': 'BdrvChildRefExport', > + 'data': { 'export-id': 'str' } } > + > +## > +# @BdrvChildRefDriver: > +# > +# @node-name: the node name of the parent block node > +# > +# @child: name of the child to be replaced, like "file" or "backing" > +# > +# Since 8.2 > +## > +{ 'struct': 'BdrvChildRefDriver', > + 'data': { 'node-name': 'str', 'child': 'str' } } > + > +## > +# @BlockdevReplace: > +# > +# @parent-type: type of the parent, which child is to be replaced > +# > +# @new-child: new child for replacement > +# > +# Since 8.2 > +## > +{ 'union': 'BlockdevReplace', > + 'base': { > + 'parent-type': 'BlockParentType', > + 'new-child': 'str' > + }, > + 'discriminator': 'parent-type', > + 'data': { > + 'qdev': 'BdrvChildRefQdev', > + 'export': 'BdrvChildRefExport', > + 'driver': 'BdrvChildRefDriver' > + } } > + > +## > +# @x-blockdev-replace: > +# > +# Replace a block-node associated with device (selected by > +# @qdev-id) or with block-export (selected by @export-id) or > +# any child of block-node (selected by @node-name and @child) > +# with @new-child block-node. > +# > +# Since 8.2 > +## > +{ 'command': 'x-blockdev-replace', 'boxed': true, > + 'data': 'BlockdevReplace' } > diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c > new file mode 100644 > index 00..0e751ce4f7 > --- /dev/null > +++ b/stubs/blk-by-qdev-id.c > @@ -0,0 +1,9 @@ > +#include "qemu/osdep.h" > +#include "qapi/error.h" > +#include "sysemu/block-backend.h" > + > +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) > +{ > +error_setg(errp, "blk '%s' not found", id); Is this expected to happen? > +return NULL; > +} [...] QAPI schema Acked-by: Markus Armbruster
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > x- seems safer for management tool that doesn't know about "unstable" > > properties.. > > Easy, traditional, and unreliable :) > > But on the other hand, changing from x- to no-prefix is already done when > > the feature is stable, and thouse who use it already use the latest version > > of interface, so, removing the prefix is just extra work. > > Exactly. > I think "x-" is still better for command line use of properties - we don't have an API to mark things unstable there, do we? -- MST
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote: > Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben: > > From: David Woodhouse > > > > There's no need to force the user to assign a vdev. We can automatically > > assign one, starting at xvda and searching until we find the first disk > > name that's unused. > > > > This means we can now allow '-drive if=xen,file=xxx' to work without an > > explicit separate -driver argument, just like if=virtio. > > > > Signed-off-by: David Woodhouse > > Actually, how does this play together with xen_config_dev_blk()? This > looks like it tried to implement a very similar thing (which is IF_XEN > even already existed). Ah yes, thanks for spotting that! I hadn't been looking at the xenfv > Are we now trying to attach each if=xen disk twice in the 'xenpv' > machine? Or if something prevents this, is it dead code. I suspect we end up creating them twice (and probably thus failing to lock the backing file). The old Xen PV device drivers in Qemu, before Paul's XenDevice conversion, were purely reactive. If they see nodes in XenStore describing a backend which they should implement, they'd do so. The code in xen_config_dev_blk() is *creating* those nodes for the frontend (guest) and backend (host/qemu) to see, which prompts the xen-block and xen-net drivers into action. In the new XenDevice model, we can just instantiate a device (e.g. qdev_new(TYPE_XEN_DISK_DEVICE) and its realize() method will create the corresponding XenStore nodes (with some help from the generic XenBus code for the common ones). The new XenDevice/XenBus model will *also* react to nodes which it discovers in XenStore, and instantiate the corresponding devices. So I suspect what'll happen is xen_config_dev_blk() will create the nodes starting at xvda (int vdev = 202 * 256 + 16 * disk->unit), and later my new code will create a new set starting from xvdb or wherever the next free one is. > I think in both cases, we would want to delete that function and remove > the loop that calls it in xen_init_pv()? Yes, I think so. The xen_config_dev_blk() one can just die, as can xen_config_dev_console() which isn't called from anywhere anyway. The vkbd/vfb ones can stay until/unless those drivers are converted to the new XenDevice model. And xen_config_dev_nic() probably just needs to loop doing the same as I did in pc_init_nic() in https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dw...@infradead.org/T/#u +if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) { +DeviceState *dev = qdev_new("xen-net-device"); +qdev_set_nic_properties(dev, nd); +qdev_realize_and_unref(dev, xen_bus, &error_fatal); ... but this just reinforces what I said there about "if qmp_device_add() can find the damn bus and do this right, why do we have to litter it through platform code?" smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v7 0/4] Migration deprecated parts
On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote: > Juan Quintela wrote: > > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com> > > [PULL 00/11] Migration 20231018 patches > > > > And here we are, at v7: > > - drop black line at the end of deprecated.rst > > - change qemu-iotest output due to warnings for deprecation. > > > > The only real change is the output of the qemu-iotest. That is the > > reason why I maintained the reviewed-by. But will be happy if anyone > > of the block people ack the changes. > > I forgot to include the link to the CI of the previous failure. > > https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229 > > tput mismatch (see > /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad) > --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out > +++ > /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad > @@ -28,6 +28,8 @@ > { 'execute': 'migrate', > 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } } > +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead > +warning: block migration is deprecated; use blockdev-mirror with NBD instead > {"return": {}} > { 'execute': 'query-status' } > {"return": {"status": "postmigrate", "singlestep": false, "running": > false}} IIUC, the JSON bits are being written on stdout, and the warnings are being written on stderr. The interleaving of the data is potentially going to be non-deterministic in the .out file. Generally you'd want a filter in the iotests that culls the warning: lines to avoid this mixing of stdout/err streams. 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 4/4] qapi: introduce CONFIG_READ event
On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > > x- seems safer for management tool that doesn't know about "unstable" > > > properties.. > > > > Easy, traditional, and unreliable :) > > > > But on the other hand, changing from x- to no-prefix is already > > > done when the feature is stable, and thouse who use it already > > > use the latest version of interface, so, removing the prefix is > > > just extra work. > > > > Exactly. > > > > I think "x-" is still better for command line use of properties - we > don't have an API to mark things unstable there, do we? Personally I like to see "x-" prefix present *everywhere* there is an unstable feature, and consider the need to rename when declaring it stable to be good thing as it sets an easily identifiable line in the sand and is self-evident to outside observers. The self-documenting nature of the "x-" prefer is what makes it most compelling to me. A patch submission, or command line invokation or an example QMP command, or a bug report, that exhibit an 'x-' prefix are an immediate red flag to anyone who sees them. If someone sees a QMP comamnd / a typical giant QEMU command line, they are never going to go look at the QAPI schema to check whether any feature used had an 'unstable' marker. The 'unstable' marker might as well not exist in most cases. IOW, having the 'unstable' flag in the QAPI schema is great for machine introspection, but it isn't a substitute for having an 'x-' prefix used for the benefit of humans IMHO. 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 v7 0/4] Migration deprecated parts
Daniel P. Berrangé wrote: > On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote: >> Juan Quintela wrote: >> > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com> >> > [PULL 00/11] Migration 20231018 patches >> > >> > And here we are, at v7: >> > - drop black line at the end of deprecated.rst >> > - change qemu-iotest output due to warnings for deprecation. >> > >> > The only real change is the output of the qemu-iotest. That is the >> > reason why I maintained the reviewed-by. But will be happy if anyone >> > of the block people ack the changes. >> >> I forgot to include the link to the CI of the previous failure. >> >> https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229 >> >> tput mismatch (see >> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad) >> --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out >> +++ >> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad >> @@ -28,6 +28,8 @@ >> { 'execute': 'migrate', >> 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } } >> +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead >> +warning: block migration is deprecated; use blockdev-mirror with NBD instead >> {"return": {}} >> { 'execute': 'query-status' } >> {"return": {"status": "postmigrate", "singlestep": false, "running": >> false}} > > IIUC, the JSON bits are being written on stdout, and the warnings > are being written on stderr. The interleaving of the data is > potentially going to be non-deterministic in the .out file. > Generally you'd want a filter in the iotests that culls the > warning: lines to avoid this mixing of stdout/err streams. Thanks. So here I am, going to v8 to create filters. Later, Juan.
[PATCH v8 0/5] Migration deprecated parts
Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com> [PULL 00/11] Migration 20231018 patches v8 now: - drop changes to 183.out - create _filter_migration_block_deprecated And now it passes the iotests. Please review patch 1. Rest are unchanged from v6. Later, Juan. And here we are, at v7: - drop black line at the end of deprecated.rst - change qemu-iotest output due to warnings for deprecation. The only real change is the output of the qemu-iotest. That is the reason why I maintained the reviewed-by. But will be happy if anyone of the block people ack the changes. Thanks, Juan. On this v6: - Fixed Markus comments - 1st patch is reviewed - dropped the RFC ones. Later, Juan. On this v5: - Rebased on top of last migration pull requesnt: - address markus comments. Basically we recommend always blockdev-mirror + NBD. In deprecated.rst we also put the posiblity of using block-incremental and block, but we state that they are also deprecated. I know, I know, I deprecated them in the following patch. - Dropped the removal of block-migration and block-incremental I am only interested in showing why I want to remove the -b/-i options. Please review. Later, Juan. On this v4: - addressed all markus comments. - rebased on latest. - improve formatting of migration.json - print block migration status when needed. - patches 7-10 are not mean to merge, they just show why we want to deprecate block migration and remove its support. - Patch 7 just drop support for -i/-b and qmp equivalents. - Patch 8 shows all the helpers and convolutions we need to have to support that -i and -d. - patch 9 drops block-incremental migration support. - patch 9 drops block migration support. Please review. Thanks, Juan. On this v3: - Rebase on top of upstream. - Changed v8.1 to 8.2 (I left the reviewed by anyways) - missing the block deprecation code, please. Please, review. Later, Juan. On this v2: - dropped -incoming deprecation Paolo came with a better solution using keyvalues. - skipped field is already ready for next pull request, so dropped. - dropped the RFC bits, nermal PATCH. - Assessed all the review comments. - Added indentation of migration.json. - Used the documentation pointer to substitute block migration. Please review. [v1] Hi this series describe the migration parts that have to be deprecated. - It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-) - skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero. - inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options. - block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it? - old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more. I don't know what to do with this code, really. * Remove it for this release? It don't work, and haven't work reliabely in quite a few time. * Deprecate it and remove in another couple of releases, i.e. normal deprecation. * Ideas? - -incoming if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here? This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit. What do you think? Later, Juan. Juan Quintela (5): qemu-iotests: Filter warnings about block migration being deprecated migration: migrate 'inc' command option is deprecated. migration: migrate 'blk' command option is deprecated. migration: Deprecate block migration migration: Deprecate old compression method docs/about/deprecated.rst| 35 qapi/migration.json | 93 migration/block.c| 3 ++ migration/migration-hmp-cmds.c | 10 migration/migration.c| 10 migration/options.c | 22 +++- tests/qemu-iotests/183 | 2 +- tests/qemu-iotests/common.filter | 7 +++ 8 files changed, 158 insertions(+), 24 deletions(-) -- 2.41.0
[PATCH v8 3/5] migration: migrate 'blk' command option is deprecated.
Use blocked-mirror with NBD instead. Acked-by: Stefan Hajnoczi Reviewed-by: Thomas Huth Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 9 + qapi/migration.json| 7 --- migration/migration-hmp-cmds.c | 5 + migration/migration.c | 5 + 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index fc6adf1dea..98b0f14e69 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -469,3 +469,12 @@ Use blockdev-mirror with NBD instead. As an intermediate step the ``inc`` functionality can be achieved by setting the ``block-incremental`` migration parameter to ``true``. But this parameter is also deprecated. + +``blk`` migrate command option (since 8.2) +'' + +Use blockdev-mirror with NBD instead. + +As an intermediate step the ``blk`` functionality can be achieved by +setting the ``block`` migration capability to ``true``. But this +capability is also deprecated. diff --git a/qapi/migration.json b/qapi/migration.json index fa7f4f2575..3765c2b662 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1526,8 +1526,8 @@ # # Features: # -# @deprecated: Member @inc is deprecated. Use blockdev-mirror with -# NBD instead. +# @deprecated: Members @inc and @blk are deprecated. Use +# blockdev-mirror with NBD instead. # # Returns: nothing on success # @@ -1550,7 +1550,8 @@ # <- { "return": {} } ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', + 'data': {'uri': 'str', + '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*detach': 'bool', '*resume': 'bool' } } diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 83176f5bae..dfe98da355 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) " use blockdev-mirror with NBD instead"); } +if (blk) { +warn_report("option '-b' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, true, resume, &err); if (hmp_handle_error(mon, err)) { diff --git a/migration/migration.c b/migration/migration.c index 27145cd99e..79b742b98b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1609,6 +1609,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, " use blockdev-mirror with NBD instead"); } +if (blk) { +warn_report("parameter 'blk' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " -- 2.41.0
[PATCH v8 1/5] qemu-iotests: Filter warnings about block migration being deprecated
Create a new filter that removes the two warnings for test 183. Signed-off-by: Juan Quintela --- tests/qemu-iotests/183 | 2 +- tests/qemu-iotests/common.filter | 7 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183 index ee62939e72..b85770458e 100755 --- a/tests/qemu-iotests/183 +++ b/tests/qemu-iotests/183 @@ -90,7 +90,7 @@ echo reply="$(_send_qemu_cmd $src \ "{ 'execute': 'migrate', 'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \ -'return\|error')" +'return\|error' | _filter_migration_block_deprecated)" echo "$reply" if echo "$reply" | grep "compiled without old-style" > /dev/null; then _notrun "migrate -b support not compiled in" diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index fc3c64bcb8..2846c83808 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -359,5 +359,12 @@ _filter_qcow2_compression_type_bit() -e 's/\(incompatible_features.*\), 3\(,.*\)/\1\2/' } +# filter warnings caused for block migration deprecation +_filter_migration_block_deprecated() +{ +gsed -e '/warning: parameter .blk. is deprecated; use blockdev-mirror with NBD instead/d' \ + -e '/warning: block migration is deprecated; use blockdev-mirror with NBD instead/d' +} + # make sure this script returns success true -- 2.41.0
[PATCH v8 5/5] migration: Deprecate old compression method
Acked-by: Stefan Hajnoczi Acked-by: Peter Xu Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 8 + qapi/migration.json | 63 ++- migration/options.c | 13 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 7ae872162d..e7f17827d3 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -488,3 +488,11 @@ devices or none. Please see "QMP invocation for live storage migration with ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst for a detailed explanation. + +old compression method (since 8.2) +'' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they fail randomly. If you need +compression, use multifd compression methods. diff --git a/qapi/migration.json b/qapi/migration.json index e3b00a215b..e6610af428 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -272,6 +272,10 @@ # Features: # # @deprecated: Member @disk is deprecated because block migration is. +# Member @compression is deprecated because it is unreliable and +# untested. It is recommended to use multifd migration, which +# offers an alternative compression implementation that is +# reliable and tested. # # Since: 0.14 ## @@ -289,7 +293,7 @@ '*blocked-reasons': ['str'], '*postcopy-blocktime': 'uint32', '*postcopy-vcpu-blocktime': ['uint32'], - '*compression': 'CompressionStats', + '*compression': { 'type': 'CompressionStats', 'features': [ 'deprecated' ] }, '*socket-address': ['SocketAddress'], '*dirty-limit-throttle-time-per-round': 'uint64', '*dirty-limit-ring-full-time': 'uint64'} } @@ -530,7 +534,10 @@ # Features: # # @deprecated: Member @block is deprecated. Use blockdev-mirror with -# NBD instead. +# NBD instead. Member @compression is deprecated because it is +# unreliable and untested. It is recommended to use multifd +# migration, which offers an alternative compression +# implementation that is reliable and tested. # # @unstable: Members @x-colo and @x-ignore-shared are experimental. # @@ -538,7 +545,8 @@ ## { 'enum': 'MigrationCapability', 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', - 'compress', 'events', 'postcopy-ram', + { 'name': 'compress', 'features': [ 'deprecated' ] }, + 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', { 'name': 'block', 'features': [ 'deprecated' ] }, @@ -844,7 +852,9 @@ # Features: # # @deprecated: Member @block-incremental is deprecated. Use -# blockdev-mirror with NBD instead. +# blockdev-mirror with NBD instead. Members @compress-level, +# @compress-threads, @decompress-threads and @compress-wait-thread +# are deprecated because @compression is deprecated. # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. @@ -854,8 +864,11 @@ { 'enum': 'MigrationParameter', 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', + { 'name': 'compress-level', 'features': [ 'deprecated' ] }, + { 'name': 'compress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'decompress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] }, + 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', @@ -1023,7 +1036,9 @@ # Features: # # @deprecated: Member @block-incremental is deprecated. Use -# blockdev-mirror with NBD instead. +# blockdev-mirror with NBD instead. Members @compress-level, +# @compress-threads, @decompress-threads and @compress-wait-thread +# are deprecated because @compression is deprecated. # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. @@ -1038,10 +1053,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', -'*compress-level': 'uint8', -'*compress-threads': 'uint8', -'*compress-wait-thread': 'bool', -'*decompress-threads': 'uint8', +'*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, +'*compress-threads':
[PATCH v8 2/5] migration: migrate 'inc' command option is deprecated.
Use blockdev-mirror with NBD instead. Reviewed-by: Thomas Huth Acked-by: Stefan Hajnoczi Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 8 qapi/migration.json| 8 +++- migration/migration-hmp-cmds.c | 5 + migration/migration.c | 5 + 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 2febd2d12f..fc6adf1dea 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -461,3 +461,11 @@ Migration ``skipped`` field in Migration stats has been deprecated. It hasn't been used for more than 10 years. +``inc`` migrate command option (since 8.2) +'' + +Use blockdev-mirror with NBD instead. + +As an intermediate step the ``inc`` functionality can be achieved by +setting the ``block-incremental`` migration parameter to ``true``. +But this parameter is also deprecated. diff --git a/qapi/migration.json b/qapi/migration.json index db3df12d6c..fa7f4f2575 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1524,6 +1524,11 @@ # # @resume: resume one paused migration, default "off". (since 3.0) # +# Features: +# +# @deprecated: Member @inc is deprecated. Use blockdev-mirror with +# NBD instead. +# # Returns: nothing on success # # Since: 0.14 @@ -1545,7 +1550,8 @@ # <- { "return": {} } ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', + 'data': {'uri': 'str', '*blk': 'bool', + '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] }, '*detach': 'bool', '*resume': 'bool' } } ## diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index a82597f18e..83176f5bae 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) const char *uri = qdict_get_str(qdict, "uri"); Error *err = NULL; +if (inc) { +warn_report("option '-i' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, true, resume, &err); if (hmp_handle_error(mon, err)) { diff --git a/migration/migration.c b/migration/migration.c index 05c0b801ba..27145cd99e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1604,6 +1604,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, { Error *local_err = NULL; +if (blk_inc) { +warn_report("parameter 'inc' is deprecated;" +" use blockdev-mirror with NBD instead"); +} + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " -- 2.41.0
[PATCH v8 4/5] migration: Deprecate block migration
It is obsolete. It is better to use driver-mirror with NBD instead. CC: Kevin Wolf CC: Eric Blake CC: Stefan Hajnoczi CC: Hanna Czenczek Acked-by: Stefan Hajnoczi Reviewed-by: Markus Armbruster Signed-off-by: Juan Quintela --- docs/about/deprecated.rst | 10 ++ qapi/migration.json | 29 - migration/block.c | 3 +++ migration/options.c | 9 - 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 98b0f14e69..7ae872162d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -478,3 +478,13 @@ Use blockdev-mirror with NBD instead. As an intermediate step the ``blk`` functionality can be achieved by setting the ``block`` migration capability to ``true``. But this capability is also deprecated. + +block migration (since 8.2) +''' + +Block migration is too inflexible. It needs to migrate all block +devices or none. + +Please see "QMP invocation for live storage migration with +``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst +for a detailed explanation. diff --git a/qapi/migration.json b/qapi/migration.json index 3765c2b662..e3b00a215b 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -269,11 +269,15 @@ # average memory load of the virtual CPU indirectly. Note that # zero means guest doesn't dirty memory. (Since 8.1) # +# Features: +# +# @deprecated: Member @disk is deprecated because block migration is. +# # Since: 0.14 ## { 'struct': 'MigrationInfo', 'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats', - '*disk': 'MigrationStats', + '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] }, '*vfio': 'VfioStats', '*xbzrle-cache': 'XBZRLECacheStats', '*total-time': 'int', @@ -525,6 +529,9 @@ # # Features: # +# @deprecated: Member @block is deprecated. Use blockdev-mirror with +# NBD instead. +# # @unstable: Members @x-colo and @x-ignore-shared are experimental. # # Since: 1.2 @@ -534,7 +541,8 @@ 'compress', 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', - 'block', 'return-path', 'pause-before-switchover', 'multifd', + { 'name': 'block', 'features': [ 'deprecated' ] }, + 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, 'validate-uuid', 'background-snapshot', @@ -835,6 +843,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -850,7 +861,7 @@ 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'avail-switchover-bandwidth', 'downtime-limit', { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, - 'block-incremental', + { 'name': 'block-incremental', 'features': [ 'deprecated' ] }, 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', @@ -1011,6 +1022,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -1040,7 +1054,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, -'*block-incremental': 'bool', +'*block-incremental': { 'type': 'bool', +'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1225,6 +1240,9 @@ # # Features: # +# @deprecated: Member @block-incremental is deprecated. Use +# blockdev-mirror with NBD instead. +# # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # @@ -1251,7 +1269,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, -'*block-incremental': 'bool', +'*block-incremental': { 'type': 'bool', +'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', diff --git a/migration/block.c b/migration/block.c index b60698d6e2..acffe88f84 100644 --- a/migration/block.c +
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
Daniel P. Berrangé writes: > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: >> > > x- seems safer for management tool that doesn't know about "unstable" >> > > properties.. >> > >> > Easy, traditional, and unreliable :) >> >> > > But on the other hand, changing from x- to no-prefix is already >> > > done when the feature is stable, and thouse who use it already >> > > use the latest version of interface, so, removing the prefix is >> > > just extra work. >> > >> > Exactly. >> > >> >> I think "x-" is still better for command line use of properties - we >> don't have an API to mark things unstable there, do we? > > Personally I like to see "x-" prefix present *everywhere* there is > an unstable feature, and consider the need to rename when declaring > it stable to be good thing as it sets an easily identifiable line > in the sand and is self-evident to outside observers. > > The self-documenting nature of the "x-" prefer is what makes it most > compelling to me. A patch submission, or command line invokation or > an example QMP command, or a bug report, that exhibit an 'x-' prefix > are an immediate red flag to anyone who sees them. Except when it isn't, like in "x-origin". > If someone sees a QMP comamnd / a typical giant QEMU command line, > they are never going to go look at the QAPI schema to check whether > any feature used had an 'unstable' marker. The 'unstable' marker > might as well not exist in most cases. > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > introspection, but it isn't a substitute for having an 'x-' prefix used > for the benefit of humans IMHO. I'm not sure there's disagreement. Quoting myself: The "x-" can remind humans "this is unstable" better than a feature flag can (for machines, it's the other way round). CLI and HMP are for humans. We continue to use "x-" there. QMP is for machines. The feature flag is the sole source of truth. Additional use of "x-" is fine, but not required.
Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command
On 18.10.23 13:45, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: Add a command that can replace bs in following BdrvChild structures: - qdev blk root child - block-export blk root child - any child of BlockDriverState selected by child-name Signed-off-by: Vladimir Sementsov-Ogievskiy [..] --- /dev/null +++ b/stubs/blk-by-qdev-id.c @@ -0,0 +1,9 @@ +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "sysemu/block-backend.h" + +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) +{ +error_setg(errp, "blk '%s' not found", id); Is this expected to happen? Yes, if call the command from qemu-storage-daemon, where qdev-monitor is not linked in. Maybe, better message would be "devices are not supported" Maybe, that possible to use some 'if': notation in qapi, to not include support for qdev into the new command, when it compiled into qemu-storage-daemon? Seems that would not be simple, as we also need to split compilation of the command somehow, now it compiled once both for qemu and qemu tools.. +return NULL; +} [...] QAPI schema Acked-by: Markus Armbruster -- Best regards, Vladimir
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > >> > > x- seems safer for management tool that doesn't know about "unstable" > >> > > properties.. > >> > > >> > Easy, traditional, and unreliable :) > >> > >> > > But on the other hand, changing from x- to no-prefix is already > >> > > done when the feature is stable, and thouse who use it already > >> > > use the latest version of interface, so, removing the prefix is > >> > > just extra work. > >> > > >> > Exactly. > >> > > >> > >> I think "x-" is still better for command line use of properties - we > >> don't have an API to mark things unstable there, do we? > > > > Personally I like to see "x-" prefix present *everywhere* there is > > an unstable feature, and consider the need to rename when declaring > > it stable to be good thing as it sets an easily identifiable line > > in the sand and is self-evident to outside observers. > > > > The self-documenting nature of the "x-" prefer is what makes it most > > compelling to me. A patch submission, or command line invokation or > > an example QMP command, or a bug report, that exhibit an 'x-' prefix > > are an immediate red flag to anyone who sees them. > > Except when it isn't, like in "x-origin". > > > If someone sees a QMP comamnd / a typical giant QEMU command line, > > they are never going to go look at the QAPI schema to check whether > > any feature used had an 'unstable' marker. The 'unstable' marker > > might as well not exist in most cases. > > > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > > introspection, but it isn't a substitute for having an 'x-' prefix used > > for the benefit of humans IMHO. > > I'm not sure there's disagreement. Quoting myself: > > The "x-" can remind humans "this is unstable" better than a feature > flag can (for machines, it's the other way round). > > CLI and HMP are for humans. We continue to use "x-" there. > > QMP is for machines. The feature flag is the sole source of truth. > Additional use of "x-" is fine, but not required. I guess we have different defintions of "for humans" in this context. I consider QMP data still relevant for humans, because humans are reviewing patches to libvirt that add usage of QMP features, or triaging bug reports that include examples of usage, and in both cases it is pretty relevant to make unstable features stand out to the human via the x- prefix IMHO. 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 4/4] qapi: introduce CONFIG_READ event
On 18.10.23 15:02, Markus Armbruster wrote: Daniel P. Berrangé writes: On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: x- seems safer for management tool that doesn't know about "unstable" properties.. Easy, traditional, and unreliable :) But on the other hand, changing from x- to no-prefix is already done when the feature is stable, and thouse who use it already use the latest version of interface, so, removing the prefix is just extra work. Exactly. I think "x-" is still better for command line use of properties - we don't have an API to mark things unstable there, do we? Personally I like to see "x-" prefix present *everywhere* there is an unstable feature, and consider the need to rename when declaring it stable to be good thing as it sets an easily identifiable line in the sand and is self-evident to outside observers. The self-documenting nature of the "x-" prefer is what makes it most compelling to me. A patch submission, or command line invokation or an example QMP command, or a bug report, that exhibit an 'x-' prefix are an immediate red flag to anyone who sees them. Except when it isn't, like in "x-origin". Interesting how many such stable x-FOO things we have? Probably we could deprecate and than rename them? -- Best regards, Vladimir
Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command
Vladimir Sementsov-Ogievskiy writes: > On 18.10.23 13:45, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy writes: >> >>> Add a command that can replace bs in following BdrvChild structures: >>> >>> - qdev blk root child >>> - block-export blk root child >>> - any child of BlockDriverState selected by child-name >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy > > [..] > >>> --- /dev/null >>> +++ b/stubs/blk-by-qdev-id.c >>> @@ -0,0 +1,9 @@ >>> +#include "qemu/osdep.h" >>> +#include "qapi/error.h" >>> +#include "sysemu/block-backend.h" >>> + >>> +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) >>> +{ >>> +error_setg(errp, "blk '%s' not found", id); >> >> Is this expected to happen? > > Yes, if call the command from qemu-storage-daemon, where qdev-monitor is not > linked in. It happens when you try to x-blockdev-replace with "parent-type": "qdev". Correct? > Maybe, better message would be > >"devices are not supported" Best to spell out which argument is the problem. Stupidest solution that could possibly work: "Parameter 'parent-type' does not accept value 'qdev'" This is exactly what we'd get if we compiled out the parts that don't make sense for qemu-storage-daemon. > Maybe, that possible to use some 'if': notation in qapi, to not include > support for qdev into the new command, when it compiled into > qemu-storage-daemon? Seems that would not be simple, as we also need to split > compilation of the command somehow, now it compiled once both for qemu and > qemu tools.. That's precisely the problem. Our reuse of parts of qemu-system-FOO's QAPI schema for qemu-storage-daemon isn't pretty, but has worked for us so far. >>> +return NULL; >>> +} >> [...] >> QAPI schema >> Acked-by: Markus Armbruster >>
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
* Daniel P. Berrangé (berra...@redhat.com) wrote: > On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: > > Daniel P. Berrangé writes: > > > > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > >> > > x- seems safer for management tool that doesn't know about > > >> > > "unstable" properties.. > > >> > > > >> > Easy, traditional, and unreliable :) > > >> > > >> > > But on the other hand, changing from x- to no-prefix is already > > >> > > done when the feature is stable, and thouse who use it already > > >> > > use the latest version of interface, so, removing the prefix is > > >> > > just extra work. > > >> > > > >> > Exactly. > > >> > > > >> > > >> I think "x-" is still better for command line use of properties - we > > >> don't have an API to mark things unstable there, do we? > > > > > > Personally I like to see "x-" prefix present *everywhere* there is > > > an unstable feature, and consider the need to rename when declaring > > > it stable to be good thing as it sets an easily identifiable line > > > in the sand and is self-evident to outside observers. > > > > > > The self-documenting nature of the "x-" prefer is what makes it most > > > compelling to me. A patch submission, or command line invokation or > > > an example QMP command, or a bug report, that exhibit an 'x-' prefix > > > are an immediate red flag to anyone who sees them. > > > > Except when it isn't, like in "x-origin". > > > > > If someone sees a QMP comamnd / a typical giant QEMU command line, > > > they are never going to go look at the QAPI schema to check whether > > > any feature used had an 'unstable' marker. The 'unstable' marker > > > might as well not exist in most cases. > > > > > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > > > introspection, but it isn't a substitute for having an 'x-' prefix used > > > for the benefit of humans IMHO. > > > > I'm not sure there's disagreement. Quoting myself: > > > > The "x-" can remind humans "this is unstable" better than a feature > > flag can (for machines, it's the other way round). > > > > CLI and HMP are for humans. We continue to use "x-" there. > > > > QMP is for machines. The feature flag is the sole source of truth. > > Additional use of "x-" is fine, but not required. > > I guess we have different defintions of "for humans" in this context. > I consider QMP data still relevant for humans, because humans are > reviewing patches to libvirt that add usage of QMP features, or > triaging bug reports that include examples of usage, and in both > cases it is pretty relevant to make unstable features stand out to > the human via the x- prefix IMHO. Using x- for events makes sense to me; the semantics of events can be quite subtle; often you don't find out how broken they are until you wire them through libvirt and up the stack; so it's not impossible you might need to change it - but then without the x- the semantics (rather than existence) of the event is carved in stone. Dave > 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 :| > -- -Open up your eyes, open up your mind, open up your code --- / Dr. David Alan Gilbert| Running GNU/Linux | Happy \ \dave @ treblig.org | | In Hex / \ _|_ http://www.treblig.org |___/
Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command
Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben: > which will allow changing job-type-specific options after job > creation. > > In the JobVerbTable, the same allow bits as for set-speed are used, > because set-speed can be considered an existing change command. > > Signed-off-by: Fiona Ebner > Reviewed-by: Vladimir Sementsov-Ogievskiy > diff --git a/job.c b/job.c > index 72d57f0934..99a2e54b54 100644 > --- a/job.c > +++ b/job.c > @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { > [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0}, > [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, > [JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, > +[JOB_VERB_CHANGE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, > }; I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before the job has even started, but it's not necessarily a problem. The implementation just need to be careful to work even in early stages. But probably the early stages include some part of JOB_STATUS_RUNNING, too, so they'd have to do this anyway. Kevin
[PULL 73/83] vhost: move and rename the conn retry times
From: Li Feng Multiple devices need this macro, move it to a common header. Signed-off-by: Li Feng Reviewed-by: Raphael Norwitz Message-Id: <20231009044735.941655-3-fen...@smartx.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/hw/virtio/vhost.h | 2 ++ hw/block/vhost-user-blk.c | 4 +--- hw/virtio/vhost-user-gpio.c | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 00e0a669b8..5e8183f64a 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -8,6 +8,8 @@ #define VHOST_F_DEVICE_IOTLB 63 #define VHOST_USER_F_PROTOCOL_FEATURES 30 +#define VU_REALIZE_CONN_RETRIES 3 + /* Generic structures common for any vhost based device. */ struct vhost_inflight { diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index eecf3f7a81..3c69fa47d5 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -32,8 +32,6 @@ #include "sysemu/sysemu.h" #include "sysemu/runstate.h" -#define REALIZE_CONNECTION_RETRIES 3 - static const int user_feature_bits[] = { VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_SEG_MAX, @@ -482,7 +480,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->inflight = g_new0(struct vhost_inflight, 1); s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues); -retries = REALIZE_CONNECTION_RETRIES; +retries = VU_REALIZE_CONN_RETRIES; assert(!*errp); do { if (*errp) { diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c index 3d7fae3984..fc784e4213 100644 --- a/hw/virtio/vhost-user-gpio.c +++ b/hw/virtio/vhost-user-gpio.c @@ -15,7 +15,6 @@ #include "standard-headers/linux/virtio_ids.h" #include "trace.h" -#define REALIZE_CONNECTION_RETRIES 3 #define VHOST_NVQS 2 /* Features required from VirtIO */ @@ -365,7 +364,7 @@ static void vu_gpio_device_realize(DeviceState *dev, Error **errp) qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL, dev, NULL, true); -retries = REALIZE_CONNECTION_RETRIES; +retries = VU_REALIZE_CONN_RETRIES; g_assert(!*errp); do { if (*errp) { -- MST
[PULL 76/83] vhost-user: fix lost reconnect
From: Li Feng 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") Signed-off-by: Li Feng Reviewed-by: Raphael Norwitz Message-Id: <20231009044735.941655-6-fen...@smartx.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/hw/virtio/vhost-user.h | 3 ++- 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 -- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h index 9f9ddf878d..6b06ecb1bd 100644 --- a/include/hw/virtio/vhost-user.h +++ b/include/hw/virtio/vhost-user.h @@ -106,6 +106,7 @@ 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); +vu_async_close_fn cb, +IOEventHandler *event_cb); #endif 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, &s->chardev, &s->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 258fba5c69..4486500cac 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -212,7 +212,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, &vs->conf.chardev, &vsc->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 fc784e4213..aff2d7eff6 100644 --- a/hw/virtio/vhost-user-gpio.c +++ b/hw/virtio/vhost-user-gpio.c @@ -289,7 +289,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, &gpio->chardev, &gpio->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 f9414f03de..b8a7b5542d 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2756,6 +2756,7 @@ typedef struct { DeviceState *dev; CharBackend *cd; struct vhost_dev *vhost; +IOEventHandler *event_cb; } VhostAsyncCallback; static void vhost_user_async_close_bh(void *opaque) @@ -2770,7 +2771,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); } @@ -2782,7 +2786,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)) { /* @@ -2798,6 +
Re: [PATCH v3 5/9] mirror: implement mirror_change method
Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben: > which allows switching the @copy-mode from 'background' to > 'write-blocking'. > > This is useful for management applications, so they can start out in > background mode to avoid limiting guest write speed and switch to > active mode when certain criteria are fulfilled. > > In presence of an iothread, the copy_mode member is now shared between > the iothread and the main thread, so turn accesses to it atomic. > > Signed-off-by: Fiona Ebner > --- > > Changes in v3: > * turn accesses to copy_mode atomic and... > * ...slightly adapt error handling in mirror_change as a > consequence It would be good to have a comment at the field declaration that it's meant to be accessed with atomics. As we don't have further synchonisation, is the idea that during the switchover it basically doesn't matter if we read the old or the new value? After reading the whole patch, it seems that the field is only ever written under the BQL, while iothreads only read it, and only once per request (after the previous patch). This is why no further synchonisation is needed. If other threads could write it, too, mirror_change() would probably have to be more careful. As the code depends on this, adding that to the comment would be useful, too. > block/mirror.c | 33 ++--- > qapi/block-core.json | 13 - > 2 files changed, 42 insertions(+), 4 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 8992c09172..889cce5414 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error > **errp) > */ > job_transition_to_ready(&s->common.job); > } > -if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) { > +if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) { > s->actively_synced = true; > } What resets s->actively_synced when we switch away from active mode? > > @@ -1246,6 +1246,32 @@ static bool commit_active_cancel(Job *job, bool force) > return force || !job_is_ready(job); > } > > +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts, > + Error **errp) > +{ > +MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); > +BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror; > +MirrorCopyMode current; This is GLOBAL_STATE_CODE(), right? Let's be explicit about it. > + > +if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) { > +return; > +} > + > +if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) { > +error_setg(errp, "Change to copy mode '%s' is not implemented", > + MirrorCopyMode_str(change_opts->copy_mode)); > +return; > +} Ah, ok, we don't even allow the switch I was wondering about above. What would be needed, apart from removing this check, to make it work? > +current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND, > + change_opts->copy_mode); > +if (current != MIRROR_COPY_MODE_BACKGROUND) { > +error_setg(errp, "Expected current copy mode '%s', got '%s'", > + MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND), > + MirrorCopyMode_str(current)); > +} The error path is strange. We return an error, but the new mode is still set. On the other hand, this is probably also the old mode unless someone added a new value to the enum, so it didn't actually change. And because this function is the only place that changes copy_mode and we're holding the BQL, the case can't even happen and this could be an assertion. > +} > + > static const BlockJobDriver mirror_job_driver = { > .job_driver = { > .instance_size = sizeof(MirrorBlockJob), > @@ -1260,6 +1286,7 @@ static const BlockJobDriver mirror_job_driver = { > .cancel = mirror_cancel, > }, > .drained_poll = mirror_drained_poll, > +.change = mirror_change, > }; Kevin
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote: > Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben: > > From: David Woodhouse > > > > There's no need to force the user to assign a vdev. We can automatically > > assign one, starting at xvda and searching until we find the first disk > > name that's unused. > > > > This means we can now allow '-drive if=xen,file=xxx' to work without an > > explicit separate -driver argument, just like if=virtio. > > > > Signed-off-by: David Woodhouse > > Actually, how does this play together with xen_config_dev_blk()? This > looks like it tried to implement a very similar thing (which is IF_XEN > even already existed). > > Are we now trying to attach each if=xen disk twice in the 'xenpv' > machine? Or if something prevents this, is it dead code? > > I think in both cases, we would want to delete that function and remove > the loop that calls it in xen_init_pv()? I tested this with an xl config which looks a bit like this... disk = [ "backendtype=qdisk,format=qcow2,vdev=xvda,access=rw,target=/var/lib/libvirt/images/fedora28.qcow2" ] device_model_override = "/home/dwmw2/git/qemu/build/qemu-system-x86_64" device_model_version = "qemu-xen" device_model_args = [ "-trace","xen*","-chardev","pty,id=mon","-mon","mon","-drive","file=/var/lib/libvirt/images/solaris11.qcow2,format=qcow2,if=xen","-nic","user,model=xen" ] The code in xen_config_dev_blk() scribbles over the disk that the toolstack has configured for me, and adds that qcow2 file from the '-drive' option on the command line, but in *raw* mode. Then the new xen-disk support kicks in and finds a free vdev, and adds the -drive properly /dev/xvdb (as qcow2). So v2 of this patch will just rip out xen_config_dev_blk() as you suggest. smime.p7s Description: S/MIME cryptographic signature