Re: [PATCH v2 5/5] vhost: add device started check in migration set log

[Jason Wang <jasowang@redhat.com>]

On 2020/5/13 下午12:15, Michael S. Tsirkin wrote:
> On Tue, May 12, 2020 at 12:35:30PM +0300, Dima Stepanov wrote:
>> On Tue, May 12, 2020 at 11:32:50AM +0800, Jason Wang wrote:
>>> On 2020/5/11 下午5:25, Dima Stepanov wrote:
>>>> On Mon, May 11, 2020 at 11:15:53AM +0800, Jason Wang wrote:
>>>>> On 2020/4/30 下午9:36, Dima Stepanov wrote:
>>>>>> If vhost-user daemon is used as a backend for the vhost device, then we
>>>>>> should consider a possibility of disconnect at any moment. If such
>>>>>> disconnect happened in the vhost_migration_log() routine the vhost
>>>>>> device structure will be clean up.
>>>>>> At the start of the vhost_migration_log() function there is a check:
>>>>>>    if (!dev->started) {
>>>>>>        dev->log_enabled = enable;
>>>>>>        return 0;
>>>>>>    }
>>>>>> To be consistent with this check add the same check after calling the
>>>>>> vhost_dev_set_log() routine. This in general help not to break a
>>>>>> migration due the assert() message. But it looks like that this code
>>>>>> should be revised to handle these errors more carefully.
>>>>>>
>>>>>> In case of vhost-user device backend the fail paths should consider the
>>>>>> state of the device. In this case we should skip some function calls
>>>>>> during rollback on the error paths, so not to get the NULL dereference
>>>>>> errors.
>>>>>>
>>>>>> Signed-off-by: Dima Stepanov<dimastep@yandex-team.ru>
>>>>>> ---
>>>>>>   hw/virtio/vhost.c | 39 +++++++++++++++++++++++++++++++++++----
>>>>>>   1 file changed, 35 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>>>> index 3ee50c4..d5ab96d 100644
>>>>>> --- a/hw/virtio/vhost.c
>>>>>> +++ b/hw/virtio/vhost.c
>>>>>> @@ -787,6 +787,17 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
>>>>>>   static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>>>>>   {
>>>>>>       int r, i, idx;
>>>>>> +
>>>>>> +    if (!dev->started) {
>>>>>> +        /*
>>>>>> +         * If vhost-user daemon is used as a backend for the
>>>>>> +         * device and the connection is broken, then the vhost_dev
>>>>>> +         * structure will be reset all its values to 0.
>>>>>> +         * Add additional check for the device state.
>>>>>> +         */
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>>       r = vhost_dev_set_features(dev, enable_log);
>>>>>>       if (r < 0) {
>>>>>>           goto err_features;
>>>>>> @@ -801,12 +812,19 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>>>>>       }
>>>>>>       return 0;
>>>>>>   err_vq:
>>>>>> -    for (; i >= 0; --i) {
>>>>>> +    /*
>>>>>> +     * Disconnect with the vhost-user daemon can lead to the
>>>>>> +     * vhost_dev_cleanup() call which will clean up vhost_dev
>>>>>> +     * structure.
>>>>>> +     */
>>>>>> +    for (; dev->started && (i >= 0); --i) {
>>>>>>           idx = dev->vhost_ops->vhost_get_vq_index(
>>>>> Why need the check of dev->started here, can started be modified outside
>>>>> mainloop? If yes, I don't get the check of !dev->started in the beginning of
>>>>> this function.
>>>>>
>>>> No dev->started can't change outside the mainloop. The main problem is
>>>> only for the vhost_user_blk daemon. Consider the case when we
>>>> successfully pass the dev->started check at the beginning of the
>>>> function, but after it we hit the disconnect on the next call on the
>>>> second or third iteration:
>>>>       r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, enable_log);
>>>> The unix socket backend device will call the disconnect routine for this
>>>> device and reset the structure. So the structure will be reset (and
>>>> dev->started set to false) inside this set_addr() call.
>>> I still don't get here. I think the disconnect can not happen in the middle
>>> of vhost_dev_set_log() since both of them were running in mainloop. And even
>>> if it can, we probably need other synchronization mechanism other than
>>> simple check here.
>> Disconnect isn't happened in the separate thread it is happened in this
>> routine inside vhost_dev_set_log. When for instance vhost_user_write()
>> call failed:
>>    vhost_user_set_log_base()
>>      vhost_user_write()
>>        vhost_user_blk_disconnect()
>>          vhost_dev_cleanup()
>>            vhost_user_backend_cleanup()
>> So the point is that if we somehow got a disconnect with the
>> vhost-user-blk daemon before the vhost_user_write() call then it will
>> continue clean up by running vhost_user_blk_disconnect() function. I
>> wrote a more detailed backtrace stack in the separate thread, which is
>> pretty similar to what we have here:
>>    Re: [PATCH v2 4/5] vhost: check vring address before calling unmap
>> The places are different but the problem is pretty similar.
>>
>> So if vhost-user commands handshake then everything is fine and
>> reconnect will work as expected. The only problem is how to handle
>> reconnect properly between vhost-user command send/receive.
>
> So vhost net had this problem too.
>
> commit e7c83a885f865128ae3cf1946f8cb538b63cbfba
> Author: Marc-André Lureau<marcandre.lureau@redhat.com>
> Date:   Mon Feb 27 14:49:56 2017 +0400
>
>      vhost-user: delay vhost_user_stop
>      
>      Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
>      may trigger a disconnect events, calling vhost_user_stop() and clearing
>      all the vhost_dev strutures holding data that vhost.c functions expect
>      to remain valid. Delay the cleanup to keep the vhost_dev structure
>      valid during the vhost.c functions.
>      
>      Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>
>      Message-id:20170227104956.24729-1-marcandre.lureau@redhat.com
>      Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
>
> it now has this code to address this:
>
>
>      case CHR_EVENT_CLOSED:
>          /* a close event may happen during a read/write, but vhost
>           * code assumes the vhost_dev remains setup, so delay the
>           * stop & clear to idle.
>           * FIXME: better handle failure in vhost code, remove bh
>           */
>          if (s->watch) {
>              AioContext *ctx = qemu_get_current_aio_context();
>
>              g_source_remove(s->watch);
>              s->watch = 0;
>              qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, NULL,
>                                       NULL, NULL, false);
>
>              aio_bh_schedule_oneshot(ctx, chr_closed_bh, opaque);
>          }
>          break;
>
> I think it's time we dropped the FIXME and moved the handling to common
> code. Jason? Marc-André?


I agree. Just to confirm, do you prefer bh or doing changes like what is 
done in this series? It looks to me bh can have more easier codes.

Thanks


>
>
>
>
>