| Message ID | 20221004182430.97638-6-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Bug fixes (prepare for preempt-full) | expand |
* 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 >
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> Reviewed-by: Juan Quintela <quintela@redhat.com>
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); }
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> --- migration/migration.c | 7 +++++++ migration/ram.c | 11 +++++------ 2 files changed, 12 insertions(+), 6 deletions(-)