Re: [PATCH v3 18/24] vfio/migration: Don't run load cleanup if load setup didn't run

["Cédric Le Goater" <clg@redhat.com>]

On 12/12/24 23:52, Maciej S. Szmigiero wrote:
> On 12.12.2024 15:30, Avihai Horon wrote:
>>
>> On 11/12/2024 1:04, Maciej S. Szmigiero wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 3.12.2024 16:09, Avihai Horon wrote:
>>>>
>>>> On 29/11/2024 19:15, Maciej S. Szmigiero wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 29.11.2024 15:08, Cédric Le Goater wrote:
>>>>>> On 11/17/24 20:20, Maciej S. Szmigiero wrote:
>>>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>>>
>>>>>>> It's possible for load_cleanup SaveVMHandler to get called without
>>>>>>> load_setup handler being called first.
>>>>>>>
>>>>>>> Since we'll be soon running cleanup operations there that access objects
>>>>>>> that need earlier initialization in load_setup let's make sure these
>>>>>>> cleanups only run when load_setup handler had indeed been called
>>>>>>> earlier.
>>>>>>>
>>>>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>>>
>>>>>> tbh, that's a bit ugly. I agree it's similar to those 'bool initialized'
>>>>>> attributes we have in some structs, so nothing new or really wrong.
>>>>>> But it does look like a workaound for a problem or cleanups missing
>>>>>> that would need time to untangle.
>>>>>>
>>>>>> I would prefer to avoid this change and address the issue from the
>>>>>> migration subsystem if possible.
>>>>>
>>>>> While it would be pretty simple to only call {load,save}_cleanup
>>>>> SaveVMHandlers when the relevant {load,save}_setup handler was
>>>>> successfully called first this would amount to a change of these
>>>>> handler semantics.
>>>>>
>>>>> This would risk introducing regressions - for example vfio_save_setup()
>>>>> doesn't clean up (free) newly allocated migration->data_buffer
>>>>> if vfio_migration_set_state() were to fail later in this handler
>>>>> and relies on an unconstitutional call to vfio_save_cleanup() in
>>>>> order to clean it up.
>>>>>
>>>>> There might be similar issues in other drivers too.
>>>>
>>>> We can put all objects related to multifd load in their own struct (as suggested by Cedric in patch #22) and allocate the struct only if multifd device state transfer is used.
>>>> Then in the cleanup flow we clean the struct only if it was allocated.
>>>>
>>>> This way we don't need to add the load_setup flag and we can keep the SaveVMHandlers semantics as is.
>>>>
>>>> Do you think this will be OK?
>>>
>>> I think here the discussion is more of whether we refactor the
>>> {load,save}_cleanup handler semantics to "cleaner" design where
>>> these handlers are only called if the relevant {load,save}_setup
>>> handler was successfully called first (but at the same time risk
>>> introducing regressions).
>>
>> Yes, and I agree with you that changing the semantics of SaveVMHandlers can be risky and may deserve a series of its own.
>> But Cedric didn't like the flag option, so I suggested to do what we usually do, AFAIU, which is to check if the structs are allocated and need cleanup.
>>
>>>
>>>
>>> If we keep the existing semantics of these handlers (like this
>>> patch set did) then it is just an implementation detail whether
>>> we keep an explicit flag like "migration->load_setup" or have
>>> a struct pointer that serves as an implicit equivalent flag
>>> (when not NULL) - I don't have a strong opinion on this particular
>>> detail.
>>>
>> I prefer the struct pointer way, it seems less cumbersome to me.
>> But it's Cedric's call at the end.
> 
> As I wrote above "I don't have a strong opinion on this particular
> detail" - I'm okay with moving these new variables to a dedicated
> struct.

I would prefer that, to isolate multifd migation support from the rest.

> I guess this means we settled on *not* changing the semantics of
> {load,save}_cleanup handler SaveVMHandlers - that was the important
> decision for me.

Handling errors locally in SaveVMHandlers and unrolling what was done
previously is better practice than relying on another callback to do
the cleanup.

Let's see when v4 comes out.

Thanks,

C.