Peter Xu <pet...@redhat.com> writes:
> 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..
Yes. For similar existing parameters, we do
assert(params->has_FOO);
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_str(MIGRATION_PARAMETER_FOO),
... params->FOO as string ...)
>> > 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).
qapi/migration.json:
##
# @MigrationParameters:
#
--> # The optional members aren't actually optional.
#
In other words, the has_FOO generated for the members of
MigrationParameters must all be true.
These members became optional when we attempted to de-duplicate
MigrationParameters and MigrateSetParameters, but failed (see commit
de63ab61241 "migrate: Share common MigrationParameters struct" and
commit 1bda8b3c695 "migration: Unshare MigrationParameters struct for
now").
