Re: [PATCH v2 5/5] vhost: add device started check in migration set log
* Dima Stepanov (dimas...@yandex-team.ru) wrote:
> On Mon, May 18, 2020 at 10:53:59AM +0100, Dr. David Alan Gilbert wrote:
> > * Dima Stepanov (dimas...@yandex-team.ru) wrote:
> > > On Mon, May 18, 2020 at 10:50:39AM +0800, Jason Wang wrote:
> > > >
> > > > On 2020/5/16 上午12:54, Dima Stepanov wrote:
> > > > >On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> > > > >>On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > > > > 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(>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.
> > > > >>>Could it be a good idea just to make disconnect in the char device
> > > > >>>but
> > > > >>>postphone clean up in the vhost-user-blk (or any other vhost-user
> > > > >>>device) itself? So we are moving the postphone logic and decision
> > > > >>>from
> > > > >>>the char device to vhost-user device. One of the idea i have is as
> > > > >>>follows:
> > > > >>> - Put ourself in the INITIALIZATION state
> > > > >>> - Start these vhost-user "handshake" commands
> > > > >>> - If we got a disconnect error, perform disconnect, but don't
> > > > >>> clean up
> > > > >>> device (it will be clean up on the roll back). I can be done by
> > > > >>> checking the state in vhost_user_..._disconnect routine or smth
> > > > >>> like it
> > > > >>
> > > > >>Any issue you saw just using the aio bh as Michael posted above.
> > > > >>
> > > > >>Then we don't need to deal with the silent vhost_dev_stop() and we
> > > > >>will have
> > > > >>codes that is much more easier to understand.
> > > > >I've implemented this solution inside
> > > > >hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> > > > >using the s->connected field. Looks good and more correct fix ). I have
> > > > >two questions here before i'll rework the fixes:
> > > > >1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> > > > >we are looking for more generic vhost-user solution? What do you think?
> > > >
> > > >
> > > > I think I agree with Michael, it's better to have a generic vhost-user
> > > > solution. But if it turns out to be not easy, we can start from fixing
> > > > vhost-user-blk.
> > > I also agree, but as i see it right now the connect/disconnect events
> > > are handled inside each vhost-user device implementation file. So it will
> > > need some global refactoring. So i suggest having this fix first and
> > > after it refactoring the code:
> > > - more devices will be involved
> > > - i see there is some difference in device handling
> >
> > I'm following bits of this discussion, some thoughts;
> > if your device doesn't support reconnect, then if, at the start of
> > migration you find that you can't start the log what is the correct
> > behaviour?
> I'm not sure here, but it looks like that in this case the device state
> will be:
> disconnect -> stopped (will not be changed during migration, because
> reconnect isn't supported)
> And because of it the device state will not be changed during migration,
> so there is no need for log and migration could be completed
> successfully.
> So as i see it (i could be wrong here) that:
> - it is okay: if device is not started and we will not change this
>state during migration + log start is failed
> - it is not okay: if device is started + log start is failed (because
>we can't handle the dirty pages and so on during migration)
Yes, that does make sense to me.
> > You can't carry on with the migration because you'd have an
> > inconsistent migration state; so I guess that's why the abort() is there
> > - but I think I'd generally prefer to fail the migration and hope the
> > vhsot device is still working for anything other than the log.
> >
> > You're going to have to be pretty careful with the ordering of reconect
> > - reconnecting on the source during a migration sounds pretty hairy, but
> > a
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Fri, May 15, 2020 at 07:54:57PM +0300, Dima Stepanov wrote:
> On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> >
> > On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > >>> 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(>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.
> > >Could it be a good idea just to make disconnect in the char device but
> > >postphone clean up in the vhost-user-blk (or any other vhost-user
> > >device) itself? So we are moving the postphone logic and decision from
> > >the char device to vhost-user device. One of the idea i have is as
> > >follows:
> > > - Put ourself in the INITIALIZATION state
> > > - Start these vhost-user "handshake" commands
> > > - If we got a disconnect error, perform disconnect, but don't clean up
> > > device (it will be clean up on the roll back). I can be done by
> > > checking the state in vhost_user_..._disconnect routine or smth like
> > > it
> >
> >
> > Any issue you saw just using the aio bh as Michael posted above.
> >
> > Then we don't need to deal with the silent vhost_dev_stop() and we will have
> > codes that is much more easier to understand.
> I've implemented this solution inside
> hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> using the s->connected field. Looks good and more correct fix ). I have
> two questions here before i'll rework the fixes:
> 1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> we are looking for more generic vhost-user solution? What do you think?
Either works I think.
> 2. For migration we require an additional information that for the
> vhost-user device it isn't an error, because i'm trigerring the
> following assert error:
> Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> -no-user-config -M q35,sata=false'.
> Program terminated with signal SIGABRT, Aborted.
> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
>
> (gdb) bt
> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> #2 0x5648ea376ee6 in vhost_log_global_start
> (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> #3 0x5648ea2dde7e in memory_global_dirty_log_start ()
> at ./memory.c:2611
> #4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
> at ./migration/ram.c:2305
> #5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
> at ./migration/ram.c:2323
> #6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
> opaque=0x5648eb1f0f20 )
> at ./migration/ram.c:2436
> #7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
> migration/savevm.c:1176
> #8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
> migration/migration.c:3416
> #9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
> util/qemu-thread-posix.c:519
> #10 0x7fb56eac56ba in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> #11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) frame 2
> #2 0x5648ea376ee6 in vhost_log_global_start
> (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> 857 abort();
> (gdb) list
> 852 {
> 853 int r;
> 854
> 855 r = vhost_migration_log(listener, true);
> 856 if (r < 0) {
> 857 abort();
> 858 }
> 859 }
> 860
> 861 static void vhost_log_global_stop(MemoryListener *listener)
> Since bh postphone the clean up, we can't use the ->started field.
> Do we have any mechanism to get the device type/state in the common
> vhost_migration_log() routine? So for example for the vhost-user/disconnect
> device we will be able to return 0. Or should we implement it and introduce
> it in this patch set?
>
> Thanks, Dima.
>
> >
> > Thank
> >
> >
> > > - vhost-user
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Wed, May 13, 2020 at 01:56:18PM +0800, Jason Wang wrote:
>
> 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
> >>---
> >> 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
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Mon, May 18, 2020 at 10:53:59AM +0100, Dr. David Alan Gilbert wrote:
> * Dima Stepanov (dimas...@yandex-team.ru) wrote:
> > On Mon, May 18, 2020 at 10:50:39AM +0800, Jason Wang wrote:
> > >
> > > On 2020/5/16 上午12:54, Dima Stepanov wrote:
> > > >On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> > > >>On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > > > 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(>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.
> > > >>>Could it be a good idea just to make disconnect in the char device but
> > > >>>postphone clean up in the vhost-user-blk (or any other vhost-user
> > > >>>device) itself? So we are moving the postphone logic and decision from
> > > >>>the char device to vhost-user device. One of the idea i have is as
> > > >>>follows:
> > > >>> - Put ourself in the INITIALIZATION state
> > > >>> - Start these vhost-user "handshake" commands
> > > >>> - If we got a disconnect error, perform disconnect, but don't clean
> > > >>> up
> > > >>> device (it will be clean up on the roll back). I can be done by
> > > >>> checking the state in vhost_user_..._disconnect routine or smth
> > > >>> like it
> > > >>
> > > >>Any issue you saw just using the aio bh as Michael posted above.
> > > >>
> > > >>Then we don't need to deal with the silent vhost_dev_stop() and we will
> > > >>have
> > > >>codes that is much more easier to understand.
> > > >I've implemented this solution inside
> > > >hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> > > >using the s->connected field. Looks good and more correct fix ). I have
> > > >two questions here before i'll rework the fixes:
> > > >1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> > > >we are looking for more generic vhost-user solution? What do you think?
> > >
> > >
> > > I think I agree with Michael, it's better to have a generic vhost-user
> > > solution. But if it turns out to be not easy, we can start from fixing
> > > vhost-user-blk.
> > I also agree, but as i see it right now the connect/disconnect events
> > are handled inside each vhost-user device implementation file. So it will
> > need some global refactoring. So i suggest having this fix first and
> > after it refactoring the code:
> > - more devices will be involved
> > - i see there is some difference in device handling
>
> I'm following bits of this discussion, some thoughts;
> if your device doesn't support reconnect, then if, at the start of
> migration you find that you can't start the log what is the correct
> behaviour?
I'm not sure here, but it looks like that in this case the device state
will be:
disconnect -> stopped (will not be changed during migration, because
reconnect isn't supported)
And because of it the device state will not be changed during migration,
so there is no need for log and migration could be completed
successfully.
So as i see it (i could be wrong here) that:
- it is okay: if device is not started and we will not change this
state during migration + log start is failed
- it is not okay: if device is started + log start is failed (because
we can't handle the dirty pages and so on during migration)
> You can't carry on with the migration because you'd have an
> inconsistent migration state; so I guess that's why the abort() is there
> - but I think I'd generally prefer to fail the migration and hope the
> vhsot device is still working for anything other than the log.
>
> You're going to have to be pretty careful with the ordering of reconect
> - reconnecting on the source during a migration sounds pretty hairy, but
> a migration can take many minutes, so if you really want to survive this
> I guess you have to.
Maybe if we get a disconnect during migration then we could postphone or
just don't make reconnect at all till the end of migration on the source
side. This will make a device left in the stopped state.
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
* Dima Stepanov (dimas...@yandex-team.ru) wrote:
> On Mon, May 18, 2020 at 10:50:39AM +0800, Jason Wang wrote:
> >
> > On 2020/5/16 上午12:54, Dima Stepanov wrote:
> > >On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> > >>On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > > 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(>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.
> > >>>Could it be a good idea just to make disconnect in the char device but
> > >>>postphone clean up in the vhost-user-blk (or any other vhost-user
> > >>>device) itself? So we are moving the postphone logic and decision from
> > >>>the char device to vhost-user device. One of the idea i have is as
> > >>>follows:
> > >>> - Put ourself in the INITIALIZATION state
> > >>> - Start these vhost-user "handshake" commands
> > >>> - If we got a disconnect error, perform disconnect, but don't clean up
> > >>> device (it will be clean up on the roll back). I can be done by
> > >>> checking the state in vhost_user_..._disconnect routine or smth
> > >>> like it
> > >>
> > >>Any issue you saw just using the aio bh as Michael posted above.
> > >>
> > >>Then we don't need to deal with the silent vhost_dev_stop() and we will
> > >>have
> > >>codes that is much more easier to understand.
> > >I've implemented this solution inside
> > >hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> > >using the s->connected field. Looks good and more correct fix ). I have
> > >two questions here before i'll rework the fixes:
> > >1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> > >we are looking for more generic vhost-user solution? What do you think?
> >
> >
> > I think I agree with Michael, it's better to have a generic vhost-user
> > solution. But if it turns out to be not easy, we can start from fixing
> > vhost-user-blk.
> I also agree, but as i see it right now the connect/disconnect events
> are handled inside each vhost-user device implementation file. So it will
> need some global refactoring. So i suggest having this fix first and
> after it refactoring the code:
> - more devices will be involved
> - i see there is some difference in device handling
I'm following bits of this discussion, some thoughts;
if your device doesn't support reconnect, then if, at the start of
migration you find that you can't start the log what is the correct
behaviour? You can't carry on with the migration because you'd have an
inconsistent migration state; so I guess that's why the abort() is there
- but I think I'd generally prefer to fail the migration and hope the
vhsot device is still working for anything other than the log.
You're going to have to be pretty careful with the ordering of reconect
- reconnecting on the source during a migration sounds pretty hairy, but
a migration can take many minutes, so if you really want to survive this
I guess you have to.
Dave
> >
> >
> > >2. For migration we require an additional information that for the
> > >vhost-user device it isn't an error, because i'm trigerring the
> > >following assert error:
> > > Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> > > -no-user-config -M q35,sata=false'.
> > > Program terminated with signal SIGABRT, Aborted.
> > > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > > [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
> > >
> > > (gdb) bt
> > > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > > #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > > #2 0x5648ea376ee6 in vhost_log_global_start
> > > (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> > > #3 0x5648ea2dde7e in memory_global_dirty_log_start ()
> > > at ./memory.c:2611
> > > #4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
> > > at ./migration/ram.c:2305
> > > #5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
> > >
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Mon, May 18, 2020 at 10:50:39AM +0800, Jason Wang wrote:
>
> On 2020/5/16 上午12:54, Dima Stepanov wrote:
> >On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> >>On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > 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(>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.
> >>>Could it be a good idea just to make disconnect in the char device but
> >>>postphone clean up in the vhost-user-blk (or any other vhost-user
> >>>device) itself? So we are moving the postphone logic and decision from
> >>>the char device to vhost-user device. One of the idea i have is as
> >>>follows:
> >>> - Put ourself in the INITIALIZATION state
> >>> - Start these vhost-user "handshake" commands
> >>> - If we got a disconnect error, perform disconnect, but don't clean up
> >>> device (it will be clean up on the roll back). I can be done by
> >>> checking the state in vhost_user_..._disconnect routine or smth like
> >>> it
> >>
> >>Any issue you saw just using the aio bh as Michael posted above.
> >>
> >>Then we don't need to deal with the silent vhost_dev_stop() and we will have
> >>codes that is much more easier to understand.
> >I've implemented this solution inside
> >hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> >using the s->connected field. Looks good and more correct fix ). I have
> >two questions here before i'll rework the fixes:
> >1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> >we are looking for more generic vhost-user solution? What do you think?
>
>
> I think I agree with Michael, it's better to have a generic vhost-user
> solution. But if it turns out to be not easy, we can start from fixing
> vhost-user-blk.
I also agree, but as i see it right now the connect/disconnect events
are handled inside each vhost-user device implementation file. So it will
need some global refactoring. So i suggest having this fix first and
after it refactoring the code:
- more devices will be involved
- i see there is some difference in device handling
>
>
> >2. For migration we require an additional information that for the
> >vhost-user device it isn't an error, because i'm trigerring the
> >following assert error:
> > Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> > -no-user-config -M q35,sata=false'.
> > Program terminated with signal SIGABRT, Aborted.
> > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
> >
> > (gdb) bt
> > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > #2 0x5648ea376ee6 in vhost_log_global_start
> > (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> > #3 0x5648ea2dde7e in memory_global_dirty_log_start ()
> > at ./memory.c:2611
> > #4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
> > at ./migration/ram.c:2305
> > #5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
> > at ./migration/ram.c:2323
> > #6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
> > opaque=0x5648eb1f0f20 )
> > at ./migration/ram.c:2436
> > #7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
> > migration/savevm.c:1176
> > #8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
> > migration/migration.c:3416
> > #9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
> > util/qemu-thread-posix.c:519
> > #10 0x7fb56eac56ba in start_thread () from
> > /lib/x86_64-linux-gnu/libpthread.so.0
> > #11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
> > (gdb) frame 2
> > #2 0x5648ea376ee6 in vhost_log_global_start
> > (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> > 857 abort();
> > (gdb) list
> > 852 {
> > 853 int r;
> > 854
> > 855
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Mon, May 18, 2020 at 10:52:08AM +0800, Jason Wang wrote:
>
> On 2020/5/16 上午11:20, Li Feng wrote:
> >Hi, Dima.
> >This abort is what I have mentioned in my previous email.
> >I have triggered this crash without any fix a week ago.
> >And I have written a test patch to let vhost_log_global_start return
> >int and propagate the error to up layer.
> >However, my change is a little large, because the origin callback
> >return void, and don't do some rollback.
> >After test, the migration could migrate to dst successfully, and fio
> >is still running perfectly, but the src vm is still stuck here, no
> >crash.
> >
> >Is it right to return this error to the up layer?
>
>
> That could be a solution or we may ask David for more suggestion.
>
> Another thing that might be useful is to block re connection during
> migration.
I've written a little more information as answer to Feng's mail. But
what if add some new callback to get the device started state (started or not).
And for the vhost-user (or at least vhost-usr-blk) devices it will use
the connected field also to return the device state:
- disconnect -> not started
For other devices we can just return the started field value as it is
right now.
No other comments mixed in below.
>
> Thanks
>
>
> >
> >Thanks,
> >Feng Li
> >
> >Dima Stepanov 于2020年5月16日周六 上午12:55写道：
> >>On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> >>>On 2020/5/13 下午5:47, Dima Stepanov wrote:
> >> 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(>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.
> Could it be a good idea just to make disconnect in the char device but
> postphone clean up in the vhost-user-blk (or any other vhost-user
> device) itself? So we are moving the postphone logic and decision from
> the char device to vhost-user device. One of the idea i have is as
> follows:
> - Put ourself in the INITIALIZATION state
> - Start these vhost-user "handshake" commands
> - If we got a disconnect error, perform disconnect, but don't clean up
> device (it will be clean up on the roll back). I can be done by
> checking the state in vhost_user_..._disconnect routine or smth like
> it
> >>>
> >>>Any issue you saw just using the aio bh as Michael posted above.
> >>>
> >>>Then we don't need to deal with the silent vhost_dev_stop() and we will
> >>>have
> >>>codes that is much more easier to understand.
> >>I've implemented this solution inside
> >>hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> >>using the s->connected field. Looks good and more correct fix ). I have
> >>two questions here before i'll rework the fixes:
> >>1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> >>we are looking for more generic vhost-user solution? What do you think?
> >>2. For migration we require an additional information that for the
> >>vhost-user device it isn't an error, because i'm trigerring the
> >>following assert error:
> >> Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> >> -no-user-config -M q35,sata=false'.
> >> Program terminated with signal SIGABRT, Aborted.
> >> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> >> [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
> >>
> >> (gdb) bt
> >> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> >> #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> >> #2 0x5648ea376ee6 in vhost_log_global_start
> >> (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> >> #3 0x5648ea2dde7e in memory_global_dirty_log_start ()
> >> at ./memory.c:2611
> >> #4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
> >> at ./migration/ram.c:2305
> >> #5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
> >> at ./migration/ram.c:2323
> >> #6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
> >> opaque=0x5648eb1f0f20 )
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Sat, May 16, 2020 at 11:20:03AM +0800, Li Feng wrote:
> Hi, Dima.
> This abort is what I have mentioned in my previous email.
Yes, i understood it and this abort() message was fixed by the previous
patch. But since we try new postphone approach this patch isn't working
and we need to get the device state somehow:
- vhost-user disconnect => device not started
> I have triggered this crash without any fix a week ago.
> And I have written a test patch to let vhost_log_global_start return
> int and propagate the error to up layer.
> However, my change is a little large, because the origin callback
> return void, and don't do some rollback.
> After test, the migration could migrate to dst successfully, and fio
> is still running perfectly, but the src vm is still stuck here, no
> crash.
>
> Is it right to return this error to the up layer?
Well it is the question we talk about with you, i'm also not sure. I can
only summarize some of the statements i used:
- device state: not started -> is okay for migration
- device state: vhost-user disconnect, this is the same as "not
started" -> is okay for migration
- at least my internal migration tests passed
So my idea for the fix here is smth like:
Add device callback, for instance vhost_dev_started() which will
return device state. And for the vhost-user device (or at least
vhost-user-blk) device this callback will consider the connected field
and return true or false.
As a result vhost_migration_log() will check device state at the start
of the routine and before return.
But if the disconnect state isn't okay for migration, then we should
return an error.
No other comments mixed in below.
>
> Thanks,
> Feng Li
>
> Dima Stepanov 于2020年5月16日周六 上午12:55写道：
> >
> > On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> > >
> > > On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > > >>> 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(>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.
> > > >Could it be a good idea just to make disconnect in the char device but
> > > >postphone clean up in the vhost-user-blk (or any other vhost-user
> > > >device) itself? So we are moving the postphone logic and decision from
> > > >the char device to vhost-user device. One of the idea i have is as
> > > >follows:
> > > > - Put ourself in the INITIALIZATION state
> > > > - Start these vhost-user "handshake" commands
> > > > - If we got a disconnect error, perform disconnect, but don't clean up
> > > > device (it will be clean up on the roll back). I can be done by
> > > > checking the state in vhost_user_..._disconnect routine or smth
> > > > like it
> > >
> > >
> > > Any issue you saw just using the aio bh as Michael posted above.
> > >
> > > Then we don't need to deal with the silent vhost_dev_stop() and we will
> > > have
> > > codes that is much more easier to understand.
> > I've implemented this solution inside
> > hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> > using the s->connected field. Looks good and more correct fix ). I have
> > two questions here before i'll rework the fixes:
> > 1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> > we are looking for more generic vhost-user solution? What do you think?
> > 2. For migration we require an additional information that for the
> > vhost-user device it isn't an error, because i'm trigerring the
> > following assert error:
> > Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> > -no-user-config -M q35,sata=false'.
> > Program terminated with signal SIGABRT, Aborted.
> > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
> >
> > (gdb) bt
> > #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > #2 0x5648ea376ee6 in vhost_log_global_start
> >
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On 2020/5/16 上午11:20, Li Feng wrote:
Hi, Dima.
This abort is what I have mentioned in my previous email.
I have triggered this crash without any fix a week ago.
And I have written a test patch to let vhost_log_global_start return
int and propagate the error to up layer.
However, my change is a little large, because the origin callback
return void, and don't do some rollback.
After test, the migration could migrate to dst successfully, and fio
is still running perfectly, but the src vm is still stuck here, no
crash.
Is it right to return this error to the up layer?
That could be a solution or we may ask David for more suggestion.
Another thing that might be useful is to block re connection during
migration.
Thanks
Thanks,
Feng Li
Dima Stepanov 于2020年5月16日周六 上午12:55写道：
On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
On 2020/5/13 下午5:47, Dima Stepanov wrote:
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(>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.
Could it be a good idea just to make disconnect in the char device but
postphone clean up in the vhost-user-blk (or any other vhost-user
device) itself? So we are moving the postphone logic and decision from
the char device to vhost-user device. One of the idea i have is as
follows:
- Put ourself in the INITIALIZATION state
- Start these vhost-user "handshake" commands
- If we got a disconnect error, perform disconnect, but don't clean up
device (it will be clean up on the roll back). I can be done by
checking the state in vhost_user_..._disconnect routine or smth like it
Any issue you saw just using the aio bh as Michael posted above.
Then we don't need to deal with the silent vhost_dev_stop() and we will have
codes that is much more easier to understand.
I've implemented this solution inside
hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
using the s->connected field. Looks good and more correct fix ). I have
two questions here before i'll rework the fixes:
1. Is it okay to make the similar fix inside vhost_user_blk_event() or
we are looking for more generic vhost-user solution? What do you think?
2. For migration we require an additional information that for the
vhost-user device it isn't an error, because i'm trigerring the
following assert error:
Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
-no-user-config -M q35,sata=false'.
Program terminated with signal SIGABRT, Aborted.
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
[Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
(gdb) bt
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
#3 0x5648ea2dde7e in memory_global_dirty_log_start ()
at ./memory.c:2611
#4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
at ./migration/ram.c:2305
#5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
at ./migration/ram.c:2323
#6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
opaque=0x5648eb1f0f20 )
at ./migration/ram.c:2436
#7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
migration/savevm.c:1176
#8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
migration/migration.c:3416
#9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
util/qemu-thread-posix.c:519
#10 0x7fb56eac56ba in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
#11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) frame 2
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
857 abort();
(gdb) list
852 {
853 int r;
854
855 r = vhost_migration_log(listener, true);
856 if (r < 0) {
857 abort();
858 }
859 }
860
861 static void
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On 2020/5/16 上午12:54, Dima Stepanov wrote:
On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
On 2020/5/13 下午5:47, Dima Stepanov wrote:
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(>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.
Could it be a good idea just to make disconnect in the char device but
postphone clean up in the vhost-user-blk (or any other vhost-user
device) itself? So we are moving the postphone logic and decision from
the char device to vhost-user device. One of the idea i have is as
follows:
- Put ourself in the INITIALIZATION state
- Start these vhost-user "handshake" commands
- If we got a disconnect error, perform disconnect, but don't clean up
device (it will be clean up on the roll back). I can be done by
checking the state in vhost_user_..._disconnect routine or smth like it
Any issue you saw just using the aio bh as Michael posted above.
Then we don't need to deal with the silent vhost_dev_stop() and we will have
codes that is much more easier to understand.
I've implemented this solution inside
hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
using the s->connected field. Looks good and more correct fix ). I have
two questions here before i'll rework the fixes:
1. Is it okay to make the similar fix inside vhost_user_blk_event() or
we are looking for more generic vhost-user solution? What do you think?
I think I agree with Michael, it's better to have a generic vhost-user
solution. But if it turns out to be not easy, we can start from fixing
vhost-user-blk.
2. For migration we require an additional information that for the
vhost-user device it isn't an error, because i'm trigerring the
following assert error:
Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
-no-user-config -M q35,sata=false'.
Program terminated with signal SIGABRT, Aborted.
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
[Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
(gdb) bt
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
#3 0x5648ea2dde7e in memory_global_dirty_log_start ()
at ./memory.c:2611
#4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
at ./migration/ram.c:2305
#5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
at ./migration/ram.c:2323
#6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
opaque=0x5648eb1f0f20 )
at ./migration/ram.c:2436
#7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
migration/savevm.c:1176
#8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
migration/migration.c:3416
#9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
util/qemu-thread-posix.c:519
#10 0x7fb56eac56ba in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
#11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) frame 2
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
857 abort();
(gdb) list
852 {
853 int r;
854
855 r = vhost_migration_log(listener, true);
856 if (r < 0) {
857 abort();
858 }
859 }
860
861 static void vhost_log_global_stop(MemoryListener *listener)
Since bh postphone the clean up, we can't use the ->started field.
Do we have any mechanism to get the device type/state in the common
vhost_migration_log() routine? So for example for the vhost-user/disconnect
device we will be able to return 0. Or should we implement it and introduce
it in this patch set?
This requires more thought, I will reply in Feng's mail.
Thanks
Thanks, Dima.
Thank
- vhost-user command returns error back to the _start() routine
- Rollback in one place in the start() routine, by calling this
postphoned clean
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
Hi, Dima.
This abort is what I have mentioned in my previous email.
I have triggered this crash without any fix a week ago.
And I have written a test patch to let vhost_log_global_start return
int and propagate the error to up layer.
However, my change is a little large, because the origin callback
return void, and don't do some rollback.
After test, the migration could migrate to dst successfully, and fio
is still running perfectly, but the src vm is still stuck here, no
crash.
Is it right to return this error to the up layer?
Thanks,
Feng Li
Dima Stepanov 于2020年5月16日周六 上午12:55写道：
>
> On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
> >
> > On 2020/5/13 下午5:47, Dima Stepanov wrote:
> > >>> 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(>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.
> > >Could it be a good idea just to make disconnect in the char device but
> > >postphone clean up in the vhost-user-blk (or any other vhost-user
> > >device) itself? So we are moving the postphone logic and decision from
> > >the char device to vhost-user device. One of the idea i have is as
> > >follows:
> > > - Put ourself in the INITIALIZATION state
> > > - Start these vhost-user "handshake" commands
> > > - If we got a disconnect error, perform disconnect, but don't clean up
> > > device (it will be clean up on the roll back). I can be done by
> > > checking the state in vhost_user_..._disconnect routine or smth like
> > > it
> >
> >
> > Any issue you saw just using the aio bh as Michael posted above.
> >
> > Then we don't need to deal with the silent vhost_dev_stop() and we will have
> > codes that is much more easier to understand.
> I've implemented this solution inside
> hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
> using the s->connected field. Looks good and more correct fix ). I have
> two questions here before i'll rework the fixes:
> 1. Is it okay to make the similar fix inside vhost_user_blk_event() or
> we are looking for more generic vhost-user solution? What do you think?
> 2. For migration we require an additional information that for the
> vhost-user device it isn't an error, because i'm trigerring the
> following assert error:
> Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
> -no-user-config -M q35,sata=false'.
> Program terminated with signal SIGABRT, Aborted.
> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> [Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
>
> (gdb) bt
> #0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> #1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
> #2 0x5648ea376ee6 in vhost_log_global_start
> (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> #3 0x5648ea2dde7e in memory_global_dirty_log_start ()
> at ./memory.c:2611
> #4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
> at ./migration/ram.c:2305
> #5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
> at ./migration/ram.c:2323
> #6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
> opaque=0x5648eb1f0f20 )
> at ./migration/ram.c:2436
> #7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
> migration/savevm.c:1176
> #8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
> migration/migration.c:3416
> #9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
> util/qemu-thread-posix.c:519
> #10 0x7fb56eac56ba in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> #11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) frame 2
> #2 0x5648ea376ee6 in vhost_log_global_start
> (listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
> 857 abort();
> (gdb) list
> 852 {
> 853 int r;
> 854
> 855 r = vhost_migration_log(listener, true);
> 856 if (r < 0) {
> 857
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Thu, May 14, 2020 at 03:34:24PM +0800, Jason Wang wrote:
>
> On 2020/5/13 下午5:47, Dima Stepanov wrote:
> >>> 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(>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.
> >Could it be a good idea just to make disconnect in the char device but
> >postphone clean up in the vhost-user-blk (or any other vhost-user
> >device) itself? So we are moving the postphone logic and decision from
> >the char device to vhost-user device. One of the idea i have is as
> >follows:
> > - Put ourself in the INITIALIZATION state
> > - Start these vhost-user "handshake" commands
> > - If we got a disconnect error, perform disconnect, but don't clean up
> > device (it will be clean up on the roll back). I can be done by
> > checking the state in vhost_user_..._disconnect routine or smth like it
>
>
> Any issue you saw just using the aio bh as Michael posted above.
>
> Then we don't need to deal with the silent vhost_dev_stop() and we will have
> codes that is much more easier to understand.
I've implemented this solution inside
hw/block/vhost-user-blk.c:vhost_user_blk_event() in the similar way by
using the s->connected field. Looks good and more correct fix ). I have
two questions here before i'll rework the fixes:
1. Is it okay to make the similar fix inside vhost_user_blk_event() or
we are looking for more generic vhost-user solution? What do you think?
2. For migration we require an additional information that for the
vhost-user device it isn't an error, because i'm trigerring the
following assert error:
Core was generated by `x86_64-softmmu/qemu-system-x86_64 -nodefaults
-no-user-config -M q35,sata=false'.
Program terminated with signal SIGABRT, Aborted.
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
[Current thread is 1 (Thread 0x7fb486ef5700 (LWP 527734))]
(gdb) bt
#0 0x7fb56e729428 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7fb56e72b02a in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
#3 0x5648ea2dde7e in memory_global_dirty_log_start ()
at ./memory.c:2611
#4 0x5648ea2e68e7 in ram_init_bitmaps (rs=0x7fb4740008c0)
at ./migration/ram.c:2305
#5 0x5648ea2e698b in ram_init_all (rsp=0x5648eb1f0f20 )
at ./migration/ram.c:2323
#6 0x5648ea2e6cc5 in ram_save_setup (f=0x5648ec609e00,
opaque=0x5648eb1f0f20 )
at ./migration/ram.c:2436
#7 0x5648ea67b7d3 in qemu_savevm_state_setup (f=0x5648ec609e00) at
migration/savevm.c:1176
#8 0x5648ea674511 in migration_thread (opaque=0x5648ec031ff0) at
migration/migration.c:3416
#9 0x5648ea85d65d in qemu_thread_start (args=0x5648ec6057f0) at
util/qemu-thread-posix.c:519
#10 0x7fb56eac56ba in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
#11 0x7fb56e7fb41d in clone () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) frame 2
#2 0x5648ea376ee6 in vhost_log_global_start
(listener=0x5648ece4eb08) at ./hw/virtio/vhost.c:857
857 abort();
(gdb) list
852 {
853 int r;
854
855 r = vhost_migration_log(listener, true);
856 if (r < 0) {
857 abort();
858 }
859 }
860
861 static void vhost_log_global_stop(MemoryListener *listener)
Since bh postphone the clean up, we can't use the ->started field.
Do we have any mechanism to get the device type/state in the common
vhost_migration_log() routine? So for example for the vhost-user/disconnect
device we will be able to return 0. Or should we implement it and introduce
it in this patch set?
Thanks, Dima.
>
> Thank
>
>
> > - vhost-user command returns error back to the _start() routine
> > - Rollback in one place in the start() routine, by calling this
> > postphoned clean up for the disconnect
> >
>
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On 2020/5/13 下午5:47, Dima Stepanov wrote:
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(>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.
Could it be a good idea just to make disconnect in the char device but
postphone clean up in the vhost-user-blk (or any other vhost-user
device) itself? So we are moving the postphone logic and decision from
the char device to vhost-user device. One of the idea i have is as
follows:
- Put ourself in the INITIALIZATION state
- Start these vhost-user "handshake" commands
- If we got a disconnect error, perform disconnect, but don't clean up
device (it will be clean up on the roll back). I can be done by
checking the state in vhost_user_..._disconnect routine or smth like it
Any issue you saw just using the aio bh as Michael posted above.
Then we don't need to deal with the silent vhost_dev_stop() and we will
have codes that is much more easier to understand.
Thank
- vhost-user command returns error back to the _start() routine
- Rollback in one place in the start() routine, by calling this
postphoned clean up for the disconnect
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Wed, May 13, 2020 at 01:56:18PM +0800, Jason Wang wrote:
>
> 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
> >>---
> >> 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
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Wed, May 13, 2020 at 11:20:50AM +0800, Jason Wang wrote:
>
> On 2020/5/12 下午5:35, 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
> >---
> > 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.
>
>
> Yes.
>
>
> >
> >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.
> >
>
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
---
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
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
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
> > >>>---
> > >>> 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
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On 2020/5/12 下午5:35, 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
---
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.
Yes.
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.
As i wrote we have a test:
- run src VM with vhost-usr-blk daemon used
- run fio inside it
- perform reconnect every X seconds (just kill and restart daemon),
X is random
- run dst VM
- perform migration
- fio should complete in dst VM
And we cycle this test like forever.
So it fails once per ~25 iteration. By adding some delays inside qemu we
were able to make the race window larger.
It would be better if we can draft some qtest for this.
Thanks
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
> >>>---
> >>> 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.
As i wrote we have a test:
- run src VM with vhost-usr-blk daemon used
- run fio inside it
- perform reconnect every X seconds (just kill and restart daemon),
X is random
- run dst VM
- perform migration
- fio should complete in dst VM
And we cycle this test like forever.
So it fails once per ~25 iteration. By adding some delays
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Tue, May 12, 2020 at 11:47:34AM +0800, Li Feng wrote:
> Hi, Dima.
>
> If vhost_migration_log return < 0, then vhost_log_global_start will
> trigger a crash.
> Does your patch have process this abort?
> If a disconnect happens in the migration stage, the correct operation
> is to stop the migration, right?
>
> 841 static void vhost_log_global_start(MemoryListener *listener)
> 842 {
> 843 int r;
> 844
> 845 r = vhost_migration_log(listener, true);
> 846 if (r < 0) {
> 847 abort();
> 848 }
> 849 }
Yes, my patch process it by not returning an error ). That is one of the
point we've talked about with Raphael and Michael in this thread. First
of all in my patches i'm still following the same logic which has been
already in upstream ./hw/virtio/vhost.c:vhost_migration_log():
...
820 if (!dev->started) {
821 dev->log_enabled = enable;
822 return 0;
823 }
...
It means, that if device not started, then continue migration without
returning any error. So i followed the same logic, if we got a
disconnect, then it will mean that device isn't started and we can
continue migration. As a result no error is returned and assert() isn't
hit.
Also there is a question from Raphael to Michael about it you can find
it in this thread, by i will add it also:
> Subject: Re: [PATCH v2 5/5] vhost: add device started check in
> migration set log
> On Wed, May 06, 2020 at 06:08:34PM -0400, Raphael Norwitz wrote:
>> In particular, we need to decide whether a migration should be
>> allowed to continue if a device disconnects durning the migration
>> stage.
>>
>> mst, any thoughts?
> Why not? It can't change state while disconnected, so it just makes
> things easier.
So it looks like a correct way to handle it. Also our internal tests
passed. Some words about our tests:
- run src VM with vhost-usr-blk daemon used
- run fio inside it
- perform reconnect every X seconds (just kill and restart
daemon), X is random
- run dst VM
- perform migration
- fio should complete in dst VM
And we cycle this test like forever. At least for now we see no new
issues.
No other comments mixed in below.
>
> Thanks,
>
> Feng Li
>
> Jason Wang 于2020年5月12日周二 上午11:33写道：
> >
> >
> > 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
> > >>> ---
> > >>> 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
> > >&
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
Hi, Dima.
If vhost_migration_log return < 0, then vhost_log_global_start will
trigger a crash.
Does your patch have process this abort?
If a disconnect happens in the migration stage, the correct operation
is to stop the migration, right?
841 static void vhost_log_global_start(MemoryListener *listener)
842 {
843 int r;
844
845 r = vhost_migration_log(listener, true);
846 if (r < 0) {
847 abort();
848 }
849 }
Thanks,
Feng Li
Jason Wang 于2020年5月12日周二 上午11:33写道：
>
>
> 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
> >>> ---
> >>> 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.
>
>
> > So
> > we shouldn't call the clean up calls because this virtqueues were clean
> > up in the disconnect call. But we should protect these calls somehow, so
> > it will not hit SIGSEGV and we will be able to pass migration.
> >
> > Just to summarize it:
> > For the vhost-user-blk devices we ca hit clean up calls twice in case of
> > vhost disconnect:
> > 1. The first time during the disconnect process. The clean up is called
> > inside it.
> > 2. The second time during roll back clean up.
> > So if it is the case we should skip p2.
> >
> >>> dev, dev->vq_index + i);
> >>> vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >>>dev->log_enabled);
> >>> }
> >>> -vhost_dev_set_features(dev,
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
---
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.
So
we shouldn't call the clean up calls because this virtqueues were clean
up in the disconnect call. But we should protect these calls somehow, so
it will not hit SIGSEGV and we will be able to pass migration.
Just to summarize it:
For the vhost-user-blk devices we ca hit clean up calls twice in case of
vhost disconnect:
1. The first time during the disconnect process. The clean up is called
inside it.
2. The second time during roll back clean up.
So if it is the case we should skip p2.
dev, dev->vq_index + i);
vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
dev->log_enabled);
}
-vhost_dev_set_features(dev, dev->log_enabled);
+if (dev->started) {
+vhost_dev_set_features(dev, dev->log_enabled);
+}
err_features:
return r;
}
@@ -832,7 +850,15 @@ static int vhost_migration_log(MemoryListener *listener,
int enable)
} else {
vhost_dev_log_resize(dev, vhost_get_log_size(dev));
r = vhost_dev_set_log(dev, true);
-if (r < 0) {
+/*
+ * The dev log resize can fail, because of disconnect
+ * with the vhost-user-blk daemon. Check the device
+ * state before calling the vhost_dev_set_log()
+ * function.
+ * Don't return error if device isn't started to be
+ * consistent with the check above.
+ */
+if (dev->started && r < 0) {
return r;
}
}
@@ -1739,7 +1765,12 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev)
fail_log:
vhost_log_put(hdev, false);
fail_vq:
-while (--i >= 0) {
+/*
+ * Disconnect with the vhost-user daemon can lead
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Sun, May 10, 2020 at 08:03:39PM -0400, Raphael Norwitz wrote:
> On Thu, May 7, 2020 at 11:35 AM Dima Stepanov wrote:
> >
> > What do you think?
> >
>
> Apologies - I tripped over the if (dev->started && r < 0) check.
> Never-mind my point with race conditions and failing migrations.
>
> Rather than modifying vhost_dev_set_log(), it may be clearer to put a
> check after vhost_dev_log_resize()? Something like:
>
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -829,11 +829,22 @@ static int vhost_migration_log(MemoryListener
> *listener, int enable)
> vhost_log_put(dev, false);
> } else {
> vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> +/*
> + * A device can be stopped because of backend disconnect inside
> + * vhost_dev_log_resize(). In this case we should mark logging
> + * enabled and return without attempting to set the backend
> + * logging state.
> + */
> +if (!dev->started) {
> +goto out_success;
> +}
> r = vhost_dev_set_log(dev, true);
> if (r < 0) {
> return r;
> }
> }
> +
> +out_success:
> dev->log_enabled = enable;
> return 0;
> }
This patch will not fix all the issues. Consider the case than you will
hit disconnect inside vhost_dev_set_log. For instance for the 3rd
virtqueue, for the following call:
vhost_virtqueue_set_addr(...)
Maybe i didn't explain very clearly the problem. The problem i've tried
to fix is only for the vhost-user-blk devices. This issue can be hit
during VHOST_USER commands "handshake". If we hit disconnect on any step
of this "handshake" then we will try to make clean up twice:
1. First during disconnect cleanup (unix socket backend).
2. Second as roll back for initialization.
If this is the case, then we shouldn't call p2, as everything was clean
up on p1. And the complicated thing is that there are several VHOST_USER
commands and we should consider the state after each. And even more,
initialization could fail because of some other reason and we hit
disconnect inside roll back clean up, in this case we should complete
clean up in the disconnect function and stop rolling back.
Hope it helps ).
>
> This seems harmless enough to me, and I see how it fixes your
> particular crash, but I would still prefer we worked towards a more
> robust solution. In particular I think we could handle this inside
> vhost-user-blk if we let the device state persist between connections
> (i.e. call vhost_dev_cleanup() inside vhost_user_blk_connect() before
> vhost_dev_init() on reconnect). This should also fix some of the
> crashes Li Feng has hit, and probably others which haven’t been
> reported yet. What do you think?
Yes, this looks like a good direction. Because all my patches are only
workarounds and i believe there can be some other issues which haven't
been reported or will be introduced ).
I still think that these patches are good to submit and to think about
more complicated/refactoring solution as the next step.
>
> If that’s unworkable I guess we will need to add these vhost level
> checks.
At least for now, i don't think its unworkable, i just think that it
will take some time to figure out how to refactor it properly. But the
SIGSEGV issue is real.
> In that case I would still prefer we add a “disconnected” flag
> in struct vhost_dev struct, and make sure it isn’t cleared by
> vhost_dev_cleanup(). That way we don’t conflate stopping a device with
> backend disconnect at the vhost level and potentially regress behavior
> for other device types.
It is also possible, but should be analyzed and properly tested. So as i
said it will take some time to figure out how to refactor it properly.
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
> >---
> > 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. So
we shouldn't call the clean up calls because this virtqueues were clean
up in the disconnect call. But we should protect these calls somehow, so
it will not hit SIGSEGV and we will be able to pass migration.
Just to summarize it:
For the vhost-user-blk devices we ca hit clean up calls twice in case of
vhost disconnect:
1. The first time during the disconnect process. The clean up is called
inside it.
2. The second time during roll back clean up.
So if it is the case we should skip p2.
>
> >dev, dev->vq_index + i);
> > vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > dev->log_enabled);
> > }
> >-vhost_dev_set_features(dev, dev->log_enabled);
> >+if (dev->started) {
> >+vhost_dev_set_features(dev, dev->log_enabled);
> >+}
> > err_features:
> > return r;
> > }
> >@@ -832,7 +850,15 @@ static int vhost_migration_log(MemoryListener
> >*listener, int enable)
> > } else {
> > vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> > r = vhost_dev_set_log(dev, true);
> >-if (r < 0) {
> >+/*
> >+ * The dev log resize can fail, because of disconnect
> >+ * with the vhost-user-blk daemon. Check the device
> >+ * state before calling the vhost_dev_set_log()
> >+ * function.
> >+ * Don't return error if device isn't started to be
> >+ * consistent with the check above.
> >+ */
> >+if (dev->started && r < 0) {
> > return r;
> > }
> > }
> >@@ -1739,7 +1765,12 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >VirtIODevice *vdev)
> > fail_log:
> > vhost_log_put(hdev, false);
> > fail_vq:
> >-while (--i >= 0) {
> >+/*
> >+ * Disconnect with the vhost-user daemon can
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
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
---
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.
dev, dev->vq_index + i);
vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
dev->log_enabled);
}
-vhost_dev_set_features(dev, dev->log_enabled);
+if (dev->started) {
+vhost_dev_set_features(dev, dev->log_enabled);
+}
err_features:
return r;
}
@@ -832,7 +850,15 @@ static int vhost_migration_log(MemoryListener *listener,
int enable)
} else {
vhost_dev_log_resize(dev, vhost_get_log_size(dev));
r = vhost_dev_set_log(dev, true);
-if (r < 0) {
+/*
+ * The dev log resize can fail, because of disconnect
+ * with the vhost-user-blk daemon. Check the device
+ * state before calling the vhost_dev_set_log()
+ * function.
+ * Don't return error if device isn't started to be
+ * consistent with the check above.
+ */
+if (dev->started && r < 0) {
return r;
}
}
@@ -1739,7 +1765,12 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev)
fail_log:
vhost_log_put(hdev, false);
fail_vq:
-while (--i >= 0) {
+/*
+ * Disconnect with the vhost-user daemon can lead to the
+ * vhost_dev_cleanup() call which will clean up vhost_dev
+ * structure.
+ */
+while ((--i >= 0) && (hdev->started)) {
vhost_virtqueue_stop(hdev,
vdev,
hdev->vqs + i,
This should be a separate patch.
Thanks
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Thu, May 7, 2020 at 11:35 AM Dima Stepanov wrote:
>
> What do you think?
>
Apologies - I tripped over the if (dev->started && r < 0) check.
Never-mind my point with race conditions and failing migrations.
Rather than modifying vhost_dev_set_log(), it may be clearer to put a
check after vhost_dev_log_resize()? Something like:
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -829,11 +829,22 @@ static int vhost_migration_log(MemoryListener
*listener, int enable)
vhost_log_put(dev, false);
} else {
vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+/*
+ * A device can be stopped because of backend disconnect inside
+ * vhost_dev_log_resize(). In this case we should mark logging
+ * enabled and return without attempting to set the backend
+ * logging state.
+ */
+if (!dev->started) {
+goto out_success;
+}
r = vhost_dev_set_log(dev, true);
if (r < 0) {
return r;
}
}
+
+out_success:
dev->log_enabled = enable;
return 0;
}
This seems harmless enough to me, and I see how it fixes your
particular crash, but I would still prefer we worked towards a more
robust solution. In particular I think we could handle this inside
vhost-user-blk if we let the device state persist between connections
(i.e. call vhost_dev_cleanup() inside vhost_user_blk_connect() before
vhost_dev_init() on reconnect). This should also fix some of the
crashes Li Feng has hit, and probably others which haven’t been
reported yet. What do you think?
If that’s unworkable I guess we will need to add these vhost level
checks. In that case I would still prefer we add a “disconnected” flag
in struct vhost_dev struct, and make sure it isn’t cleared by
vhost_dev_cleanup(). That way we don’t conflate stopping a device with
backend disconnect at the vhost level and potentially regress behavior
for other device types.
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Wed, May 06, 2020 at 06:08:34PM -0400, Raphael Norwitz wrote:
> As you correctly point out, this code needs to be looked at more
> carefully so that
> if the device does disconnect in the background we can handle the migration
> path
> gracefully. In particular, we need to decide whether a migration
> should be allowed
> to continue if a device disconnects durning the migration stage.
>From what i see from the code it is allowed. At the start of the
hw/virtio/vhost.c:vhost_migration_log() routine there is a check:
if (!dev->started) {
dev->log_enabled = enable;
return 0;
}
So our changes had the same idea. If device isn't started then 0 can be
returned. Please note, that if we want to return error here then the
following assert will be hit (hw/virtio/vhost.c)
static void vhost_log_global_start(MemoryListener *listener)
{
int r;
r = vhost_migration_log(listener, true);
if (r < 0) {
abort();
}
}
But as i mentioned we didn't change this logic, we just propogate it on
the whole migration start process during vhost handshake. After it our
tests passed successfully.
>
> mst, any thoughts?
>
> Have you looked at the suggestion I gave Li Feng to move vhost_dev_cleanup()
> into the connection path in vhost-user-blk? I’m not sure if he’s
> actively working on it,
> but I would prefer if we can find a way to keep some state around
> between reconnects
> so we aren’t constantly checking dev->started. A device can be stopped
> for reasons
> other than backend disconnect so I’d rather not reuse this field to
> check for backend
> disconnect failures.
In fact i didn't try to use >started field to signal about disconnect.
What i tried to follow is that if device not started (because of
disconnect or any other reason), there is no need to continue
initialization and we can proceed with the next migration step.
>
> On Thu, Apr 30, 2020 at 9:57 AM 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
>
> Could you point to the specific asserts which are being triggered?
Just to be clear here. The assert message i mentioned is described
above. I wanted to explain why we followed the "(!dev->started) return 0"
logic. And in this case we didn't return error and return 0.
But the first error we hit during migration testing was SIGSEGV:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x56354db0a74a in vhost_dev_has_iommu (dev=0x563550562b00)
at hw/virtio/vhost.c:299
299 return vdev->dma_as != _space_memory &&
(gdb) p vdev
$1 = (VirtIODevice *) 0x0
(gdb) bt
#0 0x56354db0a74a in vhost_dev_has_iommu (dev=0x563550562b00)
at hw/virtio/vhost.c:299
#1 0x56354db0bb76 in vhost_dev_set_features (dev=0x563550562b00,
enable_log=true)
at hw/virtio/vhost.c:777
#2 0x56354db0bc1e in vhost_dev_set_log (dev=0x563550562b00,
enable_log=true)
at hw/virtio/vhost.c:790
#3 0x56354db0be58 in vhost_migration_log (listener=0x563550562b08,
enable=1)
at hw/virtio/vhost.c:834
#4 0x56354db0be9b in vhost_log_global_start (listener=0x563550562b08)
at hw/virtio/vhost.c:847
#5 0x56354da72e7e in memory_global_dirty_log_start ()
at memory.c:2611
...
>
> > 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
> > ---
> > 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;
>
> A couple points here
>
>
> (1) This will fail the live migration if the device is disconnected.
> That my be the right thing
> to do, but if there are cases where migrations can proceed with
> a disconnected device,
> this may not be desirable.
I'm not sure that it is correct. VM could be migrated successfully
during
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
On Wed, May 06, 2020 at 06:08:34PM -0400, Raphael Norwitz wrote:
> As you correctly point out, this code needs to be looked at more
> carefully so that
> if the device does disconnect in the background we can handle the migration
> path
> gracefully. In particular, we need to decide whether a migration
> should be allowed
> to continue if a device disconnects durning the migration stage.
>
> mst, any thoughts?
Why not? It can't change state while disconnected, so it just makes
things easier.
> Have you looked at the suggestion I gave Li Feng to move vhost_dev_cleanup()
> into the connection path in vhost-user-blk? I’m not sure if he’s
> actively working on it,
> but I would prefer if we can find a way to keep some state around
> between reconnects
> so we aren’t constantly checking dev->started. A device can be stopped
> for reasons
> other than backend disconnect so I’d rather not reuse this field to
> check for backend
> disconnect failures.
>
> On Thu, Apr 30, 2020 at 9:57 AM 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
>
> Could you point to the specific asserts which are being triggered?
>
> > 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
> > ---
> > 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;
>
> A couple points here
>
>
> (1) This will fail the live migration if the device is disconnected.
> That my be the right thing
> to do, but if there are cases where migrations can proceed with
> a disconnected device,
> this may not be desirable.
>
> (2) This looks racy. As far as I can tell vhost_dev_set_log() is only
> called by vhost_migration_log(),
> and as you say one of the first things vhost_migration_log does
> is return if dev->started is not
> set. What’s to stop a disconnect from clearing the vdev right
> after this check, just before
> vhost_dev_set_features() is called?
>
> As stated above, I would prefer it if we could add some state which
> would persist between
> reconnects which could then be checked in the vhost-user code before
> interacting with
> the backend. I understand this will be a much more involved change and
> will require a lot
> of thought.
>
> Also, regarding (1) above, if the original check in
> vhost_migration_log() returns success if the
> device is not started why return an error here? I imagine this could
> lead to some inconsistent
> behavior if the device disconnects before the first check verses
> before the second.
>
> > +
> > +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)
> > }
Re: [PATCH v2 5/5] vhost: add device started check in migration set log
As you correctly point out, this code needs to be looked at more
carefully so that
if the device does disconnect in the background we can handle the migration path
gracefully. In particular, we need to decide whether a migration
should be allowed
to continue if a device disconnects durning the migration stage.
mst, any thoughts?
Have you looked at the suggestion I gave Li Feng to move vhost_dev_cleanup()
into the connection path in vhost-user-blk? I’m not sure if he’s
actively working on it,
but I would prefer if we can find a way to keep some state around
between reconnects
so we aren’t constantly checking dev->started. A device can be stopped
for reasons
other than backend disconnect so I’d rather not reuse this field to
check for backend
disconnect failures.
On Thu, Apr 30, 2020 at 9:57 AM 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
Could you point to the specific asserts which are being triggered?
> 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
> ---
> 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;
A couple points here
(1) This will fail the live migration if the device is disconnected.
That my be the right thing
to do, but if there are cases where migrations can proceed with
a disconnected device,
this may not be desirable.
(2) This looks racy. As far as I can tell vhost_dev_set_log() is only
called by vhost_migration_log(),
and as you say one of the first things vhost_migration_log does
is return if dev->started is not
set. What’s to stop a disconnect from clearing the vdev right
after this check, just before
vhost_dev_set_features() is called?
As stated above, I would prefer it if we could add some state which
would persist between
reconnects which could then be checked in the vhost-user code before
interacting with
the backend. I understand this will be a much more involved change and
will require a lot
of thought.
Also, regarding (1) above, if the original check in
vhost_migration_log() returns success if the
device is not started why return an error here? I imagine this could
lead to some inconsistent
behavior if the device disconnects before the first check verses
before the second.
> +
> +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)
> }
