| Message ID | 20240918000207.182683-3-dave@treblig.org (mailing list archive) |
|---|---|
| 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.
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"}