Daniel P. Berrangé <berra...@redhat.com> writes:
> On Mon, Oct 23, 2023 at 05:36:07PM -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 for the secondary channels of fixed-ram
>> migration, which can guarantee that writes are page aligned.
>>
>> However the parameter could be made to affect other types of
>> file-based migrations in the future.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> include/qemu/osdep.h | 2 ++
>> migration/file.c | 15 ++++++++++++---
>> migration/migration-hmp-cmds.c | 10 ++++++++++
>> migration/options.c | 30 ++++++++++++++++++++++++++++++
>> migration/options.h | 1 +
>> qapi/migration.json | 17 ++++++++++++++---
>> util/osdep.c | 9 +++++++++
>> 7 files changed, 78 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 475a1c62ff..ea5d29ab9b 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -597,6 +597,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/file.c b/migration/file.c
>> index ad75225f43..3d3c58ecad 100644
>> --- a/migration/file.c
>> +++ b/migration/file.c
>> @@ -11,9 +11,9 @@
>> #include "qemu/error-report.h"
>> #include "channel.h"
>> #include "file.h"
>> -#include "migration.h"
>> #include "io/channel-file.h"
>> #include "io/channel-util.h"
>> +#include "migration.h"
>> #include "options.h"
>> #include "trace.h"
>>
>> @@ -77,9 +77,18 @@ void file_send_channel_create(QIOTaskFunc f, void *data)
>> QIOChannelFile *ioc;
>> QIOTask *task;
>> Error *errp = NULL;
>> + int flags = outgoing_args.flags;
>>
>> - ioc = qio_channel_file_new_path(outgoing_args.fname,
>> - outgoing_args.flags,
>> + if (migrate_direct_io() && qemu_has_direct_io()) {
>> + /*
>> + * Enable O_DIRECT for the secondary channels. These are used
>> + * for sending ram pages and writes should be guaranteed to be
>> + * aligned to at least page size.
>> + */
>> + flags |= O_DIRECT;
>> + }
>
> IMHO we should not be silently ignoring the user's request for
> direct I/O if we've been comiled for a platform which can't
> support it. We should fail the setting of the direct I/O
> parameter
>
Good point.
> Also this is referencing O_DIRECT without any #ifdef check.
>
Ack.
>> +
>> + ioc = qio_channel_file_new_path(outgoing_args.fname, flags,
>> outgoing_args.mode, &errp);
>> if (!ioc) {
>> file_migration_cancel(errp);
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index a82597f18e..eab5ac3588 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -387,6 +387,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const
>> QDict *qdict)
>> monitor_printf(mon, "%s: %" PRIu64 " MB/s\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
>> params->vcpu_dirty_limit);
>> +
>> + 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);
>> @@ -661,6 +667,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> p->has_vcpu_dirty_limit = true;
>> visit_type_size(v, param, &p->vcpu_dirty_limit, &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 2193d69e71..6d0e3c26ae 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -817,6 +817,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 fixed-ram */
>> + if (!s->capabilities[MIGRATION_CAPABILITY_FIXED_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();
>> @@ -1025,6 +1041,11 @@ MigrationParameters
>> *qmp_query_migrate_parameters(Error **errp)
>> params->has_vcpu_dirty_limit = true;
>> params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
>>
>> + if (s->parameters.has_direct_io) {
>> + params->has_direct_io = true;
>> + params->direct_io = s->parameters.direct_io;
>> + }
>> +
>> return params;
>> }
>>
>> @@ -1059,6 +1080,7 @@ void migrate_params_init(MigrationParameters *params)
>> params->has_announce_step = true;
>> params->has_x_vcpu_dirty_limit_period = true;
>> params->has_vcpu_dirty_limit = true;
>> + params->has_direct_io = qemu_has_direct_io();
>> }
>>
>> /*
>> @@ -1356,6 +1378,10 @@ static void
>> migrate_params_test_apply(MigrateSetParameters *params,
>> if (params->has_vcpu_dirty_limit) {
>> dest->vcpu_dirty_limit = params->vcpu_dirty_limit;
>> }
>> +
>> + if (params->has_direct_io) {
>> + dest->direct_io = params->direct_io;
>> + }
>> }
>>
>> static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>> @@ -1486,6 +1512,10 @@ static void migrate_params_apply(MigrateSetParameters
>> *params, Error **errp)
>> if (params->has_vcpu_dirty_limit) {
>> s->parameters.vcpu_dirty_limit = params->vcpu_dirty_limit;
>> }
>> +
>> + if (params->has_direct_io) {
>
> #ifndef O_DIRECT
> error_setg(errp, "Direct I/O is not supported on this platform");
> #endif
>
> Should also be doing a check for the 'fixed-ram' capability being
> set at this point.
>
Ok.
