Re: [PATCH v2 28/29] migration: Add direct-io parameter

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@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 <farosas@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.