Fabiano Rosas <faro...@suse.de> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> 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").
>
> So what's the blocker for merging MigrationSetParameters and
> MigrationParameters? Just the tls_* options?
I believe the latest attempt was Peter's "[PATCH v3 0/4] qapi/migration:
Dedup migration parameter objects and fix tls-authz crash" last
September.
I can't recall offhand what exactly blocked its merge. Suggest you read
the review thread[*], and if that leads to questions, ask them either in
replies to the old thread, or right here.
One additional issue hasn't been discussed much so far, I think: merging
the two copies of the doc comments. They are big and quite similar
(that's why we want to deduplicate), but they're not identical. In
particular, MigrationSetParameters' doc comment talks more about
defaults. That's because talking about defaults makes no sense
whatsoever for MigrationParameters (we do it anyway, of course, just
less). Merging the two will require some careful word-smithing, and
perhaps some compromises on doc quality.
[*]
https://lore.kernel.org/qemu-devel/20230905162335.235619-1-pet...@redhat.com/
