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

Thanks for reply!

On 4/23/26 19:15, Raphael Norwitz wrote:
>
>
> 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 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 <[email protected]>
>>>> 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;
>>>>     /**
>>>
>