Re: [PATCH v3 1/1] vhost-user-fs: add migration type property

[Michael S. Tsirkin]

On Wed, Feb 22, 2023 at 07:15:40PM +0200, Anton Kuchin wrote:
> On 22/02/2023 18:43, Michael S. Tsirkin wrote:
> > On Wed, Feb 22, 2023 at 06:14:31PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > On 22.02.23 17:25, Anton Kuchin wrote:
> > > > > > > +static int vhost_user_fs_pre_save(void *opaque)
> > > > > > > +{
> > > > > > > +    VHostUserFS *fs = opaque;
> > > > > > > +    g_autofree char *path = 
> > > > > > > object_get_canonical_path(OBJECT(fs));
> > > > > > > +
> > > > > > > +    switch (fs->migration_type) {
> > > > > > > +    case VHOST_USER_MIGRATION_TYPE_NONE:
> > > > > > > +        error_report("Migration is blocked by device %s", path);
> > > > > > > +        break;
> > > > > > > +    case VHOST_USER_MIGRATION_TYPE_EXTERNAL:
> > > > > > > +        return 0;
> > > > > > > +    default:
> > > > > > > +        error_report("Migration type '%s' is not supported by 
> > > > > > > device %s",
> > > > > > > +                     
> > > > > > > VhostUserMigrationType_str(fs->migration_type), path);
> > > > > > > +        break;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return -1;
> > > > > > > +}
> > > > > > Should we also add this as .pre_load, to force user select correct 
> > > > > > migration_type on target too?
> > > > > In fact, I would claim we only want pre_load.
> > > > > When qemu is started on destination we know where it's migrated
> > > > > from so this flag can be set.
> > > > > When qemu is started on source we generally do not yet know so
> > > > > we don't know whether it's safe to set this flag.
> > > But destination is a "source" for next migration, so there shouldn't be 
> > > real difference.
> > And whether to allow that migration should be decided by destination of
> > that migration.
> 
> Destination can just refuse to load unsupported state. But this happens
> automatically if migration code finds unknown subsection and needs no
> explicit check by device .pre_load.

Does it happen with the patch in question?
What is this unknown subsection you are talking about?


> > 
> > 
> > > The new property has ".realized_set_allowed = true", so, as I understand 
> > > it may be changed at any time, so that's not a problem.
> > > 
> > > > This property selects if VM can migrate and if it can what should qemu 
> > > > put
> > > > to the migration stream. So we select on source what type of migration 
> > > > is
> > > > allowed for this VM, destination can't check anything at load time.
> > > OK, so the new field "migration" regulates only outgoing migration and do 
> > > nothing for incoming. On incoming migration the migration stream itself 
> > > defines the type of device migration.
> > > Worth mentioning in doc?
> > > 
> > > -- 
> > > Best regards,
> > > Vladimir