On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
> d...@treblig.org writes:
>
> > From: "Dr. David Alan Gilbert" <d...@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 <d...@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
