On Tue, Oct 04, 2022 at 11:54:02AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > Removing referencing to RAMState.f in compress_page_with_multi_thread() and
> > flush_compressed_data().
> >
> > Compression code by default isn't compatible with having >1 channels (or it
> > won't currently know which channel to flush the compressed data), so to
> > make it simple we always flush on the default to_dst_file port until
> > someone wants to add >1 ports support, as rs->f right now can really
> > change (after postcopy preempt is introduced).
> >
> > There should be no functional change at all after patch applied, since as
> > long as rs->f referenced in compression code, it must be to_dst_file.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
>
> Yes, I think that's true - although I think we need to add checks to
> stop someone trying to enable compression+multifd?
We could. I think they can be enabled and migration will just ignore the
multifd feature as ram_save_target_page() handles compress earlier. Even
if save_compress_page() fails with -EBUSY we'll still skip multifd:
if (!save_page_use_compression(rs) && migrate_use_multifd()
&& !migration_in_postcopy()) {
return ram_save_multifd_page(rs, block, offset);
}
Explicitly disable compression would be still cleaner, then we can even
drop above check on save_page_use_compression(). Slight risk of breaking
someone's config, but I don't think it should be majority.
If that looks good, I can add one patch for it (probably in the other
patchset, though, which I'll repost today).
>
> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Thanks!
--
Peter Xu
