Commits on Sep 27, 2023

1. migration: Fix race that dest preempt thread close too early …

   We hit intermit CI issue on failing at migration-test over the unit test
   preempt/plain:
   
   qemu-system-x86_64: Unable to read from socket: Connection reset by peer
   Memory content inconsistency at 5b43000 first_byte = bd last_byte = bc current = 4f hit_edge = 1
   **
   ERROR:../tests/qtest/migration-test.c:300:check_guests_ram: assertion failed: (bad == 0)
   (test program exited with status code -6)
   
   Fabiano debugged into it and found that the preempt thread can quit even
   without receiving all the pages, which can cause guest not receiving all
   the pages and corrupt the guest memory.
   
   To make sure preempt thread finished receiving all the pages, we can rely
   on the page_requested_count being zero because preempt channel will only
   receive requested page faults. Note, not all the faulted pages are required
   to be sent via the preempt channel/thread; imagine the case when a
   requested page is just queued into the background main channel for
   migration, the src qemu will just still send it via the background channel.
   
   Here instead of spinning over reading the count, we add a condvar so the
   main thread can wait on it if that unusual case happened, without burning
   the cpu for no good reason, even if the duration is short; so even if we
   spin in this rare case is probably fine.  It's just better to not do so.
   
   The condvar is only used when that special case is triggered.  Some memory
   ordering trick is needed to guarantee it from happening (against the
   preempt thread status field), so the main thread will always get a kick
   when that triggers correctly.
   
   Closes: https://gitlab.com/qemu-project/qemu/-/issues/1886
   Debugged-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-2-farosas@suse.de>

   

   xzpeter authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   cf02f29 View commit details

   Browse the repository at this point in the history

2. migration: Fix possible race when setting rp_state.error …

   We don't need to set the rp_state.error right after a shutdown because
   qemu_file_shutdown() always sets the QEMUFile error, so the return
   path thread would have seen it and set the rp error itself.
   
   Setting the error outside of the thread is also racy because the
   thread could clear it after we set it.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-3-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   28a8347 View commit details

   Browse the repository at this point in the history

3. migration: Fix possible races when shutting down the return path …

   We cannot call qemu_file_shutdown() on the return path file without
   taking the file lock. The return path thread could be running it's
   cleanup code and have just cleared the from_dst_file pointer.
   
   Checking ms->to_dst_file for errors could also race with
   migrate_fd_cleanup() which clears the to_dst_file pointer.
   
   Protect both accesses by taking the file lock.
   
   This was caught by inspection, it should be rare, but the next patches
   will start calling this code from other places, so let's do the
   correct thing.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-4-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   639decf View commit details

   Browse the repository at this point in the history

4. migration: Fix possible race when shutting down to_dst_file …

   It's not safe to call qemu_file_shutdown() on the to_dst_file without
   first checking for the file's presence under the lock. The cleanup of
   this file happens at postcopy_pause() and migrate_fd_cleanup() which
   are not necessarily running in the same thread as migrate_fd_cancel().
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-5-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   7478fb0 View commit details

   Browse the repository at this point in the history

5. migration: Remove redundant cleanup of postcopy_qemufile_src …

   This file is owned by the return path thread which is already doing
   cleanup.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-6-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   b3b1011 View commit details

   Browse the repository at this point in the history

6. migration: Consolidate return path closing code …

   We'll start calling the await_return_path_close_on_source() function
   from other parts of the code, so move all of the related checks and
   tracepoints into it.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-7-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   d50f5dc View commit details

   Browse the repository at this point in the history

7. migration: Replace the return path retry logic …

   Replace the return path retry logic with finishing and restarting the
   thread. This fixes a race when resuming the migration that leads to a
   segfault.
   
   Currently when doing postcopy we consider that an IO error on the
   return path file could be due to a network intermittency. We then keep
   the thread alive but have it do cleanup of the 'from_dst_file' and
   wait on the 'postcopy_pause_rp' semaphore. When the user issues a
   migrate resume, a new return path is opened and the thread is allowed
   to continue.
   
   There's a race condition in the above mechanism. It is possible for
   the new return path file to be setup *before* the cleanup code in the
   return path thread has had a chance to run, leading to the *new* file
   being closed and the pointer set to NULL. When the thread is released
   after the resume, it tries to dereference 'from_dst_file' and crashes:
   
   Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
   [Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
   0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
   154         return f->last_error;
   
   (gdb) bt
    #0  0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
    #1  0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206
    #2  0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876
    #3  0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541
    #4  0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477
    #5  0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
   
   Here's the race (important bit is open_return_path happening before
   migration_release_dst_files):
   
   migration                 | qmp                         | return path
   --------------------------+-----------------------------+---------------------------------
   			    qmp_migrate_pause()
   			     shutdown(ms->to_dst_file)
   			      f->last_error = -EIO
   migrate_detect_error()
    postcopy_pause()
     set_state(PAUSED)
     wait(postcopy_pause_sem)
   			    qmp_migrate(resume)
   			    migrate_fd_connect()
   			     resume = state == PAUSED
   			     open_return_path <-- TOO SOON!
   			     set_state(RECOVER)
   			     post(postcopy_pause_sem)
   							(incoming closes to_src_file)
   							res = qemu_file_get_error(rp)
   							migration_release_dst_files()
   							ms->rp_state.from_dst_file = NULL
     post(postcopy_pause_rp_sem)
   							postcopy_pause_return_path_thread()
   							  wait(postcopy_pause_rp_sem)
   							rp = ms->rp_state.from_dst_file
   							goto retry
   							qemu_file_get_error(rp)
   							SIGSEGV
   -------------------------------------------------------------------------------------------
   
   We can keep the retry logic without having the thread alive and
   waiting. The only piece of data used by it is the 'from_dst_file' and
   it is only allowed to proceed after a migrate resume is issued and the
   semaphore released at migrate_fd_connect().
   
   Move the retry logic to outside the thread by waiting for the thread
   to finish before pausing the migration.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-8-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   ef796ee View commit details

   Browse the repository at this point in the history

8. migration: Move return path cleanup to main migration thread …

   Now that the return path thread is allowed to finish during a paused
   migration, we can move the cleanup of the QEMUFiles to the main
   migration thread.
   
   Reviewed-by: Peter Xu <peterx@redhat.com>
   Signed-off-by: Fabiano Rosas <farosas@suse.de>
   Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
   Message-ID: <20230918172822.19052-9-farosas@suse.de>

   Fabiano Rosas authored and Stefan Hajnoczi committed Sep 27, 2023
   Copy the full SHA

   36e9aab View commit details

   Browse the repository at this point in the history