|
From: | Anton Kuchin |
Subject: | Re: [PATCH v2 1/1] vhost-user-fs: add property to allow migration |
Date: | Fri, 17 Feb 2023 01:33:31 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 |
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 fishystatic 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
[Prev in Thread] | Current Thread | [Next in Thread] |