| Message ID | 20220719122345.253713-1-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] migration: Avoid false-positive on non-supported scenarios for zero-copy-send | expand |
* Leonardo Bras (leobras@redhat.com) wrote: > Migration with zero-copy-send currently has it's limitations, as it can't > be used with TLS nor any kind of compression. In such scenarios, it should > output errors during parameter / capability setting. > > But currently there are some ways of setting this not-supported scenarios > without printing the error message: > > !) For 'compression' capability, it works by enabling it together with > zero-copy-send. This happens because the validity test for zero-copy uses > the helper unction migrate_use_compression(), which check for compression > presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS]. > > The point here is: the validity test happens before the capability gets > enabled. If all of them get enabled together, this test will not return > error. > > In order to fix that, replace migrate_use_compression() by directly testing > the cap_list parameter migrate_caps_check(). > > 2) For features enabled by parameters such as TLS & 'multifd_compression', > there was also a possibility of setting non-supported scenarios: setting > zero-copy-send first, then setting the unsupported parameter. > > In order to fix that, also add a check for parameters conflicting with > zero-copy-send on migrate_params_check(). > > 3) XBZRLE is also a compression capability, so it makes sense to also add > it to the list of capabilities which are not supported with zero-copy-send. > > Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability") > Signed-off-by: Leonardo Bras <leobras@redhat.com> Yeh, it's unusual in that you need to check both the capabilities and parameters; where as we have the inidividual 'caps_check' and 'params_check'. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/migration.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 78f5057373..c6260e54bf 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list, > #ifdef CONFIG_LINUX > if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] && > (!cap_list[MIGRATION_CAPABILITY_MULTIFD] || > - migrate_use_compression() || > + cap_list[MIGRATION_CAPABILITY_COMPRESS] || > + cap_list[MIGRATION_CAPABILITY_XBZRLE] || > + migrate_multifd_compression() || > migrate_use_tls())) { > error_setg(errp, > "Zero copy only available for non-compressed non-TLS multifd migration"); > @@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) > error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: "); > return false; > } > + > +#ifdef CONFIG_LINUX > + if (migrate_use_zero_copy_send() && > + ((params->has_multifd_compression && params->multifd_compression) || > + (params->has_tls_creds && params->tls_creds && *params->tls_creds))) { > + error_setg(errp, > + "Zero copy only available for non-compressed non-TLS multifd migration"); > + return false; > + } > +#endif > + > return true; > } > > -- > 2.37.1 >
On Tue, Jul 19, 2022 at 09:23:45AM -0300, Leonardo Bras wrote: > Migration with zero-copy-send currently has it's limitations, as it can't > be used with TLS nor any kind of compression. In such scenarios, it should > output errors during parameter / capability setting. > > But currently there are some ways of setting this not-supported scenarios > without printing the error message: > > !) For 'compression' capability, it works by enabling it together with > zero-copy-send. This happens because the validity test for zero-copy uses > the helper unction migrate_use_compression(), which check for compression > presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS]. > > The point here is: the validity test happens before the capability gets > enabled. If all of them get enabled together, this test will not return > error. > > In order to fix that, replace migrate_use_compression() by directly testing > the cap_list parameter migrate_caps_check(). > > 2) For features enabled by parameters such as TLS & 'multifd_compression', > there was also a possibility of setting non-supported scenarios: setting > zero-copy-send first, then setting the unsupported parameter. > > In order to fix that, also add a check for parameters conflicting with > zero-copy-send on migrate_params_check(). > > 3) XBZRLE is also a compression capability, so it makes sense to also add > it to the list of capabilities which are not supported with zero-copy-send. > > Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability") > Signed-off-by: Leonardo Bras <leobras@redhat.com> > --- > migration/migration.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 78f5057373..c6260e54bf 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list, > #ifdef CONFIG_LINUX > if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] && > (!cap_list[MIGRATION_CAPABILITY_MULTIFD] || > - migrate_use_compression() || > + cap_list[MIGRATION_CAPABILITY_COMPRESS] || > + cap_list[MIGRATION_CAPABILITY_XBZRLE] || > + migrate_multifd_compression() || > migrate_use_tls())) { > error_setg(errp, > "Zero copy only available for non-compressed non-TLS multifd migration"); > @@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) > error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: "); > return false; > } > + > +#ifdef CONFIG_LINUX A trivial nit: we don't need this since migrate_use_zero_copy_send() will be defined unconditionally, and it's returning false with !CONFIG_LINUX. So feel free to drop this if there's a new version. > + if (migrate_use_zero_copy_send() && > + ((params->has_multifd_compression && params->multifd_compression) || > + (params->has_tls_creds && params->tls_creds && *params->tls_creds))) { > + error_setg(errp, > + "Zero copy only available for non-compressed non-TLS multifd migration"); > + return false; > + } > +#endif Reviewed-by: Peter Xu <peterx@redhat.com> Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 78f5057373..c6260e54bf 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list, #ifdef CONFIG_LINUX if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] && (!cap_list[MIGRATION_CAPABILITY_MULTIFD] || - migrate_use_compression() || + cap_list[MIGRATION_CAPABILITY_COMPRESS] || + cap_list[MIGRATION_CAPABILITY_XBZRLE] || + migrate_multifd_compression() || migrate_use_tls())) { error_setg(errp, "Zero copy only available for non-compressed non-TLS multifd migration"); @@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: "); return false; } + +#ifdef CONFIG_LINUX + if (migrate_use_zero_copy_send() && + ((params->has_multifd_compression && params->multifd_compression) || + (params->has_tls_creds && params->tls_creds && *params->tls_creds))) { + error_setg(errp, + "Zero copy only available for non-compressed non-TLS multifd migration"); + return false; + } +#endif + return true; }
Migration with zero-copy-send currently has it's limitations, as it can't be used with TLS nor any kind of compression. In such scenarios, it should output errors during parameter / capability setting. But currently there are some ways of setting this not-supported scenarios without printing the error message: !) For 'compression' capability, it works by enabling it together with zero-copy-send. This happens because the validity test for zero-copy uses the helper unction migrate_use_compression(), which check for compression presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS]. The point here is: the validity test happens before the capability gets enabled. If all of them get enabled together, this test will not return error. In order to fix that, replace migrate_use_compression() by directly testing the cap_list parameter migrate_caps_check(). 2) For features enabled by parameters such as TLS & 'multifd_compression', there was also a possibility of setting non-supported scenarios: setting zero-copy-send first, then setting the unsupported parameter. In order to fix that, also add a check for parameters conflicting with zero-copy-send on migrate_params_check(). 3) XBZRLE is also a compression capability, so it makes sense to also add it to the list of capabilities which are not supported with zero-copy-send. Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability") Signed-off-by: Leonardo Bras <leobras@redhat.com> --- migration/migration.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)