Re: [PATCH v2 11/18] migration/multifd: Add direct-io support

[Peter Xu <peterx@redhat.com>]

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 <farosas@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