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

[Fabiano Rosas]

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.