Re: [PATCH V4 06/19] physmem: preserve ram blocks for cpr

[Peter Xu]

On Thu, Dec 12, 2024 at 03:38:14PM -0500, Steven Sistare wrote:
> On 12/9/2024 3:07 PM, Peter Xu wrote:
> > On Mon, Dec 02, 2024 at 05:19:58AM -0800, Steve Sistare wrote:
> > > Save the memfd for ramblocks in CPR state, along with a name that
> > > uniquely identifies it.  The block's idstr is not yet set, so it
> > > cannot be used for this purpose.  Find the saved memfd in new QEMU when
> > > creating a block.  If the block size is larger in new QEMU, extend the
> > > block using fallocate, and the extra space will be useable after a guest
> > > reset.
> > > 
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > ---
> > >   system/physmem.c | 36 ++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 34 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/system/physmem.c b/system/physmem.c
> > > index 0bcb2cc..aa095a3 100644
> > > --- a/system/physmem.c
> > > +++ b/system/physmem.c
> > > @@ -70,6 +70,7 @@
> > >   #include "qemu/pmem.h"
> > > +#include "migration/cpr.h"
> > >   #include "migration/vmstate.h"
> > >   #include "qemu/range.h"
> > > @@ -1661,6 +1662,19 @@ void qemu_ram_unset_idstr(RAMBlock *block)
> > >       }
> > >   }
> > > +static char *cpr_name(RAMBlock *block)
> > > +{
> > > +    MemoryRegion *mr = block->mr;
> > > +    const char *mr_name = memory_region_name(mr);
> > > +    g_autofree char *id = mr->dev ? qdev_get_dev_path(mr->dev) : NULL;
> > > +
> > > +    if (id) {
> > > +        return g_strdup_printf("%s/%s", id, mr_name);
> > > +    } else {
> > > +        return g_strdup(mr_name);
> > > +    }
> > > +}
> > > +
> > >   size_t qemu_ram_pagesize(RAMBlock *rb)
> > >   {
> > >       return rb->page_size;
> > > @@ -2080,8 +2094,18 @@ static bool qemu_ram_alloc_shared(RAMBlock 
> > > *new_block, Error **errp)
> > >   {
> > >       size_t max_length = new_block->max_length;
> > >       MemoryRegion *mr = new_block->mr;
> > > -    const char *name = memory_region_name(mr);
> > > -    int fd;
> > > +    g_autofree char *name = cpr_name(new_block);
> > > +    int fd = cpr_find_fd(name, 0);
> > 
> > If to use the proposed patch in the reply of patch 2, here this should be
> > able to be moved to qemu_ram_alloc_anonymous_fd(), IIUC.
> > 
> > > +
> > > +    if (fd >= 0) {
> > > +        if (lseek(fd, 0, SEEK_END) < max_length && ftruncate(fd, 
> > > max_length)) {
> > > +            error_setg_errno(errp, errno,
> > > +                             "cannot grow ram block %s fd %d to %ld 
> > > bytes",
> > > +                             name, fd, max_length);
> > > +            goto err;
> > > +        }
> > 
> > I remember we discussed something similar to this, do we need ftruncate()
> > at all?  I think not.
> > 
> > This happens when booting QEMU, so I don't think it's relevant yet to what
> > size used in src, as this is dest.
> > 
> > It starts to get relevant only when cpr migration starts on src, it sents
> > ramblocks at the beginning, then parse_ramblock() will properly resize any
> > ramblock to whatever size it should use.
> > 
> > If the resize didn't happen it can only mean that used_length is correctly
> > matched on both sides.
> > 
> > So I don't see why a special truncate() call is needed yet..
> 
> You suggested truncate:
> 
>   
> https://lore.kernel.org/qemu-devel/47d6d984-7002-4086-bb10-b191168f141f@oracle.com/
> 
>   "So after such system reset, QEMU might start to see new ROM code loaded
>   here (not the one that got migrated anymore, which will only match the
>   version installed on src QEMU).  Here the problem is the new firmware can
>   be larger, so I _think_ we need to make sure max_length is not modified by
>   CPR to allow resizing happen here, while if we use truncate=true here it
>   should just work in all cases."
> 
> ... but you suggested passing a truncate bool to the file_ram_alloc call after
> cpr_find_fd.  I could do that instead.  However, if qemu_ram_alloc_shared uses
> qemu_ram_alloc_from_fd instead of file_ram_alloc, per your suggestion in 
> patch 2,
> then I will still call ftruncate here, because qemu_ram_alloc_from_fd  does 
> not
> take a truncate argument.

My memory was when reuse qemu_ram_alloc_from_fd() in that suggestion of
patch 2, it will only create zero-length fd (with fsize=0) and leave all
the rest to qemu_ram_alloc_from_fd(), then with that:

qemu_ram_alloc_from_fd:
    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, offset,
                                     errp);

So that'll always have truncate==!file_size==1. Then truncate will be done
at file_ram_alloc() later, iiuc.

    if (truncate && ftruncate(fd, offset + memory)) {
        perror("ftruncate");
    }

Would this work?

Meanwhile, this whole ram resize discussion reminded me that to reuse
qemu_ram_alloc_from_fd(), we may also want to make sure to pass ->resized()
hook from qemu_ram_alloc_internal() to the fd helper too..  IOW, we may
want to keep qemu_ram_resize() invoke those hooks, even after switching to
fd-based for aux mems.

Maybe the size / max_size also need to be passed over?  As for fd ramblock
it used to be always the same on used_length/max_length, but not anymore
when we switch aux mem to fd based.  Please feel free to double check..

-- 
Peter Xu