| Message ID | 20240228152127.18769-18-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: File based migration with multifd and mapped-ram | expand |
On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote: > Allow multifd to open file-backed channels. This will be used when > enabling the mapped-ram migration stream format which expects a > seekable transport. > > The QIOChannel read and write methods will use the preadv/pwritev > versions which don't update the file offset at each call so we can > reuse the fd without re-opening for every channel. > > Contrary to the socket migration, the file migration doesn't need an > asynchronous channel creation process, so expose > multifd_channel_connect() and call it directly. > > Note that this is just setup code and multifd cannot yet make use of > the file channels. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > - moved flags change to another patch > - removed channels_created assert > --- > migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- > migration/file.h | 4 ++++ > migration/multifd.c | 18 +++++++++++++++--- > migration/multifd.h | 1 + > 4 files changed, 59 insertions(+), 5 deletions(-) > > diff --git a/migration/file.c b/migration/file.c > index 22d052a71f..83328a7a1b 100644 > --- a/migration/file.c > +++ b/migration/file.c > @@ -12,12 +12,17 @@ > #include "channel.h" > #include "file.h" > #include "migration.h" > +#include "multifd.h" > #include "io/channel-file.h" > #include "io/channel-util.h" > #include "trace.h" > > #define OFFSET_OPTION ",offset=" > > +static struct FileOutgoingArgs { > + char *fname; > +} outgoing_args; > + > /* Remove the offset option from @filespec and return it in @offsetp. */ > > int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > return 0; > } > > +void file_cleanup_outgoing_migration(void) > +{ > + g_free(outgoing_args.fname); > + outgoing_args.fname = NULL; > +} > + > +bool file_send_channel_create(gpointer opaque, Error **errp) > +{ > + QIOChannelFile *ioc; > + int flags = O_WRONLY; > + bool ret = true; > + > + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > + if (!ioc) { > + ret = false; > + goto out; > + } > + > + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > + > +out: > + /* > + * File channel creation is synchronous. However posting this > + * semaphore here is simpler than adding a special case. > + */ > + multifd_send_channel_created(); > + > + return ret; > +} > + > void file_start_outgoing_migration(MigrationState *s, > FileMigrationArgs *file_args, Error **errp) > { > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, > > trace_migration_file_outgoing(filename); > > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, > - 0600, errp); > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, > + 0660, errp); It seems this is still leftover? > if (!fioc) { > return; > } > > + outgoing_args.fname = g_strdup(filename); > + > ioc = QIO_CHANNEL(fioc); > if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { > return; > diff --git a/migration/file.h b/migration/file.h > index 37d6a08bfc..4577f9efdd 100644 > --- a/migration/file.h > +++ b/migration/file.h > @@ -9,10 +9,14 @@ > #define QEMU_MIGRATION_FILE_H > > #include "qapi/qapi-types-migration.h" > +#include "io/task.h" > +#include "channel.h" > > void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); > > void file_start_outgoing_migration(MigrationState *s, > FileMigrationArgs *file_args, Error **errp); > 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); > #endif > diff --git a/migration/multifd.c b/migration/multifd.c > index 3574fd3953..f155223303 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -17,6 +17,7 @@ > #include "exec/ramblock.h" > #include "qemu/error-report.h" > #include "qapi/error.h" > +#include "file.h" > #include "ram.h" > #include "migration.h" > #include "migration-stats.h" > @@ -28,6 +29,7 @@ > #include "threadinfo.h" > #include "options.h" > #include "qemu/yank.h" > +#include "io/channel-file.h" > #include "io/channel-socket.h" > #include "yank_functions.h" > > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > { > if (p->c) { > migration_ioc_unregister_yank(p->c); > + qio_channel_close(p->c, NULL); s/NULL/&error_abort/? > object_unref(OBJECT(p->c)); > p->c = NULL; > } > @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > static void multifd_send_cleanup_state(void) > { > + file_cleanup_outgoing_migration(); > socket_cleanup_outgoing_migration(); > qemu_sem_destroy(&multifd_send_state->channels_created); > qemu_sem_destroy(&multifd_send_state->channels_ready); > @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p, > return true; > } > > -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) > { > qio_channel_set_delay(ioc, false); > > @@ -1045,9 +1049,14 @@ out: > error_free(local_err); > } > > -static void multifd_new_send_channel_create(gpointer opaque) > +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) > { > + if (!multifd_use_packets()) { > + return file_send_channel_create(opaque, errp); > + } > + > socket_send_channel_create(multifd_new_send_channel_async, opaque); > + return true; > } > > bool multifd_send_setup(void) > @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void) > p->page_size = qemu_target_page_size(); > p->page_count = page_count; > p->write_flags = 0; > - multifd_new_send_channel_create(p); > + > + if (!multifd_new_send_channel_create(p, &local_err)) { > + return -1; "-1" is unfortunately a "true"!.. > + } > } > > /* > diff --git a/migration/multifd.h b/migration/multifd.h > index 1d8bbaf96b..db8887f088 100644 > --- a/migration/multifd.h > +++ b/migration/multifd.h > @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p) > p->iovs_num++; > } > > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc); > > #endif > -- > 2.35.3 >
On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote: > On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote: > > Allow multifd to open file-backed channels. This will be used when > > enabling the mapped-ram migration stream format which expects a > > seekable transport. > > > > The QIOChannel read and write methods will use the preadv/pwritev > > versions which don't update the file offset at each call so we can > > reuse the fd without re-opening for every channel. > > > > Contrary to the socket migration, the file migration doesn't need an > > asynchronous channel creation process, so expose > > multifd_channel_connect() and call it directly. > > > > Note that this is just setup code and multifd cannot yet make use of > > the file channels. > > > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > --- > > - moved flags change to another patch > > - removed channels_created assert > > --- > > migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- > > migration/file.h | 4 ++++ > > migration/multifd.c | 18 +++++++++++++++--- > > migration/multifd.h | 1 + > > 4 files changed, 59 insertions(+), 5 deletions(-) > > > > diff --git a/migration/file.c b/migration/file.c > > index 22d052a71f..83328a7a1b 100644 > > --- a/migration/file.c > > +++ b/migration/file.c > > @@ -12,12 +12,17 @@ > > #include "channel.h" > > #include "file.h" > > #include "migration.h" > > +#include "multifd.h" > > #include "io/channel-file.h" > > #include "io/channel-util.h" > > #include "trace.h" > > > > #define OFFSET_OPTION ",offset=" > > > > +static struct FileOutgoingArgs { > > + char *fname; > > +} outgoing_args; > > + > > /* Remove the offset option from @filespec and return it in @offsetp. */ > > > > int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > > return 0; > > } > > > > +void file_cleanup_outgoing_migration(void) > > +{ > > + g_free(outgoing_args.fname); > > + outgoing_args.fname = NULL; > > +} > > + > > +bool file_send_channel_create(gpointer opaque, Error **errp) > > +{ > > + QIOChannelFile *ioc; > > + int flags = O_WRONLY; > > + bool ret = true; > > + > > + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > > + if (!ioc) { > > + ret = false; > > + goto out; > > + } > > + > > + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > > + > > +out: > > + /* > > + * File channel creation is synchronous. However posting this > > + * semaphore here is simpler than adding a special case. > > + */ > > + multifd_send_channel_created(); > > + > > + return ret; > > +} > > + > > void file_start_outgoing_migration(MigrationState *s, > > FileMigrationArgs *file_args, Error **errp) > > { > > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, > > > > trace_migration_file_outgoing(filename); > > > > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, > > - 0600, errp); > > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, > > + 0660, errp); > > It seems this is still leftover? > > > if (!fioc) { > > return; > > } > > > > + outgoing_args.fname = g_strdup(filename); > > + > > ioc = QIO_CHANNEL(fioc); > > if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { > > return; > > diff --git a/migration/file.h b/migration/file.h > > index 37d6a08bfc..4577f9efdd 100644 > > --- a/migration/file.h > > +++ b/migration/file.h > > @@ -9,10 +9,14 @@ > > #define QEMU_MIGRATION_FILE_H > > > > #include "qapi/qapi-types-migration.h" > > +#include "io/task.h" > > +#include "channel.h" > > > > void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); > > > > void file_start_outgoing_migration(MigrationState *s, > > FileMigrationArgs *file_args, Error **errp); > > 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); > > #endif > > diff --git a/migration/multifd.c b/migration/multifd.c > > index 3574fd3953..f155223303 100644 > > --- a/migration/multifd.c > > +++ b/migration/multifd.c > > @@ -17,6 +17,7 @@ > > #include "exec/ramblock.h" > > #include "qemu/error-report.h" > > #include "qapi/error.h" > > +#include "file.h" > > #include "ram.h" > > #include "migration.h" > > #include "migration-stats.h" > > @@ -28,6 +29,7 @@ > > #include "threadinfo.h" > > #include "options.h" > > #include "qemu/yank.h" > > +#include "io/channel-file.h" > > #include "io/channel-socket.h" > > #include "yank_functions.h" > > > > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > { > > if (p->c) { > > migration_ioc_unregister_yank(p->c); > > + qio_channel_close(p->c, NULL); > > s/NULL/&error_abort/? Or we can drop this line? IIUC iochannel finalize() will always close it, or it could be a separate bug. > > > object_unref(OBJECT(p->c)); > > p->c = NULL; > > } > > @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > > > static void multifd_send_cleanup_state(void) > > { > > + file_cleanup_outgoing_migration(); > > socket_cleanup_outgoing_migration(); > > qemu_sem_destroy(&multifd_send_state->channels_created); > > qemu_sem_destroy(&multifd_send_state->channels_ready); > > @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p, > > return true; > > } > > > > -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) > > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) > > { > > qio_channel_set_delay(ioc, false); > > > > @@ -1045,9 +1049,14 @@ out: > > error_free(local_err); > > } > > > > -static void multifd_new_send_channel_create(gpointer opaque) > > +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) > > { > > + if (!multifd_use_packets()) { > > + return file_send_channel_create(opaque, errp); > > + } > > + > > socket_send_channel_create(multifd_new_send_channel_async, opaque); > > + return true; > > } > > > > bool multifd_send_setup(void) > > @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void) > > p->page_size = qemu_target_page_size(); > > p->page_count = page_count; > > p->write_flags = 0; > > - multifd_new_send_channel_create(p); > > + > > + if (!multifd_new_send_channel_create(p, &local_err)) { > > + return -1; > > "-1" is unfortunately a "true"!.. > > > + } > > } > > > > /* > > diff --git a/migration/multifd.h b/migration/multifd.h > > index 1d8bbaf96b..db8887f088 100644 > > --- a/migration/multifd.h > > +++ b/migration/multifd.h > > @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p) > > p->iovs_num++; > > } > > > > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc); > > > > #endif > > -- > > 2.35.3 > > > > -- > Peter Xu
Peter Xu <peterx@redhat.com> writes: > On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote: >> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote: >> > Allow multifd to open file-backed channels. This will be used when >> > enabling the mapped-ram migration stream format which expects a >> > seekable transport. >> > >> > The QIOChannel read and write methods will use the preadv/pwritev >> > versions which don't update the file offset at each call so we can >> > reuse the fd without re-opening for every channel. >> > >> > Contrary to the socket migration, the file migration doesn't need an >> > asynchronous channel creation process, so expose >> > multifd_channel_connect() and call it directly. >> > >> > Note that this is just setup code and multifd cannot yet make use of >> > the file channels. >> > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> >> > --- >> > - moved flags change to another patch >> > - removed channels_created assert >> > --- >> > migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- >> > migration/file.h | 4 ++++ >> > migration/multifd.c | 18 +++++++++++++++--- >> > migration/multifd.h | 1 + >> > 4 files changed, 59 insertions(+), 5 deletions(-) >> > >> > diff --git a/migration/file.c b/migration/file.c >> > index 22d052a71f..83328a7a1b 100644 >> > --- a/migration/file.c >> > +++ b/migration/file.c >> > @@ -12,12 +12,17 @@ >> > #include "channel.h" >> > #include "file.h" >> > #include "migration.h" >> > +#include "multifd.h" >> > #include "io/channel-file.h" >> > #include "io/channel-util.h" >> > #include "trace.h" >> > >> > #define OFFSET_OPTION ",offset=" >> > >> > +static struct FileOutgoingArgs { >> > + char *fname; >> > +} outgoing_args; >> > + >> > /* Remove the offset option from @filespec and return it in @offsetp. */ >> > >> > int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) >> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) >> > return 0; >> > } >> > >> > +void file_cleanup_outgoing_migration(void) >> > +{ >> > + g_free(outgoing_args.fname); >> > + outgoing_args.fname = NULL; >> > +} >> > + >> > +bool file_send_channel_create(gpointer opaque, Error **errp) >> > +{ >> > + QIOChannelFile *ioc; >> > + int flags = O_WRONLY; >> > + bool ret = true; >> > + >> > + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); >> > + if (!ioc) { >> > + ret = false; >> > + goto out; >> > + } >> > + >> > + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); >> > + >> > +out: >> > + /* >> > + * File channel creation is synchronous. However posting this >> > + * semaphore here is simpler than adding a special case. >> > + */ >> > + multifd_send_channel_created(); >> > + >> > + return ret; >> > +} >> > + >> > void file_start_outgoing_migration(MigrationState *s, >> > FileMigrationArgs *file_args, Error **errp) >> > { >> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, >> > >> > trace_migration_file_outgoing(filename); >> > >> > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, >> > - 0600, errp); >> > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, >> > + 0660, errp); >> >> It seems this is still leftover? >> >> > if (!fioc) { >> > return; >> > } >> > >> > + outgoing_args.fname = g_strdup(filename); >> > + >> > ioc = QIO_CHANNEL(fioc); >> > if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { >> > return; >> > diff --git a/migration/file.h b/migration/file.h >> > index 37d6a08bfc..4577f9efdd 100644 >> > --- a/migration/file.h >> > +++ b/migration/file.h >> > @@ -9,10 +9,14 @@ >> > #define QEMU_MIGRATION_FILE_H >> > >> > #include "qapi/qapi-types-migration.h" >> > +#include "io/task.h" >> > +#include "channel.h" >> > >> > void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); >> > >> > void file_start_outgoing_migration(MigrationState *s, >> > FileMigrationArgs *file_args, Error **errp); >> > 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); >> > #endif >> > diff --git a/migration/multifd.c b/migration/multifd.c >> > index 3574fd3953..f155223303 100644 >> > --- a/migration/multifd.c >> > +++ b/migration/multifd.c >> > @@ -17,6 +17,7 @@ >> > #include "exec/ramblock.h" >> > #include "qemu/error-report.h" >> > #include "qapi/error.h" >> > +#include "file.h" >> > #include "ram.h" >> > #include "migration.h" >> > #include "migration-stats.h" >> > @@ -28,6 +29,7 @@ >> > #include "threadinfo.h" >> > #include "options.h" >> > #include "qemu/yank.h" >> > +#include "io/channel-file.h" >> > #include "io/channel-socket.h" >> > #include "yank_functions.h" >> > >> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) >> > { >> > if (p->c) { >> > migration_ioc_unregister_yank(p->c); >> > + qio_channel_close(p->c, NULL); >> >> s/NULL/&error_abort/? > > Or we can drop this line? IIUC iochannel finalize() will always close it, > or it could be a separate bug. > We need it so the fsync happens. The finalize() will be a noop because the qio_channel_file_close() will clear the fd. Not the cleanest, but it works. >> >> > object_unref(OBJECT(p->c)); >> > p->c = NULL; >> > } >> > @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) >> > >> > static void multifd_send_cleanup_state(void) >> > { >> > + file_cleanup_outgoing_migration(); >> > socket_cleanup_outgoing_migration(); >> > qemu_sem_destroy(&multifd_send_state->channels_created); >> > qemu_sem_destroy(&multifd_send_state->channels_ready); >> > @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p, >> > return true; >> > } >> > >> > -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) >> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) >> > { >> > qio_channel_set_delay(ioc, false); >> > >> > @@ -1045,9 +1049,14 @@ out: >> > error_free(local_err); >> > } >> > >> > -static void multifd_new_send_channel_create(gpointer opaque) >> > +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) >> > { >> > + if (!multifd_use_packets()) { >> > + return file_send_channel_create(opaque, errp); >> > + } >> > + >> > socket_send_channel_create(multifd_new_send_channel_async, opaque); >> > + return true; >> > } >> > >> > bool multifd_send_setup(void) >> > @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void) >> > p->page_size = qemu_target_page_size(); >> > p->page_count = page_count; >> > p->write_flags = 0; >> > - multifd_new_send_channel_create(p); >> > + >> > + if (!multifd_new_send_channel_create(p, &local_err)) { >> > + return -1; >> >> "-1" is unfortunately a "true"!.. >> >> > + } >> > } >> > >> > /* >> > diff --git a/migration/multifd.h b/migration/multifd.h >> > index 1d8bbaf96b..db8887f088 100644 >> > --- a/migration/multifd.h >> > +++ b/migration/multifd.h >> > @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p) >> > p->iovs_num++; >> > } >> > >> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc); >> > >> > #endif >> > -- >> > 2.35.3 >> > >> >> -- >> Peter Xu
On Thu, Feb 29, 2024 at 11:27:44AM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote: > >> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote: > >> > Allow multifd to open file-backed channels. This will be used when > >> > enabling the mapped-ram migration stream format which expects a > >> > seekable transport. > >> > > >> > The QIOChannel read and write methods will use the preadv/pwritev > >> > versions which don't update the file offset at each call so we can > >> > reuse the fd without re-opening for every channel. > >> > > >> > Contrary to the socket migration, the file migration doesn't need an > >> > asynchronous channel creation process, so expose > >> > multifd_channel_connect() and call it directly. > >> > > >> > Note that this is just setup code and multifd cannot yet make use of > >> > the file channels. > >> > > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> > --- > >> > - moved flags change to another patch > >> > - removed channels_created assert > >> > --- > >> > migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- > >> > migration/file.h | 4 ++++ > >> > migration/multifd.c | 18 +++++++++++++++--- > >> > migration/multifd.h | 1 + > >> > 4 files changed, 59 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/migration/file.c b/migration/file.c > >> > index 22d052a71f..83328a7a1b 100644 > >> > --- a/migration/file.c > >> > +++ b/migration/file.c > >> > @@ -12,12 +12,17 @@ > >> > #include "channel.h" > >> > #include "file.h" > >> > #include "migration.h" > >> > +#include "multifd.h" > >> > #include "io/channel-file.h" > >> > #include "io/channel-util.h" > >> > #include "trace.h" > >> > > >> > #define OFFSET_OPTION ",offset=" > >> > > >> > +static struct FileOutgoingArgs { > >> > + char *fname; > >> > +} outgoing_args; > >> > + > >> > /* Remove the offset option from @filespec and return it in @offsetp. */ > >> > > >> > int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > >> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > >> > return 0; > >> > } > >> > > >> > +void file_cleanup_outgoing_migration(void) > >> > +{ > >> > + g_free(outgoing_args.fname); > >> > + outgoing_args.fname = NULL; > >> > +} > >> > + > >> > +bool file_send_channel_create(gpointer opaque, Error **errp) > >> > +{ > >> > + QIOChannelFile *ioc; > >> > + int flags = O_WRONLY; > >> > + bool ret = true; > >> > + > >> > + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > >> > + if (!ioc) { > >> > + ret = false; > >> > + goto out; > >> > + } > >> > + > >> > + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > >> > + > >> > +out: > >> > + /* > >> > + * File channel creation is synchronous. However posting this > >> > + * semaphore here is simpler than adding a special case. > >> > + */ > >> > + multifd_send_channel_created(); > >> > + > >> > + return ret; > >> > +} > >> > + > >> > void file_start_outgoing_migration(MigrationState *s, > >> > FileMigrationArgs *file_args, Error **errp) > >> > { > >> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, > >> > > >> > trace_migration_file_outgoing(filename); > >> > > >> > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, > >> > - 0600, errp); > >> > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, > >> > + 0660, errp); > >> > >> It seems this is still leftover? > >> > >> > if (!fioc) { > >> > return; > >> > } > >> > > >> > + outgoing_args.fname = g_strdup(filename); > >> > + > >> > ioc = QIO_CHANNEL(fioc); > >> > if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { > >> > return; > >> > diff --git a/migration/file.h b/migration/file.h > >> > index 37d6a08bfc..4577f9efdd 100644 > >> > --- a/migration/file.h > >> > +++ b/migration/file.h > >> > @@ -9,10 +9,14 @@ > >> > #define QEMU_MIGRATION_FILE_H > >> > > >> > #include "qapi/qapi-types-migration.h" > >> > +#include "io/task.h" > >> > +#include "channel.h" > >> > > >> > void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); > >> > > >> > void file_start_outgoing_migration(MigrationState *s, > >> > FileMigrationArgs *file_args, Error **errp); > >> > 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); > >> > #endif > >> > diff --git a/migration/multifd.c b/migration/multifd.c > >> > index 3574fd3953..f155223303 100644 > >> > --- a/migration/multifd.c > >> > +++ b/migration/multifd.c > >> > @@ -17,6 +17,7 @@ > >> > #include "exec/ramblock.h" > >> > #include "qemu/error-report.h" > >> > #include "qapi/error.h" > >> > +#include "file.h" > >> > #include "ram.h" > >> > #include "migration.h" > >> > #include "migration-stats.h" > >> > @@ -28,6 +29,7 @@ > >> > #include "threadinfo.h" > >> > #include "options.h" > >> > #include "qemu/yank.h" > >> > +#include "io/channel-file.h" > >> > #include "io/channel-socket.h" > >> > #include "yank_functions.h" > >> > > >> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > >> > { > >> > if (p->c) { > >> > migration_ioc_unregister_yank(p->c); > >> > + qio_channel_close(p->c, NULL); > >> > >> s/NULL/&error_abort/? > > > > Or we can drop this line? IIUC iochannel finalize() will always close it, > > or it could be a separate bug. > > > > We need it so the fsync happens. The finalize() will be a noop because > the qio_channel_file_close() will clear the fd. Not the cleanest, but it > works. It is always wise to explicitly call 'close'. If something still has a GSource watch registered against the QIOChannel, that GSource will be holding a reference on the QIOChannel and will thus prevent finalize() ever running. By calling close() you guarantee the channel is closed, even if you've mistakenly leaked a GSource somewhere. Finalize still won't run in that case, but at least the FD is gone, and the HUP might cause the GSource callback to trigger correct cleanup; With regards, Daniel
On Thu, Feb 29, 2024 at 02:34:13PM +0000, Daniel P. Berrangé wrote: > On Thu, Feb 29, 2024 at 11:27:44AM -0300, Fabiano Rosas wrote: > > Peter Xu <peterx@redhat.com> writes: > > > > > On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote: > > >> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote: > > >> > Allow multifd to open file-backed channels. This will be used when > > >> > enabling the mapped-ram migration stream format which expects a > > >> > seekable transport. > > >> > > > >> > The QIOChannel read and write methods will use the preadv/pwritev > > >> > versions which don't update the file offset at each call so we can > > >> > reuse the fd without re-opening for every channel. > > >> > > > >> > Contrary to the socket migration, the file migration doesn't need an > > >> > asynchronous channel creation process, so expose > > >> > multifd_channel_connect() and call it directly. > > >> > > > >> > Note that this is just setup code and multifd cannot yet make use of > > >> > the file channels. > > >> > > > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > >> > --- > > >> > - moved flags change to another patch > > >> > - removed channels_created assert > > >> > --- > > >> > migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- > > >> > migration/file.h | 4 ++++ > > >> > migration/multifd.c | 18 +++++++++++++++--- > > >> > migration/multifd.h | 1 + > > >> > 4 files changed, 59 insertions(+), 5 deletions(-) > > >> > > > >> > diff --git a/migration/file.c b/migration/file.c > > >> > index 22d052a71f..83328a7a1b 100644 > > >> > --- a/migration/file.c > > >> > +++ b/migration/file.c > > >> > @@ -12,12 +12,17 @@ > > >> > #include "channel.h" > > >> > #include "file.h" > > >> > #include "migration.h" > > >> > +#include "multifd.h" > > >> > #include "io/channel-file.h" > > >> > #include "io/channel-util.h" > > >> > #include "trace.h" > > >> > > > >> > #define OFFSET_OPTION ",offset=" > > >> > > > >> > +static struct FileOutgoingArgs { > > >> > + char *fname; > > >> > +} outgoing_args; > > >> > + > > >> > /* Remove the offset option from @filespec and return it in @offsetp. */ > > >> > > > >> > int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > > >> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) > > >> > return 0; > > >> > } > > >> > > > >> > +void file_cleanup_outgoing_migration(void) > > >> > +{ > > >> > + g_free(outgoing_args.fname); > > >> > + outgoing_args.fname = NULL; > > >> > +} > > >> > + > > >> > +bool file_send_channel_create(gpointer opaque, Error **errp) > > >> > +{ > > >> > + QIOChannelFile *ioc; > > >> > + int flags = O_WRONLY; > > >> > + bool ret = true; > > >> > + > > >> > + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > > >> > + if (!ioc) { > > >> > + ret = false; > > >> > + goto out; > > >> > + } > > >> > + > > >> > + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > > >> > + > > >> > +out: > > >> > + /* > > >> > + * File channel creation is synchronous. However posting this > > >> > + * semaphore here is simpler than adding a special case. > > >> > + */ > > >> > + multifd_send_channel_created(); > > >> > + > > >> > + return ret; > > >> > +} > > >> > + > > >> > void file_start_outgoing_migration(MigrationState *s, > > >> > FileMigrationArgs *file_args, Error **errp) > > >> > { > > >> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, > > >> > > > >> > trace_migration_file_outgoing(filename); > > >> > > > >> > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, > > >> > - 0600, errp); > > >> > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, > > >> > + 0660, errp); > > >> > > >> It seems this is still leftover? > > >> > > >> > if (!fioc) { > > >> > return; > > >> > } > > >> > > > >> > + outgoing_args.fname = g_strdup(filename); > > >> > + > > >> > ioc = QIO_CHANNEL(fioc); > > >> > if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { > > >> > return; > > >> > diff --git a/migration/file.h b/migration/file.h > > >> > index 37d6a08bfc..4577f9efdd 100644 > > >> > --- a/migration/file.h > > >> > +++ b/migration/file.h > > >> > @@ -9,10 +9,14 @@ > > >> > #define QEMU_MIGRATION_FILE_H > > >> > > > >> > #include "qapi/qapi-types-migration.h" > > >> > +#include "io/task.h" > > >> > +#include "channel.h" > > >> > > > >> > void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); > > >> > > > >> > void file_start_outgoing_migration(MigrationState *s, > > >> > FileMigrationArgs *file_args, Error **errp); > > >> > 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); > > >> > #endif > > >> > diff --git a/migration/multifd.c b/migration/multifd.c > > >> > index 3574fd3953..f155223303 100644 > > >> > --- a/migration/multifd.c > > >> > +++ b/migration/multifd.c > > >> > @@ -17,6 +17,7 @@ > > >> > #include "exec/ramblock.h" > > >> > #include "qemu/error-report.h" > > >> > #include "qapi/error.h" > > >> > +#include "file.h" > > >> > #include "ram.h" > > >> > #include "migration.h" > > >> > #include "migration-stats.h" > > >> > @@ -28,6 +29,7 @@ > > >> > #include "threadinfo.h" > > >> > #include "options.h" > > >> > #include "qemu/yank.h" > > >> > +#include "io/channel-file.h" > > >> > #include "io/channel-socket.h" > > >> > #include "yank_functions.h" > > >> > > > >> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > >> > { > > >> > if (p->c) { > > >> > migration_ioc_unregister_yank(p->c); > > >> > + qio_channel_close(p->c, NULL); > > >> > > >> s/NULL/&error_abort/? > > > > > > Or we can drop this line? IIUC iochannel finalize() will always close it, > > > or it could be a separate bug. > > > > > > > We need it so the fsync happens. The finalize() will be a noop because > > the qio_channel_file_close() will clear the fd. Not the cleanest, but it > > works. > > It is always wise to explicitly call 'close'. > > If something still has a GSource watch registered against > the QIOChannel, that GSource will be holding a reference > on the QIOChannel and will thus prevent finalize() ever > running. > > By calling close() you guarantee the channel is closed, > even if you've mistakenly leaked a GSource somewhere. > Finalize still won't run in that case, but at least the > FD is gone, and the HUP might cause the GSource callback > to trigger correct cleanup; I see. Let's add a comment to explain why we do this explicit close(), then? It wasn't that clear, and we also don't do that on recv side. It seems also only useful for "file:", we can mention that in the comment if so. Thanks,
diff --git a/migration/file.c b/migration/file.c index 22d052a71f..83328a7a1b 100644 --- a/migration/file.c +++ b/migration/file.c @@ -12,12 +12,17 @@ #include "channel.h" #include "file.h" #include "migration.h" +#include "multifd.h" #include "io/channel-file.h" #include "io/channel-util.h" #include "trace.h" #define OFFSET_OPTION ",offset=" +static struct FileOutgoingArgs { + char *fname; +} outgoing_args; + /* Remove the offset option from @filespec and return it in @offsetp. */ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) return 0; } +void file_cleanup_outgoing_migration(void) +{ + g_free(outgoing_args.fname); + outgoing_args.fname = NULL; +} + +bool file_send_channel_create(gpointer opaque, Error **errp) +{ + QIOChannelFile *ioc; + int flags = O_WRONLY; + bool ret = true; + + ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); + if (!ioc) { + ret = false; + goto out; + } + + multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); + +out: + /* + * File channel creation is synchronous. However posting this + * semaphore here is simpler than adding a special case. + */ + multifd_send_channel_created(); + + return ret; +} + void file_start_outgoing_migration(MigrationState *s, FileMigrationArgs *file_args, Error **errp) { @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s, trace_migration_file_outgoing(filename); - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, - 0600, errp); + fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY, + 0660, errp); if (!fioc) { return; } + outgoing_args.fname = g_strdup(filename); + ioc = QIO_CHANNEL(fioc); if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) { return; diff --git a/migration/file.h b/migration/file.h index 37d6a08bfc..4577f9efdd 100644 --- a/migration/file.h +++ b/migration/file.h @@ -9,10 +9,14 @@ #define QEMU_MIGRATION_FILE_H #include "qapi/qapi-types-migration.h" +#include "io/task.h" +#include "channel.h" void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); void file_start_outgoing_migration(MigrationState *s, FileMigrationArgs *file_args, Error **errp); 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); #endif diff --git a/migration/multifd.c b/migration/multifd.c index 3574fd3953..f155223303 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -17,6 +17,7 @@ #include "exec/ramblock.h" #include "qemu/error-report.h" #include "qapi/error.h" +#include "file.h" #include "ram.h" #include "migration.h" #include "migration-stats.h" @@ -28,6 +29,7 @@ #include "threadinfo.h" #include "options.h" #include "qemu/yank.h" +#include "io/channel-file.h" #include "io/channel-socket.h" #include "yank_functions.h" @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) { if (p->c) { migration_ioc_unregister_yank(p->c); + qio_channel_close(p->c, NULL); object_unref(OBJECT(p->c)); p->c = NULL; } @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) static void multifd_send_cleanup_state(void) { + file_cleanup_outgoing_migration(); socket_cleanup_outgoing_migration(); qemu_sem_destroy(&multifd_send_state->channels_created); qemu_sem_destroy(&multifd_send_state->channels_ready); @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p, return true; } -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc) { qio_channel_set_delay(ioc, false); @@ -1045,9 +1049,14 @@ out: error_free(local_err); } -static void multifd_new_send_channel_create(gpointer opaque) +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) { + if (!multifd_use_packets()) { + return file_send_channel_create(opaque, errp); + } + socket_send_channel_create(multifd_new_send_channel_async, opaque); + return true; } bool multifd_send_setup(void) @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void) p->page_size = qemu_target_page_size(); p->page_count = page_count; p->write_flags = 0; - multifd_new_send_channel_create(p); + + if (!multifd_new_send_channel_create(p, &local_err)) { + return -1; + } } /* diff --git a/migration/multifd.h b/migration/multifd.h index 1d8bbaf96b..db8887f088 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p) p->iovs_num++; } +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc); #endif
Allow multifd to open file-backed channels. This will be used when enabling the mapped-ram migration stream format which expects a seekable transport. The QIOChannel read and write methods will use the preadv/pwritev versions which don't update the file offset at each call so we can reuse the fd without re-opening for every channel. Contrary to the socket migration, the file migration doesn't need an asynchronous channel creation process, so expose multifd_channel_connect() and call it directly. Note that this is just setup code and multifd cannot yet make use of the file channels. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- - moved flags change to another patch - removed channels_created assert --- migration/file.c | 41 +++++++++++++++++++++++++++++++++++++++-- migration/file.h | 4 ++++ migration/multifd.c | 18 +++++++++++++++--- migration/multifd.h | 1 + 4 files changed, 59 insertions(+), 5 deletions(-)