[Virtio-fs] [RFC 2/2] vhost-user-fs: Implement stateful migration

[Anton Kuchin]

On 21/03/2023 19:49, Hanna Czenczek wrote:
> On 20.03.23 13:39, Anton Kuchin wrote:
>> On 20/03/2023 11:33, Hanna Czenczek wrote:
>>> On 17.03.23 19:37, Anton Kuchin wrote:
>>>> On 17/03/2023 19:52, Hanna Czenczek wrote:
>>>>> On 17.03.23 18:19, Anton Kuchin wrote:
>>>>>> On 13/03/2023 19:48, Hanna Czenczek wrote:
>>>>>>> A virtio-fs device's VM state consists of:
>>>>>>> - the virtio device (vring) state (VMSTATE_VIRTIO_DEVICE)
>>>>>>> - the back-end's (virtiofsd's) internal state
>>>>>>>
>>>>>>> We get/set the latter via the new vhost-user operations 
>>>>>>> FS_SET_STATE_FD,
>>>>>>> FS_GET_STATE, and FS_SET_STATE.
>>>>>>>
>>>
>>> [...]
>>>
>>>>>>>   static const VMStateDescription vuf_vmstate = {
>>>>>>>       .name = "vhost-user-fs",
>>>>>>> -    .unmigratable = 1,
>>>>>>> +    .version_id = 1,
>>>>>>> +    .fields = (VMStateField[]) {
>>>>>>> +        VMSTATE_VIRTIO_DEVICE,
>>>>>>> +        {
>>>>>>> +            .name = "back-end",
>>>>>>> +            .info = &(const VMStateInfo) {
>>>>>>> +                .name = "virtio-fs back-end state",
>>>>>>> +                .get = vuf_load_state,
>>>>>>> +                .put = vuf_save_state,
>>>>>>> +            },
>>>>>>> +        },
>>>>>>
>>>>>> I've been working on stateless migration patch [1] and there was 
>>>>>> discussed that we
>>>>>> need to keep some kind of blocker by default if orchestrators 
>>>>>> rely on unmigratable
>>>>>> field in virtio-fs vmstate to block the migration.
>>>>>> For this purpose I've implemented flag that selects "none" or 
>>>>>> "external" and is checked
>>>>>> in pre_save, so it could be extended with "internal" option.
>>>>>> We didn't come to conclusion if we also need to check incoming 
>>>>>> migration, the discussion
>>>>>> has stopped for a while but I'm going back to it now.
>>>>>>
>>>>>> I would appreciate if you have time to take a look at the 
>>>>>> discussion and consider the idea
>>>>>> proposed there to store internal state as a subsection of vmstate 
>>>>>> to make it as an option
>>>>>> but not mandatory.
>>>>>>
>>>>>> [1] 
>>>>>> https://patchew.org/QEMU/20230217170038.1273710-1-antonkuchin@yandex-team.ru/
>>>>>
>>>>> So far I’ve mostly considered these issues orthogonal.  If your 
>>>>> stateless migration goes in first, then state is optional and I’ll 
>>>>> adjust this series.
>>>>> If stateful migration goes in first, then your series can simply 
>>>>> make state optional by introducing the external option, no?
>>>>
>>>> Not really. State can be easily extended by subsections but not 
>>>> trimmed. Maybe this can be worked around by defining two types of 
>>>> vmstate and selecting the correct one at migration, but I'm not sure.
>>>
>>> I thought your patch included a switch on the vhost-user-fs device 
>>> (on the qemu side) to tell qemu what migration data to expect. Can 
>>> we not transfer a 0-length state for 'external', and assert this on 
>>> the destination side?
>>
>> Looks like I really need to make the description of my patch and the 
>> documentation more clear :) My patch proposes switch on _source_ side 
>> to select which data to save in the stream mostly to protect old 
>> orchestrators that don't expect virtio-fs to be migratable (and for 
>> internal case it can be extended to select if qemu needs to request 
>> state from backend), Michael insists that we also need to check on 
>> destination but I disagree because I believe that we can figure this 
>> out from stream data without additional device flags.
>>
>>>
>>>>>
>>>>> But maybe we could also consider making stateless migration a 
>>>>> special case of stateful migration; if we had stateful migration, 
>>>>> can’t we just implement stateless migration by telling virtiofsd 
>>>>> that it should submit a special “I have no state” pseudo-state, 
>>>>> i.e. by having a switch on virtiofsd instead?
>>>>
>>>> Sure. Backend can send empty state (as your patch treats 0 length 
>>>> as a valid response and not error) or dummy state that can be 
>>>> recognized as stateless. The only potential problem is that then we 
>>>> need support in backend for new commands even to return dummy 
>>>> state, and if backend can support both types then we'll need some 
>>>> switch in backend to reply with real or empty state.
>>>
>>> Yes, exactly.
>>>
>>>>>
>>>>> Off the top of my head, some downsides of that approach would be
>>>>> (1) it’d need a switch on the virtiofsd side, not on the qemu side 
>>>>> (not sure if that’s a downside, but a difference for sure),
>>>>
>>>> Why would you? It seems to me that this affects only how qemu 
>>>> treats the vmstate of device. If the state was requested backend 
>>>> sends it to qemu. If state subsection is present in stream qemu 
>>>> sends it to the backend for loading. Stateless one just doesn't 
>>>> request state from the backend. Or am I missing something?
>>>>
>>>>> and (2) we’d need at least some support for this on the virtiofsd 
>>>>> side, i.e. practically can’t come quicker than stateful migration 
>>>>> support.
>>>>
>>>> Not much, essentially this is just a reconnect. I've sent a draft 
>>>> of a reconnect patch for old C-virtiofsd, for rust version it takes 
>>>> much longer because I'm learning rust and I'm not really good at it 
>>>> yet.
>>>
>>> I meant these two downsides not for your proposal, but instead if we 
>>> implemented stateless migration only in the back-end; i.e. the 
>>> front-end would only implement stateful migration, and the back-end 
>>> would send and accept an empty state.
>>>
>>> Then, qemu would always request a “state” (even if it turns out 
>>> empty for stateless migration), and qemu would always treat it the 
>>> same, so there wouldn’t be a switch on the qemu side, but only on 
>>> the virtiofsd side.  Doesn’t really matter, but what does matter is 
>>> that we’d need to implement the migration interface in virtiofsd, 
>>> even if in the end no state is transferred.
>>>
>>> So exactly what you’ve said above (“The only potential problem is 
>>> […]”). :)
>>>
>>> Hanna
>>>
>>
>> Oh, yes, we were talking about the same thing. So do you agree that 
>> storing internal state data in subsection will allow backend code to 
>> be more straightforward without additional switches?
>
> Sounds good.  I think we should rename VHOST_USER_MIGRATION_TYPE_NONE 
> to ..._INTERNAL, though, and then use that (default) mode for stateful 
> migration (via VMState), because I think that should be the default 
> migration type (and while there’s no support for it, it will just 
> continue to block migration).

My plan was to add new ..._INTERNAL option and keep ..._NONE as default 
one that blocks migration unless orchestrator explicitly selects 
migration type to avoid accidental migrations of old orchestrators that 
expect blocker for virtio-fs. But if INTERNAL blocks migration when 
backend doesn't support new commands your idea to make it default may be 
even better.

>
>
> So I suppose you mean something like this, where 
> vuf_stateful_migration() basically returns `fs->migration_type == 
> VHOST_USER_MIGRATION_TYPE_INTERNAL`, and on the destination, you’d 
> check whether the subsection is present to decide which kind of 
> migration it is, right?

Yes, exactly.

>
>
> static const VMStateDescription vuf_backend_vmstate;
>
> static const VMStateDescription vuf_vmstate = {
>     .name = "vhost-user-fs",
>     .version_id = 0,
>     .fields = (VMStateField[]) {
>         VMSTATE_VIRTIO_DEVICE,
>         VMSTATE_END_OF_LIST()
>     },
>     .subsections = (const VMStateDescription*[]) {
>         &vuf_backend_vmstate,
>         NULL,
>     }
> };
>
> static const VMStateDescription vuf_backend_vmstate = {
>     .name = "vhost-user-fs-backend",
>     .version_id = 0,
>     .needed = vuf_stateful_migration,
>     .fields = (VMStateField[]) {
>         {
>             .name = "back-end",
>             .info = &(const VMStateInfo) {
>                 .name = "virtio-fs back-end state",
>                 .get = vuf_load_state,
>                 .put = vuf_save_state,
>             },
>         },
>         VMSTATE_END_OF_LIST()
>     },
> };
>