Re: [PATCH 04/14] migration: Remove RAMState.f references in compression code

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 04, 2022 at 11:54:02AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@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 <peterx@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 <dgilbert@redhat.com>

Thanks!

-- 
Peter Xu