Re: [PATCH v2 3/4] migration/postcopy: Detect file system on dest host

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Postcopy requires the memory support userfaultfd to work.  Right now we
> check it but it's a bit too late (when switching to postcopy migration).
>
> Do that early right at enabling of postcopy.
>
> Note that this is still only a best effort because ramblocks can be
> dynamically created.  We can add check in hostmem creations and fail if
> postcopy enabled, but maybe that's too aggressive.
>
> Still, we have chance to fail the most obvious where we know there's an
> existing unsupported ramblock.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

> -static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
> +static int test_ramblock_postcopiable(RAMBlock *rb)

This should return a bool, right?
Notice that it was already there, just noticing.

>  {
>      const char *block_name = qemu_ram_get_idstr(rb);
>      ram_addr_t length = qemu_ram_get_used_length(rb);
>      size_t pagesize = qemu_ram_pagesize(rb);
> +    QemuFsType fs;

You can move the variable definition to the only block that it is used.

>      if (length % pagesize) {
>          error_report("Postcopy requires RAM blocks to be a page size 
> multiple,"
> @@ -348,6 +350,15 @@ static int test_ramblock_postcopiable(RAMBlock *rb, void 
> *opaque)
>                       "page size of 0x%zx", block_name, length, pagesize);
>          return 1;
>      }
> +
> +    if (rb->fd >= 0) {
> +        fs = qemu_fd_getfs(rb->fd);

Minor nit: Seeing it in use, I wonder if it is clearer to name the function:

qemu_fd_get_filesystem(fd)

> +        if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
> +            error_report("Host backend files need to be TMPFS or HUGETLBFS 
> only");

I think that the "only" is not needed on that error message.

> +            return 1;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -366,6 +377,7 @@ bool 
> postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>      struct uffdio_range range_struct;
>      uint64_t feature_mask;
>      Error *local_err = NULL;
> +    RAMBlock *block;
>  
>      if (qemu_target_page_size() > pagesize) {
>          error_report("Target page size bigger than host page size");
> @@ -390,9 +402,23 @@ bool 
> postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>          goto out;
>      }
>  
> -    /* We don't support postcopy with shared RAM yet */
> -    if (foreach_not_ignored_block(test_ramblock_postcopiable, NULL)) {
> -        goto out;
> +    /*
> +     * We don't support postcopy with some type of ramblocks.
> +     *
> +     * NOTE: we explicitly ignored ramblock_is_ignored() instead we checked
> +     * all possible ramblocks.  This is because this function can be called
> +     * when creating the migration object, during the phase RAM_MIGRATABLE
> +     * is not even properly set for all the ramblocks.
> +     *
> +     * A side effect of this is we'll also check against RAM_SHARED
> +     * ramblocks even if migrate_ignore_shared() is set (in which case
> +     * we'll never migrate RAM_SHARED at all), but normally this shouldn't
> +     * affect in reality, or we can revisit.
> +     */

I think we can tweak the English of that two paragraphs.

> +    RAMBLOCK_FOREACH(block) {
> +        if (test_ramblock_postcopiable(block)) {
> +            goto out;
> +        }
>      }
>  
>      /*

In the big scheme of things, patch is ok for me.

Later, Juan.