Markus Armbruster <[email protected]> writes: > Fabiano Rosas <[email protected]> writes: > >> Markus Armbruster <[email protected]> writes: >> >>> Peter Xu <[email protected]> writes: >>> >>>> On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: >>>>> Peter Xu <[email protected]> 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 <[email protected]> >>>>> >> Signed-off-by: Fabiano Rosas <[email protected]> >>>>> >> --- >>>>> >> 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 had a vague memory of this, thanks for bringing it up. I'll go over that series and see what can be done. > > 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. > I was playing with this yesterday and it occurred to me as well that the docs are not quite the same. Maybe we can have a general description common for both use-cases and a few extra words for what happens differently when writing vs. reading. > > > [*] > https://lore.kernel.org/qemu-devel/[email protected]/
