From: Peter Xu <peterx@redhat.com>
To: Fabiano Rosas <farosas@suse.de>
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, 3 May 2024 17:18:47 -0400 [thread overview]
Message-ID: <ZjVUt179hf4njXmr@x1n> (raw)
In-Reply-To: <87y18qmxa3.fsf@suse.de>
On Fri, May 03, 2024 at 05:54:28PM -0300, Fabiano Rosas wrote:
> 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.
Thanks, please give it a shot, I hope it will work with either way.
One thing to mention is, if you want to play with the qemu_has_direct_io()
approach with no "#ifdefs", you can't keep qemu_has_direct_io() in osdep.c,
but you must define it in osdep.h as static inline functions. Otherwise I
think osdep.o is forced to include it as a function so that trick won't work.
Just try compile without O_DIRECT should see.
--
Peter Xu
next prev parent reply other threads:[~2024-05-03 21:19 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
2024-05-03 21:18 ` Peter Xu [this message]
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=ZjVUt179hf4njXmr@x1n \
--to=peterx@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=cfontana@suse.de \
--cc=farosas@suse.de \
--cc=jfehlig@suse.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).