| Message ID | 20240426142042.14573-5-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/mapped-ram: Add direct-io support | expand |
Fabiano Rosas <farosas@suse.de> writes: > 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 <armbru@redhat.com> > Signed-off-by: Fabiano Rosas <farosas@suse.de> [...] > diff --git a/qapi/migration.json b/qapi/migration.json > index 8c65b90328..1a8a4b114c 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -914,6 +914,9 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @direct-io: Open migration files with O_DIRECT when possible. This > +# requires that the @mapped-ram capability is enabled. (since 9.1) > +# Two spaces between sentences for consistency, please. > # Features: > # > # @deprecated: Member @block-incremental is deprecated. Use > @@ -948,7 +951,8 @@ > { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, > 'vcpu-dirty-limit', > 'mode', > - 'zero-page-detection'] } > + 'zero-page-detection', > + 'direct-io'] } > > ## > # @MigrateSetParameters: [...]
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 <armbru@redhat.com> > Signed-off-by: Fabiano Rosas <farosas@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. 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. Always displaying direct_io parameter also helps when we simply want to check qemu version and whether it supports this feature in general. > + } > } > > static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > @@ -1570,6 +1596,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_zero_page_detection) { > s->parameters.zero_page_detection = params->zero_page_detection; > } > + > + if (params->has_direct_io) { > + s->parameters.direct_io = params->direct_io; > + } > } > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) > diff --git a/migration/options.h b/migration/options.h > index ab8199e207..aa5509cd2a 100644 > --- a/migration/options.h > +++ b/migration/options.h > @@ -76,6 +76,7 @@ uint8_t migrate_cpu_throttle_increment(void); > uint8_t migrate_cpu_throttle_initial(void); > bool migrate_cpu_throttle_tailslow(void); > int migrate_decompress_threads(void); > +bool migrate_direct_io(void); > uint64_t migrate_downtime_limit(void); > uint8_t migrate_max_cpu_throttle(void); > uint64_t migrate_max_bandwidth(void); > diff --git a/qapi/migration.json b/qapi/migration.json > index 8c65b90328..1a8a4b114c 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -914,6 +914,9 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @direct-io: Open migration files with O_DIRECT when possible. This > +# requires that the @mapped-ram capability is enabled. (since 9.1) Here it seems to imply setting direct-io=true will fail if mapped-ram not enabled, but in reality it's fine, it'll just be ignored. I think that's the right thing to do to reduce correlation effects between params/caps (otherwise, when unset mapped-ram cap, we'll need to double check again to unset direct-io too; just cumbersome). I suggest we state the fact, that this field is ignored when mapped-ram capability is not enabled, rather than "requires mapped-ram". Same to all the rest two places in qapi doc. > +# > # Features: > # > # @deprecated: Member @block-incremental is deprecated. Use > @@ -948,7 +951,8 @@ > { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, > 'vcpu-dirty-limit', > 'mode', > - 'zero-page-detection'] } > + 'zero-page-detection', > + 'direct-io'] } > > ## > # @MigrateSetParameters: > @@ -1122,6 +1126,9 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @direct-io: Open migration files with O_DIRECT when possible. This > +# requires that the @mapped-ram capability is enabled. (since 9.1) > +# > # Features: > # > # @deprecated: Member @block-incremental is deprecated. Use > @@ -1176,7 +1183,8 @@ > 'features': [ 'unstable' ] }, > '*vcpu-dirty-limit': 'uint64', > '*mode': 'MigMode', > - '*zero-page-detection': 'ZeroPageDetection'} } > + '*zero-page-detection': 'ZeroPageDetection', > + '*direct-io': 'bool' } } > > ## > # @migrate-set-parameters: > @@ -1354,6 +1362,9 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @direct-io: Open migration files with O_DIRECT when possible. This > +# requires that the @mapped-ram capability is enabled. (since 9.1) > +# > # Features: > # > # @deprecated: Member @block-incremental is deprecated. Use > @@ -1405,7 +1416,8 @@ > 'features': [ 'unstable' ] }, > '*vcpu-dirty-limit': 'uint64', > '*mode': 'MigMode', > - '*zero-page-detection': 'ZeroPageDetection'} } > + '*zero-page-detection': 'ZeroPageDetection', > + '*direct-io': 'bool' } } > > ## > # @query-migrate-parameters: > diff --git a/util/osdep.c b/util/osdep.c > index e996c4744a..d0227a60ab 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -277,6 +277,15 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) > } > #endif > > +bool qemu_has_direct_io(void) > +{ > +#ifdef O_DIRECT > + return true; > +#else > + return false; > +#endif > +} > + > static int qemu_open_cloexec(const char *name, int flags, mode_t mode) > { > int ret; > -- > 2.35.3 >
Peter Xu <peterx@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 <armbru@redhat.com> >> Signed-off-by: Fabiano Rosas <farosas@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? > > 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 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. > > Always displaying direct_io parameter also helps when we simply want to > check qemu version and whether it supports this feature in general. > Makes sense. >> + } >> } >> >> static void migrate_params_apply(MigrateSetParameters *params, Error **errp) >> @@ -1570,6 +1596,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) >> if (params->has_zero_page_detection) { >> s->parameters.zero_page_detection = params->zero_page_detection; >> } >> + >> + if (params->has_direct_io) { >> + s->parameters.direct_io = params->direct_io; >> + } >> } >> >> void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) >> diff --git a/migration/options.h b/migration/options.h >> index ab8199e207..aa5509cd2a 100644 >> --- a/migration/options.h >> +++ b/migration/options.h >> @@ -76,6 +76,7 @@ uint8_t migrate_cpu_throttle_increment(void); >> uint8_t migrate_cpu_throttle_initial(void); >> bool migrate_cpu_throttle_tailslow(void); >> int migrate_decompress_threads(void); >> +bool migrate_direct_io(void); >> uint64_t migrate_downtime_limit(void); >> uint8_t migrate_max_cpu_throttle(void); >> uint64_t migrate_max_bandwidth(void); >> diff --git a/qapi/migration.json b/qapi/migration.json >> index 8c65b90328..1a8a4b114c 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -914,6 +914,9 @@ >> # See description in @ZeroPageDetection. Default is 'multifd'. >> # (since 9.0) >> # >> +# @direct-io: Open migration files with O_DIRECT when possible. This >> +# requires that the @mapped-ram capability is enabled. (since 9.1) > > Here it seems to imply setting direct-io=true will fail if mapped-ram not > enabled, but in reality it's fine, it'll just be ignored. I think that's > the right thing to do to reduce correlation effects between params/caps > (otherwise, when unset mapped-ram cap, we'll need to double check again to > unset direct-io too; just cumbersome). > > I suggest we state the fact, that this field is ignored when mapped-ram > capability is not enabled, rather than "requires mapped-ram". Same to all > the rest two places in qapi doc. > Ok. >> +# >> # Features: >> # >> # @deprecated: Member @block-incremental is deprecated. Use >> @@ -948,7 +951,8 @@ >> { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, >> 'vcpu-dirty-limit', >> 'mode', >> - 'zero-page-detection'] } >> + 'zero-page-detection', >> + 'direct-io'] } >> >> ## >> # @MigrateSetParameters: >> @@ -1122,6 +1126,9 @@ >> # See description in @ZeroPageDetection. Default is 'multifd'. >> # (since 9.0) >> # >> +# @direct-io: Open migration files with O_DIRECT when possible. This >> +# requires that the @mapped-ram capability is enabled. (since 9.1) >> +# >> # Features: >> # >> # @deprecated: Member @block-incremental is deprecated. Use >> @@ -1176,7 +1183,8 @@ >> 'features': [ 'unstable' ] }, >> '*vcpu-dirty-limit': 'uint64', >> '*mode': 'MigMode', >> - '*zero-page-detection': 'ZeroPageDetection'} } >> + '*zero-page-detection': 'ZeroPageDetection', >> + '*direct-io': 'bool' } } >> >> ## >> # @migrate-set-parameters: >> @@ -1354,6 +1362,9 @@ >> # See description in @ZeroPageDetection. Default is 'multifd'. >> # (since 9.0) >> # >> +# @direct-io: Open migration files with O_DIRECT when possible. This >> +# requires that the @mapped-ram capability is enabled. (since 9.1) >> +# >> # Features: >> # >> # @deprecated: Member @block-incremental is deprecated. Use >> @@ -1405,7 +1416,8 @@ >> 'features': [ 'unstable' ] }, >> '*vcpu-dirty-limit': 'uint64', >> '*mode': 'MigMode', >> - '*zero-page-detection': 'ZeroPageDetection'} } >> + '*zero-page-detection': 'ZeroPageDetection', >> + '*direct-io': 'bool' } } >> >> ## >> # @query-migrate-parameters: >> diff --git a/util/osdep.c b/util/osdep.c >> index e996c4744a..d0227a60ab 100644 >> --- a/util/osdep.c >> +++ b/util/osdep.c >> @@ -277,6 +277,15 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) >> } >> #endif >> >> +bool qemu_has_direct_io(void) >> +{ >> +#ifdef O_DIRECT >> + return true; >> +#else >> + return false; >> +#endif >> +} >> + >> static int qemu_open_cloexec(const char *name, int flags, mode_t mode) >> { >> int ret; >> -- >> 2.35.3 >>
On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@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 <armbru@redhat.com> > >> Signed-off-by: Fabiano Rosas <farosas@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.
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 <armbru@redhat.com> > Signed-off-by: Fabiano Rosas <farosas@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"); > + } > } > > 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; > + } > } > > static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > @@ -1570,6 +1596,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_zero_page_detection) { > s->parameters.zero_page_detection = params->zero_page_detection; > } > + > + if (params->has_direct_io) { > + s->parameters.direct_io = params->direct_io; > + } > } I would expect to see something added to migrat_params_check() that calls qemu_has_direct_io() and reports an error if the platform lacks O_DIRECT, so mgmt apps see when they're trying to use O_DIRECT on a bad platform straightaway, rather than only when migration starts later. Alternatively, and perhaps better would be for use to have a meson.build check for O_DIRECT, and then make all the QAPI features have a condition on CONFIG_O_DIRECT, so QAPI rejects any use of 'direct-io' feature at input time. With regards, Daniel
Peter Xu <peterx@redhat.com> writes: > On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: >> Peter Xu <peterx@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 <armbru@redhat.com> >> >> Signed-off-by: Fabiano Rosas <farosas@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").
Markus Armbruster <armbru@redhat.com> writes: > Peter Xu <peterx@redhat.com> writes: > >> On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: >>> Peter Xu <peterx@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 <armbru@redhat.com> >>> >> Signed-off-by: Fabiano Rosas <farosas@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?
Fabiano Rosas <farosas@suse.de> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Peter Xu <peterx@redhat.com> writes: >> >>> On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: >>>> Peter Xu <peterx@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 <armbru@redhat.com> >>>> >> Signed-off-by: Fabiano Rosas <farosas@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-peterx@redhat.com/
Markus Armbruster <armbru@redhat.com> writes: > Fabiano Rosas <farosas@suse.de> writes: > >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Peter Xu <peterx@redhat.com> writes: >>> >>>> On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote: >>>>> Peter Xu <peterx@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 <armbru@redhat.com> >>>>> >> Signed-off-by: Fabiano Rosas <farosas@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 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/20230905162335.235619-1-peterx@redhat.com/
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"); + } } 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; + } } static void migrate_params_apply(MigrateSetParameters *params, Error **errp) @@ -1570,6 +1596,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) if (params->has_zero_page_detection) { s->parameters.zero_page_detection = params->zero_page_detection; } + + if (params->has_direct_io) { + s->parameters.direct_io = params->direct_io; + } } void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) diff --git a/migration/options.h b/migration/options.h index ab8199e207..aa5509cd2a 100644 --- a/migration/options.h +++ b/migration/options.h @@ -76,6 +76,7 @@ uint8_t migrate_cpu_throttle_increment(void); uint8_t migrate_cpu_throttle_initial(void); bool migrate_cpu_throttle_tailslow(void); int migrate_decompress_threads(void); +bool migrate_direct_io(void); uint64_t migrate_downtime_limit(void); uint8_t migrate_max_cpu_throttle(void); uint64_t migrate_max_bandwidth(void); diff --git a/qapi/migration.json b/qapi/migration.json index 8c65b90328..1a8a4b114c 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -914,6 +914,9 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @direct-io: Open migration files with O_DIRECT when possible. This +# requires that the @mapped-ram capability is enabled. (since 9.1) +# # Features: # # @deprecated: Member @block-incremental is deprecated. Use @@ -948,7 +951,8 @@ { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, 'vcpu-dirty-limit', 'mode', - 'zero-page-detection'] } + 'zero-page-detection', + 'direct-io'] } ## # @MigrateSetParameters: @@ -1122,6 +1126,9 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @direct-io: Open migration files with O_DIRECT when possible. This +# requires that the @mapped-ram capability is enabled. (since 9.1) +# # Features: # # @deprecated: Member @block-incremental is deprecated. Use @@ -1176,7 +1183,8 @@ 'features': [ 'unstable' ] }, '*vcpu-dirty-limit': 'uint64', '*mode': 'MigMode', - '*zero-page-detection': 'ZeroPageDetection'} } + '*zero-page-detection': 'ZeroPageDetection', + '*direct-io': 'bool' } } ## # @migrate-set-parameters: @@ -1354,6 +1362,9 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @direct-io: Open migration files with O_DIRECT when possible. This +# requires that the @mapped-ram capability is enabled. (since 9.1) +# # Features: # # @deprecated: Member @block-incremental is deprecated. Use @@ -1405,7 +1416,8 @@ 'features': [ 'unstable' ] }, '*vcpu-dirty-limit': 'uint64', '*mode': 'MigMode', - '*zero-page-detection': 'ZeroPageDetection'} } + '*zero-page-detection': 'ZeroPageDetection', + '*direct-io': 'bool' } } ## # @query-migrate-parameters: diff --git a/util/osdep.c b/util/osdep.c index e996c4744a..d0227a60ab 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -277,6 +277,15 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) } #endif +bool qemu_has_direct_io(void) +{ +#ifdef O_DIRECT + return true; +#else + return false; +#endif +} + static int qemu_open_cloexec(const char *name, int flags, mode_t mode) { int ret;