Re: [PATCH v2 5/5] migration: Disable multifd explicitly with compression

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> Multifd thread model does not work for compression, explicitly disable it.
> 
> Note that previuosly even we can enable both of them, nothing will go
> wrong, because the compression code has higher priority so multifd feature
> will just be ignored.  Now we'll fail even earlier at config time so the
> user should be aware of the consequence better.
> 
> Note that there can be a slight chance of breaking existing users, but
> let's assume they're not majority and not serious users, or they should
> have found that multifd is not working already.
> 
> With that, we can safely drop the check in ram_save_target_page() for using
> multifd, because when multifd=on then compression=off, then the removed
> check on save_page_use_compression() will also always return false too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Yes, and of course if they were trying to use it, they should have been
trying to use multifd-compression parameter instead, which is different
code.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c |  7 +++++++
>  migration/ram.c       | 11 +++++------
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 844bca1ff6..ef00bff0b3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1349,6 +1349,13 @@ static bool migrate_caps_check(bool *cap_list,
>          }
>      }
>  
> +    if (cap_list[MIGRATION_CAPABILITY_MULTIFD]) {
> +        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
> +            error_setg(errp, "Multifd is not compatible with compress");
> +            return false;
> +        }
> +    }
> +
>      return true;
>  }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 1d42414ecc..1338e47665 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2305,13 +2305,12 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
>      }
>  
>      /*
> -     * Do not use multifd for:
> -     * 1. Compression as the first page in the new block should be posted out
> -     *    before sending the compressed page
> -     * 2. In postcopy as one whole host page should be placed
> +     * Do not use multifd in postcopy as one whole host page should be
> +     * placed.  Meanwhile postcopy requires atomic update of pages, so even
> +     * if host page size == guest page size the dest guest during run may
> +     * still see partially copied pages which is data corruption.
>       */
> -    if (!save_page_use_compression(rs) && migrate_use_multifd()
> -        && !migration_in_postcopy()) {
> +    if (migrate_use_multifd() && !migration_in_postcopy()) {
>          return ram_save_multifd_page(rs, block, offset);
>      }
>  
> -- 
> 2.37.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK