[Virtio-fs] [PATCH] vhost-user-fs: add capability to allow migration

[Anton Kuchin]

On 19/01/2023 21:00, Dr. David Alan Gilbert wrote:
> * Anton Kuchin (antonkuchin at yandex-team.ru) wrote:
>> On 19/01/2023 14:51, Michael S. Tsirkin wrote:
>>> On Sun, Jan 15, 2023 at 07:09: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 we can give an option to orchestrator to override this if it can
>>>> guarantee that state will be preserved (e.g. it uses migration to
>>>> update qemu and dst will run on the same host as src and use the same
>>>> socket endpoints).
>>>>
>>>> This patch keeps default behavior that prevents migration with such devices
>>>> but adds migration capability 'vhost-user-fs' to explicitly allow migration.
>>>>
>>>> Signed-off-by: Anton Kuchin <antonkuchin at yandex-team.ru>
>>>> ---
>>>>    hw/virtio/vhost-user-fs.c | 25 ++++++++++++++++++++++++-
>>>>    qapi/migration.json       |  7 ++++++-
>>>>    2 files changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>>>> index f5049735ac..13d920423e 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 "migration/migration.h"
>>>>    static const int user_feature_bits[] = {
>>>>        VIRTIO_F_VERSION_1,
>>>> @@ -298,9 +299,31 @@ static struct vhost_dev *vuf_get_vhost(VirtIODevice *vdev)
>>>>        return &fs->vhost_dev;
>>>>    }
>>>> +static int vhost_user_fs_pre_save(void *opaque)
>>>> +{
>>>> +    MigrationState *s = migrate_get_current();
>>>> +
>>>> +    if (!s->enabled_capabilities[MIGRATION_CAPABILITY_VHOST_USER_FS]) {
>>>> +        error_report("Migration of vhost-user-fs devices requires internal FUSE "
>>>> +                     "state of backend to be preserved. If orchestrator can "
>>>> +                     "guarantee this (e.g. dst connects to the same backend "
>>>> +                     "instance or backend state is migrated) set 'vhost-user-fs' "
>>>> +                     "migration capability to true to enable migration.");
>>> Isn't it possible that some backends are same and some are not?
>>> Shouldn't this be a device property then?
>> If some are not the same it is not guaranteed that correct FUSE
>> state is present there, so orchestrator shouldn't set the capability
>> because this can result in destination devices being broken (they'll
>> be fine after the remount in guest, but this is guest visible and is
>> not acceptable).
> Shouldn't this be something that's negotiated as a feature bit in the
> vhost-user protocol - i.e. to say whether the device can do migration?
>
> However, I think what you're saying is that a migration might only be
> doable in some cases - e.g. a local migration - and that's hard for
> either qemu or the daemon to discover by itself; so it's right that
> an orchestrator enables it.
> (I'm not sure the error_report message needs to be quite so verbose -
> normally a one line clear message is enough that people can then look
> up).


Exactly, migration depends not only on device capabilities but also
on backends, hosts, management and other environment not known to
qemu or backend so I can't see how this can be device config or
negotiated via the protocol.

In the message I tried to make clear that just enabling the capability
without proper setup can be dangerous to reduce temptation to use it
recklessly and get broken virtiofs device later. I'll try to shorten
the message. I would appreciate if you could help me with the wording
(I'm not a native speaker so building a short sentence with proper
level of warning is challenging)

>
>> I can imagine smart orchestrator and backend that can transfer
>> internal FUSE state, but we are not there yet, and this would be
>> their responsibility then to ensure endpoint compatibility between src
>> and dst and set the capability (that's why I put "e.g." and "or" in
>> the error description).
> You also need the vhost-user device to be able to do the dirty bitmap
> updates; is that being checked somewhere?
>
> Dave

Yes, this is done by generic vhost and vhost-user code on device init.
In vhost_user_backend_init function if backend doesn't support
VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol feature migration blocker
is assigned to device. And in vhost_dev_init there is one more check
for VHOST_F_LOG_ALL feature that assigns another migration blocker if
this is not supported.

>
>>>
>>>
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    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[] = {
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 88ecf86ac8..9a229ea884 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -477,6 +477,11 @@
>>>>    #                    will be handled faster.  This is a performance feature and
>>>>    #                    should not affect the correctness of postcopy migration.
>>>>    #                    (since 7.1)
>>>> +# @vhost-user-fs: If enabled, the migration process will allow migration of
>>>> +#                 vhost-user-fs devices, this should be enabled only when
>>>> +#                 backend can preserve local FUSE state e.g. for qemu update
>>>> +#                 when dst reconects to the same endpoints after migration.
>>>> +#                 (since 8.0)
>>>>    #
>>>>    # Features:
>>>>    # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>>> @@ -492,7 +497,7 @@
>>>>               'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>>>>               { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>>>>               'validate-uuid', 'background-snapshot',
>>>> -           'zero-copy-send', 'postcopy-preempt'] }
>>>> +           'zero-copy-send', 'postcopy-preempt', 'vhost-user-fs'] }
>>> I kind of dislike that it's such a specific flag. Is only vhost-user-fs
>>> ever going to be affected? Any way to put it in a way that is more generic?
>> Here I agree with you: I would prefer less narrow naming too. But I
>> didn't manage to come up with one. Looks like many other vhost-user
>> devices could benefit from this so maybe "vhost-user-stateless" or
>> something like this would be better.
>> I'm not sure that other types of devices could handle reconnect to
>> the old endpoint as easy as vhost-user-fs, but anyway the support for
>> this flag needs to be implemented for each device individually.
>> What do you think? Any ideas would be appreciated.
>>
>>>
>>>>    ##
>>>>    # @MigrationCapabilityStatus:
>>>> -- 
>>>> 2.34.1