[PATCH v4 0/2] vhost-user reconnect issues during vhost initialization

Dima Stepanov posted 2 patches 3 years, 11 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1590396396.git.dimastep@yandex-team.ru
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Raphael Norwitz <raphael.norwitz@nutanix.com>, Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
chardev/char-socket.c     |  7 ++++---
hw/block/vhost-user-blk.c | 38 +++++++++++++++++++++++++++++++++++++-
2 files changed, 41 insertions(+), 4 deletions(-)
[PATCH v4 0/2] vhost-user reconnect issues during vhost initialization
Posted by Dima Stepanov 3 years, 11 months ago
Changes is v4:
- Update the "[PATCH v4 2/2] vhost-user-blk: delay
  vhost_user_blk_disconnect" patch based on Raphael's comment and Li
  Feng previous commit:
  https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg02255.html
  Don't change the vhost_user_blk_device_realize() function. Update the
  comment for the CHR_EVENT_CLOSED event.

Changes in v3:
- "[PATCH v3 1/2] char-socket: return -1 in case of disconnect during
  tcp_chr_write" made a small cleanup suggested by Li Feng. Added
  "Reviewed-by: Marc-André Lureau"
- Rework the vhost_user_blk_disconnect call logic to delay it.
- Remove the migration patch from the patch set, since we are still
  having some discussion about it. In general the current idea is good,
  but need to make some more investigation of how to handle reconnect
  during migration properly

Changes in v2:
- Add to CC list: Li Feng <fengli@smartx.com>, since it looks like that we
are working on pretty similar issues
- Remove [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate
disconnect on init. Going to send this functionality in the separate
patch, with the LIBVHOST_USER_DEBUG rework. Need to think how to reuse
this option and silence the messages first.
- Remove [RFC PATCH v1 3/7] char-socket: initialize reconnect timer only if
close is emitted. This will be handled in the separate patchset:
[PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection by Li
Feng

v1:
During vhost-user reconnect functionality we hit several issues, if
vhost-user-blk daemon is "crashed" or made disconnect during vhost
initialization. The general scenario is as follows:
  - vhost start routine is called
  - vhost write failed due to SIGPIPE
  - this call the disconnect routine and vhost_dev_cleanup routine
    which set to 0 all the field of the vhost_dev structure
  - return back to vhost start routine with the error
  - on the fail path vhost start routine tries to rollback the changes
    by using vhost_dev struct fields which were already reset
  - sometimes this leads to SIGSEGV, sometimes to SIGABRT
Before revising the vhost-user initialization code, we suggest adding
the sanity checks to be aware of the possible disconnect event and that
the vhost_dev structure can be in "uninitialized" state.

The vhost-user-blk daemon is updated with the additional
"--simulate-disconnect-stage=CASENUM" argument to simulate disconnect during
VHOST device initialization. For instance:
  1. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=1
     This command will simulate disconnect in the SET_VRING_CALL handler.
     In this case the vhost device in QEMU is not set the started field to
     true.
  2. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=2
     This command will simulate disconnect in the SET_VRING_NUM handler.
     In this case the started field is set to true.
These two cases test different QEMU parts. Also to trigger different code paths
disconnect should be simulated in two ways:
  - before any successful initialization
  - make successful initialization once and try to simulate disconnects
Also we catch SIGABRT on the migration start if vhost-user daemon disconnected
during vhost-user set log commands communication.

Dima Stepanov (2):
  char-socket: return -1 in case of disconnect during tcp_chr_write
  vhost-user-blk: delay vhost_user_blk_disconnect

 chardev/char-socket.c     |  7 ++++---
 hw/block/vhost-user-blk.c | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.7.4


Re: [PATCH v4 0/2] vhost-user reconnect issues during vhost initialization
Posted by Stefan Hajnoczi 3 years, 7 months ago
On Thu, May 28, 2020 at 12:11:17PM +0300, Dima Stepanov wrote:
> Before revising the vhost-user initialization code, we suggest adding
> the sanity checks to be aware of the possible disconnect event and that
> the vhost_dev structure can be in "uninitialized" state.
> 
> The vhost-user-blk daemon is updated with the additional
> "--simulate-disconnect-stage=CASENUM" argument to simulate disconnect during
> VHOST device initialization. For instance:
>   1. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=1
>      This command will simulate disconnect in the SET_VRING_CALL handler.
>      In this case the vhost device in QEMU is not set the started field to
>      true.
>   2. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=2
>      This command will simulate disconnect in the SET_VRING_NUM handler.
>      In this case the started field is set to true.
> These two cases test different QEMU parts. Also to trigger different code paths
> disconnect should be simulated in two ways:
>   - before any successful initialization
>   - make successful initialization once and try to simulate disconnects
> Also we catch SIGABRT on the migration start if vhost-user daemon disconnected
> during vhost-user set log commands communication.
> 
> Dima Stepanov (2):
>   char-socket: return -1 in case of disconnect during tcp_chr_write
>   vhost-user-blk: delay vhost_user_blk_disconnect
> 
>  chardev/char-socket.c     |  7 ++++---
>  hw/block/vhost-user-blk.c | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 4 deletions(-)

Hi Dima,
The cover letter mentions ./vhost-user-blk --simulate-disconnect-stage=N
to simulate unexpected disconnects but I don't see that option in
contrib/vhost-user-blk/.

Is there a test case?

Thanks,
Stefan