[PATCH v2 0/1] virtio-fs: implement option for stateless migration.

[PATCH v2 0/1] virtio-fs: implement option for stateless migration.

[Anton Kuchin]

v2: Use device property instead of migration capability

Anton Kuchin (1):
  vhost-user-fs: add property to allow migration

 hw/core/qdev-properties-system.c    | 10 +++++++++
 hw/virtio/vhost-user-fs.c           | 34 ++++++++++++++++++++++++++++-
 include/hw/qdev-properties-system.h |  1 +
 include/hw/virtio/vhost-user-fs.h   |  1 +
 qapi/migration.json                 | 16 ++++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.37.2

[PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Anton Kuchin]

Now any vhost-user-fs device makes VM unmigratable, that also prevents
qemu update without stopping the VM. In most cases that makes sense
because qemu has no way to transfer FUSE session state.

But it is good to have an option for orchestrator to tune this according to
backend capabilities and migration configuration.

This patch adds device property 'migration' that is 'none' by default
to keep old behaviour but can be set to 'external' to explicitly allow
migration with minimal virtio device state in migration stream if daemon
has some way to sync FUSE state on src and dst without help from qemu.

Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/core/qdev-properties-system.c    | 10 +++++++++
 hw/virtio/vhost-user-fs.c           | 34 ++++++++++++++++++++++++++++-
 include/hw/qdev-properties-system.h |  1 +
 include/hw/virtio/vhost-user-fs.h   |  1 +
 qapi/migration.json                 | 16 ++++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index d42493f630..d9b1aa2a5d 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1143,3 +1143,13 @@ const PropertyInfo qdev_prop_uuid = {
     .set   = set_uuid,
     .set_default_value = set_default_uuid_auto,
 };
+
+const PropertyInfo qdev_prop_vhost_user_migration_type = {
+    .name = "VhostUserMigrationType",
+    .description = "none/external",
+    .enum_table = &VhostUserMigrationType_lookup,
+    .get = qdev_propinfo_get_enum,
+    .set = qdev_propinfo_set_enum,
+    .set_default_value = qdev_propinfo_set_default_value_enum,
+    .realized_set_allowed = true,
+};
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index 83fc20e49e..e2a5b6cfdf 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -24,6 +24,7 @@
 #include "hw/virtio/vhost-user-fs.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
+#include "qapi/qapi-types-migration.h"
 
 static const int user_feature_bits[] = {
     VIRTIO_F_VERSION_1,
@@ -298,9 +299,36 @@ static struct vhost_dev *vuf_get_vhost(VirtIODevice *vdev)
     return &fs->vhost_dev;
 }
 
+static int vhost_user_fs_pre_save(void *opaque)
+{
+    VHostUserFS *fs = (VHostUserFS *)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;
+}
+
 static const VMStateDescription vuf_vmstate = {
     .name = "vhost-user-fs",
-    .unmigratable = 1,
+    .minimum_version_id = 0,
+    .version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_UINT8(migration_type, VHostUserFS),
+        VMSTATE_END_OF_LIST()
+    },
+   .pre_save = vhost_user_fs_pre_save,
 };
 
 static Property vuf_properties[] = {
@@ -309,6 +337,10 @@ static Property vuf_properties[] = {
     DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
                        conf.num_request_queues, 1),
     DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
+    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
+                         VHOST_USER_MIGRATION_TYPE_NONE,
+                         qdev_prop_vhost_user_migration_type,
+                         uint8_t),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 0ac327ae60..1a67591590 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
 extern const PropertyInfo qdev_prop_off_auto_pcibar;
 extern const PropertyInfo qdev_prop_pcie_link_speed;
 extern const PropertyInfo qdev_prop_pcie_link_width;
+extern const PropertyInfo qdev_prop_vhost_user_migration_type;
 
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
diff --git a/include/hw/virtio/vhost-user-fs.h b/include/hw/virtio/vhost-user-fs.h
index 94c3aaa84e..3ebce77be5 100644
--- a/include/hw/virtio/vhost-user-fs.h
+++ b/include/hw/virtio/vhost-user-fs.h
@@ -40,6 +40,7 @@ struct VHostUserFS {
     VirtQueue **req_vqs;
     VirtQueue *hiprio_vq;
     int32_t bootindex;
+    uint8_t migration_type;
 
     /*< public >*/
 };
diff --git a/qapi/migration.json b/qapi/migration.json
index c84fa10e86..ababd605a2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2178,3 +2178,19 @@
   'data': { 'job-id': 'str',
             'tag': 'str',
             'devices': ['str'] } }
+
+##
+# @VhostUserMigrationType:
+#
+# Type of vhost-user device migration.
+#
+# @none: Migration is not supported, attempts to migrate with this device
+#        will be blocked.
+#
+# @external: Migration stream contains only virtio device state,
+#            deamon state should be transfered externally by orchestrator.
+#
+# Since: 8.0
+##
+{ 'enum': 'VhostUserMigrationType',
+  'data': [ 'none', 'external' ] }
-- 
2.37.2

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Juan Quintela]

Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
> Now any vhost-user-fs device makes VM unmigratable, that also prevents
> qemu update without stopping the VM. In most cases that makes sense
> because qemu has no way to transfer FUSE session state.
>
> But it is good to have an option for orchestrator to tune this according to
> backend capabilities and migration configuration.
>
> This patch adds device property 'migration' that is 'none' by default
> to keep old behaviour but can be set to 'external' to explicitly allow
> migration with minimal virtio device state in migration stream if daemon
> has some way to sync FUSE state on src and dst without help from qemu.
>
> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>

Reviewed-by: Juan Quintela <quintela@redhat.com>

The migration bits are correct.

And I can think a better way to explain that one device is migrated
externally.

If you have to respin:

> +static int vhost_user_fs_pre_save(void *opaque)
> +{
> +    VHostUserFS *fs = (VHostUserFS *)opaque;

This hack is useless.
I know that there are still lots of code that still have it.


Now remember that I have no clue about vhost-user-fs.

But this looks fishy
>  static const VMStateDescription vuf_vmstate = {
>      .name = "vhost-user-fs",
> -    .unmigratable = 1,
> +    .minimum_version_id = 0,
> +    .version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_VIRTIO_DEVICE,
> +        VMSTATE_UINT8(migration_type, VHostUserFS),
> +        VMSTATE_END_OF_LIST()
> +    },
> +   .pre_save = vhost_user_fs_pre_save,
>  };
>  
>  static Property vuf_properties[] = {
> @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>                         conf.num_request_queues, 1),
>      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
> +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
> +                         VHOST_USER_MIGRATION_TYPE_NONE,
> +                         qdev_prop_vhost_user_migration_type,
> +                         uint8_t),
>      DEFINE_PROP_END_OF_LIST(),

We have four properties here (5 with the new migration one), and you
only migrate one.

This looks fishy, but I don't know if it makes sense.
If they _have_ to be configured the same on source and destination, I
would transfer them and check in post_load that the values are correct.

Later, Juan.

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
> Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
> > Now any vhost-user-fs device makes VM unmigratable, that also prevents
> > qemu update without stopping the VM. In most cases that makes sense
> > because qemu has no way to transfer FUSE session state.
> >
> > But it is good to have an option for orchestrator to tune this according to
> > backend capabilities and migration configuration.
> >
> > This patch adds device property 'migration' that is 'none' by default
> > to keep old behaviour but can be set to 'external' to explicitly allow
> > migration with minimal virtio device state in migration stream if daemon
> > has some way to sync FUSE state on src and dst without help from qemu.
> >
> > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> The migration bits are correct.
> 
> And I can think a better way to explain that one device is migrated
> externally.
> 
> If you have to respin:
> 
> > +static int vhost_user_fs_pre_save(void *opaque)
> > +{
> > +    VHostUserFS *fs = (VHostUserFS *)opaque;
> 
> This hack is useless.

meaning the cast? yes.

> I know that there are still lots of code that still have it.
> 
> 
> Now remember that I have no clue about vhost-user-fs.
> 
> But this looks fishy
> >  static const VMStateDescription vuf_vmstate = {
> >      .name = "vhost-user-fs",
> > -    .unmigratable = 1,
> > +    .minimum_version_id = 0,
> > +    .version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_VIRTIO_DEVICE,
> > +        VMSTATE_UINT8(migration_type, VHostUserFS),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +   .pre_save = vhost_user_fs_pre_save,
> >  };
> >  
> >  static Property vuf_properties[] = {
> > @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
> >      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
> >                         conf.num_request_queues, 1),
> >      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
> > +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
> > +                         VHOST_USER_MIGRATION_TYPE_NONE,
> > +                         qdev_prop_vhost_user_migration_type,
> > +                         uint8_t),
> >      DEFINE_PROP_END_OF_LIST(),
> 
> We have four properties here (5 with the new migration one), and you
> only migrate one.
> 
> This looks fishy, but I don't know if it makes sense.
> If they _have_ to be configured the same on source and destination, I
> would transfer them and check in post_load that the values are correct.
> 
> Later, Juan.

Weird suggestion.  We generally don't do this kind of check - that
would be open-coding each property. It's management's job to make
sure things are consistent.

-- 
MST

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 11:11:22AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
> > Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
> > > Now any vhost-user-fs device makes VM unmigratable, that also prevents
> > > qemu update without stopping the VM. In most cases that makes sense
> > > because qemu has no way to transfer FUSE session state.
> > >
> > > But it is good to have an option for orchestrator to tune this according to
> > > backend capabilities and migration configuration.
> > >
> > > This patch adds device property 'migration' that is 'none' by default
> > > to keep old behaviour but can be set to 'external' to explicitly allow
> > > migration with minimal virtio device state in migration stream if daemon
> > > has some way to sync FUSE state on src and dst without help from qemu.
> > >
> > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > 
> > The migration bits are correct.
> > 
> > And I can think a better way to explain that one device is migrated
> > externally.
> > 
> > If you have to respin:
> > 
> > > +static int vhost_user_fs_pre_save(void *opaque)
> > > +{
> > > +    VHostUserFS *fs = (VHostUserFS *)opaque;
> > 
> > This hack is useless.
> 
> meaning the cast? yes.
> 
> > I know that there are still lots of code that still have it.
> > 
> > 
> > Now remember that I have no clue about vhost-user-fs.
> > 
> > But this looks fishy
> > >  static const VMStateDescription vuf_vmstate = {
> > >      .name = "vhost-user-fs",
> > > -    .unmigratable = 1,
> > > +    .minimum_version_id = 0,
> > > +    .version_id = 0,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_VIRTIO_DEVICE,
> > > +        VMSTATE_UINT8(migration_type, VHostUserFS),
> > > +        VMSTATE_END_OF_LIST()

In fact why do we want to migrate this property?
We generally don't, we only migrate state.

> > > +    },
> > > +   .pre_save = vhost_user_fs_pre_save,
> > >  };
> > >  
> > >  static Property vuf_properties[] = {
> > > @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
> > >      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
> > >                         conf.num_request_queues, 1),
> > >      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
> > > +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
> > > +                         VHOST_USER_MIGRATION_TYPE_NONE,
> > > +                         qdev_prop_vhost_user_migration_type,
> > > +                         uint8_t),
> > >      DEFINE_PROP_END_OF_LIST(),
> > 
> > We have four properties here (5 with the new migration one), and you
> > only migrate one.
> > 
> > This looks fishy, but I don't know if it makes sense.
> > If they _have_ to be configured the same on source and destination, I
> > would transfer them and check in post_load that the values are correct.
> > 
> > Later, Juan.
> 
> Weird suggestion.  We generally don't do this kind of check - that
> would be open-coding each property. It's management's job to make
> sure things are consistent.
> 
> -- 
> MST

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Feb 16, 2023 at 11:11:22AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
>> > Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
>> > > Now any vhost-user-fs device makes VM unmigratable, that also prevents
>> > > qemu update without stopping the VM. In most cases that makes sense
>> > > because qemu has no way to transfer FUSE session state.
>> > >
>> > > But it is good to have an option for orchestrator to tune this according to
>> > > backend capabilities and migration configuration.
>> > >
>> > > This patch adds device property 'migration' that is 'none' by default
>> > > to keep old behaviour but can be set to 'external' to explicitly allow
>> > > migration with minimal virtio device state in migration stream if daemon
>> > > has some way to sync FUSE state on src and dst without help from qemu.
>> > >
>> > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
>> > 
>> > Reviewed-by: Juan Quintela <quintela@redhat.com>
>> > 
>> > The migration bits are correct.
>> > 
>> > And I can think a better way to explain that one device is migrated
>> > externally.
>> > 
>> > If you have to respin:
>> > 
>> > > +static int vhost_user_fs_pre_save(void *opaque)
>> > > +{
>> > > +    VHostUserFS *fs = (VHostUserFS *)opaque;
>> > 
>> > This hack is useless.
>> 
>> meaning the cast? yes.
>> 
>> > I know that there are still lots of code that still have it.
>> > 
>> > 
>> > Now remember that I have no clue about vhost-user-fs.
>> > 
>> > But this looks fishy
>> > >  static const VMStateDescription vuf_vmstate = {
>> > >      .name = "vhost-user-fs",
>> > > -    .unmigratable = 1,
>> > > +    .minimum_version_id = 0,
>> > > +    .version_id = 0,
>> > > +    .fields = (VMStateField[]) {
>> > > +        VMSTATE_VIRTIO_DEVICE,
>> > > +        VMSTATE_UINT8(migration_type, VHostUserFS),
>> > > +        VMSTATE_END_OF_LIST()
>
> In fact why do we want to migrate this property?
> We generally don't, we only migrate state.

See previous discussion.
In a nutshell, we are going to have internal migration in the future
(not done yet).

Later, Juan.

>> > > +    },
>> > > +   .pre_save = vhost_user_fs_pre_save,
>> > >  };
>> > >  
>> > >  static Property vuf_properties[] = {
>> > > @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>> > >      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>> > >                         conf.num_request_queues, 1),
>> > >      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
>> > > +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
>> > > +                         VHOST_USER_MIGRATION_TYPE_NONE,
>> > > +                         qdev_prop_vhost_user_migration_type,
>> > > +                         uint8_t),
>> > >      DEFINE_PROP_END_OF_LIST(),
>> > 
>> > We have four properties here (5 with the new migration one), and you
>> > only migrate one.
>> > 
>> > This looks fishy, but I don't know if it makes sense.
>> > If they _have_ to be configured the same on source and destination, I
>> > would transfer them and check in post_load that the values are correct.
>> > 
>> > Later, Juan.
>> 
>> Weird suggestion.  We generally don't do this kind of check - that
>> would be open-coding each property. It's management's job to make
>> sure things are consistent.
>> 
>> -- 
>> MST

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Anton Kuchin]

On 16/02/2023 18:22, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Thu, Feb 16, 2023 at 11:11:22AM -0500, Michael S. Tsirkin wrote:
>>> On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
>>>> Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
>>>>> Now any vhost-user-fs device makes VM unmigratable, that also prevents
>>>>> qemu update without stopping the VM. In most cases that makes sense
>>>>> because qemu has no way to transfer FUSE session state.
>>>>>
>>>>> But it is good to have an option for orchestrator to tune this according to
>>>>> backend capabilities and migration configuration.
>>>>>
>>>>> This patch adds device property 'migration' that is 'none' by default
>>>>> to keep old behaviour but can be set to 'external' to explicitly allow
>>>>> migration with minimal virtio device state in migration stream if daemon
>>>>> has some way to sync FUSE state on src and dst without help from qemu.
>>>>>
>>>>> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>>
>>>> The migration bits are correct.
>>>>
>>>> And I can think a better way to explain that one device is migrated
>>>> externally.

I'm bad at wording but I'll try to improve this one.
Suggestions will be really appreciated.

>>>>
>>>> If you have to respin:
>>>>
>>>>> +static int vhost_user_fs_pre_save(void *opaque)
>>>>> +{
>>>>> +    VHostUserFS *fs = (VHostUserFS *)opaque;
>>>> This hack is useless.

I will. Will get rid of that, thanks.

>>> meaning the cast? yes.
>>>
>>>> I know that there are still lots of code that still have it.
>>>>
>>>>
>>>> Now remember that I have no clue about vhost-user-fs.
>>>>
>>>> But this looks fishy
>>>>>   static const VMStateDescription vuf_vmstate = {
>>>>>       .name = "vhost-user-fs",
>>>>> -    .unmigratable = 1,
>>>>> +    .minimum_version_id = 0,
>>>>> +    .version_id = 0,
>>>>> +    .fields = (VMStateField[]) {
>>>>> +        VMSTATE_VIRTIO_DEVICE,
>>>>> +        VMSTATE_UINT8(migration_type, VHostUserFS),
>>>>> +        VMSTATE_END_OF_LIST()
>> In fact why do we want to migrate this property?
>> We generally don't, we only migrate state.
> See previous discussion.
> In a nutshell, we are going to have internal migration in the future
> (not done yet).
>
> Later, Juan.

I think Michael is right. We don't need it at destination to know
what data is in the stream because subsections will tell us all we
need to know.

>
>>>>> +    },
>>>>> +   .pre_save = vhost_user_fs_pre_save,
>>>>>   };
>>>>>   
>>>>>   static Property vuf_properties[] = {
>>>>> @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>>>>>       DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>>>>>                          conf.num_request_queues, 1),
>>>>>       DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
>>>>> +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
>>>>> +                         VHOST_USER_MIGRATION_TYPE_NONE,
>>>>> +                         qdev_prop_vhost_user_migration_type,
>>>>> +                         uint8_t),
>>>>>       DEFINE_PROP_END_OF_LIST(),
>>>> We have four properties here (5 with the new migration one), and you
>>>> only migrate one.
>>>>
>>>> This looks fishy, but I don't know if it makes sense.
>>>> If they _have_ to be configured the same on source and destination, I
>>>> would transfer them and check in post_load that the values are correct.
>>>>
>>>> Later, Juan.
>>> Weird suggestion.  We generally don't do this kind of check - that
>>> would be open-coding each property. It's management's job to make
>>> sure things are consistent.
>>>
>>> -- 
>>> MST
>

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Anton Kuchin]

/* resend with fixed to: and cc: */

On 16/02/2023 18:22, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Thu, Feb 16, 2023 at 11:11:22AM -0500, Michael S. Tsirkin wrote:
>>> On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
>>>> Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
>>>>> Now any vhost-user-fs device makes VM unmigratable, that also prevents
>>>>> qemu update without stopping the VM. In most cases that makes sense
>>>>> because qemu has no way to transfer FUSE session state.
>>>>>
>>>>> But it is good to have an option for orchestrator to tune this according to
>>>>> backend capabilities and migration configuration.
>>>>>
>>>>> This patch adds device property 'migration' that is 'none' by default
>>>>> to keep old behaviour but can be set to 'external' to explicitly allow
>>>>> migration with minimal virtio device state in migration stream if daemon
>>>>> has some way to sync FUSE state on src and dst without help from qemu.
>>>>>
>>>>> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>>
>>>> The migration bits are correct.
>>>>
>>>> And I can think a better way to explain that one device is migrated
>>>> externally.

I'm bad at wording but I'll try to improve this one.
Suggestions will be really appreciated.

>>>>
>>>> If you have to respin:
>>>>
>>>>> +static int vhost_user_fs_pre_save(void *opaque)
>>>>> +{
>>>>> +    VHostUserFS *fs = (VHostUserFS *)opaque;
>>>> This hack is useless.

I will. Will get rid of that, thanks.

>>> meaning the cast? yes.
>>>
>>>> I know that there are still lots of code that still have it.
>>>>
>>>>
>>>> Now remember that I have no clue about vhost-user-fs.
>>>>
>>>> But this looks fishy
>>>>>   static const VMStateDescription vuf_vmstate = {
>>>>>       .name = "vhost-user-fs",
>>>>> -    .unmigratable = 1,
>>>>> +    .minimum_version_id = 0,
>>>>> +    .version_id = 0,
>>>>> +    .fields = (VMStateField[]) {
>>>>> +        VMSTATE_VIRTIO_DEVICE,
>>>>> +        VMSTATE_UINT8(migration_type, VHostUserFS),
>>>>> +        VMSTATE_END_OF_LIST()
>> In fact why do we want to migrate this property?
>> We generally don't, we only migrate state.
> See previous discussion.
> In a nutshell, we are going to have internal migration in the future
> (not done yet).
>
> Later, Juan.

I think Michael is right. We don't need it at destination to know
what data is in the stream because subsections will tell us all we
need to know.

>
>>>>> +    },
>>>>> +   .pre_save = vhost_user_fs_pre_save,
>>>>>   };
>>>>>   
>>>>>   static Property vuf_properties[] = {
>>>>> @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>>>>>       DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>>>>>                          conf.num_request_queues, 1),
>>>>>       DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
>>>>> +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
>>>>> +                         VHOST_USER_MIGRATION_TYPE_NONE,
>>>>> +                         qdev_prop_vhost_user_migration_type,
>>>>> +                         uint8_t),
>>>>>       DEFINE_PROP_END_OF_LIST(),
>>>> We have four properties here (5 with the new migration one), and you
>>>> only migrate one.
>>>>
>>>> This looks fishy, but I don't know if it makes sense.
>>>> If they _have_ to be configured the same on source and destination, I
>>>> would transfer them and check in post_load that the values are correct.
>>>>
>>>> Later, Juan.
>>> Weird suggestion.  We generally don't do this kind of check - that
>>> would be open-coding each property. It's management's job to make
>>> sure things are consistent.
>>>
>>> -- 
>>> MST
>

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Feb 16, 2023 at 03:14:05PM +0100, Juan Quintela wrote:
>> Anton Kuchin <antonkuchin@yandex-team.ru> wrote:
>> > Now any vhost-user-fs device makes VM unmigratable, that also prevents
>> > qemu update without stopping the VM. In most cases that makes sense
>> > because qemu has no way to transfer FUSE session state.
>> >
>> > But it is good to have an option for orchestrator to tune this according to
>> > backend capabilities and migration configuration.
>> >
>> > This patch adds device property 'migration' that is 'none' by default
>> > to keep old behaviour but can be set to 'external' to explicitly allow
>> > migration with minimal virtio device state in migration stream if daemon
>> > has some way to sync FUSE state on src and dst without help from qemu.
>> >
>> > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 
>> The migration bits are correct.
>> 
>> And I can think a better way to explain that one device is migrated
>> externally.
>> 
>> If you have to respin:
>> 
>> > +static int vhost_user_fs_pre_save(void *opaque)
>> > +{
>> > +    VHostUserFS *fs = (VHostUserFS *)opaque;
>> 
>> This hack is useless.
>
> meaning the cast? yes.

Yeap.  Sorry.


>> I know that there are still lots of code that still have it.
>> 
>> 
>> Now remember that I have no clue about vhost-user-fs.
>> 
>> But this looks fishy
>> >  static const VMStateDescription vuf_vmstate = {
>> >      .name = "vhost-user-fs",
>> > -    .unmigratable = 1,
>> > +    .minimum_version_id = 0,
>> > +    .version_id = 0,
>> > +    .fields = (VMStateField[]) {
>> > +        VMSTATE_VIRTIO_DEVICE,
>> > +        VMSTATE_UINT8(migration_type, VHostUserFS),
>> > +        VMSTATE_END_OF_LIST()
>> > +    },
>> > +   .pre_save = vhost_user_fs_pre_save,
>> >  };
>> >  
>> >  static Property vuf_properties[] = {
>> > @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>> >      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>> >                         conf.num_request_queues, 1),
>> >      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
>> > +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
>> > +                         VHOST_USER_MIGRATION_TYPE_NONE,
>> > +                         qdev_prop_vhost_user_migration_type,
>> > +                         uint8_t),
>> >      DEFINE_PROP_END_OF_LIST(),
>> 
>> We have four properties here (5 with the new migration one), and you
>> only migrate one.
>> 
>> This looks fishy, but I don't know if it makes sense.
>> If they _have_ to be configured the same on source and destination, I
>> would transfer them and check in post_load that the values are correct.
>> 
>> Later, Juan.
>
> Weird suggestion.  We generally don't do this kind of check - that
> would be open-coding each property. It's management's job to make
> sure things are consistent.

I was wondering why we don't need the other properties.
If you think we don't need them, fine with me.

Later, Juan.

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 6345 bytes --]

On Thu, Feb 16, 2023 at 04:00:03PM +0200, Anton Kuchin wrote:
> Now any vhost-user-fs device makes VM unmigratable, that also prevents
> qemu update without stopping the VM. In most cases that makes sense
> because qemu has no way to transfer FUSE session state.
> 
> But it is good to have an option for orchestrator to tune this according to
> backend capabilities and migration configuration.
> 
> This patch adds device property 'migration' that is 'none' by default
> to keep old behaviour but can be set to 'external' to explicitly allow
> migration with minimal virtio device state in migration stream if daemon
> has some way to sync FUSE state on src and dst without help from qemu.
> 
> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> ---
>  hw/core/qdev-properties-system.c    | 10 +++++++++
>  hw/virtio/vhost-user-fs.c           | 34 ++++++++++++++++++++++++++++-
>  include/hw/qdev-properties-system.h |  1 +
>  include/hw/virtio/vhost-user-fs.h   |  1 +
>  qapi/migration.json                 | 16 ++++++++++++++
>  5 files changed, 61 insertions(+), 1 deletion(-)

Looks okay to me. Comments below.

> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index d42493f630..d9b1aa2a5d 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -1143,3 +1143,13 @@ const PropertyInfo qdev_prop_uuid = {
>      .set   = set_uuid,
>      .set_default_value = set_default_uuid_auto,
>  };
> +
> +const PropertyInfo qdev_prop_vhost_user_migration_type = {
> +    .name = "VhostUserMigrationType",
> +    .description = "none/external",
> +    .enum_table = &VhostUserMigrationType_lookup,
> +    .get = qdev_propinfo_get_enum,
> +    .set = qdev_propinfo_set_enum,
> +    .set_default_value = qdev_propinfo_set_default_value_enum,
> +    .realized_set_allowed = true,
> +};
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 83fc20e49e..e2a5b6cfdf 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -24,6 +24,7 @@
>  #include "hw/virtio/vhost-user-fs.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> +#include "qapi/qapi-types-migration.h"
>  
>  static const int user_feature_bits[] = {
>      VIRTIO_F_VERSION_1,
> @@ -298,9 +299,36 @@ static struct vhost_dev *vuf_get_vhost(VirtIODevice *vdev)
>      return &fs->vhost_dev;
>  }
>  
> +static int vhost_user_fs_pre_save(void *opaque)
> +{
> +    VHostUserFS *fs = (VHostUserFS *)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;
> +}
> +
>  static const VMStateDescription vuf_vmstate = {
>      .name = "vhost-user-fs",
> -    .unmigratable = 1,
> +    .minimum_version_id = 0,
> +    .version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_VIRTIO_DEVICE,
> +        VMSTATE_UINT8(migration_type, VHostUserFS),

Maybe add a comment since Michael asked what the purpose of this field
is:

  /* For verifying that source/destination migration= properties match */
  VMSTATE_UINT8(migration_type, VHostUserFS),

Come to think of it...where is the code that checks the vmstate
migration_type field matches the destination device's migration=
property?

> +        VMSTATE_END_OF_LIST()
> +    },
> +   .pre_save = vhost_user_fs_pre_save,
>  };
>  
>  static Property vuf_properties[] = {
> @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>      DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>                         conf.num_request_queues, 1),
>      DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
> +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
> +                         VHOST_USER_MIGRATION_TYPE_NONE,
> +                         qdev_prop_vhost_user_migration_type,
> +                         uint8_t),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index 0ac327ae60..1a67591590 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
>  extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  extern const PropertyInfo qdev_prop_pcie_link_speed;
>  extern const PropertyInfo qdev_prop_pcie_link_width;
> +extern const PropertyInfo qdev_prop_vhost_user_migration_type;
>  
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
> diff --git a/include/hw/virtio/vhost-user-fs.h b/include/hw/virtio/vhost-user-fs.h
> index 94c3aaa84e..3ebce77be5 100644
> --- a/include/hw/virtio/vhost-user-fs.h
> +++ b/include/hw/virtio/vhost-user-fs.h
> @@ -40,6 +40,7 @@ struct VHostUserFS {
>      VirtQueue **req_vqs;
>      VirtQueue *hiprio_vq;
>      int32_t bootindex;
> +    uint8_t migration_type;
>  
>      /*< public >*/
>  };
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..ababd605a2 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -2178,3 +2178,19 @@
>    'data': { 'job-id': 'str',
>              'tag': 'str',
>              'devices': ['str'] } }
> +
> +##
> +# @VhostUserMigrationType:
> +#
> +# Type of vhost-user device migration.
> +#
> +# @none: Migration is not supported, attempts to migrate with this device
> +#        will be blocked.
> +#
> +# @external: Migration stream contains only virtio device state,
> +#            deamon state should be transfered externally by orchestrator.

s/deamon/daemon/
s/transfered/transferred/

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'VhostUserMigrationType',
> +  'data': [ 'none', 'external' ] }
> -- 
> 2.37.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration

[Anton Kuchin]

On 16/02/2023 23:09, Stefan Hajnoczi wrote:
> On Thu, Feb 16, 2023 at 04:00:03PM +0200, Anton Kuchin wrote:
>> Now any vhost-user-fs device makes VM unmigratable, that also prevents
>> qemu update without stopping the VM. In most cases that makes sense
>> because qemu has no way to transfer FUSE session state.
>>
>> But it is good to have an option for orchestrator to tune this according to
>> backend capabilities and migration configuration.
>>
>> This patch adds device property 'migration' that is 'none' by default
>> to keep old behaviour but can be set to 'external' to explicitly allow
>> migration with minimal virtio device state in migration stream if daemon
>> has some way to sync FUSE state on src and dst without help from qemu.
>>
>> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
>> ---
>>   hw/core/qdev-properties-system.c    | 10 +++++++++
>>   hw/virtio/vhost-user-fs.c           | 34 ++++++++++++++++++++++++++++-
>>   include/hw/qdev-properties-system.h |  1 +
>>   include/hw/virtio/vhost-user-fs.h   |  1 +
>>   qapi/migration.json                 | 16 ++++++++++++++
>>   5 files changed, 61 insertions(+), 1 deletion(-)
> Looks okay to me. Comments below.
>
>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> index d42493f630..d9b1aa2a5d 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -1143,3 +1143,13 @@ const PropertyInfo qdev_prop_uuid = {
>>       .set   = set_uuid,
>>       .set_default_value = set_default_uuid_auto,
>>   };
>> +
>> +const PropertyInfo qdev_prop_vhost_user_migration_type = {
>> +    .name = "VhostUserMigrationType",
>> +    .description = "none/external",
>> +    .enum_table = &VhostUserMigrationType_lookup,
>> +    .get = qdev_propinfo_get_enum,
>> +    .set = qdev_propinfo_set_enum,
>> +    .set_default_value = qdev_propinfo_set_default_value_enum,
>> +    .realized_set_allowed = true,
>> +};
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index 83fc20e49e..e2a5b6cfdf 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -24,6 +24,7 @@
>>   #include "hw/virtio/vhost-user-fs.h"
>>   #include "monitor/monitor.h"
>>   #include "sysemu/sysemu.h"
>> +#include "qapi/qapi-types-migration.h"
>>   
>>   static const int user_feature_bits[] = {
>>       VIRTIO_F_VERSION_1,
>> @@ -298,9 +299,36 @@ static struct vhost_dev *vuf_get_vhost(VirtIODevice *vdev)
>>       return &fs->vhost_dev;
>>   }
>>   
>> +static int vhost_user_fs_pre_save(void *opaque)
>> +{
>> +    VHostUserFS *fs = (VHostUserFS *)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;
>> +}
>> +
>>   static const VMStateDescription vuf_vmstate = {
>>       .name = "vhost-user-fs",
>> -    .unmigratable = 1,
>> +    .minimum_version_id = 0,
>> +    .version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_VIRTIO_DEVICE,
>> +        VMSTATE_UINT8(migration_type, VHostUserFS),
> Maybe add a comment since Michael asked what the purpose of this field
> is:
>
>    /* For verifying that source/destination migration= properties match */
>    VMSTATE_UINT8(migration_type, VHostUserFS),
>
> Come to think of it...where is the code that checks the vmstate
> migration_type field matches the destination device's migration=
> property?

It's a good idea to have a comment here, thanks.
This field is not really for verifying that source and destination match.
It just describes what data is packed into the stream.
In fact this property could be different and this breaks nothing:
e.g. when we migrate from version that can export only stateless stream
to one that finally supports internal migration.

Said that I start thinking that we don't need it in the stream at all
because extra data can be detected just by presence of additional
subsection.

>
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +   .pre_save = vhost_user_fs_pre_save,
>>   };
>>   
>>   static Property vuf_properties[] = {
>> @@ -309,6 +337,10 @@ static Property vuf_properties[] = {
>>       DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
>>                          conf.num_request_queues, 1),
>>       DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 128),
>> +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
>> +                         VHOST_USER_MIGRATION_TYPE_NONE,
>> +                         qdev_prop_vhost_user_migration_type,
>> +                         uint8_t),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
>> index 0ac327ae60..1a67591590 100644
>> --- a/include/hw/qdev-properties-system.h
>> +++ b/include/hw/qdev-properties-system.h
>> @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
>>   extern const PropertyInfo qdev_prop_off_auto_pcibar;
>>   extern const PropertyInfo qdev_prop_pcie_link_speed;
>>   extern const PropertyInfo qdev_prop_pcie_link_width;
>> +extern const PropertyInfo qdev_prop_vhost_user_migration_type;
>>   
>>   #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
>>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
>> diff --git a/include/hw/virtio/vhost-user-fs.h b/include/hw/virtio/vhost-user-fs.h
>> index 94c3aaa84e..3ebce77be5 100644
>> --- a/include/hw/virtio/vhost-user-fs.h
>> +++ b/include/hw/virtio/vhost-user-fs.h
>> @@ -40,6 +40,7 @@ struct VHostUserFS {
>>       VirtQueue **req_vqs;
>>       VirtQueue *hiprio_vq;
>>       int32_t bootindex;
>> +    uint8_t migration_type;
>>   
>>       /*< public >*/
>>   };
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c84fa10e86..ababd605a2 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -2178,3 +2178,19 @@
>>     'data': { 'job-id': 'str',
>>               'tag': 'str',
>>               'devices': ['str'] } }
>> +
>> +##
>> +# @VhostUserMigrationType:
>> +#
>> +# Type of vhost-user device migration.
>> +#
>> +# @none: Migration is not supported, attempts to migrate with this device
>> +#        will be blocked.
>> +#
>> +# @external: Migration stream contains only virtio device state,
>> +#            deamon state should be transfered externally by orchestrator.
> s/deamon/daemon/
> s/transfered/transferred/

Will fix, thanks

>
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'VhostUserMigrationType',
>> +  'data': [ 'none', 'external' ] }
>> -- 
>> 2.37.2
>>