Peter Xu <pet...@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 <pet...@redhat.com>
Reviewed-by: Juan Quintela <quint...@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.
