Peter Xu <pet...@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 <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);
> }
Sure
