On Fri, Sep 01, 2023 at 03:29:51PM -0300, Fabiano Rosas wrote: > Peter Xu <pet...@redhat.com> writes: > > > On Thu, Aug 31, 2023 at 03:39:16PM -0300, Fabiano Rosas wrote: > >> @@ -1166,16 +1183,9 @@ static void migrate_fd_cleanup(MigrationState *s) > >> qemu_mutex_lock_iothread(); > >> > >> multifd_save_cleanup(); > >> - qemu_mutex_lock(&s->qemu_file_lock); > >> - tmp = s->to_dst_file; > >> - s->to_dst_file = NULL; > >> - qemu_mutex_unlock(&s->qemu_file_lock); > >> - /* > >> - * Close the file handle without the lock to make sure the > >> - * critical section won't block for long. > >> - */ > >> - migration_ioc_unregister_yank_from_file(tmp); > >> - qemu_fclose(tmp); > >> + > >> + migration_ioc_unregister_yank_from_file(s->to_dst_file); > > > > I think you suggested that we should always take the file lock when > > operating on them, so this is slightly going backwards to not hold any lock > > when doing it. But doing so in migrate_fd_cleanup() is probably fine (as it > > serializes with bql on all the rest qmp commands, neither should migration > > thread exist at this point). Your call; it's still much cleaner. > > I think I was mistaken. We need the lock on the thread that clears the > pointer so that we can safely dereference it on another thread under the > lock. > > Here we're accessing it from the same thread that later does the > clearing. So that's a slightly different problem.
But this is not the only place to clear it, so you still need to justify why the other call sites (e.g., postcopy_pause() won't happen in parallel with this call site. The good thing about your proposal (of always taking that lock) is we avoid those justifications, as you said before. :) Thanks, -- Peter Xu