On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Fri, Apr 26, 2024 at 11:20:37AM -0300, Fabiano Rosas wrote:
> >> Add the direct-io migration parameter that tells the migration code to
> >> use O_DIRECT when opening the migration stream file whenever possible.
> >>
> >> This is currently only used with the mapped-ram migration that has a
> >> clear window guaranteed to perform aligned writes.
> >>
> >> Acked-by: Markus Armbruster <arm...@redhat.com>
> >> Signed-off-by: Fabiano Rosas <faro...@suse.de>
> >> ---
> >> include/qemu/osdep.h | 2 ++
> >> migration/migration-hmp-cmds.c | 11 +++++++++++
> >> migration/options.c | 30 ++++++++++++++++++++++++++++++
> >> migration/options.h | 1 +
> >> qapi/migration.json | 18 +++++++++++++++---
> >> util/osdep.c | 9 +++++++++
> >> 6 files changed, 68 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >> index c7053cdc2b..645c14a65d 100644
> >> --- a/include/qemu/osdep.h
> >> +++ b/include/qemu/osdep.h
> >> @@ -612,6 +612,8 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t
> >> len, bool exclusive);
> >> bool qemu_has_ofd_lock(void);
> >> #endif
> >>
> >> +bool qemu_has_direct_io(void);
> >> +
> >> #if defined(__HAIKU__) && defined(__i386__)
> >> #define FMT_pid "%ld"
> >> #elif defined(WIN64)
> >> diff --git a/migration/migration-hmp-cmds.c
> >> b/migration/migration-hmp-cmds.c
> >> index 7e96ae6ffd..8496a2b34e 100644
> >> --- a/migration/migration-hmp-cmds.c
> >> +++ b/migration/migration-hmp-cmds.c
> >> @@ -397,6 +397,13 @@ void hmp_info_migrate_parameters(Monitor *mon, const
> >> QDict *qdict)
> >> monitor_printf(mon, "%s: %s\n",
> >> MigrationParameter_str(MIGRATION_PARAMETER_MODE),
> >> qapi_enum_lookup(&MigMode_lookup, params->mode));
> >> +
> >> + if (params->has_direct_io) {
> >> + monitor_printf(mon, "%s: %s\n",
> >> + MigrationParameter_str(
> >> + MIGRATION_PARAMETER_DIRECT_IO),
> >> + params->direct_io ? "on" : "off");
> >> + }
> >
> > This will be the first parameter to optionally display here. I think it's
> > a sign of misuse of has_direct_io field..
> >
> > IMHO has_direct_io should be best to be kept as "whether direct_io field is
> > valid" and that's all of it. It hopefully shouldn't contain more
> > information than that, or otherwise it'll be another small challenge we
> > need to overcome when we can remove all these has_* fields, and can also be
> > easily overlooked.
>
> I don't think I understand why we have those has_* fields. I thought my
> usage of 'params->has_direct_io = qemu_has_direct_io()' was the correct
> one, i.e. checking whether QEMU has any support for that parameter. Can
> you help me out here?
Here params is the pointer to "struct MigrationParameters", which is
defined in qapi/migration.json. And we have had "has_*" only because we
allow optional fields with asterisks:
{ 'struct': 'MigrationParameters',
'data': { '*announce-initial': 'size',
...
} }
So that's why it better only means "whether this field existed", because
it's how it is defined.
IIRC we (or say, Markus) used to have some attempts deduplicates those
*MigrationParameter* things, and if success we have chance to drop has_*
fields (in which case we simply always have them; that "has_" makes more
sense only if in a QMP session to allow user only specify one or more
things if not all).
>
> >
> > IMHO what we should do is assert has_direct_io==true here too, meanwhile...
> >
> >> }
> >>
> >> qapi_free_MigrationParameters(params);
> >> @@ -690,6 +697,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> >> QDict *qdict)
> >> p->has_mode = true;
> >> visit_type_MigMode(v, param, &p->mode, &err);
> >> break;
> >> + case MIGRATION_PARAMETER_DIRECT_IO:
> >> + p->has_direct_io = true;
> >> + visit_type_bool(v, param, &p->direct_io, &err);
> >> + break;
> >> default:
> >> assert(0);
> >> }
> >> diff --git a/migration/options.c b/migration/options.c
> >> index 239f5ecfb4..ae464aa4f2 100644
> >> --- a/migration/options.c
> >> +++ b/migration/options.c
> >> @@ -826,6 +826,22 @@ int migrate_decompress_threads(void)
> >> return s->parameters.decompress_threads;
> >> }
> >>
> >> +bool migrate_direct_io(void)
> >> +{
> >> + MigrationState *s = migrate_get_current();
> >> +
> >> + /* For now O_DIRECT is only supported with mapped-ram */
> >> + if (!s->capabilities[MIGRATION_CAPABILITY_MAPPED_RAM]) {
> >> + return false;
> >> + }
> >> +
> >> + if (s->parameters.has_direct_io) {
> >> + return s->parameters.direct_io;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> uint64_t migrate_downtime_limit(void)
> >> {
> >> MigrationState *s = migrate_get_current();
> >> @@ -1061,6 +1077,11 @@ MigrationParameters
> >> *qmp_query_migrate_parameters(Error **errp)
> >> params->has_zero_page_detection = true;
> >> params->zero_page_detection = s->parameters.zero_page_detection;
> >>
> >> + if (s->parameters.has_direct_io) {
> >> + params->has_direct_io = true;
> >> + params->direct_io = s->parameters.direct_io;
> >> + }
> >> +
> >> return params;
> >> }
> >>
> >> @@ -1097,6 +1118,7 @@ void migrate_params_init(MigrationParameters *params)
> >> params->has_vcpu_dirty_limit = true;
> >> params->has_mode = true;
> >> params->has_zero_page_detection = true;
> >> + params->has_direct_io = qemu_has_direct_io();
> >> }
> >>
> >> /*
> >> @@ -1416,6 +1438,10 @@ static void
> >> migrate_params_test_apply(MigrateSetParameters *params,
> >> if (params->has_zero_page_detection) {
> >> dest->zero_page_detection = params->zero_page_detection;
> >> }
> >> +
> >> + if (params->has_direct_io) {
> >> + dest->direct_io = params->direct_io;
> >
> > .. do proper check here to make sure the current QEMU is built with direct
> > IO support, then fail QMP migrate-set-parameters otherwise when someone
> > tries to enable it on a QEMU that doesn't support it.
>
> I'm already checking at migrate_params_init() with
> qemu_has_direct_io(). But ok, you want to move it here... Is this
> function the correct one instead of migrate_params_check()? I see these
Oh I perhaps commented on the wrong line. migrate_params_check() is the
place where we should throw such error and check for O_DIRECT for sure..
> TODO comments mentioning QAPI_CLONE(), we can't clone the object if this
> one parameter needs special treatment. I might be getting all this
> wrong, bear with me.
Nah, I think I just wanted to comment inside migrate_params_check() but I
did it all wrong, sorry.
--
Peter Xu
