Re: [PATCH 4/9] migration: Add direct-io parameter

[Peter Xu]

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.

-- 
Peter Xu