Re: [PATCH v5 8/8] migration: Add a wrapper to cleanup migration files

[Peter Xu <peterx@redhat.com>]

On Fri, Sep 01, 2023 at 03:29:51PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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