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

[Peter Xu]

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..

> +        goto have_fd;
> +    }
>  
>      if (qemu_memfd_available()) {
>          fd = qemu_memfd_create(name, max_length + mr->align, 0, 0, 0, errp);
> @@ -2111,7 +2135,9 @@ static bool qemu_ram_alloc_shared(RAMBlock *new_block, 
> Error **errp)
>              return true;
>          }
>      }
> +    cpr_save_fd(name, 0, fd);
>  
> +have_fd:
>      new_block->mr->align = QEMU_VMALLOC_ALIGN;
>      new_block->host = file_ram_alloc(new_block, max_length, fd, false, 0, 
> errp);
>  
> @@ -2122,6 +2148,8 @@ static bool qemu_ram_alloc_shared(RAMBlock *new_block, 
> Error **errp)
>          return true;
>      }
>  
> +err:
> +    cpr_delete_fd(name, 0);
>      close(fd);
>      return false;
>  }
> @@ -2234,6 +2262,8 @@ static void reclaim_ramblock(RAMBlock *block)
>  
>  void qemu_ram_free(RAMBlock *block)
>  {
> +    g_autofree char *name = NULL;
> +
>      if (!block) {
>          return;
>      }
> @@ -2244,6 +2274,8 @@ void qemu_ram_free(RAMBlock *block)
>      }
>  
>      qemu_mutex_lock_ramlist();
> +    name = cpr_name(block);
> +    cpr_delete_fd(name, 0);
>      QLIST_REMOVE_RCU(block, next);
>      ram_list.mru_block = NULL;
>      /* Write list before version */
> -- 
> 1.8.3.1
> 

-- 
Peter Xu