Re: [PATCH v3 5/5] vhost-user: add skip_drain param to GET_VRING_BASE

Apologies for the late reply.

On 5/12/26 1:55 AM, Alexandr Moshkov wrote:
> Gentle ping :)
> 

[...]

>>>>> On 4/16/26 5:26 AM, Alexandr Moshkov wrote:
>>>>>> Greetings! Thanks for reply!
>>>>>>
>>>>>>
>>>>>> On 4/15/26 20:21, Raphael Norwitz wrote:
>>>>>>> I’m not sure I like using the num field in vhost_vring_state to 
>>>>>>> set magic values which affect protocol behavior for 
>>>>>>> GET_VRING_BASE. It feels like a hack to me. I would think the 
>>>>>>> proper solution if we want to support migration from new to old 
>>>>>>> would be to have new use a different new message entirely. Can we 
>>>>>>> do that?
>>>>>>
>>>>>> I think we can, but I thought at first that this will be almost a 
>>>>>> complete copy of GET_VRING_BASE message, with the exception of 
>>>>>> waiting for drain of requests, so I choose to expand existing 
>>>>>> message.
>>>>>>
>>>>>
>>>>> I think a new message would be cleaner. Anyone else have thoughts?
>>>>>
>>>>>> If this is not an appropriate approach, is it better to make a new 
>>>>>> message like GET_VRING_BASE or a separate message used together 
>>>>>> with default GET_VRING_BASE message (for example, message for 
>>>>>> setting some kind of status on the server)? What should I name 
>>>>>> this new message?
>>>>>>
>>>>>
>>>>> Maybe GET_VRING_BASE_SKIP_DRAIN? How would a separate message used 
>>>>> together with default GET_VRING_BASE message work? 
>>>>
>>>> I was thinking about a message something like SET_VRING_ENABLE - for 
>>>> example SET_SKIP_DRAIN_ENABLE, that would enable/disable some state 
>>>> (skip_drain) in the backend.
>>>
>>> I'd need to see the flow in more detail but sounds promising.
>>
>> On migration vhost-user-blk firstly send SET_SKIP_DRAIN_ENABLE with 
>> num = 1 message if `inflight-migration` device parameter 
>> and VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT enabled. Then send 
>> GET_VRING_BASE and continue work as usual.
>>
>> On SET_SKIP_DRAIN_ENABLE backend sets inner state (skip_drain) so when 
>> GET_VRING_BASE is called and skip_drain is true the drain will skipped.
>>
>> What do you think?
>>

I'm happy with that in theory but would like more clarity on the 
details. In particular, I'm not sure what you mean by "vhost-user-blk 
firstly send SET_SKIP_DRAIN_ENABLE". To add a new message I would think 
we would have to do a vhost-user protocol feature negotiation to gate it.

Also what are the precise semantics for SET_SKIP_DRAIN_ENABLE? Would it 
be sent on backend connect or when we're about to migrate?

I was hoping others would comment but at this point I'd suggest drafting 
the code and then we can re-review.

>>>
>>>>
>>>>>
>>>>>> I also fix other comments, thanks!
>>>>>>
>>>>>>>
>>>>>>> On 3/30/26 11:52 AM, Alexandr Moshkov wrote:
>>>>>>>> In case of migration of QEMU from the new version (where the
>>>>>>>> inllight-migration parameter is present), to the old one (where 
>>>>>>>> it is
>>>>>>>
>>>>>>> nit: spelling inllight-migration
>>>>>>>
>>>>>>>> absent) there is no way to disable this feature on the backend 
>>>>>>>> during
>>>>>>>> runtime.
>>>>>>>>
>>>>>>>> This commit slightly changes the semantics of the protocol feature
>>>>>>>> VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT. Enabling this 
>>>>>>>> feature
>>>>>>>> adds a new parameter for GET_VRING_BASE, which allows to control 
>>>>>>>> the
>>>>>>>> drain in-flight requests on the backend.
>>>>>>>> Thus, QEMU will be able to turn this feature on GET_VRING_BASE 
>>>>>>>> off and
>>>>>>>> on anytime.
>>>>>>>>
>>>>>>>> In vhost-user-blk use inflight_migration param to enable 
>>>>>>>> skip_drain to
>>>>>>>> suspend in-flight I/O requests, and then migrate them throught 
>>>>>>>> inflight
>>>>>>>> subsection.
>>>>>>>>
>>>>>>>> Also now QEMU will always try to setup
>>>>>>>> VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT protocol featrue with
>>>>>>>> backend. This will allow to use skip_drain parameter on 
>>>>>>>> GET_VRING_BASE
>>>>>>>> message.
>>>>>>>>
>>>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex- 
>>>>>>>> team.ru>
>>>>>>>> Signed-off-by: Alexandr Moshkov <[email protected]>
>>>>>>>> ---
>>>>>>>>   docs/interop/vhost-user.rst    |  6 ++----
>>>>>>>>   hw/block/vhost-user-blk.c      | 30 +++++++++++++++++++++++ 
>>>>>>>> +------
>>>>>>>>   hw/virtio/vhost-user.c         |  3 +--
>>>>>>>>   hw/virtio/vhost.c              |  1 +
>>>>>>>>   include/hw/virtio/vhost-user.h |  1 -
>>>>>>>>   5 files changed, 28 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost- 
>>>>>>>> user.rst
>>>>>>>> index bfa75ff9a3..63efc87264 100644
>>>>>>>> --- a/docs/interop/vhost-user.rst
>>>>>>>> +++ b/docs/interop/vhost-user.rst
>>>>>>>> @@ -1255,14 +1255,12 @@ Front-end message types
>>>>>>>>     *suspended*, see :ref:`Suspended device state
>>>>>>>>     <suspended_device_state>`.
>>>>>>>>   -  The request payload's *num* field is currently reserved and 
>>>>>>>> must be
>>>>>>>> -  set to 0.
>>>>>>>> -
>>>>>>>>     By default, the back-end must complete all inflight I/O 
>>>>>>>> requests for the
>>>>>>>>     specified vring before stopping it.
>>>>>>>>       If the ``VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT`` 
>>>>>>>> protocol
>>>>>>>> -  feature has been negotiated, the back-end may suspend in- 
>>>>>>>> flight I/O
>>>>>>>> +  feature has been negotiated, using request payload's *num* 
>>>>>>>> field,
>>>>>>>> +  when *num* is set to 1, QEMU can tell the back-end to suspend 
>>>>>>>> in- flight I/O
>>>>>>>>     requests and record them as described in :ref:`Inflight I/O 
>>>>>>>> tracking
>>>>>>>>     <inflight_io_tracking>` instead of completing them before 
>>>>>>>> stopping the vring.
>>>>>>>>     How to suspend an in-flight request depends on the 
>>>>>>>> implementation of the back-end
>>>>>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>>>>>>> index a3ecd83f54..522e9d34b5 100644
>>>>>>>> --- a/hw/block/vhost-user-blk.c
>>>>>>>> +++ b/hw/block/vhost-user-blk.c
>>>>>>>> @@ -134,10 +134,7 @@ static bool 
>>>>>>>> vhost_user_blk_inflight_needed(void *opaque)
>>>>>>>>   {
>>>>>>>>       struct VHostUserBlk *s = opaque;
>>>>>>>>   -    bool inflight_migration = virtio_has_feature(s- 
>>>>>>>> >dev.protocol_features,
>>>>>>>> - VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT);
>>>>>>>> -
>>>>>>>> -    return inflight_migration;
>>>>>>>> +    return s->inflight_migration;
>>>>>>>>   }
>>>>>>>>     @@ -232,11 +229,14 @@ static int 
>>>>>>>> vhost_user_blk_stop(VirtIODevice *vdev)
>>>>>>>>           return 0;
>>>>>>>>       }
>>>>>>>>   +    bool skip_drain = vhost_user_blk_inflight_needed(s) &&
>>>>>>>
>>>>>>> nit: declarations on top
>>>>>>>
>>>>>>>> + runstate_check(RUN_STATE_FINISH_MIGRATE);
>>>>>>>> +
>>>>>>>>       force_stop = s->skip_get_vring_base_on_force_shutdown &&
>>>>>>>>                    qemu_force_shutdown_requested();
>>>>>>>>         ret = force_stop ? vhost_dev_force_stop(&s->dev, vdev, 
>>>>>>>> true) :
>>>>>>>> -                       vhost_dev_stop(&s->dev, vdev, true, false);
>>>>>>>> +                       vhost_dev_stop(&s->dev, vdev, true, 
>>>>>>>> skip_drain);
>>>>>>>>         if (k->set_guest_notifiers(qbus->parent, s->dev.nvqs, 
>>>>>>>> false) < 0) {
>>>>>>>>           error_report("vhost guest notifier cleanup failed: 
>>>>>>>> %d", ret);
>>>>>>>> @@ -364,7 +364,6 @@ static int 
>>>>>>>> vhost_user_blk_connect(DeviceState *dev, Error **errp)
>>>>>>>>       vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>>>>>>>>         s->vhost_user.supports_config = true;
>>>>>>>> -    s->vhost_user.supports_inflight_migration = s- 
>>>>>>>> >inflight_migration;
>>>>>>>>       ret = vhost_dev_init(&s->dev, &s->vhost_user, 
>>>>>>>> VHOST_BACKEND_TYPE_USER, 0,
>>>>>>>>                            errp);
>>>>>>>>       if (ret < 0) {
>>>>>>>> @@ -580,10 +579,29 @@ static struct vhost_dev 
>>>>>>>> *vhost_user_blk_get_vhost(VirtIODevice *vdev)
>>>>>>>>       return &s->dev;
>>>>>>>>   }
>>>>>>>>   +static bool vhost_user_blk_pre_save(void *opaque, Error **errp)
>>>>>>>> +{
>>>>>>>> +    VHostUserBlk *s = VHOST_USER_BLK(opaque);
>>>>>>>> +
>>>>>>>> +    bool inflight_migration_enabled = virtio_has_feature(
>>>>>>>> +        s->dev.protocol_features,
>>>>>>>> + VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT);
>>>>>>>> +    if (vhost_user_blk_inflight_needed(s) && ! 
>>>>>>>> inflight_migration_enabled) {
>>>>>>>> +        error_setg(errp, "can't migrate vhost-user-blk device: "
>>>>>>>> +                         "backend doesn't support "
>>>>>>>> + "VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT "
>>>>>>>> +                         "protocol feature");
>>>>>>>> +        return false;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static const VMStateDescription 
>>>>>>>> vmstate_vhost_user_blk_inflight = {
>>>>>>>>       .name = "vhost-user-blk/inflight",
>>>>>>>>       .version_id = 1,
>>>>>>>>       .needed = vhost_user_blk_inflight_needed,
>>>>>>>> +    .pre_save_errp = vhost_user_blk_pre_save,
>>>>>>>>       .fields = (const VMStateField[]) {
>>>>>>>>           VMSTATE_VHOST_INFLIGHT_REGION(inflight, VHostUserBlk),
>>>>>>>>           VMSTATE_END_OF_LIST()
>>>>>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>>>>>> index bb8f8eab77..ed95ec7523 100644
>>>>>>>> --- a/hw/virtio/vhost-user.c
>>>>>>>> +++ b/hw/virtio/vhost-user.c
>>>>>>>> @@ -2225,8 +2225,7 @@ static int vhost_user_backend_init(struct 
>>>>>>>> vhost_dev *dev, void *opaque,
>>>>>>>>               }
>>>>>>>>           }
>>>>>>>>   -        if (!u->user->supports_inflight_migration ||
>>>>>>>> -            !virtio_has_feature(protocol_features,
>>>>>>>> +        if (!virtio_has_feature(protocol_features,
>>>>>>>> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
>>>>>>>>               protocol_features &= ~(1ULL <<
>>>>>>>> VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT);
>>>>>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>>>>>> index ac4d6fca73..54e4ce2660 100644
>>>>>>>> --- a/hw/virtio/vhost.c
>>>>>>>> +++ b/hw/virtio/vhost.c
>>>>>>>> @@ -1398,6 +1398,7 @@ static int do_vhost_virtqueue_stop(struct 
>>>>>>>> vhost_dev *dev,
>>>>>>>>       int vhost_vq_index = dev->vhost_ops- 
>>>>>>>> >vhost_get_vq_index(dev, idx);
>>>>>>>>       struct vhost_vring_state state = {
>>>>>>>>           .index = vhost_vq_index,
>>>>>>>> +        .num = skip_drain,
>>>>>>>>       };
>>>>>>>>       int r = 0;
>>>>>>>>   diff --git a/include/hw/virtio/vhost-user.h b/include/hw/ 
>>>>>>>> virtio/ vhost-user.h
>>>>>>>> index 53fe996686..c95bad5ddc 100644
>>>>>>>> --- a/include/hw/virtio/vhost-user.h
>>>>>>>> +++ b/include/hw/virtio/vhost-user.h
>>>>>>>> @@ -69,7 +69,6 @@ typedef struct VhostUserState {
>>>>>>>>       GPtrArray *notifiers;
>>>>>>>>       int memory_slots;
>>>>>>>>       bool supports_config;
>>>>>>>> -    bool supports_inflight_migration;
>>>>>>>>   } VhostUserState;
>>>>>>>>     /**
>>>>>>>
>>>>>
>>>