Re: [PATCH] migration: ram block cpr blockers

[Steven Sistare <steven.sistare@oracle.com>]

On 1/17/2025 1:16 PM, Peter Xu wrote:
> On Fri, Jan 17, 2025 at 09:46:11AM -0800, Steve Sistare wrote:
>> +/*
>> + * Return true if ram contents would be lost during CPR.
>> + * Return false for ram_device because it is remapped in new QEMU.  Do not
>> + * exclude rom, even though it is readonly, because the rom file could change
>> + * in new QEMU.  Return false for non-migratable blocks.  They are either
>> + * re-created in new QEMU, or are handled specially, or are covered by a
>> + * device-level CPR blocker.  Return false for an fd, because it is visible and
>> + * can be remapped in new QEMU.
>> + */
>> +static bool ram_is_volatile(RAMBlock *rb)
>> +{
>> +    MemoryRegion *mr = rb->mr;
>> +
>> +    return mr &&
>> +        memory_region_is_ram(mr) &&
>> +        !memory_region_is_ram_device(mr) &&
>> +        (!qemu_ram_is_shared(rb) || !qemu_ram_is_named_file(rb)) &&
>> +        qemu_ram_is_migratable(rb) &&
>> +        rb->fd < 0;
>> +}
> 
> Blocking guest_memfd looks ok, but comparing to add one more block
> notifier, can we check all ramblocks once in migrate_prepare(), and fail
> that command directly if it fails the check?

In an upcoming patch, I will be adding an option analogous to only-migratable which
prevents QEMU from starting if anything would block cpr-transfer.  That option
will be checked when blockers are added, like for only-migratable. migrate_prepare
is too late.

> OTOH, is there any simpler way to simplify the check conditions?  It'll be
> at least nice to break these checks into smaller if conditions for
> readability..

I thought the function header comments made it clear, but I could move each
comment next to each condition:

     ...
     /*
      * Return false for an fd, because it is visible and can be remapped in
      * new QEMU.
      */
     if (rb->fd >= 0) {
         return false;
     }
     ...

> I wonder if we could stick with looping over all ramblocks, then make sure
> each of them is on the cpr saved fd list.  It may need to make
> cpr_save_fd() always register with the name of ramblock to do such lookup,
> or maybe we could also cache the ramblock pointer in CprFd, then the lookup
> will be a pointer match check.

Some ramblocks are not on the list, such as named files.  Plus looping in
migrate_prepare is too late as noted above.

IMO what I have already implemented using blockers is clean and elegant.

- Steve