On Thu, May 23, 2024 at 04:05:41PM -0300, Fabiano Rosas wrote:
> When multifd is used along with mapped-ram, we can take benefit of a
> filesystem that supports the O_DIRECT flag and perform direct I/O in
> the multifd threads. This brings a significant performance improvement
> because direct-io writes bypass the page cache which would otherwise
> be thrashed by the multifd data which is unlikely to be needed again
> in a short period of time.
>
> To be able to use a multifd channel opened with O_DIRECT, we must
> ensure that a certain aligment is used. Filesystems usually require a
> block-size alignment for direct I/O. The way to achieve this is by
> enabling the mapped-ram feature, which already aligns its I/O properly
> (see MAPPED_RAM_FILE_OFFSET_ALIGNMENT at ram.c).
>
> By setting O_DIRECT on the multifd channels, all writes to the same
> file descriptor need to be aligned as well, even the ones that come
> from outside multifd, such as the QEMUFile I/O from the main migration
> code. This makes it impossible to use the same file descriptor for the
> QEMUFile and for the multifd channels. The various flags and metadata
> written by the main migration code will always be unaligned by virtue
> of their small size. To workaround this issue, we'll require a second
> file descriptor to be used exclusively for direct I/O.
>
> The second file descriptor can be obtained by QEMU by re-opening the
> migration file (already possible), or by being provided by the user or
> management application (support to be added in future patches).
>
> Signed-off-by: Fabiano Rosas <faro...@suse.de>
> ---
> migration/file.c | 31 ++++++++++++++++++++++++++-----
> migration/file.h | 1 -
> migration/migration.c | 23 +++++++++++++++++++++++
> 3 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/migration/file.c b/migration/file.c
> index ba5b5c44ff..ac4d206492 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -50,12 +50,31 @@ void file_cleanup_outgoing_migration(void)
> outgoing_args.fname = NULL;
> }
>
> +static void file_enable_direct_io(int *flags)
> +{
> +#ifdef O_DIRECT
> + if (migrate_direct_io()) {
> + *flags |= O_DIRECT;
> + }
> +#else
> + /* it should have been rejected when setting the parameter */
> + g_assert_not_reached();
> +#endif
> +}
> +
> bool file_send_channel_create(gpointer opaque, Error **errp)
> {
> QIOChannelFile *ioc;
> int flags = O_WRONLY;
> bool ret = true;
>
> + /*
> + * Attempt to enable O_DIRECT for the secondary channels. These
> + * are used for sending ram pages and writes should be guaranteed
> + * to be aligned to at least page size.
> + */
> + file_enable_direct_io(&flags);
Call this only if enabled? That looks clearer, IMHO:
if (migrate_direct_io()) {
file_enable_direct_io(&flags);
}
Then:
static void file_enable_direct_io(int *flags)
{
#ifdef O_DIRECT
*flags |= O_DIRECT;
#else
/* it should have been rejected when setting the parameter */
g_assert_not_reached();
#endif
}
If you remember we have similar multifd calls, and I hoped all multifd
functions are only invoked when multifd is enabled first. Same thing.
> +
> ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
> if (!ioc) {
> ret = false;
> @@ -116,21 +135,23 @@ static gboolean
> file_accept_incoming_migration(QIOChannel *ioc,
> return G_SOURCE_REMOVE;
> }
>
> -void file_create_incoming_channels(QIOChannel *ioc, Error **errp)
> +static void file_create_incoming_channels(QIOChannel *ioc, char *filename,
> + Error **errp)
> {
> - int i, fd, channels = 1;
> + int i, channels = 1;
> g_autofree QIOChannel **iocs = NULL;
> + int flags = O_RDONLY;
>
> if (migrate_multifd()) {
> channels += migrate_multifd_channels();
> + file_enable_direct_io(&flags);
Same here.
Other than that looks good.
Thanks,
> }
>
> iocs = g_new0(QIOChannel *, channels);
> - fd = QIO_CHANNEL_FILE(ioc)->fd;
> iocs[0] = ioc;
>
> for (i = 1; i < channels; i++) {
> - QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp);
> + QIOChannelFile *fioc = qio_channel_file_new_path(filename, flags, 0,
> errp);
>
> if (!fioc) {
> while (i) {
> @@ -170,7 +191,7 @@ void file_start_incoming_migration(FileMigrationArgs
> *file_args, Error **errp)
> return;
> }
>
> - file_create_incoming_channels(QIO_CHANNEL(fioc), errp);
> + file_create_incoming_channels(QIO_CHANNEL(fioc), filename, errp);
> }
>
> int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
> diff --git a/migration/file.h b/migration/file.h
> index 7699c04677..9f71e87f74 100644
> --- a/migration/file.h
> +++ b/migration/file.h
> @@ -20,7 +20,6 @@ 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_create_incoming_channels(QIOChannel *ioc, Error **errp);
> 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);
> diff --git a/migration/migration.c b/migration/migration.c
> index e1b269624c..e03c80b3aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -155,6 +155,16 @@ static bool migration_needs_seekable_channel(void)
> return migrate_mapped_ram();
> }
>
> +static bool migration_needs_extra_fds(void)
> +{
> + /*
> + * When doing direct-io, multifd requires two different,
> + * non-duplicated file descriptors so we can use one of them for
> + * unaligned IO.
> + */
> + return migrate_multifd() && migrate_direct_io();
> +}
> +
> static bool transport_supports_seeking(MigrationAddress *addr)
> {
> if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
> @@ -164,6 +174,12 @@ static bool transport_supports_seeking(MigrationAddress
> *addr)
> return false;
> }
>
> +static bool transport_supports_extra_fds(MigrationAddress *addr)
> +{
> + /* file: works because QEMU can open it multiple times */
> + return addr->transport == MIGRATION_ADDRESS_TYPE_FILE;
> +}
> +
> static bool
> migration_channels_and_transport_compatible(MigrationAddress *addr,
> Error **errp)
> @@ -180,6 +196,13 @@
> migration_channels_and_transport_compatible(MigrationAddress *addr,
> return false;
> }
>
> + if (migration_needs_extra_fds() &&
> + !transport_supports_extra_fds(addr)) {
> + error_setg(errp,
> + "Migration requires a transport that allows for extra fds
> (e.g. file)");
> + return false;
> + }
> +
> return true;
> }
>
> --
> 2.35.3
>
>
--
Peter Xu
