From: Fabiano Rosas <farosas@suse.de>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org, berrange@redhat.com, armbru@redhat.com,
Claudio Fontana <cfontana@suse.de>, Jim Fehlig <jfehlig@suse.com>
Subject: Re: [PATCH 5/9] migration/multifd: Add direct-io support
Date: Fri, 03 May 2024 17:54:28 -0300 [thread overview]
Message-ID: <87y18qmxa3.fsf@suse.de> (raw)
In-Reply-To: <ZjUs-2mhVDrPd3Kl@x1n>
Peter Xu <peterx@redhat.com> writes:
> On Fri, Apr 26, 2024 at 11:20:38AM -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 | 22 +++++++++++++++++++---
>> migration/migration.c | 23 +++++++++++++++++++++++
>> 2 files changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/file.c b/migration/file.c
>> index 8f30999400..b9265b14dd 100644
>> --- a/migration/file.c
>> +++ b/migration/file.c
>> @@ -83,17 +83,33 @@ void file_cleanup_outgoing_migration(void)
>>
>> bool file_send_channel_create(gpointer opaque, Error **errp)
>> {
>> - QIOChannelFile *ioc;
>> + QIOChannelFile *ioc = NULL;
>> int flags = O_WRONLY;
>> - bool ret = true;
>> + bool ret = false;
>> +
>> + if (migrate_direct_io()) {
>> +#ifdef O_DIRECT
>> + /*
>> + * 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.
>> + */
>> + flags |= O_DIRECT;
>> +#else
>> + error_setg(errp, "System does not support O_DIRECT");
>> + error_append_hint(errp,
>> + "Try disabling direct-io migration capability\n");
>> + goto out;
>> +#endif
>
> Hopefully if we can fail migrate-set-parameters correctly always, we will
> never trigger this error.
>
> I know Linux used some trick like this to even avoid such ifdefs:
>
> if (qemu_has_direct_io() && migrate_direct_io()) {
> // reference O_DIRECT
> }
>
> So as long as qemu_has_direct_io() can return a constant "false" when
> O_DIRECT not defined, the compiler is smart enough to ignore the O_DIRECT
> inside the block.
>
> Even if it won't work, we can still avoid that error (and rely on the
> set-parameter failure):
>
> #ifdef O_DIRECT
> if (migrate_direct_io()) {
> // reference O_DIRECT
> }
> #endif
>
> Then it should run the same, just to try making ifdefs as light as
> possible..
Ok.
Just FYI, in v2 I'm adding direct-io to migration incoming side as well,
so I put this logic into a helper:
static bool file_enable_direct_io(int *flags, Error **errp)
{
if (migrate_direct_io()) {
#ifdef O_DIRECT
*flags |= O_DIRECT;
#else
error_setg(errp, "System does not support O_DIRECT");
error_append_hint(errp,
"Try disabling direct-io migration capability\n");
return false;
#endif
}
return true;
}
But I'll apply your suggestions nonetheless.
>
>> + }
>>
>> 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));
>> + ret = true;
>>
>> out:
>> /*
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b5af6b5105..cb923a3f62 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_multiple_fds(void)
>
> If I suggest to rename this, would you agree? :)
>
Sure, although this is the more accurate usage than "multifd" hehe.
> I'd try with "migrate_needs_extra_fd()" or "migrate_needs_two_fds()",
> or... just to avoid "multi" + "fd" used altogether, perhaps.
>
> Other than that looks all good.
>
> Thanks,
>
>> +{
>> + /*
>> + * 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_multiple_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_multiple_fds() &&
>> + !transport_supports_multiple_fds(addr)) {
>> + error_setg(errp,
>> + "Migration requires a transport that allows for multiple fds (e.g. file)");
>> + return false;
>> + }
>> +
>> return true;
>> }
>>
>> --
>> 2.35.3
>>
next prev parent reply other threads:[~2024-05-03 20:55 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 14:20 [PATCH 0/9] migration/mapped-ram: Add direct-io support Fabiano Rosas
2024-04-26 14:20 ` [PATCH 1/9] monitor: Honor QMP request for fd removal immediately Fabiano Rosas
2024-05-03 16:02 ` Peter Xu
2024-05-16 21:46 ` Fabiano Rosas
2024-05-08 7:17 ` Daniel P. Berrangé
2024-05-16 22:00 ` Fabiano Rosas
2024-05-17 7:33 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 2/9] migration: Fix file migration with fdset Fabiano Rosas
2024-05-03 16:23 ` Peter Xu
2024-05-03 19:56 ` Fabiano Rosas
2024-05-03 21:04 ` Peter Xu
2024-05-03 21:31 ` Fabiano Rosas
2024-05-03 21:56 ` Peter Xu
2024-05-08 8:02 ` Daniel P. Berrangé
2024-05-08 12:49 ` Peter Xu
2024-05-08 8:00 ` Daniel P. Berrangé
2024-05-08 20:45 ` Fabiano Rosas
2024-04-26 14:20 ` [PATCH 3/9] tests/qtest/migration: Fix file migration offset check Fabiano Rosas
2024-05-03 16:47 ` Peter Xu
2024-05-03 20:36 ` Fabiano Rosas
2024-05-03 21:08 ` Peter Xu
2024-05-08 8:10 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 4/9] migration: Add direct-io parameter Fabiano Rosas
2024-04-26 14:33 ` Markus Armbruster
2024-05-03 18:05 ` Peter Xu
2024-05-03 20:49 ` Fabiano Rosas
2024-05-03 21:16 ` Peter Xu
2024-05-14 14:10 ` Markus Armbruster
2024-05-14 17:57 ` Fabiano Rosas
2024-05-15 7:17 ` Markus Armbruster
2024-05-15 12:51 ` Fabiano Rosas
2024-05-08 8:25 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 5/9] migration/multifd: Add direct-io support Fabiano Rosas
2024-05-03 18:29 ` Peter Xu
2024-05-03 20:54 ` Fabiano Rosas [this message]
2024-05-03 21:18 ` Peter Xu
2024-05-08 8:27 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 6/9] tests/qtest/migration: Add tests for file migration with direct-io Fabiano Rosas
2024-05-03 18:38 ` Peter Xu
2024-05-03 21:05 ` Fabiano Rosas
2024-05-03 21:25 ` Peter Xu
2024-05-08 8:34 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 7/9] monitor: fdset: Match against O_DIRECT Fabiano Rosas
2024-05-03 18:53 ` Peter Xu
2024-05-03 21:19 ` Fabiano Rosas
2024-05-03 22:16 ` Peter Xu
2024-04-26 14:20 ` [PATCH 8/9] migration: Add support for fdset with multifd + file Fabiano Rosas
2024-05-08 8:53 ` Daniel P. Berrangé
2024-05-08 18:23 ` Peter Xu
2024-05-08 20:39 ` Fabiano Rosas
2024-05-09 8:08 ` Daniel P. Berrangé
2024-05-17 22:43 ` Fabiano Rosas
2024-05-18 8:36 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 9/9] tests/qtest/migration: Add a test for mapped-ram with passing of fds Fabiano Rosas
2024-05-08 8:56 ` Daniel P. Berrangé
2024-05-02 20:01 ` [PATCH 0/9] migration/mapped-ram: Add direct-io support Peter Xu
2024-05-02 20:34 ` Fabiano Rosas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y18qmxa3.fsf@suse.de \
--to=farosas@suse.de \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=cfontana@suse.de \
--cc=jfehlig@suse.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).