| Message ID | 20240918000207.182683-3-dave@treblig.org |
|---|---|
| State | New |
| Headers | show |
| Series | Migration deadcode removal | expand |
dave@treblig.org writes: > From: "Dr. David Alan Gilbert" <dave@treblig.org> > > migrate_zero_blocks is unused since > eef0bae3a7 ("migration: Remove block migration") > > Remove it. > That whole zero-blocks capability was just for old-school > block migration anyway. > > Remove the capability as well. > > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> > --- > migration/options.c | 8 -------- > migration/options.h | 1 - > qapi/migration.json | 10 +--------- > 3 files changed, 1 insertion(+), 18 deletions(-) > > diff --git a/migration/options.c b/migration/options.c > index 9460c5dee9..997e060612 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -177,7 +177,6 @@ Property migration_properties[] = { > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", Property of (pseudo-)device "migration". The "x-" prefix suggests we expect management software not to rely on it. Okay. [...] > diff --git a/qapi/migration.json b/qapi/migration.json > index b66cccf107..82d0fc962e 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -389,13 +389,6 @@ > # footprint is mlock()'d on demand or all at once. Refer to > # docs/rdma.txt for usage. Disabled by default. (since 2.0) > # > -# @zero-blocks: During storage migration encode blocks of zeroes > -# efficiently. This essentially saves 1MB of zeroes per block on > -# the wire. Enabling requires source and target VM to support > -# this feature. To enable it is sufficient to enable the > -# capability on the source VM. The feature is disabled by > -# default. (since 1.6) > -# > # @events: generate events for each migration state change (since 2.4) > # > # @auto-converge: If enabled, QEMU will automatically throttle down > @@ -483,7 +476,7 @@ > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > 'events', 'postcopy-ram', > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > 'release-ram', This is used by migrate-set-capabilities and query-migrate-capabilities, via ['MigrationCapabilityStatus']. query-migrate-capabilities is unaffected: it couldn't return zero-blocks anymore even before the patch. migrate-set-capabilities changes incompatibly, I'm afraid. Before the patch: {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} {"return": {}} Afterwards: {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} If we had somehow rejected the capability when it made no sense, removing it now it never makes sense would be obviously fine. The straight & narrow path is to deprecate now, remove later. If we believe nothing relies on it, we can bend the rules and remove right away. Missing then: update to docs/about/removed-features.rst. > @@ -542,7 +535,6 @@ > # {"state": false, "capability": "xbzrle"}, > # {"state": false, "capability": "rdma-pin-all"}, > # {"state": false, "capability": "auto-converge"}, > -# {"state": false, "capability": "zero-blocks"}, > # {"state": true, "capability": "events"}, > # {"state": false, "capability": "postcopy-ram"}, > # {"state": false, "capability": "x-colo"} Example for query-migrate-capabilities. Good.
On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote: > dave@treblig.org writes: > > > From: "Dr. David Alan Gilbert" <dave@treblig.org> > > > > migrate_zero_blocks is unused since > > eef0bae3a7 ("migration: Remove block migration") > > > > Remove it. > > That whole zero-blocks capability was just for old-school > > block migration anyway. > > > > Remove the capability as well. > > > > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> > > --- > > migration/options.c | 8 -------- > > migration/options.h | 1 - > > qapi/migration.json | 10 +--------- > > 3 files changed, 1 insertion(+), 18 deletions(-) > > > > diff --git a/migration/options.c b/migration/options.c > > index 9460c5dee9..997e060612 100644 > > --- a/migration/options.c > > +++ b/migration/options.c > > @@ -177,7 +177,6 @@ Property migration_properties[] = { > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), > > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), > > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), > > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), > > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), > > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", > > Property of (pseudo-)device "migration". The "x-" prefix suggests we > expect management software not to rely on it. Okay. > > [...] > > > diff --git a/qapi/migration.json b/qapi/migration.json > > index b66cccf107..82d0fc962e 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -389,13 +389,6 @@ > > # footprint is mlock()'d on demand or all at once. Refer to > > # docs/rdma.txt for usage. Disabled by default. (since 2.0) > > # > > -# @zero-blocks: During storage migration encode blocks of zeroes > > -# efficiently. This essentially saves 1MB of zeroes per block on > > -# the wire. Enabling requires source and target VM to support > > -# this feature. To enable it is sufficient to enable the > > -# capability on the source VM. The feature is disabled by > > -# default. (since 1.6) > > -# > > # @events: generate events for each migration state change (since 2.4) > > # > > # @auto-converge: If enabled, QEMU will automatically throttle down > > @@ -483,7 +476,7 @@ > > # Since: 1.2 > > ## > > { 'enum': 'MigrationCapability', > > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > > 'events', 'postcopy-ram', > > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > > 'release-ram', > > This is used by migrate-set-capabilities and query-migrate-capabilities, > via ['MigrationCapabilityStatus']. > > query-migrate-capabilities is unaffected: it couldn't return zero-blocks > anymore even before the patch. > > migrate-set-capabilities changes incompatibly, I'm afraid. Before the > patch: > > {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} > {"return": {}} > > Afterwards: > > {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} > > If we had somehow rejected the capability when it made no sense, > removing it now it never makes sense would be obviously fine. > > The straight & narrow path is to deprecate now, remove later. I wonder whether we can make this one simpler, as IIUC this cap depends on the block migration feature, which properly went through the deprecation process and got removed in the previous release. IOW, currently QEMU behaves the same with this cap on/off, ignoring it completely. I think it means the deprecation message (even if we provide some for two extra releases..) wouldn't be anything helpful as anyone who uses this feature already got affected before this patch.. this feature, together with block migration, are simply all gone already? > > If we believe nothing relies on it, we can bend the rules and remove > right away. Missing then: update to docs/about/removed-features.rst. Indeed, we could mention there. Thanks, > > > @@ -542,7 +535,6 @@ > > # {"state": false, "capability": "xbzrle"}, > > # {"state": false, "capability": "rdma-pin-all"}, > > # {"state": false, "capability": "auto-converge"}, > > -# {"state": false, "capability": "zero-blocks"}, > > # {"state": true, "capability": "events"}, > > # {"state": false, "capability": "postcopy-ram"}, > > # {"state": false, "capability": "x-colo"} > > Example for query-migrate-capabilities. Good. >
Peter Xu <peterx@redhat.com> writes: > On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote: >> dave@treblig.org writes: >> >> > From: "Dr. David Alan Gilbert" <dave@treblig.org> >> > >> > migrate_zero_blocks is unused since >> > eef0bae3a7 ("migration: Remove block migration") >> > >> > Remove it. >> > That whole zero-blocks capability was just for old-school >> > block migration anyway. >> > >> > Remove the capability as well. >> > >> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> >> > --- >> > migration/options.c | 8 -------- >> > migration/options.h | 1 - >> > qapi/migration.json | 10 +--------- >> > 3 files changed, 1 insertion(+), 18 deletions(-) >> > >> > diff --git a/migration/options.c b/migration/options.c >> > index 9460c5dee9..997e060612 100644 >> > --- a/migration/options.c >> > +++ b/migration/options.c >> > @@ -177,7 +177,6 @@ Property migration_properties[] = { >> > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), >> > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), >> > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), >> > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), >> > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), >> > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", >> >> Property of (pseudo-)device "migration". The "x-" prefix suggests we >> expect management software not to rely on it. Okay. >> >> [...] >> >> > diff --git a/qapi/migration.json b/qapi/migration.json >> > index b66cccf107..82d0fc962e 100644 >> > --- a/qapi/migration.json >> > +++ b/qapi/migration.json >> > @@ -389,13 +389,6 @@ >> > # footprint is mlock()'d on demand or all at once. Refer to >> > # docs/rdma.txt for usage. Disabled by default. (since 2.0) >> > # >> > -# @zero-blocks: During storage migration encode blocks of zeroes >> > -# efficiently. This essentially saves 1MB of zeroes per block on >> > -# the wire. Enabling requires source and target VM to support >> > -# this feature. To enable it is sufficient to enable the >> > -# capability on the source VM. The feature is disabled by >> > -# default. (since 1.6) >> > -# >> > # @events: generate events for each migration state change (since 2.4) >> > # >> > # @auto-converge: If enabled, QEMU will automatically throttle down >> > @@ -483,7 +476,7 @@ >> > # Since: 1.2 >> > ## >> > { 'enum': 'MigrationCapability', >> > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', >> > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', >> > 'events', 'postcopy-ram', >> > { 'name': 'x-colo', 'features': [ 'unstable' ] }, >> > 'release-ram', >> >> This is used by migrate-set-capabilities and query-migrate-capabilities, >> via ['MigrationCapabilityStatus']. >> >> query-migrate-capabilities is unaffected: it couldn't return zero-blocks >> anymore even before the patch. >> >> migrate-set-capabilities changes incompatibly, I'm afraid. Before the >> patch: >> >> {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} >> {"return": {}} >> >> Afterwards: >> >> {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} >> >> If we had somehow rejected the capability when it made no sense, >> removing it now it never makes sense would be obviously fine. >> >> The straight & narrow path is to deprecate now, remove later. > > I wonder whether we can make this one simpler, as IIUC this cap depends on > the block migration feature, which properly went through the deprecation > process and got removed in the previous release. > > IOW, currently QEMU behaves the same with this cap on/off, ignoring it > completely. I think it means the deprecation message (even if we provide > some for two extra releases..) wouldn't be anything helpful as anyone who > uses this feature already got affected before this patch.. this feature, > together with block migration, are simply all gone already? We break compatibility for users who supply capability @zero-blocks even though they are not using block migration. Before this patch, the capability is silently ignored. Afterwards, we reject it. This harmless misuse was *not* affected by our prior removal of block migration. It *is* affected by the proposed removal of the capability. We either treat this in struct accordance to our rules: deprecate now, remove later. Or we bend our them: >> If we believe nothing relies on it, we can bend the rules and remove >> right away. Not for me to decide. >> Missing then: update to docs/about/removed-features.rst. > > Indeed, we could mention there. > > Thanks, > >> >> > @@ -542,7 +535,6 @@ >> > # {"state": false, "capability": "xbzrle"}, >> > # {"state": false, "capability": "rdma-pin-all"}, >> > # {"state": false, "capability": "auto-converge"}, >> > -# {"state": false, "capability": "zero-blocks"}, >> > # {"state": true, "capability": "events"}, >> > # {"state": false, "capability": "postcopy-ram"}, >> > # {"state": false, "capability": "x-colo"} >> >> Example for query-migrate-capabilities. Good.
Markus Armbruster <armbru@redhat.com> writes: > Peter Xu <peterx@redhat.com> writes: > >> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote: >>> dave@treblig.org writes: >>> >>> > From: "Dr. David Alan Gilbert" <dave@treblig.org> >>> > >>> > migrate_zero_blocks is unused since >>> > eef0bae3a7 ("migration: Remove block migration") >>> > >>> > Remove it. >>> > That whole zero-blocks capability was just for old-school >>> > block migration anyway. >>> > >>> > Remove the capability as well. >>> > >>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> >>> > --- >>> > migration/options.c | 8 -------- >>> > migration/options.h | 1 - >>> > qapi/migration.json | 10 +--------- >>> > 3 files changed, 1 insertion(+), 18 deletions(-) >>> > >>> > diff --git a/migration/options.c b/migration/options.c >>> > index 9460c5dee9..997e060612 100644 >>> > --- a/migration/options.c >>> > +++ b/migration/options.c >>> > @@ -177,7 +177,6 @@ Property migration_properties[] = { >>> > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >>> > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), >>> > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), >>> > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), >>> > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), >>> > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), >>> > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", >>> >>> Property of (pseudo-)device "migration". The "x-" prefix suggests we >>> expect management software not to rely on it. Okay. >>> >>> [...] >>> >>> > diff --git a/qapi/migration.json b/qapi/migration.json >>> > index b66cccf107..82d0fc962e 100644 >>> > --- a/qapi/migration.json >>> > +++ b/qapi/migration.json >>> > @@ -389,13 +389,6 @@ >>> > # footprint is mlock()'d on demand or all at once. Refer to >>> > # docs/rdma.txt for usage. Disabled by default. (since 2.0) >>> > # >>> > -# @zero-blocks: During storage migration encode blocks of zeroes >>> > -# efficiently. This essentially saves 1MB of zeroes per block on >>> > -# the wire. Enabling requires source and target VM to support >>> > -# this feature. To enable it is sufficient to enable the >>> > -# capability on the source VM. The feature is disabled by >>> > -# default. (since 1.6) >>> > -# >>> > # @events: generate events for each migration state change (since 2.4) >>> > # >>> > # @auto-converge: If enabled, QEMU will automatically throttle down >>> > @@ -483,7 +476,7 @@ >>> > # Since: 1.2 >>> > ## >>> > { 'enum': 'MigrationCapability', >>> > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', >>> > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', >>> > 'events', 'postcopy-ram', >>> > { 'name': 'x-colo', 'features': [ 'unstable' ] }, >>> > 'release-ram', >>> >>> This is used by migrate-set-capabilities and query-migrate-capabilities, >>> via ['MigrationCapabilityStatus']. >>> >>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks >>> anymore even before the patch. >>> >>> migrate-set-capabilities changes incompatibly, I'm afraid. Before the >>> patch: >>> >>> {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} >>> {"return": {}} >>> >>> Afterwards: >>> >>> {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} >>> >>> If we had somehow rejected the capability when it made no sense, >>> removing it now it never makes sense would be obviously fine. >>> >>> The straight & narrow path is to deprecate now, remove later. >> >> I wonder whether we can make this one simpler, as IIUC this cap depends on >> the block migration feature, which properly went through the deprecation >> process and got removed in the previous release. >> >> IOW, currently QEMU behaves the same with this cap on/off, ignoring it >> completely. I think it means the deprecation message (even if we provide >> some for two extra releases..) wouldn't be anything helpful as anyone who >> uses this feature already got affected before this patch.. this feature, >> together with block migration, are simply all gone already? > > We break compatibility for users who supply capability @zero-blocks even > though they are not using block migration. > > Before this patch, the capability is silently ignored. > > Afterwards, we reject it. > > This harmless misuse was *not* affected by our prior removal of block > migration. > > It *is* affected by the proposed removal of the capability. How does this policy_skip thing works? Could we automatically warn whenever a capability has the 'deprecated' feature in migration.json? Also, some of the incompatibility errors in migrate_caps_check() could be simplified with something like a new: 'features': [ 'conflicts': [ 'cap1', 'cap2' ] ] to indicate which caps are incompatible between themselves. > > We either treat this in struct accordance to our rules: deprecate now, > remove later. Or we bend our them: > >>> If we believe nothing relies on it, we can bend the rules and remove >>> right away. > > Not for me to decide. > I'm fine either way, but in any case: -- >8 -- From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas <farosas@suse.de> Date: Thu, 19 Sep 2024 09:49:44 -0300 Subject: [PATCH] migration: Deprecate zero-blocks capability The zero-blocks capability was meant to be used along with the block migration, which has been removed already in commit eef0bae3a7 ("migration: Remove block migration"). Setting zero-blocks is currently a noop, but the outright removal of the capability would cause and error in case some users are still setting it. Put the capability through the deprecation process. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- docs/about/deprecated.rst | 6 ++++++ migration/options.c | 4 ++++ qapi/migration.json | 5 ++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index ed31d4b0b2..47cabb6fcc 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been deprecated in favor of explicitly using the ``file:`` URI with the file descriptor being passed as an ``fdset``. Refer to the ``add-fd`` command documentation for details on the ``fdset`` usage. + +``zero-blocks`` capability (since 9.2) +'''''''''''''''''''''''''''''''''''''' + +The ``zero-blocks`` capability was part of the block migration which +doesn't exist anymore since it was removed in QEMU v9.1. diff --git a/migration/options.c b/migration/options.c index 147cd2b8fd..b828bad0d9 100644 --- a/migration/options.c +++ b/migration/options.c @@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) ERRP_GUARD(); MigrationIncomingState *mis = migration_incoming_get_current(); + if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) { + warn_report("zero-blocks capability is deprecated"); + } + #ifndef CONFIG_REPLICATION if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { error_setg(errp, "QEMU compiled without replication module" diff --git a/qapi/migration.json b/qapi/migration.json index b66cccf107..3af6aa1740 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -479,11 +479,14 @@ # Features: # # @unstable: Members @x-colo and @x-ignore-shared are experimental. +# @deprecated: Member @zero-blocks is deprecated as being part of +# block migration which was already removed. # # Since: 1.2 ## { 'enum': 'MigrationCapability', - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', + { 'name': 'zero-blocks', 'features': [ 'deprecated' ] }, 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram',
* Fabiano Rosas (farosas@suse.de) wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Peter Xu <peterx@redhat.com> writes: > > > >> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote: > >>> dave@treblig.org writes: > >>> > >>> > From: "Dr. David Alan Gilbert" <dave@treblig.org> > >>> > > >>> > migrate_zero_blocks is unused since > >>> > eef0bae3a7 ("migration: Remove block migration") > >>> > > >>> > Remove it. > >>> > That whole zero-blocks capability was just for old-school > >>> > block migration anyway. > >>> > > >>> > Remove the capability as well. > >>> > > >>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> > >>> > --- > >>> > migration/options.c | 8 -------- > >>> > migration/options.h | 1 - > >>> > qapi/migration.json | 10 +--------- > >>> > 3 files changed, 1 insertion(+), 18 deletions(-) > >>> > > >>> > diff --git a/migration/options.c b/migration/options.c > >>> > index 9460c5dee9..997e060612 100644 > >>> > --- a/migration/options.c > >>> > +++ b/migration/options.c > >>> > @@ -177,7 +177,6 @@ Property migration_properties[] = { > >>> > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > >>> > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), > >>> > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), > >>> > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), > >>> > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), > >>> > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), > >>> > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", > >>> > >>> Property of (pseudo-)device "migration". The "x-" prefix suggests we > >>> expect management software not to rely on it. Okay. > >>> > >>> [...] > >>> > >>> > diff --git a/qapi/migration.json b/qapi/migration.json > >>> > index b66cccf107..82d0fc962e 100644 > >>> > --- a/qapi/migration.json > >>> > +++ b/qapi/migration.json > >>> > @@ -389,13 +389,6 @@ > >>> > # footprint is mlock()'d on demand or all at once. Refer to > >>> > # docs/rdma.txt for usage. Disabled by default. (since 2.0) > >>> > # > >>> > -# @zero-blocks: During storage migration encode blocks of zeroes > >>> > -# efficiently. This essentially saves 1MB of zeroes per block on > >>> > -# the wire. Enabling requires source and target VM to support > >>> > -# this feature. To enable it is sufficient to enable the > >>> > -# capability on the source VM. The feature is disabled by > >>> > -# default. (since 1.6) > >>> > -# > >>> > # @events: generate events for each migration state change (since 2.4) > >>> > # > >>> > # @auto-converge: If enabled, QEMU will automatically throttle down > >>> > @@ -483,7 +476,7 @@ > >>> > # Since: 1.2 > >>> > ## > >>> > { 'enum': 'MigrationCapability', > >>> > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > >>> > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > >>> > 'events', 'postcopy-ram', > >>> > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > >>> > 'release-ram', > >>> > >>> This is used by migrate-set-capabilities and query-migrate-capabilities, > >>> via ['MigrationCapabilityStatus']. > >>> > >>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks > >>> anymore even before the patch. > >>> > >>> migrate-set-capabilities changes incompatibly, I'm afraid. Before the > >>> patch: > >>> > >>> {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} > >>> {"return": {}} > >>> > >>> Afterwards: > >>> > >>> {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} > >>> > >>> If we had somehow rejected the capability when it made no sense, > >>> removing it now it never makes sense would be obviously fine. > >>> > >>> The straight & narrow path is to deprecate now, remove later. > >> > >> I wonder whether we can make this one simpler, as IIUC this cap depends on > >> the block migration feature, which properly went through the deprecation > >> process and got removed in the previous release. > >> > >> IOW, currently QEMU behaves the same with this cap on/off, ignoring it > >> completely. I think it means the deprecation message (even if we provide > >> some for two extra releases..) wouldn't be anything helpful as anyone who > >> uses this feature already got affected before this patch.. this feature, > >> together with block migration, are simply all gone already? > > > > We break compatibility for users who supply capability @zero-blocks even > > though they are not using block migration. > > > > Before this patch, the capability is silently ignored. > > > > Afterwards, we reject it. > > > > This harmless misuse was *not* affected by our prior removal of block > > migration. > > > > It *is* affected by the proposed removal of the capability. > > How does this policy_skip thing works? Could we automatically warn > whenever a capability has the 'deprecated' feature in migration.json? > > Also, some of the incompatibility errors in migrate_caps_check() could > be simplified with something like a new: > 'features': [ 'conflicts': [ 'cap1', 'cap2' ] ] > to indicate which caps are incompatible between themselves. > > > > > We either treat this in struct accordance to our rules: deprecate now, > > remove later. Or we bend our them: > > > >>> If we believe nothing relies on it, we can bend the rules and remove > >>> right away. > > > > Not for me to decide. > > > > I'm fine either way, but in any case: OK, so I'll split my code to just remove the dead function rather than the actual capability; Thanks for Fabiano for doing the Deprecation stuff! Dave > -- >8 -- > >From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001 > From: Fabiano Rosas <farosas@suse.de> > Date: Thu, 19 Sep 2024 09:49:44 -0300 > Subject: [PATCH] migration: Deprecate zero-blocks capability > > The zero-blocks capability was meant to be used along with the block > migration, which has been removed already in commit eef0bae3a7 > ("migration: Remove block migration"). > > Setting zero-blocks is currently a noop, but the outright removal of > the capability would cause and error in case some users are still > setting it. Put the capability through the deprecation process. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > docs/about/deprecated.rst | 6 ++++++ > migration/options.c | 4 ++++ > qapi/migration.json | 5 ++++- > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index ed31d4b0b2..47cabb6fcc 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been > deprecated in favor of explicitly using the ``file:`` URI with the > file descriptor being passed as an ``fdset``. Refer to the ``add-fd`` > command documentation for details on the ``fdset`` usage. > + > +``zero-blocks`` capability (since 9.2) > +'''''''''''''''''''''''''''''''''''''' > + > +The ``zero-blocks`` capability was part of the block migration which > +doesn't exist anymore since it was removed in QEMU v9.1. > diff --git a/migration/options.c b/migration/options.c > index 147cd2b8fd..b828bad0d9 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) > ERRP_GUARD(); > MigrationIncomingState *mis = migration_incoming_get_current(); > > + if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) { > + warn_report("zero-blocks capability is deprecated"); > + } > + > #ifndef CONFIG_REPLICATION > if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { > error_setg(errp, "QEMU compiled without replication module" > diff --git a/qapi/migration.json b/qapi/migration.json > index b66cccf107..3af6aa1740 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -479,11 +479,14 @@ > # Features: > # > # @unstable: Members @x-colo and @x-ignore-shared are experimental. > +# @deprecated: Member @zero-blocks is deprecated as being part of > +# block migration which was already removed. > # > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > + { 'name': 'zero-blocks', 'features': [ 'deprecated' ] }, > 'events', 'postcopy-ram', > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > 'release-ram', > -- > 2.35.3 >
On Thu, Sep 19, 2024 at 01:00:34PM +0000, Dr. David Alan Gilbert wrote: > * Fabiano Rosas (farosas@suse.de) wrote: > > Markus Armbruster <armbru@redhat.com> writes: > > > > > Peter Xu <peterx@redhat.com> writes: > > > > > >> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote: > > >>> dave@treblig.org writes: > > >>> > > >>> > From: "Dr. David Alan Gilbert" <dave@treblig.org> > > >>> > > > >>> > migrate_zero_blocks is unused since > > >>> > eef0bae3a7 ("migration: Remove block migration") > > >>> > > > >>> > Remove it. > > >>> > That whole zero-blocks capability was just for old-school > > >>> > block migration anyway. > > >>> > > > >>> > Remove the capability as well. > > >>> > > > >>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> > > >>> > --- > > >>> > migration/options.c | 8 -------- > > >>> > migration/options.h | 1 - > > >>> > qapi/migration.json | 10 +--------- > > >>> > 3 files changed, 1 insertion(+), 18 deletions(-) > > >>> > > > >>> > diff --git a/migration/options.c b/migration/options.c > > >>> > index 9460c5dee9..997e060612 100644 > > >>> > --- a/migration/options.c > > >>> > +++ b/migration/options.c > > >>> > @@ -177,7 +177,6 @@ Property migration_properties[] = { > > >>> > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > >>> > DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), > > >>> > DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), > > >>> > - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), > > >>> > DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), > > >>> > DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), > > >>> > DEFINE_PROP_MIG_CAP("x-postcopy-preempt", > > >>> > > >>> Property of (pseudo-)device "migration". The "x-" prefix suggests we > > >>> expect management software not to rely on it. Okay. > > >>> > > >>> [...] > > >>> > > >>> > diff --git a/qapi/migration.json b/qapi/migration.json > > >>> > index b66cccf107..82d0fc962e 100644 > > >>> > --- a/qapi/migration.json > > >>> > +++ b/qapi/migration.json > > >>> > @@ -389,13 +389,6 @@ > > >>> > # footprint is mlock()'d on demand or all at once. Refer to > > >>> > # docs/rdma.txt for usage. Disabled by default. (since 2.0) > > >>> > # > > >>> > -# @zero-blocks: During storage migration encode blocks of zeroes > > >>> > -# efficiently. This essentially saves 1MB of zeroes per block on > > >>> > -# the wire. Enabling requires source and target VM to support > > >>> > -# this feature. To enable it is sufficient to enable the > > >>> > -# capability on the source VM. The feature is disabled by > > >>> > -# default. (since 1.6) > > >>> > -# > > >>> > # @events: generate events for each migration state change (since 2.4) > > >>> > # > > >>> > # @auto-converge: If enabled, QEMU will automatically throttle down > > >>> > @@ -483,7 +476,7 @@ > > >>> > # Since: 1.2 > > >>> > ## > > >>> > { 'enum': 'MigrationCapability', > > >>> > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > > >>> > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > > >>> > 'events', 'postcopy-ram', > > >>> > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > > >>> > 'release-ram', > > >>> > > >>> This is used by migrate-set-capabilities and query-migrate-capabilities, > > >>> via ['MigrationCapabilityStatus']. > > >>> > > >>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks > > >>> anymore even before the patch. > > >>> > > >>> migrate-set-capabilities changes incompatibly, I'm afraid. Before the > > >>> patch: > > >>> > > >>> {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}} > > >>> {"return": {}} > > >>> > > >>> Afterwards: > > >>> > > >>> {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}} > > >>> > > >>> If we had somehow rejected the capability when it made no sense, > > >>> removing it now it never makes sense would be obviously fine. > > >>> > > >>> The straight & narrow path is to deprecate now, remove later. > > >> > > >> I wonder whether we can make this one simpler, as IIUC this cap depends on > > >> the block migration feature, which properly went through the deprecation > > >> process and got removed in the previous release. > > >> > > >> IOW, currently QEMU behaves the same with this cap on/off, ignoring it > > >> completely. I think it means the deprecation message (even if we provide > > >> some for two extra releases..) wouldn't be anything helpful as anyone who > > >> uses this feature already got affected before this patch.. this feature, > > >> together with block migration, are simply all gone already? > > > > > > We break compatibility for users who supply capability @zero-blocks even > > > though they are not using block migration. > > > > > > Before this patch, the capability is silently ignored. > > > > > > Afterwards, we reject it. > > > > > > This harmless misuse was *not* affected by our prior removal of block > > > migration. > > > > > > It *is* affected by the proposed removal of the capability. > > > > How does this policy_skip thing works? Could we automatically warn > > whenever a capability has the 'deprecated' feature in migration.json? > > > > Also, some of the incompatibility errors in migrate_caps_check() could > > be simplified with something like a new: > > 'features': [ 'conflicts': [ 'cap1', 'cap2' ] ] > > to indicate which caps are incompatible between themselves. > > > > > > > > We either treat this in struct accordance to our rules: deprecate now, > > > remove later. Or we bend our them: > > > > > >>> If we believe nothing relies on it, we can bend the rules and remove > > >>> right away. > > > > > > Not for me to decide. > > > > > > > I'm fine either way, but in any case: > > OK, so I'll split my code to just remove the dead function rather than the > actual capability; Thanks for Fabiano for doing the Deprecation stuff! I'll wait for a final ACK from Markus, then I can queue below patch directly into migration-next. Thanks all! > > Dave > > > -- >8 -- > > >From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001 > > From: Fabiano Rosas <farosas@suse.de> > > Date: Thu, 19 Sep 2024 09:49:44 -0300 > > Subject: [PATCH] migration: Deprecate zero-blocks capability > > > > The zero-blocks capability was meant to be used along with the block > > migration, which has been removed already in commit eef0bae3a7 > > ("migration: Remove block migration"). > > > > Setting zero-blocks is currently a noop, but the outright removal of > > the capability would cause and error in case some users are still > > setting it. Put the capability through the deprecation process. > > > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > --- > > docs/about/deprecated.rst | 6 ++++++ > > migration/options.c | 4 ++++ > > qapi/migration.json | 5 ++++- > > 3 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > > index ed31d4b0b2..47cabb6fcc 100644 > > --- a/docs/about/deprecated.rst > > +++ b/docs/about/deprecated.rst > > @@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been > > deprecated in favor of explicitly using the ``file:`` URI with the > > file descriptor being passed as an ``fdset``. Refer to the ``add-fd`` > > command documentation for details on the ``fdset`` usage. > > + > > +``zero-blocks`` capability (since 9.2) > > +'''''''''''''''''''''''''''''''''''''' > > + > > +The ``zero-blocks`` capability was part of the block migration which > > +doesn't exist anymore since it was removed in QEMU v9.1. > > diff --git a/migration/options.c b/migration/options.c > > index 147cd2b8fd..b828bad0d9 100644 > > --- a/migration/options.c > > +++ b/migration/options.c > > @@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) > > ERRP_GUARD(); > > MigrationIncomingState *mis = migration_incoming_get_current(); > > > > + if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) { > > + warn_report("zero-blocks capability is deprecated"); > > + } > > + > > #ifndef CONFIG_REPLICATION > > if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { > > error_setg(errp, "QEMU compiled without replication module" > > diff --git a/qapi/migration.json b/qapi/migration.json > > index b66cccf107..3af6aa1740 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -479,11 +479,14 @@ > > # Features: > > # > > # @unstable: Members @x-colo and @x-ignore-shared are experimental. > > +# @deprecated: Member @zero-blocks is deprecated as being part of > > +# block migration which was already removed. > > # > > # Since: 1.2 > > ## > > { 'enum': 'MigrationCapability', > > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', > > + { 'name': 'zero-blocks', 'features': [ 'deprecated' ] }, > > 'events', 'postcopy-ram', > > { 'name': 'x-colo', 'features': [ 'unstable' ] }, > > 'release-ram', > > -- > > 2.35.3 > > > -- > -----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 |_______/ >
diff --git a/migration/options.c b/migration/options.c index 9460c5dee9..997e060612 100644 --- a/migration/options.c +++ b/migration/options.c @@ -177,7 +177,6 @@ Property migration_properties[] = { DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL), DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE), - DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS), DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS), DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM), DEFINE_PROP_MIG_CAP("x-postcopy-preempt", @@ -339,13 +338,6 @@ bool migrate_xbzrle(void) return s->capabilities[MIGRATION_CAPABILITY_XBZRLE]; } -bool migrate_zero_blocks(void) -{ - MigrationState *s = migrate_get_current(); - - return s->capabilities[MIGRATION_CAPABILITY_ZERO_BLOCKS]; -} - bool migrate_zero_copy_send(void) { MigrationState *s = migrate_get_current(); diff --git a/migration/options.h b/migration/options.h index 36e7b3723f..79084eed0d 100644 --- a/migration/options.h +++ b/migration/options.h @@ -40,7 +40,6 @@ bool migrate_release_ram(void); bool migrate_return_path(void); bool migrate_validate_uuid(void); bool migrate_xbzrle(void); -bool migrate_zero_blocks(void); bool migrate_zero_copy_send(void); /* diff --git a/qapi/migration.json b/qapi/migration.json index b66cccf107..82d0fc962e 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -389,13 +389,6 @@ # footprint is mlock()'d on demand or all at once. Refer to # docs/rdma.txt for usage. Disabled by default. (since 2.0) # -# @zero-blocks: During storage migration encode blocks of zeroes -# efficiently. This essentially saves 1MB of zeroes per block on -# the wire. Enabling requires source and target VM to support -# this feature. To enable it is sufficient to enable the -# capability on the source VM. The feature is disabled by -# default. (since 1.6) -# # @events: generate events for each migration state change (since 2.4) # # @auto-converge: If enabled, QEMU will automatically throttle down @@ -483,7 +476,7 @@ # Since: 1.2 ## { 'enum': 'MigrationCapability', - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', @@ -542,7 +535,6 @@ # {"state": false, "capability": "xbzrle"}, # {"state": false, "capability": "rdma-pin-all"}, # {"state": false, "capability": "auto-converge"}, -# {"state": false, "capability": "zero-blocks"}, # {"state": true, "capability": "events"}, # {"state": false, "capability": "postcopy-ram"}, # {"state": false, "capability": "x-colo"}