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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> 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);
>        }

Sure