Re: [PATCH] migration: ram block cpr blockers

[Steven Sistare]

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