Re: [PATCH v0 0/4] background snapshot

[Denis Plotnikov]

On 22.07.2020 18:42, Denis Plotnikov wrote:
> On 22.07.2020 17:50, Peter Xu wrote:
> > Hi, Denis,
> Hi, Peter
> > ...
> > > How to use:
> > > 1. enable background snapshot capability
> > >     virsh qemu-monitor-command vm --hmp migrate_set_capability 
> > > background-snapshot on
> > >
> > > 2. stop the vm
> > >     virsh qemu-monitor-command vm --hmp stop
> > >
> > > 3. Start the external migration to a file
> > >     virsh qemu-monitor-command cent78-bs --hmp migrate exec:'cat > 
> > > ./vm_state'
> > >
> > > 4. Wait for the migration finish and check that the migration has 
> > > completed state.
> > Thanks for continued working on this project! I have two high level 
> > questions
> > before dig into the patches.
> >
> > Firstly, is step 2 required?  Can we use a single QMP command to take 
> > snapshots
> > (which can still be a "migrate" command)?
>
> With this series it is required, but steps 2 and 3 should be merged 
> into a single one.
> > Meanwhile, we might also want to check around the type of backend 
> > RAM.  E.g.,
> > shmem and hugetlbfs are still not supported for uffd-wp (which I'm still
> > working on).  I didn't check explicitly whether we'll simply fail the 
> > migration
> > for those cases since the uffd ioctls will fail for those kinds of 
> > RAM.  It
> > would be okay if we handle all the ioctl failures gracefully,
>
> The ioctl's result is processed but the patch has a flaw - it ignores 
> the result of ioctl check. Need to fix it.

It happens here:

+int ram_write_tracking_start(void)
+{
+    if (page_fault_thread_start()) {
+        return -1;
+    }
+
+    ram_block_list_create();
+    ram_block_list_set_readonly(); << this returns -1 in case of failure but I 
just ignore it here
+
+    return 0;
+}

> > or it would be
> > even better if we directly fail when we want to enable live snapshot 
> > capability
> > for a guest who contains other types of ram besides private anonymous 
> > memories.
>
> I agree, but to know whether shmem or hugetlbfs are supported by the 
> current kernel we should
> execute the ioctl for all memory regions on the capability enabling.
>
> Thanks,
> Denis