[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] migration: Fix iocs leaks during file and fd migration
|
From: |
Fabiano Rosas |
|
Subject: |
Re: [PATCH] migration: Fix iocs leaks during file and fd migration |
|
Date: |
Wed, 13 Mar 2024 12:11:17 -0300 |
Peter Xu <peterx@redhat.com> writes:
> On Wed, Mar 13, 2024 at 10:50:18AM -0300, Fabiano Rosas wrote:
>> The memory for the io channels is being leaked in three different ways
>> during file migration:
>>
>> 1) if the offset check fails we never drop the ioc reference;
>>
>> 2) if multifd is not enabled, we allocate an extra channel for no
>> reason;
>
> This leak should also happen even if multifd enabled; it just always leak
> one unconditionally..
>
>>
>> 3) if multifd is enabled but channel creation fails when calling
>> dup(), we leave the previous channels around along with the glib
>> polling;
>>
>> Fix all issues by restructuring the code to first allocate the
>> channels and only register the watches when all channels have been
>> created.
>>
>> The file and fd migrations can share this code because both are backed
>> by the file migration infrastructure.
>>
>> Fixes: 2dd7ee7a51 ("migration/multifd: Add incoming QIOChannelFile support")
>> Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
>> Reported-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> based-on: https://gitlab.com/peterx/migration-stable
>
> Invalid link. :)
lol, all kinds of wrong.
The correct one would be:
https://gitlab.com/peterx/qemu/-/commits/migration-stable
>
> I am not sure whether patchew will understand this even if the link is
> correct, but I hope!
I was thinking more of helping reviewers than patchew actually.
>
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1211909831
>> ---
>> migration/fd.c | 28 +++-------------------
>> migration/file.c | 62 +++++++++++++++++++++++++++++++-----------------
>> migration/file.h | 3 +++
>> 3 files changed, 46 insertions(+), 47 deletions(-)
>>
>> diff --git a/migration/fd.c b/migration/fd.c
>> index 4e2a63a73d..b74a3eb8c8 100644
>> --- a/migration/fd.c
>> +++ b/migration/fd.c
>> @@ -18,6 +18,7 @@
>> #include "qapi/error.h"
>> #include "channel.h"
>> #include "fd.h"
>> +#include "file.h"
>> #include "migration.h"
>> #include "monitor/monitor.h"
>> #include "io/channel-file.h"
>> @@ -68,19 +69,9 @@ void fd_start_outgoing_migration(MigrationState *s, const
>> char *fdname, Error **
>> object_unref(OBJECT(ioc));
>> }
>>
>> -static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
>> - GIOCondition condition,
>> - gpointer opaque)
>> -{
>> - migration_channel_process_incoming(ioc);
>> - object_unref(OBJECT(ioc));
>> - return G_SOURCE_REMOVE;
>> -}
>> -
>> void fd_start_incoming_migration(const char *fdname, Error **errp)
>> {
>> QIOChannel *ioc;
>> - QIOChannelFile *fioc;
>> int fd = monitor_fd_param(monitor_cur(), fdname, errp);
>> if (fd == -1) {
>> return;
>> @@ -96,24 +87,11 @@ void fd_start_incoming_migration(const char *fdname,
>> Error **errp)
>>
>> qio_channel_set_name(ioc, "migration-fd-incoming");
>> qio_channel_add_watch_full(ioc, G_IO_IN,
>> - fd_accept_incoming_migration,
>> + file_accept_incoming_migration,
>> NULL, NULL,
>> g_main_context_get_thread_default());
>
> Mostly correct, but IIUC we'll still leak this main io watch if below
> failed, similar issue for file path below. We may need to manage all IOCs
> including the main one.
>
Yes, good point. Sigh... I'll get it right eventually.
>>
>> if (migrate_multifd()) {
>> - int channels = migrate_multifd_channels();
>> -
>> - while (channels--) {
>> - fioc = qio_channel_file_new_dupfd(fd, errp);
>> - if (!fioc) {
>> - return;
>> - }
>> -
>> - qio_channel_set_name(ioc, "migration-fd-incoming");
>> - qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN,
>> - fd_accept_incoming_migration,
>> - NULL, NULL,
>> - g_main_context_get_thread_default());
>> - }
>> + file_recv_channels_create(fd, errp);
>> }
>> }
>> diff --git a/migration/file.c b/migration/file.c
>> index e56c5eb0a5..bb264115eb 100644
>> --- a/migration/file.c
>> +++ b/migration/file.c
>> @@ -106,22 +106,48 @@ void file_start_outgoing_migration(MigrationState *s,
>> migration_channel_connect(s, ioc, NULL, NULL);
>> }
>>
>> -static gboolean file_accept_incoming_migration(QIOChannel *ioc,
>> - GIOCondition condition,
>> - gpointer opaque)
>> +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition
>> condition,
>> + gpointer opaque)
>> {
>> migration_channel_process_incoming(ioc);
>> object_unref(OBJECT(ioc));
>> return G_SOURCE_REMOVE;
>> }
>>
>> +void file_recv_channels_create(int fd, Error **errp)
>> +{
>> + int i, channels = migrate_multifd_channels();
>> + g_autofree QIOChannelFile **fiocs = g_new0(QIOChannelFile *, channels);
>> +
>> + for (i = 0; i < channels; i++) {
>> + QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp);
>> +
>> + if (!fioc) {
>> + while (i) {
>> + object_unref(fiocs[--i]);
>> + }
>> + return;
>> + }
>> +
>> + fiocs[i] = fioc;
>> + }
>> +
>> + for (i = 0; i < channels; i++) {
>> + QIOChannelFile *fioc = fiocs[i];
>> +
>> + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming");
>> + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN,
>> + file_accept_incoming_migration,
>> + NULL, NULL,
>> + g_main_context_get_thread_default());
>> + }
>> +}
>> +
>> void file_start_incoming_migration(FileMigrationArgs *file_args, Error
>> **errp)
>> {
>> g_autofree char *filename = g_strdup(file_args->filename);
>> QIOChannelFile *fioc = NULL;
>> uint64_t offset = file_args->offset;
>> - int channels = 1;
>> - int i = 0;
>>
>> trace_migration_file_incoming(filename);
>>
>> @@ -132,28 +158,20 @@ void file_start_incoming_migration(FileMigrationArgs
>> *file_args, Error **errp)
>>
>> if (offset &&
>> qio_channel_io_seek(QIO_CHANNEL(fioc), offset, SEEK_SET, errp) < 0)
>> {
>> + object_unref(OBJECT(fioc));
>> return;
>> }
>>
>> + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming");
>> + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN,
>> + file_accept_incoming_migration,
>> + NULL, NULL,
>> + g_main_context_get_thread_default());
>> +
>> +
>> if (migrate_multifd()) {
>> - channels += migrate_multifd_channels();
>> + file_recv_channels_create(fioc->fd, errp);
>> }
>> -
>> - do {
>> - QIOChannel *ioc = QIO_CHANNEL(fioc);
>> -
>> - qio_channel_set_name(ioc, "migration-file-incoming");
>> - qio_channel_add_watch_full(ioc, G_IO_IN,
>> - file_accept_incoming_migration,
>> - NULL, NULL,
>> - g_main_context_get_thread_default());
>> -
>> - fioc = qio_channel_file_new_dupfd(fioc->fd, errp);
>> -
>> - if (!fioc) {
>> - break;
>> - }
>> - } while (++i < channels);
>> }
>>
>> int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
>> diff --git a/migration/file.h b/migration/file.h
>> index 9f71e87f74..940122db0f 100644
>> --- a/migration/file.h
>> +++ b/migration/file.h
>> @@ -20,6 +20,9 @@ void file_start_outgoing_migration(MigrationState *s,
>> int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
>> void file_cleanup_outgoing_migration(void);
>> bool file_send_channel_create(gpointer opaque, Error **errp);
>> +void file_recv_channels_create(int fd, Error **errp);
>> +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition
>> condition,
>> + gpointer opaque);
>> int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
>> int niov, RAMBlock *block, Error **errp);
>> int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp);
>> --
>> 2.35.3
>>