* Re: [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init
@ 2020-04-24 2:44 Raphael Norwitz
0 siblings, 0 replies; 4+ messages in thread
From: Raphael Norwitz @ 2020-04-24 2:44 UTC (permalink / raw)
To: dimastep; +Cc: marcandre.lureau, qemu-devel
I’m not opposed to adding this kind of debugging functionality to the
vhost-user-blk sample. It could be helpful to easily test these cases
in the future.
That said, I'm not sure how others will feel about adding these kind
of debugging capabilities to libvhost-user. Marc-Andre, thoughts?
If we go this route I would prefer to add the debugging options to the
vhost-user-blk sample in a separate patch.
On Thu, Apr 23, 2020 at 09:39:32PM +0300, Dima Stepanov wrote:
>
> Add "--simulate-disconnect-stage" option for the testing purposes.
> This option can be used to test the vhost-user reconnect functionality:
> ./vhost-user-blk ... --simulate-disconnect-stage=<CASENUM>
> In this case the daemon will "crash" in the middle of the VHOST comands
> communication. Case nums are as follows:
> 1 - make assert in the handler of the SET_VRING_CALL command
> 2 - make assert in the handler of the SET_VRING_NUM command
> Main purpose is to test QEMU reconnect functionality. Such fail
> injection should not lead to QEMU crash and should be handled
> successfully.
> Also update the "GOptionEntry entries" definition with the final NULL
> item according to API.
>
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
> contrib/libvhost-user/libvhost-user.c | 30 ++++++++++++++++++++++++++++++
> contrib/libvhost-user/libvhost-user.h | 13 +++++++++++++
> contrib/vhost-user-blk/vhost-user-blk.c | 14 +++++++++++++-
> 3 files changed, 56 insertions(+), 1 deletion(-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [RFC PATCH v1 0/7] vhost-user reconnect issues during vhost initialization @ 2020-04-23 18:39 Dima Stepanov 2020-04-23 18:39 ` [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init Dima Stepanov 0 siblings, 1 reply; 4+ messages in thread From: Dima Stepanov @ 2020-04-23 18:39 UTC (permalink / raw) To: qemu-devel Cc: fam, kwolf, yc-core, qemu-block, mst, jasowang, dgilbert, mreitz, arei.gonglei, stefanha, marcandre.lureau, pbonzini, raphael.norwitz 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 (7): contrib/vhost-user-blk: add option to simulate disconnect on init char-socket: return -1 in case of disconnect during tcp_chr_write char-socket: initialize reconnect timer only if close is emitted vhost: introduce wrappers to set guest notifiers for virtio device vhost-user-blk: add mechanism to track the guest notifiers init state vhost: check vring address before calling unmap vhost: add device started check in migration set log backends/cryptodev-vhost.c | 26 +++++--- backends/vhost-user.c | 16 ++--- chardev/char-socket.c | 18 ++--- contrib/libvhost-user/libvhost-user.c | 30 +++++++++ contrib/libvhost-user/libvhost-user.h | 13 ++++ contrib/vhost-user-blk/vhost-user-blk.c | 14 +++- hw/block/vhost-user-blk.c | 23 +++---- hw/net/vhost_net.c | 30 +++++---- hw/scsi/vhost-scsi-common.c | 15 ++--- hw/virtio/vhost-user-fs.c | 17 ++--- hw/virtio/vhost-vsock.c | 18 +++-- hw/virtio/vhost.c | 115 +++++++++++++++++++++++++++++--- hw/virtio/virtio.c | 13 ++++ include/hw/virtio/vhost.h | 5 ++ include/hw/virtio/virtio.h | 1 + 15 files changed, 256 insertions(+), 98 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init 2020-04-23 18:39 [RFC PATCH v1 0/7] vhost-user reconnect issues during vhost initialization Dima Stepanov @ 2020-04-23 18:39 ` Dima Stepanov [not found] ` <20200422160206.GA30919@localhost.localdomain> 0 siblings, 1 reply; 4+ messages in thread From: Dima Stepanov @ 2020-04-23 18:39 UTC (permalink / raw) To: qemu-devel Cc: fam, kwolf, yc-core, qemu-block, mst, jasowang, dgilbert, mreitz, arei.gonglei, stefanha, marcandre.lureau, pbonzini, raphael.norwitz Add "--simulate-disconnect-stage" option for the testing purposes. This option can be used to test the vhost-user reconnect functionality: ./vhost-user-blk ... --simulate-disconnect-stage=<CASENUM> In this case the daemon will "crash" in the middle of the VHOST comands communication. Case nums are as follows: 1 - make assert in the handler of the SET_VRING_CALL command 2 - make assert in the handler of the SET_VRING_NUM command Main purpose is to test QEMU reconnect functionality. Such fail injection should not lead to QEMU crash and should be handled successfully. Also update the "GOptionEntry entries" definition with the final NULL item according to API. Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> --- contrib/libvhost-user/libvhost-user.c | 30 ++++++++++++++++++++++++++++++ contrib/libvhost-user/libvhost-user.h | 13 +++++++++++++ contrib/vhost-user-blk/vhost-user-blk.c | 14 +++++++++++++- 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index 3bca996..5215214 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -73,6 +73,14 @@ #define VHOST_USER_VERSION 1 #define LIBVHOST_USER_DEBUG 0 +/* + * Inject fail in different places in daemon. This will trigger different + * paths in QEMU. Main purpose is to test the reconnect functionality + * during vhost initialization step. + */ +#define VHOST_SDISCONNECT_SET_VRING_CALL 1 +#define VHOST_SDISCONNECT_SET_VRING_NUM 2 + #define DPRINT(...) \ do { \ if (LIBVHOST_USER_DEBUG) { \ @@ -861,6 +869,11 @@ vu_set_vring_num_exec(VuDev *dev, VhostUserMsg *vmsg) DPRINT("State.index: %d\n", index); DPRINT("State.num: %d\n", num); dev->vq[index].vring.num = num; + if (dev->simulate_init_disconnect == VHOST_SDISCONNECT_SET_VRING_NUM) { + DPRINT("Simulate vhost daemon crash during initialization.\n"); + assert(0); + return false; + } return false; } @@ -1161,6 +1174,13 @@ vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg) DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64); + /* Simulate crash during initialization. */ + if (dev->simulate_init_disconnect == VHOST_SDISCONNECT_SET_VRING_CALL) { + DPRINT("Simulate vhost daemon crash during initialization.\n"); + assert(0); + return false; + } + if (!vu_check_queue_msg_file(dev, vmsg)) { return false; } @@ -2073,6 +2093,16 @@ vu_queue_empty(VuDev *dev, VuVirtq *vq) return vring_avail_idx(vq) == vq->last_avail_idx; } +/* + * Set the flag to simulate the vhost-user daemon crash during + * initialization. This is used to test reconnect functionality. + */ +void +vu_simulate_init_disconnect(VuDev *dev, int should_simulate) +{ + dev->simulate_init_disconnect = should_simulate; +} + static bool vring_notify(VuDev *dev, VuVirtq *vq) { diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h index f30394f..9f75e86 100644 --- a/contrib/libvhost-user/libvhost-user.h +++ b/contrib/libvhost-user/libvhost-user.h @@ -388,6 +388,9 @@ struct VuDev { /* Postcopy data */ int postcopy_ufd; bool postcopy_listening; + + /* Fields to simulate test cases. */ + int simulate_init_disconnect; }; typedef struct VuVirtqElement { @@ -645,4 +648,14 @@ void vu_queue_get_avail_bytes(VuDev *vdev, VuVirtq *vq, unsigned int *in_bytes, bool vu_queue_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int in_bytes, unsigned int out_bytes); +/** + * vu_simulate_init_disconnect: + * @dev: a VuDev context + * @should_simulate: expected simulation behaviour (true or false) + * + * Set the flag to simulate the vhost-user daemon crash during + * initialization. This is used to test reconnect functionality. + */ +void vu_simulate_init_disconnect(VuDev *dev, int should_simulate); + #endif /* LIBVHOST_USER_H */ diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c index 6fd91c7..6ac37ca 100644 --- a/contrib/vhost-user-blk/vhost-user-blk.c +++ b/contrib/vhost-user-blk/vhost-user-blk.c @@ -581,6 +581,7 @@ static char *opt_socket_path; static char *opt_blk_file; static gboolean opt_print_caps; static gboolean opt_read_only; +static gboolean opt_simulate_disconnect; static GOptionEntry entries[] = { { "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, &opt_print_caps, @@ -592,7 +593,14 @@ static GOptionEntry entries[] = { {"blk-file", 'b', 0, G_OPTION_ARG_FILENAME, &opt_blk_file, "block device or file path", "PATH"}, { "read-only", 'r', 0, G_OPTION_ARG_NONE, &opt_read_only, - "Enable read-only", NULL } + "Enable read-only", NULL }, + { "simulate-disconnect-stage", 0, 0, G_OPTION_ARG_INT, + &opt_simulate_disconnect, + "Simulate disconnect during initialization for the testing purposes.\n" + "\t1 - make assert in the handler of the SET_VRING_CALL command\n" + "\t2 - make assert in the handler of the SET_VRING_NUM command\n", + "CASENUM" }, + { NULL }, }; int main(int argc, char **argv) @@ -656,6 +664,10 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } + /* Set testing flags. */ + vu_simulate_init_disconnect(&vdev_blk->parent.parent, + opt_simulate_disconnect); + g_main_loop_run(vdev_blk->loop); g_main_loop_unref(vdev_blk->loop); g_option_context_free(context); -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
[parent not found: <20200422160206.GA30919@localhost.localdomain>]
* Re: [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init [not found] ` <20200422160206.GA30919@localhost.localdomain> @ 2020-04-24 10:17 ` Marc-André Lureau 2020-04-25 9:30 ` Dima Stepanov 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2020-04-24 10:17 UTC (permalink / raw) To: Raphael Norwitz; +Cc: Dima Stepanov, qemu-devel Hi On Fri, Apr 24, 2020 at 4:32 AM Raphael Norwitz <raphael.norwitz@nutanix.com> wrote: > > I’m not opposed to adding this kind of debugging functionality to the > vhost-user-blk sample. It could be helpful to easily test these cases > in the future. > > That said, I'm not sure how others will feel about adding these kind > of debugging capabilities to libvhost-user. Marc-Andre, thoughts? Maybe we should only enable this code if LIBVHOST_USER_DEBUG is set? And to make logging silent by default, we shouldn't print them unless VHOST_USER_DEBUG env is set? > > If we go this route I would prefer to add the debugging options to the > vhost-user-blk sample in a separate patch. > > On Thu, Apr 23, 2020 at 09:39:32PM +0300, Dima Stepanov wrote: > > > > Add "--simulate-disconnect-stage" option for the testing purposes. > > This option can be used to test the vhost-user reconnect functionality: > > ./vhost-user-blk ... --simulate-disconnect-stage=<CASENUM> > > In this case the daemon will "crash" in the middle of the VHOST comands > > communication. Case nums are as follows: > > 1 - make assert in the handler of the SET_VRING_CALL command > > 2 - make assert in the handler of the SET_VRING_NUM command > > Main purpose is to test QEMU reconnect functionality. Such fail > > injection should not lead to QEMU crash and should be handled > > successfully. > > Also update the "GOptionEntry entries" definition with the final NULL > > item according to API. > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > --- > > contrib/libvhost-user/libvhost-user.c | 30 ++++++++++++++++++++++++++++++ > > contrib/libvhost-user/libvhost-user.h | 13 +++++++++++++ > > contrib/vhost-user-blk/vhost-user-blk.c | 14 +++++++++++++- > > 3 files changed, 56 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init 2020-04-24 10:17 ` Marc-André Lureau @ 2020-04-25 9:30 ` Dima Stepanov 0 siblings, 0 replies; 4+ messages in thread From: Dima Stepanov @ 2020-04-25 9:30 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, Raphael Norwitz On Fri, Apr 24, 2020 at 12:17:54PM +0200, Marc-André Lureau wrote: > Hi > > On Fri, Apr 24, 2020 at 4:32 AM Raphael Norwitz > <raphael.norwitz@nutanix.com> wrote: > > > > I’m not opposed to adding this kind of debugging functionality to the > > vhost-user-blk sample. It could be helpful to easily test these cases > > in the future. > > > > That said, I'm not sure how others will feel about adding these kind > > of debugging capabilities to libvhost-user. Marc-Andre, thoughts? > > Maybe we should only enable this code if LIBVHOST_USER_DEBUG is set? > > And to make logging silent by default, we shouldn't print them unless > VHOST_USER_DEBUG env is set? Yes, it is a good idea to move this code under LIBVHOST_USER_DEBUG. Agree. Will update it in version 2, but need more feedback on other patches first. > > > > > If we go this route I would prefer to add the debugging options to the > > vhost-user-blk sample in a separate patch. > > > > On Thu, Apr 23, 2020 at 09:39:32PM +0300, Dima Stepanov wrote: > > > > > > Add "--simulate-disconnect-stage" option for the testing purposes. > > > This option can be used to test the vhost-user reconnect functionality: > > > ./vhost-user-blk ... --simulate-disconnect-stage=<CASENUM> > > > In this case the daemon will "crash" in the middle of the VHOST comands > > > communication. Case nums are as follows: > > > 1 - make assert in the handler of the SET_VRING_CALL command > > > 2 - make assert in the handler of the SET_VRING_NUM command > > > Main purpose is to test QEMU reconnect functionality. Such fail > > > injection should not lead to QEMU crash and should be handled > > > successfully. > > > Also update the "GOptionEntry entries" definition with the final NULL > > > item according to API. > > > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > > --- > > > contrib/libvhost-user/libvhost-user.c | 30 ++++++++++++++++++++++++++++++ > > > contrib/libvhost-user/libvhost-user.h | 13 +++++++++++++ > > > contrib/vhost-user-blk/vhost-user-blk.c | 14 +++++++++++++- > > > 3 files changed, 56 insertions(+), 1 deletion(-) > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-25 9:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-24 2:44 [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init Raphael Norwitz -- strict thread matches above, loose matches on Subject: below -- 2020-04-23 18:39 [RFC PATCH v1 0/7] vhost-user reconnect issues during vhost initialization Dima Stepanov 2020-04-23 18:39 ` [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init Dima Stepanov [not found] ` <20200422160206.GA30919@localhost.localdomain> 2020-04-24 10:17 ` Marc-André Lureau 2020-04-25 9:30 ` Dima Stepanov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.