On 22/02/2023 14:20, Vladimir Sementsov-Ogievskiy wrote:
On 17.02.23 20:00, Anton Kuchin wrote:
Migration of vhost-user-fs device requires transfer of FUSE internal
state
from backend. There is no standard way to do it now so by default
migration
must be blocked. But if this state can be externally transferred by
orchestrator give it an option to explicitly allow migration.
Signed-off-by: Anton Kuchin <antonkuc...@yandex-team.ru>
---
hw/core/qdev-properties-system.c | 10 +++++++++
hw/virtio/vhost-user-fs.c | 32 ++++++++++++++++++++++++++++-
include/hw/qdev-properties-system.h | 1 +
include/hw/virtio/vhost-user-fs.h | 2 ++
qapi/migration.json | 16 +++++++++++++++
5 files changed, 60 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..7deb9df5ec 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -298,9 +298,35 @@ static struct vhost_dev
*vuf_get_vhost(VirtIODevice *vdev)
return &fs->vhost_dev;
}
+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?
Why do we need it? Enum forces user to select at least one of the sane
option
and I believe this is enough. As this property affects only save and
don't know
what we can do at load.
+
static const VMStateDescription vuf_vmstate = {
.name = "vhost-user-fs",
- .unmigratable = 1,
+ .minimum_version_id = 0,
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_VIRTIO_DEVICE,
+ VMSTATE_END_OF_LIST()
+ },
+ .pre_save = vhost_user_fs_pre_save,
};
static Property vuf_properties[] = {
@@ -309,6 +335,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,
+ VhostUserMigrationType),
1. I see, other similar qdev_prop_* use DEFINE_PROP_SIGNED
I don't think this should be signed. Enum values are non-negative so
compilers
(at least gcc and clang that I checked) evaluate underlying enum type to
be unsigned int.
I don't know why other property types use signed, may be they have
reasons or just this
is how they were initially implemented.
2. All of them except only qdev_prop_fdc_drive_type, define also a
convenient macro in include/hw/qdev-properties-system.h
This makes sense if property is used in more than one place, in this
case I don't see any
benefit from writing more code to handle this specific case. Maybe if
property finds its
usage in other devices this can be done.
should we follow these patterns?
